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

UnnecessaryApply: fix false positive when lambda has multiple member references #3564

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

t-kameyama
Copy link
Contributor

Fixes #3561

@codecov
Copy link

codecov bot commented Mar 17, 2021

Codecov Report

Merging #3564 (a387781) into main (aaf09dc) will decrease coverage by 0.03%.
The diff coverage is 60.00%.

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

@@             Coverage Diff              @@
##               main    #3564      +/-   ##
============================================
- Coverage     78.22%   78.19%   -0.04%     
- Complexity     2826     2834       +8     
============================================
  Files           466      466              
  Lines          9136     9137       +1     
  Branches       1724     1726       +2     
============================================
- Hits           7147     7145       -2     
  Misses         1059     1059              
- Partials        930      933       +3     
Impacted Files Coverage Δ Complexity Δ
.../arturbosch/detekt/rules/style/UnnecessaryApply.kt 80.00% <60.00%> (-9.66%) 16.00 <8.00> (+8.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 aaf09dc...a268f6d. Read the comment docs.

@BraisGabin
Copy link
Member

Can we wait a bit to merge this, I'm not so sure about this issue. It seems like a true positive to me.

@BraisGabin
Copy link
Member

What do you think about what I said in the issue?

@t-kameyama
Copy link
Contributor Author

t-kameyama commented Mar 18, 2021

class Foo {
    fun bar(f: () -> Unit) {}
    var baz = 1
}

fun test(foo: Foo) {
    // case 1
    foo.apply {
        println(baz)
        println(baz)
    }

    // case 2
    foo.apply {
        bar {
            println(baz)
            println(baz)
        }
    }
}

Since it's not flagged in case 1, I don't think it should be flagged in case 2 as well. I understand that this rule does not flag when lambda has multiple member references.

@BraisGabin
Copy link
Member

Maybe we should update it and make it flag the first case too. Promoting with or let

@chao2zhang chao2zhang added this to the 1.16.1 milestone Mar 19, 2021
Base automatically changed from master to main March 21, 2021 18:44
@chao2zhang chao2zhang modified the milestones: 1.16.1, 1.17.0 Mar 25, 2021
@t-kameyama t-kameyama force-pushed the issue_3561 branch 2 times, most recently from 790d486 to 08505d0 Compare March 29, 2021 09:47
@t-kameyama
Copy link
Contributor Author

Maybe we should update it and make it flag the first case too. Promoting with or let

I don't think so. The first case can be replaced with let, but it's not an unnecessary apply.

Copy link
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

Since it's not flagged in case 1, I don't think it should be flagged in case 2 as well. I understand that this rule does not flag when lambda has multiple member references.

I agree with you here. The problem that I'm talking about is already in the rule so this PR just make it more consistent so we should merge this. I think that we need to rethink a bit this rule but that's another topic 👍.

@BraisGabin BraisGabin merged commit 4228e43 into detekt:main Mar 30, 2021
@t-kameyama t-kameyama deleted the issue_3561 branch March 30, 2021 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unnecessary Apply rule being applied for cases when apply is needed
4 participants