Skip to content
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

Update Lint checks for Kotlin consumers #432

Merged
merged 1 commit into from
Aug 10, 2021
Merged

Conversation

jrodbx
Copy link
Collaborator

@jrodbx jrodbx commented Aug 8, 2021

Stacked on #431

cc: @tnorbye

Closes #401. Closes #317.

@jrodbx jrodbx requested a review from JakeWharton August 8, 2021 21:47
if ("format" == methodName && evaluator.isMemberInClass(method, "java.lang.String")) {
if ("format" == methodName &&
(evaluator.isMemberInClass(method, "java.lang.String") ||
evaluator.isMemberInClass(method, "kotlin.text.StringsKt__StringsJVMKt"))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cuz String.format is an extension function on kotlin.String

@@ -122,8 +125,9 @@ class WrongTimberUsageDetector : Detector(), UastScanner {
}
if (current.isMethodCall()) {
val psiMethod = (current as UCallExpression).resolve()
if (Pattern.matches(TIMBER_TREE_LOG_METHOD_REGEXP, psiMethod!!.name)
&& context.evaluator.isMemberInClass(psiMethod, "timber.log.Timber")
if (psiMethod != null &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cuz arrayOf is part of the Kotlin stdlib and not resolvable

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an intrinsic that resolves to a regular Java array creation. That being said I don't understand the comment in the first place.

Copy link
Collaborator Author

@jrodbx jrodbx Aug 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that Lint/Psi reference resolution is limited to types for which it can find the source. Since we don't own the source of arrayOf (or any method that might wrap in this case), psiMethod will be null.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note quite -- it just needs to be on the classpath (which is why the earlier string format comparison resolves to a method in the special class kotlin.text.StringsKt__StringsJVMKt). Lint normally tries to put the android.jar classes into the classpath (if it's an android project), as well as the Kotlin runtime jars (if the unit test contains Kotlin).

Copy link
Collaborator Author

@jrodbx jrodbx Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dug into this a little more and it looks like arrayOf is on the classpath:

in kotlin/text/StringsJVM.kt

@kotlin.internal.InlineOnly
public inline fun String.Companion.format(format: String, vararg args: Any?): String = java.lang.String.format(format, *args)

in kotlin/Library.kt

public inline fun <reified @PureReifiable T> arrayOf(vararg elements: T): Array<T>

However, that resolution is returning null, because the KotlinUFunctionCallExpression.resolvedCall.resultingDescriptor (wat!) is a DeserializedCallableMemberDescriptor whose containingDeclaration is neither a LazyJavaPackageFragment or a DeserializedClassDescriptor, but rather of type org.jetbrains.kotlin.serialization.deserialization.builtins.BuiltInsPackageFragmentImpl.

See here: https://cs.android.com/android-studio/intellij-kotlin/+/master:uast/uast-kotlin/src/org/jetbrains/uast/kotlin/internal/kotlinInternalUastUtils.kt;l=389

Is this a UAST bug, @tnorbye?

fixSource1 += "$methodName(${throwable.asSourceString()}, ${msg.asSourceString()})"
fixSource2 += "$methodName(${throwable.asSourceString()}, ${msg.asSourceString()})"
fixSource1 += "$methodName(${throwable.sourcePsi?.text}, ${msg.asSourceString()})"
fixSource2 += "$methodName(${throwable.sourcePsi?.text}, ${msg.asSourceString()})"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cuz Exception() source strings rendered as <init>(). conversation here: https://groups.google.com/g/lint-dev/c/7FYbR7fSl0I

@jrodbx jrodbx force-pushed the jrod/2021-08-08/lint-kotlin branch from 3b41c5d to b913a14 Compare August 8, 2021 21:56
@jrodbx jrodbx force-pushed the jrod/2021-08-08/konvert-detector branch from b04d9ef to 5c002f3 Compare August 10, 2021 02:59
Base automatically changed from jrod/2021-08-08/konvert-detector to master August 10, 2021 03:05
@jrodbx jrodbx force-pushed the jrod/2021-08-08/lint-kotlin branch from b913a14 to c83e3eb Compare August 10, 2021 03:28
@jrodbx
Copy link
Collaborator Author

jrodbx commented Aug 10, 2021

Merging for now, can address the psiMethod null check discussion in a follow-up.

@jrodbx jrodbx merged commit 8537da2 into master Aug 10, 2021
@jrodbx jrodbx deleted the jrod/2021-08-08/lint-kotlin branch August 10, 2021 03:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants