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

IgnoredReturnValue: add option returnValueTypes to enable rule for particular types #4922

Merged
merged 3 commits into from
Sep 10, 2022

Conversation

osipxd
Copy link
Contributor

@osipxd osipxd commented Jun 5, 2022

@BraisGabin said:
We could extend IgnoredReturnValue and make it receive a list of types from the configuration. The rule will flag any call to a function that returns one of those types if the returned is ignored.

This PR adds config option returnValueTypes (is this option name OK?) to make it possible to enable the rule for functions returning specified types.
Also, I've added kotlinx.coroutines.flow.Flow to the default config.

Resolves #4640

@@ -92,6 +92,15 @@ class FindingsAssert(actual: List<Finding>) :
}

class FindingAssert(val actual: Finding?) : AbstractAssert<FindingAssert, Finding>(actual, FindingAssert::class.java) {

fun hasSourceLocation(line: Int, column: Int) = apply {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added this function to make it possible to use this assert in asserts chain:

assertThat(findings)
    .singleElement()
    .hasSourceLocation(line = 5, column = 10)
    .hasMessage("The call onEach is returning a value that is ignored.")

instead of

assertThat(findings).singleElement()
assertThat(findings).hasSourceLocation(line = 5, column = 10)
assertThat(findings.first()).hasMessage("The call onEach is returning a value that is ignored.")

@cortinico
Copy link
Member

By looking at this rule API, I think we should add also the restrictToReturnValueTypes boolean config.

  IgnoredReturnValue
    active: true
    restrictToAnnotatedMethods: true
    returnValueAnnotations:
      - '*.CheckResult'
      - '*.CheckReturnValue'
    ignoreReturnValueAnnotations:
      - '*.CanIgnoreReturnValue'
+   restrictToReturnValueTypes: true
+   returnValueTypes:
+     - 'kotlinx.coroutines.flow.Flow'

The rule should then check if restrictToReturnValueTypes is true and the returned type is one of the listed -> Then report.

It makes for a more explicit configuration and it aligns to restrictToAnnotatedMethods/returnValueAnnotations

@codecov
Copy link

codecov bot commented Jun 5, 2022

Codecov Report

Merging #4922 (638ada3) into main (fc23515) will increase coverage by 84.83%.
The diff coverage is 71.42%.

❗ Current head 638ada3 differs from pull request most recent head 94a3d4c. Consider uploading reports for the commit 94a3d4c to get more accurate results

@@             Coverage Diff             @@
##             main    #4922       +/-   ##
===========================================
+ Coverage        0   84.83%   +84.83%     
- Complexity      0     3515     +3515     
===========================================
  Files           0      497      +497     
  Lines           0    11554    +11554     
  Branches        0     2140     +2140     
===========================================
+ Hits            0     9802     +9802     
- Misses          0      686      +686     
- Partials        0     1066     +1066     
Impacted Files Coverage Δ
...arturbosch/detekt/rules/bugs/IgnoredReturnValue.kt 82.50% <71.42%> (ø)
...n/kotlin/io/gitlab/arturbosch/detekt/rules/Junk.kt 66.66% <0.00%> (ø)
...t/rules/coroutines/SuspendFunWithFlowReturnType.kt 84.00% <0.00%> (ø)
...ch/detekt/rules/bugs/UnnecessaryNotNullOperator.kt 88.23% <0.00%> (ø)
...kt/generator/collection/DefaultActivationStatus.kt 100.00% <0.00%> (ø)
...ab/arturbosch/detekt/core/config/AllRulesConfig.kt 66.66% <0.00%> (ø)
...kotlin/io/gitlab/arturbosch/detekt/api/Findings.kt 70.00% <0.00%> (ø)
...b/arturbosch/detekt/rules/naming/NamingProvider.kt 100.00% <0.00%> (ø)
...lab/arturbosch/detekt/formatting/FormattingRule.kt 97.95% <0.00%> (ø)
...tlab/arturbosch/detekt/core/reporting/Reporting.kt 94.00% <0.00%> (ø)
... and 488 more

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 fc23515...94a3d4c. Read the comment docs.

@osipxd
Copy link
Contributor Author

osipxd commented Jun 6, 2022

By looking at this rule API, I think we should add also the restrictToReturnValueTypes boolean config.
The rule should then check if restrictToReturnValueTypes is true and the returned type is one of the listed -> Then report.

It makes for a more explicit configuration and it aligns to restrictToAnnotatedMethods/returnValueAnnotations

Thank you for API review!
Yes, i thought about it, but prefix restrictTo implies "exclusiveness" isn't it? In my mind restrictToAnnotatedMethods means "this rule will check only methods annotated with the specified annotations", and restrictToReturnValueTypes means "this rule will check only menthods returning the specified types", so it may be confusing if both of these flags are true.
But maybe it is only my feeling because of language barrier.

@cortinico
Copy link
Member

Yes, i thought about it, but prefix restrictTo implies "exclusiveness" isn't it? In my mind restrictToAnnotatedMethods means "this rule will check only methods annotated with the specified annotations", and restrictToReturnValueTypes means "this rule will check only menthods returning the specified types", so it may be confusing if both of these flags are true.

Yes that's correct. If enabled, the rule will flag only function which are returning the values types you listed. If this is not the case, then I misunderstood what was the semantic of your change.

@osipxd
Copy link
Contributor Author

osipxd commented Jun 7, 2022

Yes that's correct. If enabled, the rule will flag only function which are returning the values types you listed. If this is not the case, then I misunderstood what was the semantic of your change.

I mean sentence "rule will check only annotated methods" conflicts with "rule will check only methods returning the specified types" because both contain word "only". So maybe there may be better naming? Something that means "always check methods returning specified types even if checks enabled only for annotated methods"?

@cortinico
Copy link
Member

So maybe there may be better naming? Something that means "always check methods returning specified types even if checks enabled only for annotated methods"?

I would suggest to don't implement this logic. The two restrict should apply on top of each other and further restrict the set of return values we inspect. What you're suggesting instead seems harder to explain and understand for users IMHO

@osipxd
Copy link
Contributor Author

osipxd commented Jun 7, 2022

I would suggest to don't implement this logic. The two restrict should apply on top of each other and further restrict the set of return values we inspect. What you're suggesting instead seems harder to explain and understand for users IMHO

Ok, I think I understand now. I'll add flag restrictToReturnValueTypes

@BraisGabin
Copy link
Member

🤔 I think that we should reframe the configuration of this rule. To me this issue should flag a function if it returns a Flow (or any other class listed on the configuration) or if it is annotated with some of the listed annotations. Then, we should have another flag that would ignore all those configurations and will flag all the cases where the return value is not Unit nor Nothing. Do that have sense?

To do that I would deprecate the current boolean flag and rename it to some naming more clear.

Copy link
Contributor Author

@osipxd osipxd left a comment

Choose a reason for hiding this comment

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

@cortinico, @BraisGabin, I've just updated this PR, can you take a look?

@osipxd osipxd marked this pull request as ready for review July 31, 2022 07:04
@BraisGabin BraisGabin added this to the 1.22.0 milestone Aug 4, 2022
@osipxd osipxd requested a review from BraisGabin August 5, 2022 06:24
@osipxd
Copy link
Contributor Author

osipxd commented Sep 9, 2022

Sorry for ping, but should it be merged?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this 👍

@@ -431,12 +431,16 @@ potential-bugs:
active: true
IgnoredReturnValue:
active: true
restrictToAnnotatedMethods: true
Copy link
Member

Choose a reason for hiding this comment

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

Reminder for self: this should be noted in the release notes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cortinico this change was not mentioned in release notes 🙈

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the reminder 👍

@BraisGabin
Copy link
Member

Thank you for the PR :)

@BraisGabin BraisGabin merged commit 04838fe into detekt:main Sep 10, 2022
@osipxd osipxd deleted the IgnoredReturnValue-by-return-type branch September 10, 2022 16:44
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add rule to check if Flow collected
3 participants