Fix validity check of annotations on properties#2368
Conversation
Annotations on properties may come from corresponding parameters if they are declared in constructors. They may not be valid on properties and need to be filtered out. The previous implementation incorrectly excludes all annotations with @target(AnnotationTarget.VALUE_PARAMETER), while what should be excluded are the ones with @target and without @target(AnnotationTarget.PROPERTY).
74eaf11 to
d1efa11
Compare
|
|
||
| internal fun KSAnnotation.isValidOnProperty(): Boolean = | ||
| annotationType.resolve().declaration.annotations.none { metaAnnotation -> | ||
| metaAnnotation.annotationType.resolve().declaration.qualifiedName?.asString() == "kotlin.annotation.Target" && |
There was a problem hiding this comment.
Nit: maybe it's just me, but I prefer to avoid using constant as much as possible (in case of, e.g., typo or some other silly mistakes) :)
That said, this can be Target::class.qualifiedName? Same for AnnotationTarget.PROPERTY below. (But not necessarily in this change.)
There was a problem hiding this comment.
Will fix in later refactoring / polishing commits.
| @@ -0,0 +1,39 @@ | |||
| /* | |||
| * Copyright 2020 Google LLC | |||
| * Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors. | |||
There was a problem hiding this comment.
Just to double-check, do these intentionally stay in 2020, or is it a good time to update the template somewhere to make this copyright comment up-to-date?
| internal fun KSAnnotation.isValidOnProperty(): Boolean = | ||
| annotationType.resolve().declaration.annotations.none { metaAnnotation -> | ||
| metaAnnotation.annotationType.resolve().declaration.qualifiedName?.asString() == "kotlin.annotation.Target" && | ||
| (metaAnnotation.arguments.singleOrNull()?.value as? ArrayList<*>)?.none { |
There was a problem hiding this comment.
It was previously any, but now none. Is that expected?
Also, because of the inversion, now the util name sounds counter-intuitive. It is read like, if none of the annotation has non-Property target, it is valid on property?
There was a problem hiding this comment.
This is expected. The default behavior, i.e. without @Target on it, allows an annotation to be attached to a property. So an annotation is invalid on a property if and only if it has @Target that has no PROPERTY. This is equivalent to: an annotation is valid iff none of its meta annotations is "@Target without PROPERTY".
| metaAnnotation.annotationType.resolve().declaration.qualifiedName?.asString() == "kotlin.annotation.Target" && | ||
| (metaAnnotation.arguments.singleOrNull()?.value as? ArrayList<*>)?.none { | ||
| (it as? KSClassDeclaration)?.qualifiedName?.asString() == "kotlin.annotation.AnnotationTarget.PROPERTY" | ||
| } ?: false |
There was a problem hiding this comment.
In the code review, it's hard to understand where this elvis operator belongs...
I wonder if the aligned indentation would help? IIUC, this is:
(metaAnno...)?.none {
(it as? ...)
} ?: falsebut it's currently:
(metaAnno...)?.none {
(it as? ...)
} ?: false?
There was a problem hiding this comment.
I agree. This is a bug of ktlint.
| // FILE: com/exampel/MyClass.kt | ||
| package com.example | ||
|
|
||
| @Target(AnnotationTarget.PROPERTY, AnnotationTarget.VALUE_PARAMETER) |
There was a problem hiding this comment.
My confusion on isValidOnProperty made me wonder what happens if the annotation target doesn't have PROPERTY...
There was a problem hiding this comment.
If the target doesn't have PROPERTY, it is invalid and should not be included in KSPropertyDeclaration.annotations.
There was a problem hiding this comment.
btw, we have this case covered in https://github.com/google/ksp/blob/main/kotlin-analysis-api/testData/annotationInDependencies.kt#L115
Annotations on properties may come from corresponding parameters if they are declared in constructors. They may not be valid on properties and need to be filtered out. The previous implementation incorrectly excludes all annotations with @target(AnnotationTarget.VALUE_PARAMETER), while what should be excluded are the ones with @target and without @target(AnnotationTarget.PROPERTY).