Use kotlinx-metadata for more accurate Kotlin checks #75
Conversation
|
|
||
| /** Flag to disable as needed. */ | ||
| private val checkMetadata = System.getProperty("slack.lint.checkMetadata", "true").toBoolean() | ||
| private val cachedClasses = ConcurrentHashMap<String, Optional<KmClass>>() |
There was a problem hiding this comment.
Do we need Optional here? Couldn't you just use KmClass?
There was a problem hiding this comment.
We do want presence modeled because if it's empty/null, we can also use that as signal to know that we already looked and there was none
| // region Delegating functions | ||
| override val dependencies: LintModelDependencies? | ||
| get() = delegate.dependencies | ||
|
|
||
| override fun extendsClass(cls: PsiClass?, className: String, strict: Boolean): Boolean = | ||
| delegate.extendsClass(cls, className, strict) | ||
|
|
||
| override fun findAnnotation( | ||
| listOwner: PsiModifierListOwner?, | ||
| vararg annotationNames: String | ||
| ): PsiAnnotation? = delegate.findAnnotation(listOwner, *annotationNames) | ||
|
|
||
| override fun findAnnotationInHierarchy( | ||
| listOwner: PsiModifierListOwner, | ||
| vararg annotationNames: String | ||
| ): PsiAnnotation? = delegate.findAnnotationInHierarchy(listOwner, *annotationNames) | ||
|
|
||
| override fun findClass(qualifiedName: String): PsiClass? = delegate.findClass(qualifiedName) | ||
|
|
||
| override fun findJarPath(element: PsiElement): String? = delegate.findJarPath(element) | ||
|
|
||
| override fun findJarPath(element: UElement): String? = delegate.findJarPath(element) | ||
|
|
||
| override fun getAllAnnotations( | ||
| owner: PsiModifierListOwner, | ||
| inHierarchy: Boolean | ||
| ): Array<PsiAnnotation> = delegate.getAllAnnotations(owner, inHierarchy) | ||
|
|
||
| override fun getAllAnnotations(owner: UAnnotated, inHierarchy: Boolean): List<UAnnotation> = | ||
| delegate.getAllAnnotations(owner, inHierarchy) | ||
|
|
||
| override fun getAnnotation( | ||
| listOwner: PsiModifierListOwner?, | ||
| vararg annotationNames: String | ||
| ): UAnnotation? = delegate.getAnnotation(listOwner, *annotationNames) | ||
|
|
||
| override fun getAnnotationInHierarchy( | ||
| listOwner: PsiModifierListOwner, | ||
| vararg annotationNames: String | ||
| ): UAnnotation? = delegate.getAnnotationInHierarchy(listOwner, *annotationNames) | ||
|
|
||
| override fun getAnnotations( | ||
| owner: PsiModifierListOwner?, | ||
| inHierarchy: Boolean, | ||
| parent: UElement? | ||
| ): List<UAnnotation> = delegate.getAnnotations(owner, inHierarchy, parent) | ||
|
|
||
| override fun getClassType(psiClass: PsiClass?): PsiClassType? = delegate.getClassType(psiClass) | ||
|
|
||
| override fun getPackage(node: PsiElement): PsiPackage? = delegate.getPackage(node) | ||
|
|
||
| override fun getPackage(node: UElement): PsiPackage? = delegate.getPackage(node) | ||
|
|
||
| override fun getTypeClass(psiType: PsiType?): PsiClass? = delegate.getTypeClass(psiType) | ||
|
|
||
| override fun implementsInterface(cls: PsiClass, interfaceName: String, strict: Boolean): Boolean = | ||
| delegate.implementsInterface(cls, interfaceName, strict) | ||
| // endregion |
There was a problem hiding this comment.
These all look to be calling straight through to the JavaEvaluator. Did you consider delegating to the delegate (in the constructor) so that none of this is needed?
class SlackJavaEvaluator(private val file: String, private val delegate: JavaEvaluator) : JavaEvaluator() by delegate {
// first override
override fun hasModifier(...) { ... }
}
There was a problem hiding this comment.
You can't do class delegation with classes, only interfaces :/
There was a problem hiding this comment.
Well that's unfortunate :-\
| // Don't use getOrPut. Kotlin's extension may still invoke the body and we don't want that | ||
| .computeIfAbsent(qualifiedName!!) { key -> | ||
| val annotation = | ||
| cls.findAnnotation("kotlin.Metadata") ?: return@computeIfAbsent Optional.empty() |
There was a problem hiding this comment.
Same here. It seems like you should be able to return@computeIfAbsent null here
There was a problem hiding this comment.
I dislike returns with labels and do my best to avoid them when possible (I think they negatively affect readability). It isn't a huge deal (I understand not everyone feels this way), but did you consider something like this:
var metadata = cachedClasses[qualifiedName!!]
if (metadata == null) {
val annotation = cls.findAnnotation("kotlin.Metadata") ?: return null
val (durationMillis, result) = measureTimeMillisWithResult { annotation.parseMetadata(key) }
slackLintLog("Took ${durationMillis}ms to parse metadata for $key.")
metadata = result
}
return metadata
There was a problem hiding this comment.
See above for the nullability issue
For computeIfAbsent - we have to use it because it guarantees the currency safety and that the block is only computed once. The code in your snippet is actually more or less what kotlin's ConcurrentHashmap.getOrPut does, which is actually racy and could call the computation block more than once. See the comment above this call too :P
There was a problem hiding this comment.
Ahh yes 👍 . I read the docs for getOrPut but didn't think to look at the docs for computeIfAbsent
| return when (metadata) { | ||
| is KotlinClassMetadata.Class -> metadata.toKmClass() | ||
| is KotlinClassMetadata.FileFacade -> null | ||
| is KotlinClassMetadata.MultiFileClassFacade -> null | ||
| is KotlinClassMetadata.MultiFileClassPart -> null | ||
| is KotlinClassMetadata.SyntheticClass -> null | ||
| is KotlinClassMetadata.Unknown -> null | ||
| null -> { | ||
| slackLintLog("Could not load metadata for $classNameHint from file $file") | ||
| null | ||
| } |
There was a problem hiding this comment.
There's a lot going on in this method. Consider extracting this when to a helper
There was a problem hiding this comment.
Cleaned it up in 26cdd85
Resolves #53
This implements a new decorating
SlackJavaEvaluatorthat leverages KotlinMetadatato understand kotlin langauge features of externally compiled elements. This is important as these elements otherwise appear to lint as just plain Java classes. I've done a bunch of testing with this in the Slack Android repo, and the timings are always ~1ms, rarely exceeding it to 2ms. Just in case though, I've gated this behavior with a system property.Currently this is only used in the DoNotMock checkers, but is usable in other future checkers.