Skip to content

Fix warnings in the linter. #4360

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 23, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ static ExecutorService newFileExecutor() {
return Executors.newSingleThreadExecutor(new NamedThreadFactory(THREAD_FILE));
}

// TODO(b/258424124): Migrate to go/firebase-android-executors
@SuppressLint("ThreadPoolCreation")
static ExecutorService newIntentHandleExecutor() {
return PoolableExecutors.factory()
.newSingleThreadExecutor(
Expand Down
9 changes: 9 additions & 0 deletions tools/lint/src/main/kotlin/CheckRegistry.kt
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package com.google.firebase.lint.checks

import com.android.tools.lint.client.api.IssueRegistry
import com.android.tools.lint.client.api.Vendor
import com.android.tools.lint.detector.api.CURRENT_API
import com.android.tools.lint.detector.api.Issue

Expand All @@ -37,4 +38,12 @@ class CheckRegistry : IssueRegistry() {
get() = CURRENT_API
override val minApi: Int
get() = 2

override val vendor: Vendor
get() =
Vendor(
"firebase",
identifier = "firebase",
feedbackUrl = "https://github.com/firebase/firebase-android-sdk/issues"
)
}
32 changes: 16 additions & 16 deletions tools/lint/src/main/kotlin/DeferredApiDetector.kt
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@

package com.google.firebase.lint.checks

import com.android.tools.lint.detector.api.AnnotationInfo
import com.android.tools.lint.detector.api.AnnotationOrigin
import com.android.tools.lint.detector.api.AnnotationUsageInfo
import com.android.tools.lint.detector.api.AnnotationUsageType
import com.android.tools.lint.detector.api.Category
import com.android.tools.lint.detector.api.Detector
Expand All @@ -24,27 +27,24 @@ import com.android.tools.lint.detector.api.Scope
import com.android.tools.lint.detector.api.Severity
import com.android.tools.lint.detector.api.SourceCodeScanner
import com.intellij.psi.PsiMethod
import org.jetbrains.uast.UAnnotation
import org.jetbrains.uast.UCallExpression
import org.jetbrains.uast.UElement

@Suppress("DetectorIsMissingAnnotations")
class DeferredApiDetector : Detector(), SourceCodeScanner {
override fun applicableAnnotations(): List<String> = listOf(ANNOTATION)

override fun visitAnnotationUsage(
context: JavaContext,
usage: UElement,
type: AnnotationUsageType,
annotation: UAnnotation,
qualifiedName: String,
method: PsiMethod?,
annotations: List<UAnnotation>,
allMemberAnnotations: List<UAnnotation>,
allClassAnnotations: List<UAnnotation>,
allPackageAnnotations: List<UAnnotation>
element: UElement,
annotationInfo: AnnotationInfo,
usageInfo: AnnotationUsageInfo
) {
if (method != null && type == AnnotationUsageType.METHOD_CALL) {
check(context, usage as UCallExpression, method)
if (
usageInfo.type == AnnotationUsageType.METHOD_CALL &&
annotationInfo.origin == AnnotationOrigin.METHOD
) {
check(context, usageInfo.usage as UCallExpression, annotationInfo.annotated as PsiMethod)
}
}

Expand All @@ -59,7 +59,7 @@ class DeferredApiDetector : Detector(), SourceCodeScanner {
INVALID_DEFERRED_API_USE,
usage,
context.getCallLocation(usage, includeReceiver = false, includeArguments = true),
"${method.name} is only safe to call in the context of a Deferred<T> dependency."
"${method.name} is only safe to call in the context of a Deferred`<T>` dependency"
)
}

Expand All @@ -75,9 +75,9 @@ class DeferredApiDetector : Detector(), SourceCodeScanner {
briefDescription = "Invalid use of @DeferredApi",
explanation =
"""
Ensures that a method which expects to be called in the context of
Deferred#whenAvailable(), is actually called this way. This is important for
supporting dynamically loaded modules, where certain dependencies become available
Ensures that a method which expects to be called in the context of \
`Deferred#whenAvailable()`, is actually called this way. This is important for \
supporting dynamically loaded modules, where certain dependencies become available \
during app's runtime and not available upon app launch.
""",
category = Category.CORRECTNESS,
Expand Down
21 changes: 13 additions & 8 deletions tools/lint/src/main/kotlin/KotlinInteropDetector.kt
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ import org.jetbrains.uast.getContainingUClass
import org.jetbrains.uast.getContainingUMethod
import org.jetbrains.uast.getParentOfType

@Suppress("DetectorIsMissingAnnotations")
class KotlinInteropDetector : Detector(), SourceCodeScanner {
companion object Issues {
private val IMPLEMENTATION =
Expand All @@ -66,13 +67,13 @@ class KotlinInteropDetector : Detector(), SourceCodeScanner {
briefDescription = "No Hard Kotlin Keywords",
explanation =
"""
Do not use Kotlin’s hard keywords as the name of methods or fields.
These require the use of backticks to escape when calling from Kotlin.
Do not use Kotlin’s hard keywords as the name of methods or fields. \
These require the use of backticks to escape when calling from Kotlin. \
Soft keywords, modifier keywords, and special identifiers are allowed.

For example, Mockito’s `when` function requires backticks when used from Kotlin:

val callable = Mockito.mock(Callable::class.java)
val callable = Mockito.mock(Callable::class.java) \
Mockito.\`when\`(callable.call()).thenReturn(/* … */)
""",
category = Category.INTEROPERABILITY_KOTLIN,
Expand All @@ -88,7 +89,7 @@ class KotlinInteropDetector : Detector(), SourceCodeScanner {
briefDescription = "Lambda Parameters Last",
explanation =
"""
To improve calling this code from Kotlin,
To improve calling this code from Kotlin, \
parameter types eligible for SAM conversion should be last.
""",
category = Category.INTEROPERABILITY_KOTLIN,
Expand All @@ -104,7 +105,7 @@ class KotlinInteropDetector : Detector(), SourceCodeScanner {
briefDescription = "Unknown nullness",
explanation =
"""
To improve referencing this code from Kotlin, consider adding
To improve referencing this code from Kotlin, consider adding \
explicit nullness information here with either `@NonNull` or `@Nullable`.
""",
category = Category.INTEROPERABILITY_KOTLIN,
Expand Down Expand Up @@ -312,7 +313,7 @@ class KotlinInteropDetector : Detector(), SourceCodeScanner {

if (getter == null) {
// Look for inherited methods
cls.superClass?.let { superClass ->
cls.javaPsi.superClass?.let { superClass ->
for (inherited in superClass.findMethodsByName(getterName1, true)) {
if (inherited.parameterList.parametersCount == 0) {
getter = inherited
Expand Down Expand Up @@ -435,7 +436,7 @@ class KotlinInteropDetector : Detector(), SourceCodeScanner {
) {
val name1 = badGetter!!.name
if (name1.startsWith("is") && methodName.startsWith("setIs") && name1[2].isUpperCase()) {
val newProperty = name1[2].toLowerCase() + name1.substring(3)
val newProperty = name1[2].lowercaseChar() + name1.substring(3)
val message =
"This method should be called `set${newProperty.capitalize()}` such " +
"that (along with the `$name1` getter) Kotlin code can access it " +
Expand Down Expand Up @@ -463,6 +464,10 @@ class KotlinInteropDetector : Detector(), SourceCodeScanner {
}
}

private fun String.capitalize() = replaceFirstChar { it.uppercase() }

private fun String.decapitalize() = replaceFirstChar { it.lowercase() }

/** Returns true if the given class has a (possibly inherited) setter of the given type */
private fun hasSetter(cls: UClass, type: PsiType?, setterName: String): Boolean {
for (method in cls.findMethodsByName(setterName, true)) {
Expand Down Expand Up @@ -570,7 +575,7 @@ class KotlinInteropDetector : Detector(), SourceCodeScanner {
val replaceLocation =
if (node is UParameter) {
location
} else if (node is UMethod && node.modifierList != null) {
} else if (node is UMethod) {
// Place the insertion point at the modifiers such that we don't
// insert the annotation for example after the "public" keyword.
// We also don't want to place it on the method range itself since
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import com.android.tools.lint.detector.api.XmlContext
import com.android.tools.lint.detector.api.XmlScanner
import org.w3c.dom.Element

@Suppress("DetectorIsMissingAnnotations")
class ManifestElementHasNoExportedAttributeDetector : Detector(), XmlScanner {

enum class Component(val xmlName: String) {
Expand Down
5 changes: 3 additions & 2 deletions tools/lint/src/main/kotlin/NonAndroidxNullabilityDetector.kt
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ private val ANDROIDX_ANNOTATIONS =
"android.support.annotation.NonNull"
)

@Suppress("DetectorIsMissingAnnotations")
class NonAndroidxNullabilityDetector : Detector(), SourceCodeScanner {
companion object Issues {
private val IMPLEMENTATION =
Expand All @@ -52,7 +53,7 @@ class NonAndroidxNullabilityDetector : Detector(), SourceCodeScanner {
val NON_ANDROIDX_NULLABILITY =
Issue.create(
id = "FirebaseNonAndroidxNullability",
briefDescription = "Use androidx nullability annotations.",
briefDescription = "Use androidx nullability annotations",
explanation = "Use androidx nullability annotations instead.",
category = Category.COMPLIANCE,
priority = 1,
Expand Down Expand Up @@ -124,7 +125,7 @@ class NonAndroidxNullabilityDetector : Detector(), SourceCodeScanner {
context.report(
NON_ANDROIDX_NULLABILITY,
context.getLocation(annotation),
"Use androidx nullability annotations.",
"Use androidx nullability annotations",
fix
)
}
Expand Down
22 changes: 12 additions & 10 deletions tools/lint/src/main/kotlin/ProviderAssignmentDetector.kt
Original file line number Diff line number Diff line change
Expand Up @@ -25,32 +25,34 @@ import com.android.tools.lint.detector.api.SourceCodeScanner
import com.intellij.psi.PsiClass
import com.intellij.psi.PsiField
import com.intellij.psi.PsiMethod
import org.jetbrains.uast.UBinaryExpression
import org.jetbrains.uast.UCallExpression
import org.jetbrains.uast.UReferenceExpression
import org.jetbrains.uast.getParentOfType
import org.jetbrains.uast.java.JavaUAssignmentExpression

private const val PROVIDER = "com.google.firebase.inject.Provider"

@Suppress("DetectorIsMissingAnnotations")
class ProviderAssignmentDetector : Detector(), SourceCodeScanner {
override fun getApplicableMethodNames() = listOf("get")

override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) {
if (!isProviderGet(method)) {
return
}
val assignmentExpression =
node.getParentOfType<JavaUAssignmentExpression>(JavaUAssignmentExpression::class.java, true)
val assignmentTarget = assignmentExpression?.leftOperand as? UReferenceExpression ?: return
val binaryOperation = node.getParentOfType<UBinaryExpression>(true) ?: return
if (binaryOperation.operatorIdentifier?.name != "=") return

val assignmentTarget = binaryOperation.leftOperand as? UReferenceExpression ?: return

// This would only be true if assigning the result of get(),
// in cases like foo = p.get().someMethod() there would be an intermediate parent
// and we don't want to trigger in such cases.
if (assignmentExpression != node.uastParent?.uastParent) {
if (binaryOperation != node.uastParent?.uastParent) {
return
}

if (hasDeferredApiAnnotation(context, assignmentExpression)) {
if (hasDeferredApiAnnotation(context, binaryOperation)) {
return
}

Expand All @@ -59,7 +61,7 @@ class ProviderAssignmentDetector : Detector(), SourceCodeScanner {
context.report(
INVALID_PROVIDER_ASSIGNMENT,
context.getCallLocation(node, includeReceiver = false, includeArguments = true),
"Provider.get() assignment to a field detected."
"`Provider.get()` assignment to a field detected"
)
}
}
Expand Down Expand Up @@ -87,9 +89,9 @@ class ProviderAssignmentDetector : Detector(), SourceCodeScanner {
briefDescription = "Invalid use of Provider<T>",
explanation =
"""
Ensures that results of Provider.get() are not stored in class fields. Doing
so may lead to bugs in the context of dynamic feature loading. Namely, optional
provider dependencies can become available during the execution of the app, so
Ensures that results of `Provider.get()` are not stored in class fields. Doing \
so may lead to bugs in the context of dynamic feature loading. Namely, optional \
provider dependencies can become available during the execution of the app, so \
dependents must be ready to handle this situation.
""",
category = Category.CORRECTNESS,
Expand Down
24 changes: 18 additions & 6 deletions tools/lint/src/main/kotlin/ThreadPoolDetector.kt
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import com.intellij.psi.PsiClass
import com.intellij.psi.PsiMethod
import org.jetbrains.uast.UCallExpression

@Suppress("DetectorIsMissingAnnotations")
class ThreadPoolDetector : Detector(), SourceCodeScanner {
override fun getApplicableMethodNames(): List<String> =
listOf(
Expand All @@ -20,7 +21,8 @@ class ThreadPoolDetector : Detector(), SourceCodeScanner {
"newScheduledThreadPool",
"newSingleThreadExecutor",
"newSingleThreadScheduledExecutor",
"newWorkStealingPool"
"newWorkStealingPool",
"factory"
)

override fun getApplicableConstructorTypes(): List<String> =
Expand All @@ -32,14 +34,14 @@ class ThreadPoolDetector : Detector(), SourceCodeScanner {
)

override fun visitMethodCall(context: JavaContext, node: UCallExpression, method: PsiMethod) {
if (!isExecutorMethod(method)) {
if (!isExecutorMethod(method) && !isPoolableFactory(method)) {
return
}

context.report(
THREAD_POOL_CREATION,
context.getCallLocation(node, includeReceiver = false, includeArguments = true),
"Creating thread pools is not allowed."
"Creating thread pools is not allowed"
)
}

Expand All @@ -51,7 +53,7 @@ class ThreadPoolDetector : Detector(), SourceCodeScanner {
context.report(
THREAD_POOL_CREATION,
context.getCallLocation(node, includeReceiver = false, includeArguments = true),
"Creating threads or thread pools is not allowed."
"Creating threads or thread pools is not allowed"
)
}

Expand All @@ -62,6 +64,14 @@ class ThreadPoolDetector : Detector(), SourceCodeScanner {
return false
}

private fun isPoolableFactory(method: PsiMethod): Boolean {
if (method.name != "factory") return false
(method.parent as? PsiClass)?.let {
return it.name == "PoolableExecutors"
}
return false
}

companion object {
private val IMPLEMENTATION =
Implementation(ThreadPoolDetector::class.java, Scope.JAVA_FILE_SCOPE)
Expand All @@ -71,10 +81,12 @@ class ThreadPoolDetector : Detector(), SourceCodeScanner {
val THREAD_POOL_CREATION =
Issue.create(
id = "ThreadPoolCreation",
briefDescription = "Creating thread pools is not allowed.",
briefDescription = "Creating thread pools is not allowed",
explanation =
"""
Please use one of the executors provided by firebase-common
Please use one of the executors provided by firebase-common.

See: https://github.com/firebase/firebase-android-sdk/blob/master/docs/executors.md
""",
category = Category.CORRECTNESS,
priority = 6,
Expand Down