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

UnusedPrivateClass: don't report imported classes #2812

Merged
merged 1 commit into from
Jun 21, 2020

Conversation

t-kameyama
Copy link
Contributor

Fixes #2809

@codecov
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #2812 into master will increase coverage by 0.00%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2812   +/-   ##
=========================================
  Coverage     80.52%   80.52%           
- Complexity     2323     2335   +12     
=========================================
  Files           386      387    +1     
  Lines          6957     6993   +36     
  Branches       1262     1269    +7     
=========================================
+ Hits           5602     5631   +29     
  Misses          726      726           
- Partials        629      636    +7     
Impacted Files Coverage Δ Complexity Δ
...rturbosch/detekt/rules/style/UnusedPrivateClass.kt 65.67% <57.14%> (-1.55%) 4.00 <0.00> (ø)
...arturbosch/detekt/rules/naming/IsPropertyNaming.kt 85.18% <0.00%> (ø) 12.00% <0.00%> (?%)
...tlab/arturbosch/detekt/rules/naming/NamingRules.kt 93.33% <0.00%> (+0.35%) 18.00% <0.00%> (ø%)

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 0649ece...e953cdd. Read the comment docs.

@t-kameyama t-kameyama force-pushed the issue_2809 branch 3 times, most recently from 6d7d35e to bd3561b Compare June 20, 2020 13:33
Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

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

Thanks!

@arturbosch arturbosch added this to the 1.10.0 milestone Jun 20, 2020
Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! Looks good!

@schalkms schalkms merged commit 82108b7 into detekt:master Jun 21, 2020
@t-kameyama t-kameyama deleted the issue_2809 branch June 21, 2020 08:26
private fun KtNamedDeclaration.isUsed(): Boolean {
if (nameAsSafeName.identifier in namedClasses) return true
val fqName = fqName?.asString()
return fqName != null && importDirectives.any { it.startsWith(fqName) }
Copy link
Member

Choose a reason for hiding this comment

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

@t-kameyama @arturbosch @schalkms isn't this a counter-example for startsWith?

import foo.bar.Baz
import foo.bar.Bazinga
fun f() {
    Baz()
}

IRL Bazinga could be generated Baz_Factory for example.

Copy link
Member

Choose a reason for hiding this comment

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

oh, it's only about private classes..., but there must be a similar example for that too.
e.g. a private class was renamed, but the original import not removed, but detekt wouldn't find it?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, the import must start with the fully qualified name of the private class not the other way. So this should be okay?
I'm not sure if I understand your example.

package foo.bar
                import foo.bar.Baz
                import foo.bar.Bazinga
                private class BazFactory

Copy link
Contributor Author

@t-kameyama t-kameyama Jun 21, 2020

Choose a reason for hiding this comment

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

False negative:

package com.example

import com.example.C.EFG.EFG1

class C {
    fun test() {
        println(EFG1)
    }

    private enum class E { // false negative
        E1
    }

    private enum class EFG {
        EFG1
    }
}

Copy link
Member

@TWiStErRob TWiStErRob Jun 22, 2020

Choose a reason for hiding this comment

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

Yep, that's what I meant.

Also be careful with nested classes inside nested classes, they might be broken now or after a fix of the false negative too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive UnusedPrivateClass when importing private enum constants directly.
4 participants