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.

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

UnusedImports: fix false positive for unresolved imports #4882

Merged
merged 1 commit into from Jul 16, 2022

Conversation

dzirbel
Copy link
Contributor

@dzirbel dzirbel commented May 31, 2022

Fix a false positive where UnusedImports would report extension function imports whose signature cannot be resolved by the BindingContext. This can happen for autogenerated classes, etc. (especially Android UI binding classes and the like).

To do this we need to generate text matchers for all the references which cannot be resolved and check if there are any unresolved matches to them, similar to the approach without type resolution.

Along the way, extract mappings inside KtImportDirective.isNotUsed() to lazy properties to avoid recalculating them for each import directive. Also make sure to map to a set for constant rather than linear lookup time.

@github-actions github-actions bot added the rules label Jun 1, 2022
@codecov
Copy link

codecov bot commented Jun 1, 2022

Codecov Report

Merging #4882 (1ab8b53) into main (f66a321) will increase coverage by 0.01%.
The diff coverage is 86.66%.

@@             Coverage Diff              @@
##               main    #4882      +/-   ##
============================================
+ Coverage     84.81%   84.82%   +0.01%     
  Complexity     3463     3463              
============================================
  Files           493      493              
  Lines         11357    11365       +8     
  Branches       2090     2093       +3     
============================================
+ Hits           9632     9640       +8     
  Misses          675      675              
  Partials       1050     1050              
Impacted Files Coverage Δ
...lab/arturbosch/detekt/rules/style/UnusedImports.kt 90.80% <86.66%> (+0.93%) ⬆️

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 f66a321...1ab8b53. Read the comment docs.

Fix a false positive where UnusedImports would report extension function imports whose signature cannot be resolved by the BindingContext. This can happen for autogenerated classes, etc. (especially Android UI binding classes and the like).

To do this we need to generate text matchers for all the references which cannot be resolved and check if there are any unresolved matches to them, similar to the approach without type resolution.

Along the way, extract mappings inside `KtImportDirective.isNotUsed()` to lazy properties to avoid recalculating them for each import directive. Also make sure to map to a set for constant rather than linear lookup time.
@cortinico
Copy link
Member

cortinico commented Jun 1, 2022

Code looks good, but I have to mention that I'm not a super big fan of this 🤔 I believe it might introduce more false positives than how much benefit it provides.

@dzirbel
Copy link
Contributor Author

dzirbel commented Jun 1, 2022

To be clear, I think this could introduce false negatives but not false positives that I can think of (meaning this change would make it possible for to detekt to miss issues but wouldn't make detekt report non-issues - that could be a positive/negative depending on your POV). But this issue has prevented my team from using this rule with type resolution since currently it marks hundreds of such imports as unused because detekt can't resolve them - we can still use NoUnusedImports from ktlint since the names match.

I'm not sure how else to approach this issue; if we have an import x.y.z.foo and somewhere in the code we are using foo() in some way, I'd much prefer detekt not to warn on that even if it can't be certain that it's a correct usage - but either approach could potentially be considered "correct". Perhaps one alternative is to add this functionality as a configuration option, but that could be confusing.

Regardless, I do want to make sure the changes to extract the sets to by lazy and mapping to a Set are included since those are very large performance improvements I believe - happy to extract that to a different PR if you'd prefer.

@BraisGabin
Copy link
Collaborator

BraisGabin commented Jun 1, 2022

I'm wondering how ktlint does this. I have this rule disabled from detekt because I use ktlint for this issue and I never had any false positive or false negative.

@dzirbel
Copy link
Contributor Author

dzirbel commented Jun 2, 2022

I assume that ktlint does string usage matching, the same as detekt without type resolution (or this change for when type resolution is enabled but can't resolve an import). The source seems to suggest the same - reference identifier text minus backticks is added to ref which is eventually compared via ref.map { it.text }.contains(name) for imports.

@BraisGabin
Copy link
Collaborator

BraisGabin commented Jul 6, 2022

We should decide what to do with this. My point of view: this is a workaround for other bug: We don't get a complete ContextBinding so we have this type of false positives. But that is a long standing bug (it's there since we introduced Type solving) and we don't have a clear plan to fix it in the near future.

So, to me, we should merge this, a false-positive is way worst than a false-negative. And we should also focus on fixing that long stading bug to avoid this type of workarounds.

Copy link
Sponsor Member

@chao2zhang chao2zhang left a comment

I believe this issue will be resolved once we are using K2 compiler plugin.

* All [namedReferences] whose [KtReferenceExpression.fqNameOrNull] cannot be resolved
* mapped to their text. String matches to such references shouldn't be marked as unused
* imports since they could match the unknown value being imported.
Copy link
Sponsor Member

@chao2zhang chao2zhang Jul 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 documentation.

@cortinico cortinico merged commit 02739c2 into detekt:main Jul 16, 2022
20 checks passed
VitalyVPinchuk pushed a commit to VitalyVPinchuk/detekt that referenced this pull request Jul 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants