New Rule: IgnoredReturnValue #2698

cortinico · 2020-05-15T19:55:33Z

I've cherry-picked @bbaldino commits and fixed the tests for the new rule IgnoredReturnValue.

I've tried this approach that checks if a PsiElement is "isolated" and the return value is not Unit. I'm not sure it's the correct one but from the tests it seems promising.

I'd be happy to receive further ideas for test cases or improvements.

Closes #209
Closes #2239
Closes #2242

codecov · 2020-05-15T20:08:31Z

Codecov Report

Merging #2698 into master will increase coverage by 0.04%.
The diff coverage is 87.50%.

@@             Coverage Diff              @@
##             master    #2698      +/-   ##
============================================
+ Coverage     79.85%   79.89%   +0.04%     
- Complexity     2305     2314       +9     
============================================
  Files           381      382       +1     
  Lines          6940     6979      +39     
  Branches       1250     1262      +12     
============================================
+ Hits           5542     5576      +34     
  Misses          779      779              
- Partials        619      624       +5

Impacted Files	Coverage Δ	Complexity Δ
...arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt	`86.84% <86.84%> (ø)`	`9.00 <9.00> (?)`
...sch/detekt/rules/providers/PotentialBugProvider.kt	`100.00% <100.00%> (ø)`	`2.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 2ec2d5a...c2bb657. Read the comment docs.

schalkms

Thanks, I like this rule 🥇
Did you run this on detekt itself or any 3rd party projects in order to check for any false-positives?

Script:
https://github.com/detekt/detekt/blob/master/scripts/get_analysis_projects.groovy

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt

schalkms · 2020-05-15T22:39:24Z

detekt-rules/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt

+        val returnType = resolvedCall.resultingDescriptor.returnType ?: return
+
+        if (returnType.isUnit()) {
+            return
+        }


nit: then the @Suppress isn't necessary anymore.

Suggested change

val returnType = resolvedCall.resultingDescriptor.returnType ?: return

if (returnType.isUnit()) {

return

}

if (resolvedCall.resultingDescriptor.returnType?.isUnit() == true) return

I'd rather keep it separated to improve readability. Feel free to reopen if you think it's still needed in the following diff.

True. Maybe the code should read if (returnType?.isUnit() == true).
I just don't want to spam the codebase with Suppress annotations.

BraisGabin · 2020-05-16T08:08:26Z

Quoting myself:

The annotation @CheckReturnValue does that more less. There was a thread about this here: #209 and there is an implementation for android lint and error-prone

You can read #2239 for more context.

But in short: I think that this rule should use the @CheckReturnValue annotation. Then we can add a flag to check for all of them.

cortinico · 2020-05-16T10:42:18Z

But in short: I think that this rule should use the @CheckReturnValue annotation. Then we can add a flag to check for all of them.

Agree on this. I'll extend the rule to account only calls to functions annotated with @CheckReturnValue

schalkms · 2020-05-17T09:54:54Z

But in short: I think that this rule should use the @CheckReturnValue annotation. Then we can add a flag to check for all of them.

I would do this in a next step and merge this PR first. It makes it also easier to review.

BraisGabin · 2020-05-17T10:37:57Z

I would do this in a next step and merge this PR first. It makes it also easier to review.

I understand your point, but, wouldn't that delay our release? I mean, we would have no-production ready code in master.

schalkms · 2020-05-17T10:54:42Z

The rule is disabled anyway.

cortinico · 2020-05-18T19:02:38Z

I've posted another diff for this rule. There are still some open points that I would love to get an input from @BraisGabin @schalkms:

Currently the annotation are checked by simple name only (== CheckReturnValue) against CheckReturnValue and CheckResult. Should this be configurable? (imho, yes)
Should annotations be checked against their fully qualified name (com.google.errorprone.annotations.CheckReturnValue and androidx.annotation.CheckResult)? (imho, not necessary given how specific are those annotations).
Should the whole annotations requirement be configurable? Like having a restrictToAnnotatedMethods: true parameter? (imho, yes)
I found a false negative scenario: when the call site is the last statement of a block. Currently the rule is just ignoring this scenario. I've added a test case for that. I wasn't able to find a simple solution to this false negative, and this is probably worth a follow-up PR

schalkms · 2020-05-18T21:02:09Z

Currently the annotation are checked by simple name only (== CheckReturnValue) against CheckReturnValue and CheckResult. Should this be configurable? (imho, yes)
=> yes, this could be done in a separate PR. This would be a nice improvement!
Should annotations be checked against their fully qualified name (com.google.errorprone.annotations.CheckReturnValue and androidx.annotation.CheckResult)? (imho, not necessary given how specific are those annotations).
=> not necessary IMHO
Should the whole annotations requirement be configurable? Like having a restrictToAnnotatedMethods: true parameter? (imho, yes)
=> yes, this could be done in a separate PR. This would be a nice improvement!
I found a false negative scenario: when the call site is the last statement of a block. Currently the rule is just ignoring this scenario. I've added a test case for that. I wasn't able to find a simple solution to this false negative, and this is probably worth a follow-up PR
=> Agreed.

HTH 🙂

BraisGabin · 2020-05-18T22:32:08Z

* Currently the annotation are checked by simple name only (`== CheckReturnValue`) against `CheckReturnValue` and `CheckResult`. Should this be configurable? (imho, yes)

* Should annotations be checked against their fully qualified name (`com.google.errorprone.annotations.CheckReturnValue` and `androidx.annotation.CheckResult`)? (imho, not necessary given how specific are those annotations).

Yes, I think that this should be configurable, and in this configuration we can support both options. The default value should be something like['*.CheckReturnValue', '*.CheckResult']. Then it's up to the user to use fully qualified name or not. The only thing to discuess here is which type of regex do we want here. Options that I see:

Full regex: Maximum flexibility, difficult to read and maintain: ['.*\.CheckReturnValue', '.*\.CheckResult']
glob: No that flexible but gives a lot of flexibility: ['*.CheckReturnValue', '*.CheckResult']
plain string: We just chech that the of the Annotation: ['CheckReturnValue', 'CheckResult']

The problem here is that in detekt we doesn't have a defined way to do this. I'll open an issue to talk about this. In my opinion the second option is the best. It provides a lot of flexibility and it's easy to read.

* Should the whole annotations requirement be configurable? Like having a `restrictToAnnotatedMethods: true` parameter? (imho, yes)

Yes

* I found a false negative scenario: when the call site is the last statement of a block. Currently the rule is just ignoring this scenario. I've added a test case for that. I wasn't able to find a simple solution to this false negative, and this is probably worth a follow-up PR

Agree.

cortinico · 2020-05-18T23:07:12Z

The problem here is that in detekt we doesn't have a defined way to do this. I'll open an issue to talk about this. In my opinion the second option is the best. It provides a lot of flexibility and it's easy to read.

I'd either go for option 2 or 3. Option 2 could be achieved using this:

detekt/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/SplitPattern.kt

Lines 90 to 96 in 57e0b91

    
           fun String.simplePatternToRegex(): Regex { 
        
               return this 
        
                   .replace(".", "\\.") 
        
                   .replace("*", ".*") 
        
                   .replace("?", ".?") 
        
                   .toRegex() 
        
           }

This will make the rule behavior consistent with the ForbiddenImport rule, that seems reasonable. Option 3 is definitely the easiest. Still don't know if we're maybe offering a "too flexible" solution for a scenario that users probably will never customize.

I'd be in for option 2, I just don't want to block this PR with #2711

BraisGabin · 2020-05-18T23:19:24Z

For me the option 2 is the best. And for sure, this doesn't need to wait for #2711. That issue is a more generic one and probably something to face for 2.0.

For me this PR can be merged right now if you agree. The missing parts are just to add more configuration but the rule right now work as intended with the default configurations. So any other feature can be added in other PR.

cortinico · 2020-05-18T23:23:00Z

Sure, go for that, I'll follow up with a couple of small PRs

schalkms reviewed May 15, 2020

View reviewed changes

bbaldino and others added 4 commits May 18, 2020 20:28

Add rule to warn on ignored return value

b0d3727

Fix the rule IgnoredReturnValue

51a37a0

Fix compilation error in test source snippet

56a71eb

Check for @CheckReturnValue and @checkresult on function call

c2bb657

cortinico force-pushed the ignored_return_value branch from b436aa9 to c2bb657 Compare May 18, 2020 18:28

BraisGabin approved these changes May 18, 2020

View reviewed changes

BraisGabin mentioned this pull request May 18, 2020

How should we match the configuration values? #2711

Closed

schalkms approved these changes May 19, 2020

View reviewed changes

schalkms merged commit 63f158d into detekt:master May 19, 2020

cortinico deleted the ignored_return_value branch May 19, 2020 09:33

arturbosch added this to the 1.10.0 milestone May 19, 2020

cortinico mentioned this pull request May 19, 2020

IgnoreReturnValue: config options #2712

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Rule: IgnoredReturnValue #2698

New Rule: IgnoredReturnValue #2698

cortinico commented May 15, 2020 •

edited

Loading

codecov bot commented May 15, 2020 •

edited

Loading

schalkms left a comment

schalkms May 15, 2020 •

edited

Loading

cortinico May 18, 2020

schalkms May 18, 2020

BraisGabin commented May 16, 2020

cortinico commented May 16, 2020

schalkms commented May 17, 2020

BraisGabin commented May 17, 2020

schalkms commented May 17, 2020

cortinico commented May 18, 2020 •

edited

Loading

schalkms commented May 18, 2020

BraisGabin commented May 18, 2020

cortinico commented May 18, 2020 •

edited

Loading

BraisGabin commented May 18, 2020

cortinico commented May 18, 2020

New Rule: IgnoredReturnValue #2698

New Rule: IgnoredReturnValue #2698

Conversation

cortinico commented May 15, 2020 • edited Loading

codecov bot commented May 15, 2020 • edited Loading

Codecov Report

schalkms left a comment

Choose a reason for hiding this comment

schalkms May 15, 2020 • edited Loading

Choose a reason for hiding this comment

cortinico May 18, 2020

Choose a reason for hiding this comment

schalkms May 18, 2020

Choose a reason for hiding this comment

BraisGabin commented May 16, 2020

cortinico commented May 16, 2020

schalkms commented May 17, 2020

BraisGabin commented May 17, 2020

schalkms commented May 17, 2020

cortinico commented May 18, 2020 • edited Loading

schalkms commented May 18, 2020

BraisGabin commented May 18, 2020

cortinico commented May 18, 2020 • edited Loading

BraisGabin commented May 18, 2020

cortinico commented May 18, 2020

cortinico commented May 15, 2020 •

edited

Loading

codecov bot commented May 15, 2020 •

edited

Loading

schalkms May 15, 2020 •

edited

Loading

cortinico commented May 18, 2020 •

edited

Loading

cortinico commented May 18, 2020 •

edited

Loading