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

Ignore actual members in UnusedPrivateMember #3669

Merged

Conversation

sczerwinski
Copy link
Contributor

The general rule of thumb should be:

  • if a rule is ignored for abstract members, it should be ignored for expect members,
  • if a rule is ignored for overridden members, it should be ignored for actual members.

See also:

Related to #3415

@chao2zhang chao2zhang added this to the 1.17.0 milestone Apr 10, 2021
@codecov
Copy link

codecov bot commented Apr 10, 2021

Codecov Report

Merging #3669 (adef6d3) into main (e9fd9f3) will increase coverage by 0.00%.
The diff coverage is 80.00%.

❗ Current head adef6d3 differs from pull request most recent head a6f6157. Consider uploading reports for the commit a6f6157 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main    #3669   +/-   ##
=========================================
  Coverage     77.95%   77.96%           
  Complexity     2870     2870           
=========================================
  Files           470      470           
  Lines          9245     9248    +3     
  Branches       1761     1763    +2     
=========================================
+ Hits           7207     7210    +3     
  Misses         1077     1077           
  Partials        961      961           
Impacted Files Coverage Δ Complexity Δ
...turbosch/detekt/rules/style/UnusedPrivateMember.kt 91.17% <80.00%> (+0.19%) 5.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 e9fd9f3...a6f6157. Read the comment docs.

@@ -204,7 +205,7 @@ private class UnusedParameterVisitor(allowedNames: Regex) : UnusedMemberVisitor(
}

override fun visitClassOrObject(klassOrObject: KtClassOrObject) {
if (klassOrObject.isExpect()) return
if (klassOrObject.isExpect() || klassOrObject.isActual()) return
Copy link
Member

Choose a reason for hiding this comment

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

How is this related to the rule of thumb if a rule is ignored for overridden members, it should be ignored for actual members.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line is actually my mistake (I went with isExpect() one step too far). Actual classes or objects should be visited. Just their actual members should be ignored.

I'll update the PR.

@sczerwinski sczerwinski force-pushed the UnusedPrivateMember/ignoreActualFunction branch from a654113 to cce1aea Compare April 10, 2021 17:25
@sczerwinski
Copy link
Contributor Author

@chao2zhang, Sorry for the mistake. I've fixed the PR, rebased and squashed for better readability.

Also an example of an actual constructor, that should be ignored:
commonMain:

expect class Foo(bar: Bar) {
    expect fun doSomething()
}

jvmMain:

actual class Foo actual constructor(private val bar: Bar) {
    actual fun doSomething() {
        bar.doSomething()
    }
}

jsMain:

actual class Foo actual constructor(bar: Bar) {
    actual fun doSomething() {
        // Do something with JS that doesn't require bar
    }
}

@sczerwinski sczerwinski force-pushed the UnusedPrivateMember/ignoreActualFunction branch from cce1aea to a6f6157 Compare April 10, 2021 18:30
@chao2zhang chao2zhang merged commit da8676d into detekt:main Apr 19, 2021
@sczerwinski sczerwinski deleted the UnusedPrivateMember/ignoreActualFunction branch April 20, 2021 09:14
chao2zhang pushed a commit to chao2zhang/detekt that referenced this pull request May 13, 2021
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 on UnusedPrivateMember for actual/expected inline method parameters.
3 participants