UnusedPrivateProperty improvements and refactoring out new UnusedVariable rule by psuzn · Pull Request #7089 · detekt/detekt

psuzn · 2024-03-25T10:18:07Z

Fixes #6702 and refactors the unused variable rules into its own separate UnusedVariable rule.

psuzn · 2024-03-25T12:14:36Z

This is more or less done, but this breaks the unused variable checks (i.e. failng tests). The previous version of UnusedPrivateProperty was also doing checks for the unused variables. I can add those checks but it would be better to put checks for unused variables in on the seperate rule and keep this UnusedPrivateProperty rule just for unused private properties and constructor parameters.

@BraisGabin @3flex

BraisGabin · 2024-03-25T15:57:57Z

This is more or less done, but this breaks the unused variable checks (i.e. failng tests). The previous version of UnusedPrivateProperty was also doing checks for the unused variables. I can add those checks but it would be better to put checks for unused variables in on the seperate rule and keep this UnusedPrivateProperty rule just for unused private properties and constructor parameters.

@BraisGabin @3flex

I'm ok about splitting this rule in two.

And there is other thing here, we are moving another rule away from plain detekt. I'm ok with that in general. Correctness is better than speed. But I wanted just to make that clear for anyone looking at this PR.

BraisGabin · 2024-03-25T15:58:35Z

    @Configuration("unused property names matching this regex are ignored")
-    private val allowedNames: Regex by config("_|ignored|expected|serialVersionUID", String::toRegex)
+    private val allowedNames: Regex by config(
+        "_|ignored|expected|serialVersionUID",


If this rule only works with properties then _ should be removed.

psuzn · 2024-03-25T16:07:59Z

@BraisGabin how should I go with the unused variable rule, I can add it to this PR or do that in a new PR (we can create a new 3rd PR as a base for both).

Also, what should be the name for the new rule UnusedVariable?

cortinico · 2024-03-26T00:39:09Z

And there is other thing here, we are moving another rule away from plain detekt. I'm ok with that in general. Correctness is better than speed. But I wanted just to make that clear for anyone looking at this PR.

I'm also fine with it 👍
Thanks for taking a stab at this @psuzn

Also, what should be the name for the new rule UnusedVariable?

UnusedVariable sounds like a good one 👍

codecov · 2024-03-26T02:57:50Z

Codecov Report

Attention: Patch coverage is 89.68254% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 84.71%. Comparing base (7b5d42d) to head (02175cb).

Files	Patch %	Lines
...rbosch/detekt/rules/style/UnusedPrivateProperty.kt	88.57%	0 Missing and 8 partials ⚠️
...ab/arturbosch/detekt/rules/style/UnusedVariable.kt	90.90%	0 Missing and 5 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #7089      +/-   ##
============================================
+ Coverage     84.67%   84.71%   +0.04%     
- Complexity     3970     3975       +5     
============================================
  Files           575      576       +1     
  Lines         12051    12139      +88     
  Branches       2476     2497      +21     
============================================
+ Hits          10204    10284      +80     
  Misses          621      621              
- Partials       1226     1234       +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

detekt-ci · 2024-03-26T10:00:04Z

	Warnings
⚠️	This PR is approved with no milestone set. If merged, it won't appear in the detekt release notes.

	Messages
📖	Thanks for adding a new rule to detekt ❤️
✅	We detekted that you added tests, to your rule, that's awesome!
✅	We detekted that you updated the `default-detekt-config.yml` file, that's awesome!

Generated by 🚫 dangerJS against 02175cb

BraisGabin

Top level properties shouldn't be handled by UnusedVariables because they aren't variables, they are properties. That should remain on UnusedPrivateProperty.

Other than that the PR looks really good!

Off-topic: Do we have a rule to check that all the parameters of a lambda are used? 🤔 I thought we have but I can't find it.

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

…ate-property' into false-negative-unused-private-property

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

BraisGabin

Add one test more and this is good to go for me :) really good job! Thank you!

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

BraisGabin · 2024-04-14T16:33:46Z

@psuzn can you rebase/merge your pr with main? It has conflicts. After that this is ready to merge.

…-private-property

psuzn · 2024-04-14T17:05:20Z

should be good to go.

detekt-ci added the rules label Mar 25, 2024

github-advanced-security AI found potential problems Mar 25, 2024

View reviewed changes

vercel Bot deployed to Preview March 25, 2024 10:26 View deployment

github-advanced-security AI found potential problems Mar 25, 2024

View reviewed changes

psuzn force-pushed the false-negative-unused-private-property branch from fd6e6d9 to ad20020 Compare March 25, 2024 12:35

vercel Bot deployed to Preview March 25, 2024 12:45 View deployment

BraisGabin reviewed Mar 25, 2024

View reviewed changes

vercel Bot deployed to Preview March 26, 2024 03:05 View deployment

vercel Bot deployed to Preview March 26, 2024 04:41 View deployment

vercel Bot deployed to Preview March 26, 2024 04:51 View deployment

github-advanced-security AI found potential problems Mar 26, 2024

View reviewed changes

vercel Bot deployed to Preview March 26, 2024 10:06 View deployment

vercel Bot deployed to Preview March 26, 2024 11:56 View deployment

vercel Bot deployed to Preview March 26, 2024 13:14 View deployment

psuzn force-pushed the false-negative-unused-private-property branch from 7cb02ac to 42b5e78 Compare March 26, 2024 14:53

vercel Bot deployed to Preview March 26, 2024 15:00 View deployment

psuzn changed the title ~~fix for unused private property in same file~~ UnusedPrivateProperty improvements and refactoring out new UnusedVariable rule Mar 26, 2024

psuzn added 6 commits March 26, 2024 21:10

wip: fix for unused private property in same file

00db0a5

minor refactor

3140be5

remove not applicable tests

6570ae5

remove '_' from allowed names

7006e16

compilation fixes

c1f3ec7

replace compileAndLintWithContext with lintWithContext

c42d17c

psuzn added 4 commits March 26, 2024 21:10

UnusedVariable rule

135bc83

tests for lamda

42e924f

type resolution annotation

3aebf92

unused property parameter false +ve

8e3c445

psuzn force-pushed the false-negative-unused-private-property branch from 42b5e78 to 8e3c445 Compare March 26, 2024 15:25

vercel Bot deployed to Preview March 26, 2024 15:31 View deployment

psuzn marked this pull request as ready for review March 26, 2024 15:46

psuzn requested a review from BraisGabin March 26, 2024 15:46

BraisGabin mentioned this pull request Mar 27, 2024

Demo false negative for UnusedPrivateProperty #6701

Closed

BraisGabin reviewed Mar 27, 2024

View reviewed changes

psuzn and others added 4 commits March 27, 2024 19:12

Apply suggestions from code review

931fdce

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

moved top level property check to UnusedPrivateProperty

40e70df

Merge remote-tracking branch 'origin-psuzn/false-negative-unused-priv…

c7f31f3

…ate-property' into false-negative-unused-private-property

Apply suggestions from code review

b20d16e

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

vercel Bot deployed to Preview March 27, 2024 13:48 View deployment

psuzn requested a review from BraisGabin March 27, 2024 14:05

BraisGabin reviewed Mar 28, 2024

View reviewed changes

Apply suggestions from code review

f482f45

Co-authored-by: Brais Gabín <braisgabin@gmail.com>

vercel Bot deployed to Preview March 28, 2024 10:22 View deployment

add support for destructuring declearation

c1d1748

vercel Bot deployed to Preview March 28, 2024 10:59 View deployment

BraisGabin approved these changes Mar 28, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into false-negative-unused…

02175cb

…-private-property

vercel Bot deployed to Preview April 14, 2024 16:49 View deployment

BraisGabin merged commit dfda4e0 into detekt:main Apr 14, 2024

cortinico added this to the 2.0.0 milestone Apr 14, 2024

Uh oh!

Conversation

psuzn commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psuzn commented Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BraisGabin commented Mar 25, 2024

Uh oh!

BraisGabin Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

psuzn Mar 25, 2024

Choose a reason for hiding this comment

Uh oh!

psuzn commented Mar 25, 2024

Uh oh!

cortinico commented Mar 26, 2024

Uh oh!

codecov Bot commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

detekt-ci commented Mar 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BraisGabin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BraisGabin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BraisGabin commented Apr 14, 2024

Uh oh!

psuzn commented Apr 14, 2024

Uh oh!

Reviewers

Assignees

Labels

psuzn commented Mar 25, 2024 •

edited

Loading

psuzn commented Mar 25, 2024 •

edited

Loading

codecov Bot commented Mar 26, 2024 •

edited

Loading

detekt-ci commented Mar 26, 2024 •

edited

Loading