AnnotationSuppressor now resolves Full Qualified Annotation Names without type solving #4570

BraisGabin · 2022-02-06T22:16:56Z

This was way more difficult than I expected. Please, review this PR commit by commit.

The more complex type of annotations are like this @com.dagger.Component.Factory. We have a part that is package and another part that are class names. But it's not clear when one ends and one the other ends. This is importan because when we find for matches for this type of annotations we should allow:

@com.dagger.Component.Factory
@Component.Factory if import com.dagger.Component is present
@Factory if import com.dagger.Component.Factory is present

But we should not allow @dagger.Component.Factory, because that's another annotation different. It can't be @com.dagger.Component.Factory. All this cases is what makes this code complex and that force me to have all those tests.

fixes #4355
fixes #4356

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt

…he callers

…ving

codecov · 2022-02-06T22:53:53Z

Codecov Report

Merging #4570 (30ce1c7) into main (1e406db) will increase coverage by 84.56%.
The diff coverage is 93.75%.

@@             Coverage Diff             @@
##             main    #4570       +/-   ##
===========================================
+ Coverage        0   84.56%   +84.56%     
- Complexity      0     3340     +3340     
===========================================
  Files           0      481      +481     
  Lines           0    11186    +11186     
  Branches        0     2038     +2038     
===========================================
+ Hits            0     9459     +9459     
- Misses          0      697      +697     
- Partials        0     1030     +1030

Impacted Files	Coverage Δ
...ch/detekt/core/suppressors/AnnotationSuppressor.kt	`94.44% <80.00%> (ø)`
...gitlab/arturbosch/detekt/api/AnnotationExcluder.kt	`91.89% <91.42%> (ø)`
...ub/detekt/psi/internal/FullQualifiedNameGuesser.kt	`90.00% <92.30%> (ø)`
...bosch/detekt/rules/complexity/LongParameterList.kt	`85.71% <100.00%> (ø)`
...tlab/arturbosch/detekt/rules/bugs/LateinitUsage.kt	`92.59% <100.00%> (ø)`
...etekt/rules/style/FunctionOnlyReturningConstant.kt	`95.74% <100.00%> (ø)`
...sch/detekt/rules/style/UnnecessaryAbstractClass.kt	`87.50% <100.00%> (ø)`
...tlab/arturbosch/detekt/rules/style/UseDataClass.kt	`81.08% <100.00%> (ø)`
...lin/io/github/detekt/tooling/api/spec/RulesSpec.kt	`100.00% <0.00%> (ø)`
...h/detekt/formatting/wrappers/SpacingAroundComma.kt	`100.00% <0.00%> (ø)`
... and 479 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1e406db...30ce1c7. Read the comment docs.

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt

cortinico · 2022-02-09T00:11:01Z

This was way more difficult than I expected. Please, review this PR commit by commit.

I went through them and I lack the relevant context as I haven't really touched this part of the codebase in a while. The code looks good and well tested. Generally, I think we're good to go with the heuristic you suggested.

detekt-psi-utils/src/test/kotlin/io/github/detekt/psi/internal/FullQualifiedNameGuesserSpec.kt

schalkms · 2022-02-11T13:12:20Z

detekt-psi-utils/src/main/kotlin/io/github/detekt/psi/internal/FullQualifiedNameGuesser.kt

@@ -39,13 +39,16 @@ class FullQualifiedNameGuesser internal constructor(
                    if (packageName != null) {
                        add("$packageName.$name")
                    }
+                    if (name.first().isLowerCase()) {


What's the reason for this check?

For that reason we use a heuristic here: If the first character is lower case we assume it's a package name. Therefore I think this is to handle cases like dagger\..*. First uppercase indicates a class name and Component\..* would not be a valid case.

Sorry @schalkms, I didn't see this comment before.

Yes the reason was that heuristic... but now that I think about that I'm not sure if this class needs to know about this. We could assume that any string that we can't match with an import could be a FullQualifiedName. But I don't know if that's better or worse... That's the issue with this mixed behaviour. The "general use cases" are easy to manage but all this edge cases are really difficult to manage because we can't know for sure.

schalkms · 2022-02-11T13:16:34Z

The drawback of having this mixed implementation for enabled and disabled type resolution is the complexity of the code. However, for now, I don't know a better solution.

3flex · 2022-02-11T21:54:16Z

The drawback of having this mixed implementation for enabled and disabled type resolution is the complexity of the code

Agree. This approach will lead to significant maintenance if for every rule or feature that's improved by using type resolution we have to also support users who choose not to use type resolution.

Is the real issue that type resolution is too difficult to use correctly? What can we do here to avoid introducing this complexity to our code?

BraisGabin · 2022-02-14T01:58:02Z

Is the real issue that type resolution is too difficult to use correctly? What can we do here to avoid introducing this complexity to our code?

As far as I know the main issue is the performance. We should rework our gradle plugin to make it type solve first.

cortinico · 2022-02-15T09:48:48Z

As far as I know the main issue is the performance

This ☝️ . We have users running detekt on pre-commit hooks. Enabling TR for them is a no-go as they'll end up impacting the development loop for a lot of engineers.

I think we should keep the current feature set for non-TR enabled project and add new features that are working with TR only.

The problem in this PR/Linked issues was that, by implementing a more advanced annotation suppressor, we actually broke the experience for users on non-TR project.

cortinico · 2022-02-19T16:33:55Z

Anything missing from this PR? I'd like to kickoff a RC1 as soon as this is merged 👍

BraisGabin · 2022-02-20T09:01:39Z

This is ready to merge for me. But, is this a notable change or just a bug fix?

chao2zhang

lgtm! Thank you for adding those test cases and that really helped me to understand

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt

chao2zhang · 2022-02-20T15:57:22Z

detekt-psi-utils/src/main/kotlin/io/github/detekt/psi/internal/FullQualifiedNameGuesser.kt

@@ -39,13 +39,16 @@ class FullQualifiedNameGuesser internal constructor(
                    if (packageName != null) {
                        add("$packageName.$name")
                    }
+                    if (name.first().isLowerCase()) {


For that reason we use a heuristic here: If the first character is lower case we assume it's a package name. Therefore I think this is to handle cases like dagger\..*. First uppercase indicates a class name and Component\..* would not be a valid case.

detekt-api/src/test/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluderSpec.kt

…/FullQualifiedNameGuesserSpec.kt Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com>

cortinico · 2022-02-22T05:52:36Z

This is ready to merge for me. But, is this a notable change or just a bug fix?

It's a notable change that we fixed a regression in the latest stable (imho this would have been point-release worthy, we skipped that so let's at least mention this on top of the release notes).

BraisGabin added this to the 1.20.0 milestone Feb 6, 2022

BraisGabin commented Feb 6, 2022

View reviewed changes

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt Show resolved Hide resolved

Simplify AnnotationExcluderSpec

e81fb00

BraisGabin force-pushed the improve-annotation-suppresor branch from c42310a to fcb3f23 Compare February 6, 2022 22:33

BraisGabin mentioned this pull request Feb 6, 2022

Integrated former method for looking up annotation names in AnnotationSuppressor #4358

Closed

BraisGabin added 5 commits February 6, 2022 23:47

Improve tests

b8ba1f1

Improve tests

2c02374

Fix FullQualifiedNameGuesser

3ddefba

Allow List<Regex> inside AnnotationExcluder to give more options to t…

95d2bc0

…he callers

Test with and without type-solving

9e1e90b

BraisGabin force-pushed the improve-annotation-suppresor branch from fcb3f23 to 1db1b8e Compare February 6, 2022 22:47

BraisGabin added 2 commits February 6, 2022 23:51

Make AnnotationExcluder use the BindingContext when available

de08d9c

AnnotationSuppressor works with Full Qualified names without type sol…

320a26e

…ving

BraisGabin force-pushed the improve-annotation-suppresor branch from 1db1b8e to 320a26e Compare February 6, 2022 22:51

cortinico reviewed Feb 9, 2022

View reviewed changes

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt Show resolved Hide resolved

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/AnnotationExcluder.kt Outdated Show resolved Hide resolved

Address PR comments

1908a88

schalkms approved these changes Feb 11, 2022

View reviewed changes

detekt-psi-utils/src/test/kotlin/io/github/detekt/psi/internal/FullQualifiedNameGuesserSpec.kt Outdated Show resolved Hide resolved

schalkms reviewed Feb 11, 2022

View reviewed changes

cortinico added the notable changes Marker for notable changes in the changelog label Feb 19, 2022

chao2zhang approved these changes Feb 20, 2022

View reviewed changes

BraisGabin and others added 2 commits February 21, 2022 09:23

Update detekt-psi-utils/src/test/kotlin/io/github/detekt/psi/internal…

d46e3a0

…/FullQualifiedNameGuesserSpec.kt Co-authored-by: M Schalk <30376729+schalkms@users.noreply.github.com>

Support star imports

550fb86

Improve readability

30ce1c7

BraisGabin force-pushed the improve-annotation-suppresor branch from 9638d52 to 30ce1c7 Compare February 21, 2022 23:18

chao2zhang approved these changes Feb 22, 2022

View reviewed changes

BraisGabin merged commit dd80d61 into main Feb 23, 2022

BraisGabin deleted the improve-annotation-suppresor branch February 23, 2022 23:03

TWiStErRob added the suppressors label Nov 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AnnotationSuppressor now resolves Full Qualified Annotation Names without type solving #4570

AnnotationSuppressor now resolves Full Qualified Annotation Names without type solving #4570

BraisGabin commented Feb 6, 2022 •

edited

codecov bot commented Feb 6, 2022 •

edited

cortinico commented Feb 9, 2022

schalkms Feb 11, 2022

chao2zhang Feb 20, 2022

BraisGabin Feb 21, 2022

schalkms commented Feb 11, 2022

3flex commented Feb 11, 2022

BraisGabin commented Feb 14, 2022

cortinico commented Feb 15, 2022

cortinico commented Feb 19, 2022

BraisGabin commented Feb 20, 2022

chao2zhang left a comment

chao2zhang Feb 20, 2022

cortinico commented Feb 22, 2022

AnnotationSuppressor now resolves Full Qualified Annotation Names without type solving #4570

AnnotationSuppressor now resolves Full Qualified Annotation Names without type solving #4570

Conversation

BraisGabin commented Feb 6, 2022 • edited

codecov bot commented Feb 6, 2022 • edited

Codecov Report

cortinico commented Feb 9, 2022

schalkms Feb 11, 2022

Choose a reason for hiding this comment

chao2zhang Feb 20, 2022

Choose a reason for hiding this comment

BraisGabin Feb 21, 2022

Choose a reason for hiding this comment

schalkms commented Feb 11, 2022

3flex commented Feb 11, 2022

BraisGabin commented Feb 14, 2022

cortinico commented Feb 15, 2022

cortinico commented Feb 19, 2022

BraisGabin commented Feb 20, 2022

chao2zhang left a comment

Choose a reason for hiding this comment

chao2zhang Feb 20, 2022

Choose a reason for hiding this comment

cortinico commented Feb 22, 2022

BraisGabin commented Feb 6, 2022 •

edited

codecov bot commented Feb 6, 2022 •

edited