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 rule: fix false positives when type resolution is enabled #3079

Merged
merged 4 commits into from
Sep 25, 2020

Conversation

t-kameyama
Copy link
Contributor

@t-kameyama t-kameyama commented Sep 16, 2020

Closes #3074

@codecov
Copy link

codecov bot commented Sep 16, 2020

Codecov Report

Merging #3079 into master will increase coverage by 0.00%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #3079   +/-   ##
=========================================
  Coverage     79.26%   79.26%           
- Complexity     2563     2567    +4     
=========================================
  Files           432      434    +2     
  Lines          7726     7737   +11     
  Branches       1469     1470    +1     
=========================================
+ Hits           6124     6133    +9     
- Misses          816      817    +1     
- Partials        786      787    +1     
Impacted Files Coverage Δ Complexity Δ
...lab/arturbosch/detekt/rules/style/UnusedImports.kt 84.12% <66.66%> (-2.76%) 4.00 <0.00> (ø)
...rturbosch/detekt/core/settings/EnvironmentAware.kt 63.15% <0.00%> (ø) 0.00% <0.00%> (ø%)
...ch/detekt/formatting/wrappers/AnnotationSpacing.kt 100.00% <0.00%> (ø) 2.00% <0.00%> (?%)
...detekt/formatting/wrappers/ArgumentListWrapping.kt 100.00% <0.00%> (ø) 2.00% <0.00%> (?%)
...ab/arturbosch/detekt/formatting/KtLintMultiRule.kt 92.30% <0.00%> (+0.24%) 8.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 1c6a7ea...2f35b03. Read the comment docs.

@3flex
Copy link
Member

3flex commented Sep 16, 2020

Thanks @t-kameyama, that's a big improvement! Unfortunately I still have some false positives with type resolution enabled.

Here com.squareup.sqldelight.android.AndroidSqliteDriver is shown as unused, but it's not.

import androidx.sqlite.db.SupportSQLiteDatabase
import androidx.sqlite.db.SupportSQLiteOpenHelper
import com.squareup.sqldelight.android.AndroidSqliteDriver

internal class DbOpenHelper : SupportSQLiteOpenHelper.Callback(DATABASE_VERSION) {

    override fun onCreate(db: SupportSQLiteDatabase) {
        val driver = AndroidSqliteDriver(db)
        Database.Schema.create(driver)
    }
}

I also have annotations in another file which are used, but they're still being reported.

@t-kameyama
Copy link
Contributor Author

@3flex
Fixed in 8ace339

@3flex
Copy link
Member

3flex commented Sep 17, 2020

Awesome :) A couple more, where the reference resolves to a companion object...

Lots of code omitted here, but the import is reported when it's used like this:

import com.bluelinelabs.conductor.RouterTransaction

RouterTransaction.with(DomainListController())

That resolves to:

class RouterTransaction {
  companion object {

    @JvmStatic
    fun with(controller: Controller): RouterTransaction = RouterTransaction(controller)
  }
}

And another example, again, lots of code omitted:

import okhttp3.ConnectionSpec

ConnectionSpec.MODERN_TLS
ConnectionSpec.COMPATIBLE_TLS

Both of those variables sit in a companion object.

@t-kameyama
Copy link
Contributor Author

@3flex
Fixed in 096c441

Copy link
Member

@3flex 3flex left a comment

Choose a reason for hiding this comment

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

Amazing! I can enable the rule with TR enabled now.

Possibly there will be other cases that pop up but this resolves my initial issue.

@cortinico
Copy link
Member

Thanks for the fix @t-kameyama
Would you be able to cherry pick #3077 so to make sure we're not introducing further regressions in the test suite for UnusedImports

@t-kameyama
Copy link
Contributor Author

@cortinico
Fixed in 2f35b03

@arturbosch
Copy link
Member

@cortinico we can close #3077 now, right?

@arturbosch arturbosch merged commit 8134e00 into detekt:master Sep 25, 2020
@arturbosch arturbosch added this to the 1.14.0 milestone Sep 25, 2020
@cortinico
Copy link
Member

@cortinico we can close #3077 now, right?

Yup 👌

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.

UnusedImports: Lots of false positives
5 participants