Potential new rule: DoubleNegativeLambda #5937

drawers · 2023-03-24T23:18:18Z

Potential new rule: DoubleNegativeLambda

Rationale

There was recently a popular post on Y-Combinator Hacker News arguing against the use of unless in Ruby.

https://news.ycombinator.com/item?id=33965933

It is arguing that:

Double negatives are hard to read: (takeUnless { !it.isOdd() } vs takeIf { it.isEven() })
It's easy for a nice simple takeUnless { it.isEven() } to become complex when some edge case is found and an extra condition is added so it becomes something like takeUnless { it.isEven() && !it.isAlreadyAccountedFor() }. Now the reader needs to unpack this expression using DeMorgan's laws

I am wondering if you would be interested in a rule that checks for double negatives like this using takeUnless? (it's okay if 'no', can keep it private)

Considerations

Should it check for filterNot and none by default as well?

To me, negatives in a filterNot block seem less harmful than in a takeUnless block. I looked for examples and I found this one in KotlinPoet:

addModifiers(
        flags.modalities
          .filterNot { it == FINAL && !isOverride } // Final is the default
          .filterNot { it == OPEN && isOverride } // Overrides are implicitly open
          .filterNot { it == OPEN && isInInterface }, // interface methods are implicitly open
      )

It doesn't seem to benefit a lot from being rewritten in the positive as filter { it != FINAL || isOverride }

Should it account for functions in backticks?

One could argue that in addition to detecting simple function names with "not" and "non" in them, it should also try and catch negation in functions with back ticks to report cases like takeUnless { it.isn't a word() }. I thought it would add a bit of complexity to the rule and detecting negation in a backtick function name would make it a harder problem where we would be trying to solve the problem of detecting negation in English which has so many different ways (isn't, can't, won't etc.)

Should it use type resolution to find functions using FQN?

To me, functions like takeUnless are quite rare. I think it would be unusual to want to distinguish between one or the other.

Disclosure

I used ChatGPT4 to help me write the rule and was thinking of writing an article about my experience (Chat GPT was not that helpful here despite Detekt code seemingly being in the training data).

Prompts and responses for transparency:
https://chat.openai.com/share/48f1ebbb-5761-4e0b-a232-d3ac6f808f89

…egative with a lambda block also containing a negative

github-actions · 2023-03-24T23:18:51Z

	Messages
📖	Thanks for adding a new rule to detekt ❤️
✅	We detekted that you added tests, to your rule, that's awesome!
✅	We detekted that you updated the `default-detekt-config.yml` file, that's awesome!

Generated by 🚫 dangerJS against dbfebfd

schalkms · 2023-03-25T00:18:13Z

This rule is definitely a good addition to the standard style ruleset! I like it.
Regarding your question, I'd keep the first version of this rule as simple as possible.
Regarding your usage of ChatGPT, I'd like to learn about your experience with PSI and detekt. I'm looking forward to your article. 🙂

codecov · 2023-03-25T03:28:29Z

Codecov Report

Merging #5937 (dbfebfd) into main (eb58da6) will increase coverage by 0.06%.
The diff coverage is 94.11%.

@@             Coverage Diff              @@
##               main    #5937      +/-   ##
============================================
+ Coverage     84.46%   84.52%   +0.06%     
- Complexity     3784     3833      +49     
============================================
  Files           546      548       +2     
  Lines         12923    13052     +129     
  Branches       2268     2302      +34     
============================================
+ Hits          10915    11032     +117     
- Misses          877      879       +2     
- Partials       1131     1141      +10

Impacted Files	Coverage Δ
...urbosch/detekt/rules/style/DoubleNegativeLambda.kt	`94.00% <94.00%> (ø)`
...rturbosch/detekt/rules/style/StyleGuideProvider.kt	`100.00% <100.00%> (ø)`

... and 10 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

BraisGabin · 2023-03-26T09:04:12Z

...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/DoubleNegativeLambda.kt

+ * Random.Default.nextInt().takeUnless { !it.isEven() }
+ * </noncompliant>
+ * <compliant>
+ * Random.Default.nextInt().takeIf { it.isOdd() }


The code in the non-compliant and the compliant doesn't behave the same. That doesn't help to understand the rule.

Suggested change

* Random.Default.nextInt().takeIf { it.isOdd() }

* Random.Default.nextInt().takeIf { it.isEven() }

Thank you, I tried to make it clearer in e7b22f1

BraisGabin · 2023-03-26T09:12:47Z

...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/DoubleNegativeLambda.kt

+    private val negatingFunctionNameParts = listOf("not", "non")
+
+    @Configuration("Function names expressed in the negative that can form double negatives with their lambda blocks.")
+    private val negativeFunctions: Set<String> by config(listOf("takeUnless")) { it.toSet() }


We could use here valueWithReason so in the reason we could say which is the positive form of that function. For example, for takeUnless it would be takeIf.

You can see examples of this at forbiddenMethodCall.

Thanks, didn't know about this feature. I attempted this in 38a187a

BraisGabin · 2023-03-26T09:13:55Z

...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/DoubleNegativeLambda.kt

+        KtTokens.NOT_IS,
+    )
+
+    private val negatingFunctionNameParts = listOf("not", "non")


Should we make this configurable?

Thanks, I think they should be configurable too. Attempted this in df6a662

BraisGabin

I really like the idea behind this rule 👍. And if you agree with the change I propose on the comments it will be a LGTM.

BraisGabin · 2023-03-27T22:34:04Z

...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/DoubleNegativeLambda.kt

+    )
+    private val negativeFunctions: List<NegativeFunction> by config(
+        valuesWithReason(
+            "takeUnless" to "takeIf",


Suggested change

"takeUnless" to "takeIf",

"takeUnless" to "Use `takeIf` instead.",

BraisGabin · 2023-03-27T22:36:50Z

...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/DoubleNegativeLambda.kt

+        append("Rewrite in the positive")
+        if (negativeFunction.positiveCounterpart != null) {
+            append(" with `${negativeFunction.positiveCounterpart}`.")
+        } else {
+            append(".")
+        }


Suggested change

append("Rewrite in the positive")

if (negativeFunction.positiveCounterpart != null) {

append(" with `${negativeFunction.positiveCounterpart}`.")

} else {

append(".")

}

if (negativeFunction.positiveCounterpart != null) {

append(negativeFunction.positiveCounterpart")

} else {

append("Rewrite in the positive.")

}

I think that reason should contain a reason, not just the counter part. And that should also make this code a bit simplier. Don't commit exactly my code, I assume that further rework is needed to land this change.

Thanks, that makes a lot more sense. I did the change in 2ded6a4

I called it a "recommendation" ("Use takeIf instead") rather than a "reason" because that seems to be what it's called in the docs:
https://github.com/detekt/detekt/blob/main/.github/CONTRIBUTING.md

but I can rename it back to "reason" if you prefer.

…rpart.

cortinico · 2023-04-05T12:26:14Z

...-rules-style/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/style/DoubleNegativeLambda.kt

+    )
+    private val negativeFunctions: List<NegativeFunction> by config(
+        valuesWithReason(
+            "takeUnless" to "Use `takeIf` instead.",


what's the reason why you haven't included none and filterNot in the default here?

Thank you for the review.

I think there is a good case to add none as a default. I searched a few big Kotlin codebases, including Detekt and KotlinPoet. I couldn't find any examples of none with a negation in its lambda. So it seems it would be something we would catch in a code review. Adding none would help explain the rule to consumers too.

I did not include filterNot because it seems there are a few cases where it seems more readable leaving it in the negative. I found this example in KotlinPoet:

addModifiers( flags.modalities .filterNot { it == FINAL && !isOverride } // Final is the default .filterNot { it == OPEN && isOverride } // Overrides are implicitly open .filterNot { it == OPEN && isInInterface }, // interface methods are implicitly open )

Rewriting this, we'd end up with:

addModifiers( flags.modalities .filter { it != FINAL && isOverride } // Final is the default .filterNot { it == OPEN && isOverride } // Overrides are implicitly open .filterNot { it == OPEN && isInInterface }, // interface methods are implicitly open )

What do you think about adding none as a default and leaving out filterNot?

What do you think about adding none as a default and leaving out filterNot?

Yup makes sense, thanks for clarifying 👍 none feels also like the most natural use case for this rule

Add rule to detekt double negatives from function name expressed in n…

f09fecb

…egative with a lambda block also containing a negative

github-actions bot added the rules label Mar 24, 2023

vercel bot deployed to Preview March 24, 2023 23:23 View deployment

Fix issue id

9063e75

vercel bot deployed to Preview March 25, 2023 03:23 View deployment

Fix test snippets not compiling

dde9fac

vercel bot deployed to Preview March 25, 2023 04:07 View deployment

Fix test snippets not compiling

ea570a5

vercel bot deployed to Preview March 25, 2023 05:08 View deployment

BraisGabin reviewed Mar 26, 2023

View reviewed changes

drawers added 8 commits March 27, 2023 21:54

Add config for negative function name parts

df6a662

Add value with reason config for negative function names

38a187a

Rename forbiddenFunction to negativeFunction

4748ba8

Rename lambdaExpression to lambdaArgument

6f7ab16

Make compliant/noncompliant example clearer

e7b22f1

Add test against logical not on prefixing brackets

6483b16

Add extra test

12b0006

Clean up FQN use in tests

bacd753

vercel bot deployed to Preview March 27, 2023 18:17 View deployment

BraisGabin reviewed Mar 27, 2023

View reviewed changes

BraisGabin added this to the 1.23.0 milestone Mar 27, 2023

Make recommendation full-text, not just the simple name of the counte…

2ded6a4

…rpart.

vercel bot deployed to Preview March 28, 2023 01:05 View deployment

BraisGabin approved these changes Mar 28, 2023

View reviewed changes

cortinico reviewed Apr 5, 2023

View reviewed changes

Add none as a default

525956e

cortinico approved these changes Apr 6, 2023

View reviewed changes

Adjust tests to account for none as a default

dbfebfd

drawers force-pushed the double-negative branch from 53e8e7c to dbfebfd Compare April 6, 2023 10:18

vercel bot deployed to Preview April 6, 2023 10:27 View deployment

cortinico added the notable changes Marker for notable changes in the changelog label Apr 6, 2023

cortinico merged commit c3bb324 into detekt:main Apr 6, 2023

TWiStErRob pushed a commit to TWiStErRob/detekt that referenced this pull request Apr 6, 2023

Potential new rule: DoubleNegativeLambda (detekt#5937)

f57c4de

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Potential new rule: DoubleNegativeLambda #5937

Potential new rule: DoubleNegativeLambda #5937

drawers commented Mar 24, 2023 •

edited

Loading

github-actions bot commented Mar 24, 2023 •

edited

Loading

schalkms commented Mar 25, 2023

codecov bot commented Mar 25, 2023 •

edited

Loading

BraisGabin Mar 26, 2023

drawers Mar 27, 2023

BraisGabin Mar 26, 2023

drawers Mar 27, 2023

BraisGabin Mar 26, 2023

drawers Mar 27, 2023

BraisGabin left a comment •

edited

Loading

BraisGabin Mar 27, 2023

BraisGabin Mar 27, 2023

drawers Mar 28, 2023

cortinico Apr 5, 2023

drawers Apr 5, 2023

cortinico Apr 6, 2023

	* Random.Default.nextInt().takeIf { it.isOdd() }
	* Random.Default.nextInt().takeIf { it.isEven() }

	"takeUnless" to "takeIf",
	"takeUnless" to "Use `takeIf` instead.",

Potential new rule: DoubleNegativeLambda #5937

Potential new rule: DoubleNegativeLambda #5937

Conversation

drawers commented Mar 24, 2023 • edited Loading

Potential new rule: DoubleNegativeLambda

Rationale

Considerations

Disclosure

github-actions bot commented Mar 24, 2023 • edited Loading

schalkms commented Mar 25, 2023

codecov bot commented Mar 25, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BraisGabin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drawers commented Mar 24, 2023 •

edited

Loading

github-actions bot commented Mar 24, 2023 •

edited

Loading

codecov bot commented Mar 25, 2023 •

edited

Loading

BraisGabin left a comment •

edited

Loading