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.

Sign up for GitHub

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

Update Lint checks for Kotlin consumers #432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 10 additions & 6 deletions timber-lint/src/main/java/timber/lint/WrongTimberUsageDetector.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,10 @@ class WrongTimberUsageDetector : Detector(), UastScanner {
val methodName = node.methodName
val evaluator = context.evaluator

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

) {
checkNestedStringFormat(context, node)
return
}
Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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?

Pattern.matches(TIMBER_TREE_LOG_METHOD_REGEXP, psiMethod.name)
&& isTimberLogMethod(psiMethod, context.evaluator)
) {
context.report(
Incident(
Expand Down Expand Up @@ -615,15 +619,15 @@ class WrongTimberUsageDetector : Detector(), UastScanner {
3 -> {
val msg = arguments[1]
val throwable = arguments[2]
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

}
else -> {
throw IllegalStateException("android.util.Log overloads should have 2 or 3 arguments")
}
}

val logCallSource = logCall.asSourceString()
val logCallSource = logCall.uastParent!!.sourcePsi?.text
return fix().group()
.add(
fix().replace().text(logCallSource).shortenNames().reformat(true).with(fixSource1).build()
Expand Down
Loading