Merge "Add lint rule for right lifecycle in addMenuProvider" into androidx-main
diff --git a/fragment/fragment-lint/src/main/java/androidx/fragment/lint/FragmentIssueRegistry.kt b/fragment/fragment-lint/src/main/java/androidx/fragment/lint/FragmentIssueRegistry.kt
index e045232..a75abfd 100644
--- a/fragment/fragment-lint/src/main/java/androidx/fragment/lint/FragmentIssueRegistry.kt
+++ b/fragment/fragment-lint/src/main/java/androidx/fragment/lint/FragmentIssueRegistry.kt
@@ -28,6 +28,7 @@
override val minApi = CURRENT_API
override val issues get() = listOf(
FragmentTagDetector.ISSUE,
+ UnsafeFragmentLifecycleObserverDetector.ADD_MENU_PROVIDER_ISSUE,
UnsafeFragmentLifecycleObserverDetector.BACK_PRESSED_ISSUE,
UnsafeFragmentLifecycleObserverDetector.LIVEDATA_ISSUE,
UseRequireInsteadOfGet.ISSUE,
diff --git a/fragment/fragment-lint/src/main/java/androidx/fragment/lint/UnsafeFragmentLifecycleObserverDetector.kt b/fragment/fragment-lint/src/main/java/androidx/fragment/lint/UnsafeFragmentLifecycleObserverDetector.kt
index e38eb4e..8ed43ac 100644
--- a/fragment/fragment-lint/src/main/java/androidx/fragment/lint/UnsafeFragmentLifecycleObserverDetector.kt
+++ b/fragment/fragment-lint/src/main/java/androidx/fragment/lint/UnsafeFragmentLifecycleObserverDetector.kt
@@ -18,6 +18,7 @@
package androidx.fragment.lint
+import androidx.fragment.lint.UnsafeFragmentLifecycleObserverDetector.Issues.ADD_MENU_PROVIDER_ISSUE
import androidx.fragment.lint.UnsafeFragmentLifecycleObserverDetector.Issues.BACK_PRESSED_ISSUE
import androidx.fragment.lint.UnsafeFragmentLifecycleObserverDetector.Issues.LIVEDATA_ISSUE
import com.android.tools.lint.detector.api.Category
@@ -86,6 +87,25 @@
),
androidSpecific = true
)
+
+ val ADD_MENU_PROVIDER_ISSUE = Issue.create(
+ id = "FragmentAddMenuProvider",
+ briefDescription = "Use getViewLifecycleOwner() as the LifecycleOwner instead of " +
+ "a Fragment instance.",
+ explanation = """The Fragment lifecycle can result in a Fragment being active \
+ longer than its view. This can lead to unexpected behavior from lifecycle aware \
+ objects remaining active longer than the Fragment's view. To solve this issue, \
+ getViewLifecycleOwner() should be used as a LifecycleOwner rather than the \
+ Fragment instance once it is safe to access the view lifecycle in a \
+ Fragment's onCreateView, onViewCreated, onActivityCreated, or \
+ onViewStateRestored methods.""",
+ category = Category.CORRECTNESS,
+ severity = Severity.ERROR,
+ implementation = Implementation(
+ UnsafeFragmentLifecycleObserverDetector::class.java, Scope.JAVA_FILE_SCOPE
+ ),
+ androidSpecific = true
+ )
}
private val lifecycleMethods = setOf(
@@ -149,8 +169,7 @@
* @return `true` if a lint error was found and reported, `false` otherwise.
*/
private fun checkCall(call: UCallExpression, psiMethod: PsiMethod): Boolean {
- val method = Method(psiMethod.containingClass?.qualifiedName, psiMethod.name)
- val issue = UNSAFE_METHODS[method] ?: return false
+ val issue = findIssueForMethod(psiMethod) ?: return false
val argMap = context.evaluator.computeArgumentMapping(call, psiMethod)
argMap.forEach { (arg, param) ->
if (arg.getExpressionType().extends(context, FRAGMENT_CLASS) &&
@@ -184,6 +203,24 @@
}
return false
}
+
+ /**
+ * Checks if the given [PsiMethod] should be associated with an [Issue]. It covers the case
+ * where the given [PsiMethod] is on the base class itself, or if it is on a class that
+ * extends the base class and calls the method instead.
+ *
+ * This allows us to catch the issue no matter where it may occur in the class hierarchy.
+ */
+ private fun findIssueForMethod(psiMethod: PsiMethod): Issue? {
+ UNSAFE_METHODS.keys.forEach { base ->
+ if (context.evaluator.extendsClass(psiMethod.containingClass, base.cls!!) &&
+ psiMethod.name == base.name
+ ) {
+ return UNSAFE_METHODS[base]
+ }
+ }
+ return null
+ }
}
/**
@@ -213,7 +250,9 @@
internal val UNSAFE_METHODS = mapOf(
Method("androidx.lifecycle.LiveData", "observe") to LIVEDATA_ISSUE,
Method("androidx.lifecycle.LiveDataKt", "observe") to LIVEDATA_ISSUE,
- Method("androidx.activity.OnBackPressedDispatcher", "addCallback") to BACK_PRESSED_ISSUE
+ Method("androidx.activity.OnBackPressedDispatcher", "addCallback") to BACK_PRESSED_ISSUE,
+ Method("androidx.core.view.MenuHost", "addMenuProvider") to ADD_MENU_PROVIDER_ISSUE,
+ Method("androidx.core.view.MenuHostHelper", "addMenuProvider") to ADD_MENU_PROVIDER_ISSUE,
)
private const val FRAGMENT_CLASS = "androidx.fragment.app.Fragment"
diff --git a/fragment/fragment-lint/src/test/java/androidx/fragment/lint/AddMenuProviderDetectorTest.kt b/fragment/fragment-lint/src/test/java/androidx/fragment/lint/AddMenuProviderDetectorTest.kt
new file mode 100644
index 0000000..51314e8
--- /dev/null
+++ b/fragment/fragment-lint/src/test/java/androidx/fragment/lint/AddMenuProviderDetectorTest.kt
@@ -0,0 +1,306 @@
+/*
+ * Copyright 2019 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+@file:Suppress("UnstableApiUsage")
+
+package androidx.fragment.lint
+
+import androidx.fragment.lint.stubs.ADD_MENU_PROVIDER_STUBS
+import com.android.tools.lint.checks.infrastructure.LintDetectorTest
+import com.android.tools.lint.checks.infrastructure.TestFile
+import com.android.tools.lint.checks.infrastructure.TestLintResult
+import com.android.tools.lint.detector.api.Detector
+import com.android.tools.lint.detector.api.Issue
+import org.junit.Test
+import org.junit.runner.RunWith
+import org.junit.runners.JUnit4
+
+@RunWith(JUnit4::class)
+class AddMenuProviderDetectorTest : LintDetectorTest() {
+
+ override fun getDetector(): Detector = UnsafeFragmentLifecycleObserverDetector()
+
+ override fun getIssues(): MutableList<Issue> =
+ mutableListOf(UnsafeFragmentLifecycleObserverDetector.ADD_MENU_PROVIDER_ISSUE)
+
+ private fun check(vararg files: TestFile): TestLintResult {
+ return lint().files(*files, *ADD_MENU_PROVIDER_STUBS)
+ .run()
+ }
+
+ @Test
+ fun pass() {
+ check(
+ kotlin(
+ """
+package com.example
+
+import androidx.fragment.app.Fragment
+import com.example.test.Foo
+
+class TestFragment : Fragment {
+
+ override fun onCreateView() {
+ requireActivity().addMenuProvider(this, getViewLifecycleOwner())
+ requireActivity().addMenuProvider(this, viewLifecycleOwner)
+ requireActivity().addMenuProvider(this, viewLifecycleOwner, Lifecycle.State.STARTED)
+ }
+
+ override fun onViewCreated() {
+ test()
+ val foo = Foo()
+ foo.addMenuProvider(requireActivity(), this)
+ foo.menuProvider(this)
+ }
+
+ private fun test() {
+ requireActivity().addMenuProvider(this, getViewLifecycleOwner())
+ test()
+ }
+}
+ """
+ ),
+ kotlin(
+ """
+package com.example.test
+
+import androidx.activity.ComponentActivity
+import androidx.fragment.app.Fragment
+import androidx.lifecycle.LifecycleOwner
+
+class Foo {
+ fun addMenuProvider(activity: ComponentActivity, fragment: Fragment) {
+ activity.addMenuProvider(fragment, LifecycleOwner())
+ activity.addMenuProvider(fragment, LifecycleOwner(), Lifecycle.State.STARTED)
+ }
+
+ fun menuProvider(fragment: Fragment) {}
+}
+ """
+ )
+ )
+ .expectClean()
+ }
+
+ @Test
+ fun inMethodFails() {
+ check(
+ kotlin(
+ """
+package com.example
+
+import androidx.fragment.app.Fragment
+
+class TestFragment : Fragment {
+
+ override fun onCreateView() {
+ requireActivity().addMenuProvider(this, this)
+ }
+}
+ """
+ )
+ )
+ .expect(
+ """
+src/com/example/TestFragment.kt:9: Error: Use viewLifecycleOwner as the LifecycleOwner. [FragmentAddMenuProvider]
+ requireActivity().addMenuProvider(this, this)
+ ~~~~
+1 errors, 0 warnings
+ """
+ )
+ .checkFix(
+ null,
+ kotlin(
+ """
+package com.example
+
+import androidx.fragment.app.Fragment
+
+class TestFragment : Fragment {
+
+ override fun onCreateView() {
+ requireActivity().addMenuProvider(this, viewLifecycleOwner)
+ }
+}
+ """
+ )
+ )
+ }
+
+ @Test
+ fun helperMethodFails() {
+ check(
+ kotlin(
+ """
+package com.example
+
+import androidx.fragment.app.Fragment
+
+class TestFragment : Fragment {
+
+ override fun onCreateView() {
+ test()
+ }
+
+ private fun test() {
+ requireActivity().addMenuProvider(this, this)
+ }
+}
+ """
+ )
+ )
+ .expect(
+ """
+src/com/example/TestFragment.kt:13: Error: Use viewLifecycleOwner as the LifecycleOwner. [FragmentAddMenuProvider]
+ requireActivity().addMenuProvider(this, this)
+ ~~~~
+1 errors, 0 warnings
+ """
+ )
+ .checkFix(
+ null,
+ kotlin(
+ """
+package com.example
+
+import androidx.fragment.app.Fragment
+
+class TestFragment : Fragment {
+
+ override fun onCreateView() {
+ test()
+ }
+
+ private fun test() {
+ requireActivity().addMenuProvider(this, viewLifecycleOwner)
+ }
+}
+ """
+ )
+ )
+ }
+
+ @Test
+ fun externalCallFails() {
+ check(
+ kotlin(
+ """
+package com.example
+
+import androidx.fragment.app.Fragment
+import com.example.test.Foo
+
+class TestFragment : Fragment {
+
+ override fun onCreateView() {
+ test()
+ }
+
+ private fun test() {
+ val foo = Foo()
+ foo.addMenuProvider(requireActivity(), this)
+ }
+}
+ """
+ ),
+ kotlin(
+ """
+package com.example.test
+
+import androidx.activity.ComponentActivity
+import androidx.fragment.app.Fragment
+
+class Foo {
+ fun addMenuProvider(activity:ComponentActivity, fragment: Fragment) {
+ activity.addMenuProvider(fragment, fragment)
+ activity.addMenuProvider(fragment, fragment, Lifecycle.State.STARTED)
+ }
+}
+ """
+ )
+ )
+ .expect(
+ """
+src/com/example/test/Foo.kt:9: Error: Unsafe call to addMenuProvider with Fragment instance as LifecycleOwner from TestFragment.onCreateView. [FragmentAddMenuProvider]
+ activity.addMenuProvider(fragment, fragment)
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+src/com/example/test/Foo.kt:10: Error: Unsafe call to addMenuProvider with Fragment instance as LifecycleOwner from TestFragment.onCreateView. [FragmentAddMenuProvider]
+ activity.addMenuProvider(fragment, fragment, Lifecycle.State.STARTED)
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+2 errors, 0 warnings
+ """
+ )
+ }
+
+ @Test
+ fun externalHelperMethodFails() {
+ check(
+ kotlin(
+ """
+package com.example
+
+import androidx.fragment.app.Fragment
+import com.example.test.Foo
+
+class TestFragment : Fragment {
+
+ override fun onCreateView() {
+ test()
+ }
+
+ private fun test() {
+ val foo = Foo()
+ foo.addMenuProvider(requireActivity(), this)
+ }
+}
+ """
+ ),
+ kotlin(
+ """
+package com.example.test
+
+import androidx.activity.ComponentActivity
+import androidx.fragment.app.Fragment
+
+class Foo {
+ private lateinit val fragment: Fragment
+
+ fun addMenuProvider(activity:ComponentActivity, fragment: Fragment) {
+ this.fragment = fragment
+ menuProvider(activity)
+ }
+
+ private fun menuProvider(activity:ComponentActivity) {
+ activity.addMenuProvider(fragment, fragment)
+ activity.addMenuProvider(fragment, fragment, Lifecycle.State.STARTED)
+ }
+}
+ """
+ )
+ )
+ .expect(
+ """
+src/com/example/test/Foo.kt:16: Error: Unsafe call to addMenuProvider with Fragment instance as LifecycleOwner from TestFragment.onCreateView. [FragmentAddMenuProvider]
+ activity.addMenuProvider(fragment, fragment)
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+src/com/example/test/Foo.kt:17: Error: Unsafe call to addMenuProvider with Fragment instance as LifecycleOwner from TestFragment.onCreateView. [FragmentAddMenuProvider]
+ activity.addMenuProvider(fragment, fragment, Lifecycle.State.STARTED)
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+2 errors, 0 warnings
+ """
+ )
+ }
+}
diff --git a/fragment/fragment-lint/src/test/java/androidx/fragment/lint/FragmentLiveDataObserveDetectorTest.kt b/fragment/fragment-lint/src/test/java/androidx/fragment/lint/FragmentLiveDataObserveDetectorTest.kt
index 85b5fef..e65b434 100644
--- a/fragment/fragment-lint/src/test/java/androidx/fragment/lint/FragmentLiveDataObserveDetectorTest.kt
+++ b/fragment/fragment-lint/src/test/java/androidx/fragment/lint/FragmentLiveDataObserveDetectorTest.kt
@@ -86,7 +86,7 @@
fun observeData(fragment: Fragment) {
val liveData = MutableLiveData<String>()
liveData.observe(LifecycleOwner(), Observer<String> {})
- liveData.observe(fragment, Observer<String> {}, true)
+ liveData.observe(fragment.viewLifecycleOwner, Observer<String> {}, true)
}
fun observe(fragment: Fragment) {}
diff --git a/fragment/fragment-lint/src/test/java/androidx/fragment/lint/stubs/Stubs.kt b/fragment/fragment-lint/src/test/java/androidx/fragment/lint/stubs/Stubs.kt
index ffdb9c4..060fab1 100644
--- a/fragment/fragment-lint/src/test/java/androidx/fragment/lint/stubs/Stubs.kt
+++ b/fragment/fragment-lint/src/test/java/androidx/fragment/lint/stubs/Stubs.kt
@@ -40,16 +40,41 @@
"""
)
+private val COMPONENT_ACTIVITY = java(
+ """
+ package androidx.activity;
+
+ import androidx.core.view.MenuHost;
+ import androidx.core.view.MenuProvider;
+ import androidx.lifecycle.Lifecycle;
+ import androidx.lifecycle.LifecycleOwner;
+
+ public class ComponentActivity implements MenuHost {
+ public void addMenuProvider(@NonNull MenuProvider provider, @NonNull LifecycleOwner owner) {
+
+ }
+
+ public void addMenuProvider(@NonNull MenuProvider provider, @NonNull LifecycleOwner owner,
+ @NonNull Lifecycle.State state) { }
+ }
+ """
+)
+
private val FRAGMENT = java(
"""
package androidx.fragment.app;
+ import androidx.activity.ComponentActivity;
+ import androidx.core.view.MenuProvider;
import androidx.lifecycle.Lifecycle;
import androidx.lifecycle.LifecycleOwner;
- public class Fragment implements LifecycleOwner {
+ public class Fragment implements LifecycleOwner, MenuProvider {
public LifecycleOwner getViewLifecycleOwner() {}
public Lifecycle getLifecycle() {}
+ public ComponentActivity requireActivity() {
+ return ComponentActivity();
+ }
}
"""
)
@@ -142,6 +167,33 @@
"""
).indented().within("src")
+private val MENU_PROVIDER = java(
+ """
+ package androidx.core.view;
+
+ import androidx.annotation.NonNull;
+
+ public interface MenuProvider { }
+ """
+)
+
+private val MENU_HOST = java(
+ """
+ package androidx.core.view;
+
+ import androidx.annotation.NonNull;
+ import androidx.lifecycle.Lifecycle;
+ import androidx.lifecycle.LifecycleOwner;
+
+ public interface MenuHost {
+ void addMenuProvider(@NonNull MenuProvider provider, @NonNull LifecycleOwner owner);
+
+ void addMenuProvider(@NonNull MenuProvider provider, @NonNull LifecycleOwner owner,
+ @NonNull Lifecycle.State state);
+ }
+ """
+)
+
private val COROUTINES = TestFiles.kt(
"kotlinx/coroutines/GlobalScope.kt",
"""
@@ -172,6 +224,7 @@
// stubs for testing calls to LiveData.observe calls
internal val LIVEDATA_STUBS = arrayOf(
+ COMPONENT_ACTIVITY,
FRAGMENT,
DIALOG_FRAGMENT,
LIFECYCLE,
@@ -179,24 +232,42 @@
LIVEDATA,
MUTABLE_LIVEDATA,
OBSERVER,
- LIVEDATA_OBSERVE_EXTENSION
+ LIVEDATA_OBSERVE_EXTENSION,
+ MENU_HOST,
+ MENU_PROVIDER
)
// stubs for testing calls to OnBackPressedDispatcher.addCallback calls
internal val BACK_CALLBACK_STUBS = arrayOf(
+ COMPONENT_ACTIVITY,
BACK_PRESSED_CALLBACK,
BACK_PRESSED_DISPATCHER,
FRAGMENT,
LIFECYCLE,
- LIFECYCLE_OWNER
+ LIFECYCLE_OWNER,
+ MENU_HOST,
+ MENU_PROVIDER
)
// stubs for testing calls to LifecycleOwner.repeatOnLifecycle
internal val REPEAT_ON_LIFECYCLE_STUBS = arrayOf(
+ COMPONENT_ACTIVITY,
REPEAT_ON_LIFECYCLE,
DIALOG_FRAGMENT,
FRAGMENT,
COROUTINES,
LIFECYCLE,
- LIFECYCLE_OWNER
+ LIFECYCLE_OWNER,
+ MENU_HOST,
+ MENU_PROVIDER
+)
+
+// stubs for testing calls to MenuHost.addMenuProvider calls
+internal val ADD_MENU_PROVIDER_STUBS = arrayOf(
+ COMPONENT_ACTIVITY,
+ FRAGMENT,
+ LIFECYCLE,
+ LIFECYCLE_OWNER,
+ MENU_HOST,
+ MENU_PROVIDER
)
