Add `UnnecessaryLambda` rule #6005

atulgpt · 2023-04-15T23:21:02Z

Implements #5836

This code and TC is taken from intellij-community [convertLambdaToReference](https://github.com/JetBrains/intellij-community/blob/master/plugins/kotlin/idea/src/org/jetbrains/kotlin/idea/intentions/ConvertLambdaToReferenceIntention.kt) intention

github-actions · 2023-04-15T23:21:31Z

	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 20a5881

codecov · 2023-04-15T23:34:02Z

Codecov Report

Merging #6005 (20a5881) into main (54c4b05) will decrease coverage by 0.28%.
The diff coverage is 59.31%.

@@             Coverage Diff              @@
##               main    #6005      +/-   ##
============================================
- Coverage     84.65%   84.38%   -0.28%     
- Complexity     3840     3911      +71     
============================================
  Files           549      550       +1     
  Lines         13068    13216     +148     
  Branches       2305     2378      +73     
============================================
+ Hits          11063    11152      +89     
- Misses          868      888      +20     
- Partials       1137     1176      +39

Impacted Files	Coverage Δ
...osch/detekt/rules/performance/UnnecessaryLambda.kt	`59.02% <59.02%> (ø)`
...ch/detekt/rules/performance/PerformanceProvider.kt	`100.00% <100.00%> (ø)`

... and 1 file 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.

cortinico

The rule logic is quite intricated. Can we vendor the ConvertLambdaToReferenceIntention.kt file and keep the rule logic smaller?

...rformance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryLambda.kt

…detekt/rules/performance/UnnecessaryLambda.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>

atulgpt · 2023-04-16T18:25:13Z

Can we vendor the ConvertLambdaToReferenceIntention.kt file and keep the rule logic smaller?

Hi @cortinico, currently, I have exactly the same code as ConvertLambdaToReferenceIntention.kt same is being followed in other rules as well(which are copied from Intellij idea plugin). What benefits will it serve to keep the code to ConvertLambdaToReferenceIntention.kt as it doesn't follow the existing convention and later if we want to add configs, or modify the behaviour of the rule it will be difficult to add those in separate ConvertLambdaToReferenceIntention.kt

But if you still feel that it should in a separate file then I will change this

Add description in code smell message Assert message in TCs

atulgpt · 2023-04-16T18:41:55Z

Hi, @cortinico / @BraisGabin any idea how to run tests against different Kotlin versions or with different Kotlin compiler flags?

marschwar · 2023-04-16T19:19:42Z

...rformance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryLambda.kt

+ * fun toDomain(i: Int) = i.toString()
+ * listOf(1).map { toDomain(it) }


Why is this code compliant? When I put this snippet into a kotlin file it does not compile.

Did you put

fun toDomain(i: Int) = i.toString() listOf(1).map { toDomain(it) }

both the lines?

marschwar · 2023-04-16T19:36:23Z

...rformance/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/performance/UnnecessaryLambda.kt

+ * Avoid creating unnecessary lambda to call a function where function reference can be used. This may have performance
+ * penalty when compared to directly passing lambda variable.


Do we have documentation, benchmarks that we could link to further explain the issue? I do not like that we state this as a fact and do not show proof. People might blindly follow the advice without second guessing it. I know I would.

I came across this post. I have no idea how credible it is. Does the performance penalty depend on the kotlin compiler version?

And applies the performance penalty to all output targets?

Hi, @marschwar, I did not run any benchmark on it. My initial motivation was to prohibit below type of use-case

fun test(lambda: () -> Unit) { thread { lambda() }.start() }

and prompt below alternate code

fun test(lambda: () -> Unit) { thread(block = lambda).start() }

But yes I agree that it will not be beneficial for all the use cases. I have also added This **may** have implying that it will not be the case always.

Finally, this rule promotes function ref for all the cases for consistency. But I am also open to suggestions

I think your original use case is quite different to what this ended up being. I am sorry, I should have made those points before you put in all the work.

I do not think that we should promote function references as there are many cases where it is not possible to use a function reference and therefor you might end up with a mix of function refs and lambdas in the same map chain.

Just out of curiosity I decompiled the generated kotlin byte code for the compliant and non-compliant version with IntelliJ and ended up with exactly the same java code. I do not have any experience with this feature but at first sight I do not think detekt should claim that this is or may be a performance issue unless we know for a fact in which circumstances that is the case.

BTW: Your original use case actually does generate an extra anonymous function in the non-compliant version making it a good candidate for a rule.

Interestingly the kotlin compiler seems to do some optimizations as both functions below are decompiled into nearly the same java code.

fun compliant(lambda: (Int) -> Unit) { repeat(3, lambda) } // and fun nonCompliant(lambda: (Int) -> Unit) { repeat(3) { lambda(it) } }

In this case it may just be a matter of style.

It is because repeat is inline fun

cortinico · 2023-04-17T14:12:11Z

But if you still feel that it should in a separate file then I will change this

I think that this case might be worth to have the copied logic in a separate file. The reason is that it's quite extensive and hard to isolate. So if we need to update the vendored logic, it should be easier

BraisGabin · 2023-04-19T15:25:58Z

To me this is not a performance rule. It is a style rule. Probably a controversial style rule. But I think like the idea of this rule.

atulgpt · 2023-04-19T15:32:49Z

To me this is not a performance rule. It is a style rule. Probably a controversial style rule. But I think like the idea of this rule.

Hi @BraisGabin / @marschwar should I move this rule to style rule section and remove the mention of performance part?

marschwar · 2023-04-19T16:29:17Z

I think it should be moved to the style rule set. But before you put in more work we should decide on the rule focus. should we do that on the original issue?
I do not like us recommending function references for no apparent reason. To me this feels like is own rule.

BraisGabin · 2023-04-20T13:27:25Z

Moving the conversation back to the issue: #5836 (comment) so we can talk more about it there. Blocking meanwhile.

marschwar · 2023-05-07T08:41:55Z

I believe this PR is obsolete now since we decided to go back to the drawing board. Can we close it @atulgpt ?

detekt-ci · 2023-05-07T09:23:54Z

	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 20a5881

github-actions bot added the rules label Apr 15, 2023

vercel bot deployed to Preview April 15, 2023 23:25 View deployment

cortinico changed the title ~~Report the usage unnecessary function to invoke a lambda argument~~ Add UnnecessaryLambda rule Apr 16, 2023

cortinico requested changes Apr 16, 2023

View reviewed changes

Update detekt-rules-performance/src/main/kotlin/io/gitlab/arturbosch/…

445b793

…detekt/rules/performance/UnnecessaryLambda.kt Co-authored-by: Nicola Corti <corti.nico@gmail.com>

vercel bot deployed to Preview April 16, 2023 17:24 View deployment

Update documentation

20a5881

Add description in code smell message Assert message in TCs

atulgpt requested a review from cortinico April 16, 2023 18:42

vercel bot deployed to Preview April 16, 2023 18:44 View deployment

marschwar reviewed Apr 16, 2023

View reviewed changes

BraisGabin mentioned this pull request Apr 20, 2023

Report the usage unneccessary function to invoke a lambda argument #5836

Open

BraisGabin added the blocked label Apr 20, 2023

atulgpt closed this May 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `UnnecessaryLambda` rule #6005

Add `UnnecessaryLambda` rule #6005

atulgpt commented Apr 15, 2023

github-actions bot commented Apr 15, 2023 •

edited

codecov bot commented Apr 15, 2023 •

edited

cortinico left a comment

atulgpt commented Apr 16, 2023 •

edited

atulgpt commented Apr 16, 2023 •

edited

marschwar Apr 16, 2023

atulgpt Apr 16, 2023

marschwar Apr 16, 2023

marschwar Apr 16, 2023

atulgpt Apr 16, 2023

marschwar Apr 16, 2023

marschwar Apr 16, 2023 •

edited

atulgpt Apr 16, 2023

cortinico commented Apr 17, 2023

BraisGabin commented Apr 19, 2023

atulgpt commented Apr 19, 2023 •

edited

marschwar commented Apr 19, 2023

BraisGabin commented Apr 20, 2023 •

edited

marschwar commented May 7, 2023

detekt-ci commented May 7, 2023

		* fun toDomain(i: Int) = i.toString()
		* listOf(1).map { toDomain(it) }

		* Avoid creating unnecessary lambda to call a function where function reference can be used. This may have performance
		* penalty when compared to directly passing lambda variable.

Add UnnecessaryLambda rule #6005

Add UnnecessaryLambda rule #6005

Conversation

atulgpt commented Apr 15, 2023

github-actions bot commented Apr 15, 2023 • edited

codecov bot commented Apr 15, 2023 • edited

Codecov Report

cortinico left a comment

Choose a reason for hiding this comment

atulgpt commented Apr 16, 2023 • edited

atulgpt commented Apr 16, 2023 • edited

marschwar Apr 16, 2023

Choose a reason for hiding this comment

atulgpt Apr 16, 2023

Choose a reason for hiding this comment

marschwar Apr 16, 2023

Choose a reason for hiding this comment

marschwar Apr 16, 2023

Choose a reason for hiding this comment

atulgpt Apr 16, 2023

Choose a reason for hiding this comment

marschwar Apr 16, 2023

Choose a reason for hiding this comment

marschwar Apr 16, 2023 • edited

Choose a reason for hiding this comment

atulgpt Apr 16, 2023

Choose a reason for hiding this comment

cortinico commented Apr 17, 2023

BraisGabin commented Apr 19, 2023

atulgpt commented Apr 19, 2023 • edited

marschwar commented Apr 19, 2023

BraisGabin commented Apr 20, 2023 • edited

marschwar commented May 7, 2023

detekt-ci commented May 7, 2023

Add `UnnecessaryLambda` rule #6005

Add `UnnecessaryLambda` rule #6005

github-actions bot commented Apr 15, 2023 •

edited

codecov bot commented Apr 15, 2023 •

edited

atulgpt commented Apr 16, 2023 •

edited

atulgpt commented Apr 16, 2023 •

edited

marschwar Apr 16, 2023 •

edited

atulgpt commented Apr 19, 2023 •

edited

BraisGabin commented Apr 20, 2023 •

edited