ForbiddenAnnotation rule #2719#5515
Conversation
BraisGabin
left a comment
BraisGabin
left a comment
There was a problem hiding this comment.
Good contribution! I really like this one!
I'm blocking this PR because we are really near to cut a new version and it's a bit late to add a new rule to it after 3 RC but it should be inside the next version :)
|
@BraisGabin we can unblock this, right? Unless we need to keep |
|
No need to block it any longer. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report
@@ Coverage Diff @@
## main #5515 +/- ##
===========================================
+ Coverage 0 85.84% +85.84%
- Complexity 0 3640 +3640
===========================================
Files 0 516 +516
Lines 0 12179 +12179
Branches 0 2176 +2176
===========================================
+ Hits 0 10455 +10455
- Misses 0 630 +630
- Partials 0 1094 +1094
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| * <noncompliant> | ||
| * @@SuppressWarnings("unused") | ||
| * class SomeClass() | ||
| * </noncompliant> |
There was a problem hiding this comment.
I would keep both a compliant and a noncompliant block here.
Maybe show the @Deprecated one or others
ov7a
force-pushed
the
forbidden-annotation
branch
from
7ce3ebd to
3e0bcaf
Compare
TWiStErRob
left a comment
TWiStErRob
left a comment
There was a problem hiding this comment.
Sorry, every time I look, I'm in a different mindset :)
Most comments are preference now, take them as you wish.
| if (annotations.isEmpty()) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Is this permature optimization? (same in above method) why would this rule be active if there are no configured annotations. @BraisGabin /@cortinico is there a way to easily check this during configuration phase? Can ConfigValidator be on a rule level? (btw. ConfigValidator has no docs.)
There was a problem hiding this comment.
I agree, there's not need for this if. I don't care that much about it either... but if we have this ifs we should also have a test to check what happen with annotations is empty. So I would say that it's better to remove it. But if you, @ov7a, prefer to keep it just add a test for it.
There was a problem hiding this comment.
I just removed it. I agree with @TWiStErRob that it would be nice to have some example of config validation.
| private fun KtTypeReference.fqNameOrNull(): FqName? { | ||
| return if (bindingContext != BindingContext.EMPTY) { | ||
| bindingContext[BindingContext.TYPE, this]?.fqNameOrNull() | ||
| } else { | ||
| null | ||
| } | ||
| } | ||
|
|
||
| private fun KtExpression.expressionTypeOrNull(): KotlinType? { | ||
| return if (bindingContext != BindingContext.EMPTY) { | ||
| bindingContext[BindingContext.EXPRESSION_TYPE_INFO, this]?.type | ||
| } else { | ||
| null | ||
| } | ||
| } |
There was a problem hiding this comment.
@RequiresTypeResolution guarantees non-empty binding context. @BraisGabin this looks like another one for the rule-authors ruleset.
| private fun KtTypeReference.fqNameOrNull(): FqName? { | |
| return if (bindingContext != BindingContext.EMPTY) { | |
| bindingContext[BindingContext.TYPE, this]?.fqNameOrNull() | |
| } else { | |
| null | |
| } | |
| } | |
| private fun KtExpression.expressionTypeOrNull(): KotlinType? { | |
| return if (bindingContext != BindingContext.EMPTY) { | |
| bindingContext[BindingContext.EXPRESSION_TYPE_INFO, this]?.type | |
| } else { | |
| null | |
| } | |
| } | |
| private fun KtTypeReference.fqNameOrNull(): FqName? = | |
| bindingContext[BindingContext.TYPE, this]?.fqNameOrNull() | |
| private fun KtExpression.expressionTypeOrNull(): KotlinType? = | |
| bindingContext[BindingContext.EXPRESSION_TYPE_INFO, this]?.type |
There was a problem hiding this comment.
Yes, I think that any check of bindingContext != BindingContext.EMPTY or bindingContext == BindingContext.EMPTY should be flagged as a code smell. That should be an easy rule to implement. @TWiStErRob do you want to create the issue for it (or the PR)
Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
Co-authored-by: Róbert Papp <papp.robert.s@gmail.com>
ov7a
force-pushed
the
forbidden-annotation
branch
from
414ffad to
83bc85f
Compare
5 participants
As discussed in #2719, this rule could be sometimes useful for cases when an annotation does not require import:
java.lang.SuppressWarnings,kotlin.jvm.Transient, etc. These cases can't be caught with theForbiddenImportrule.Also, this rule can be useful for pushing people towards the Kotlin implementations (see this issue for an example).
More specifically, forbidding
kotlin.jvm.Transientcan be useful in applications withkotlinx.serialization:kotlin.jvm.Transientdoes not affect@Serializableclasses.ForbiddenAnnotationcan be a quick-fix for that.