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

Fix by adding last method call to the set #6567

Merged
merged 2 commits into from Nov 26, 2023

Conversation

atulgpt
Copy link
Contributor

@atulgpt atulgpt commented Oct 26, 2023

Fixes #6540

.forEach { namedReferencesInKDoc.add(it.split(".")[0]) }
.forEach {
namedReferencesInKDoc.add(it.split(".")[0])
namedReferencesInKDoc.add(it.split(".").last())
Copy link
Contributor Author

@atulgpt atulgpt Oct 26, 2023

Choose a reason for hiding this comment

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

I have added the last method name in the set which will be matched against the import. This might in some scenarios produce false -ve but those cases will be kind of rare and fixing that might require some juggling with binding context to find the eligible last method call(whose extensions are already imported)

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (602b42a) 85.18% compared to head (890e919) 85.18%.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6567   +/-   ##
=========================================
  Coverage     85.18%   85.18%           
  Complexity     4061     4061           
=========================================
  Files           565      565           
  Lines         13321    13325    +4     
  Branches       2401     2401           
=========================================
+ Hits          11347    11351    +4     
  Misses          773      773           
  Partials       1201     1201           
Files Coverage Δ
...tlab/arturbosch/detekt/rules/style/UnusedImport.kt 93.18% <100.00%> (+0.32%) ⬆️

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

.forEach { namedReferencesInKDoc.add(it.split(".")[0]) }
.forEach {
namedReferencesInKDoc.add(it.split(".")[0])
namedReferencesInKDoc.add(it.split(".").last())
Copy link
Member

Choose a reason for hiding this comment

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

We could store the result of split on a variable to avoid the splitting twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the code

package com.example

import android.text.TextWatcher
import android.text.beforeTextChanged
Copy link
Member

Choose a reason for hiding this comment

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

This is the false negative that you were talking about, right?

Copy link
Contributor Author

@atulgpt atulgpt Oct 27, 2023

Choose a reason for hiding this comment

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

Hi @BraisGabin yes it is a false negative. There are two currently I can think of

@Test
fun `false -ve #1`() {
    val main = """
        package com.example

        import android.text.TextWatcher
        import android.text.beforeTextChanged

        class TestClass {
            /**
             * Below we are referring instance method no extension method
             * [TextWatcher.beforeTextChanged]
             */
            fun test() {
                TODO()
            }
        }
    """.trimIndent()
    val additional1 = """
        package android.text

        class TextWatcher {
            fun beforeTextChanged() {}
        }
    """.trimIndent()
    val additional2 = """
        package android.text

        fun TextWatcher.beforeTextChanged() {}
    """.trimIndent()
    assertThat(subject.lintWithContext(env, main, additional1, additional2)).isEmpty()
}

@Test
fun `false -ve #2`() {
    val main = """
        package com.example

        import android.text.TextWatcher
        import android.text.beforeTextChanged

        import test.TextWatcher
        import test.beforeTextChanged
        
        class TestClass {
            /**
             * Below we are referring non existing method extension method receiver is different
             * [TextWatcher.beforeTextChanged]
             */
            fun test() {
                TODO()
            }
        }
    """.trimIndent()
    val additional1 = """
        package android.text

        class TextWatcher
        class TextWatcher2
    """.trimIndent()
    val additional2 = """
        package android.text

        fun TextWatcher2.beforeTextChanged() {}
    """.trimIndent()
    assertThat(subject.lintWithContext(env, main, additional1, additional2)).isEmpty()
}

Copy link
Member

Choose a reason for hiding this comment

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

Then I'll remove the import. We don't want a test that enforces a false positive. We could have an ignored test with the false positive. But, probably, it's better to just remove this import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I have updated the TC to make the TC a valid cass

@cortinico
Copy link
Member

@atulgpt can we makesure the website preview doesn't fail?

@cortinico cortinico added the pick request Marker for PRs that should be ported to the 1.0 release branch label Oct 29, 2023
package com.example

import android.text.TextWatcher
import android.text.beforeTextChanged
Copy link
Member

Choose a reason for hiding this comment

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

To make this case valid shouldn't we remove this line?

Suggested change
import android.text.beforeTextChanged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it needs to be imported otherwise you will get the squiggly line in the Kdoc. See class TextWatcher doesn't have this method defined there

package com.example

import android.text.TextWatcher
import android.text.beforeTextChanged
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Suggested change
import android.text.beforeTextChanged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same comment as above

@BraisGabin BraisGabin merged commit cc54114 into detekt:main Nov 26, 2023
23 checks passed
@BraisGabin BraisGabin added this to the 2.0.0 milestone Nov 26, 2023
@atulgpt atulgpt deleted the fixes/6540/unused-import-extension branch November 26, 2023 17:31
cortinico pushed a commit that referenced this pull request Jan 31, 2024
* Fix by adding last method call to the set

* Create a variable for split

Make TC valid case
mgroth0 pushed a commit to mgroth0/detekt that referenced this pull request Feb 11, 2024
* Fix by adding last method call to the set

* Create a variable for split

Make TC valid case
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pick request Marker for PRs that should be ported to the 1.0 release branch rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UnusedImport false positive when extension method import is only used in documentation
4 participants