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

New Rule: ObjectLiteralToLambda #3599

Merged
merged 27 commits into from
May 12, 2021
Merged

Conversation

zmunm
Copy link
Contributor

@zmunm zmunm commented Mar 23, 2021

apply #3528

I hope there isn't much I've missed.

Thank you @cortinico

@codecov
Copy link

codecov bot commented Mar 23, 2021

Codecov Report

Merging #3599 (fd9222a) into main (4495b36) will increase coverage by 0.02%.
The diff coverage is 78.41%.

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

@@             Coverage Diff              @@
##               main    #3599      +/-   ##
============================================
+ Coverage     77.97%   78.00%   +0.02%     
- Complexity     2837     2897      +60     
============================================
  Files           467      472       +5     
  Lines          9139     9306     +167     
  Branches       1737     1776      +39     
============================================
+ Hits           7126     7259     +133     
- Misses         1071     1077       +6     
- Partials        942      970      +28     
Impacted Files Coverage Δ Complexity Δ
...t/generator/collection/RuleSetProviderCollector.kt 75.47% <0.00%> (-1.89%) 4.00 <0.00> (ø)
...kotlin/io/gitlab/arturbosch/detekt/DetektPlugin.kt 6.25% <ø> (+0.25%) 0.00 <0.00> (ø)
...lab/arturbosch/detekt/internal/ClassLoaderCache.kt 85.71% <ø> (ø) 0.00 <0.00> (ø)
...o/gitlab/arturbosch/detekt/internal/DetektPlain.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...o/gitlab/arturbosch/detekt/internal/SharedTasks.kt 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...ekt/generator/collection/DocumentationCollector.kt 70.00% <70.00%> (ø) 15.00 <15.00> (?)
...ekt/generator/collection/ConfigurationCollector.kt 70.58% <70.58%> (ø) 20.00 <20.00> (?)
...b/arturbosch/detekt/api/internal/ConfigProperty.kt 80.00% <80.00%> (ø) 0.00 <0.00> (?)
...rbosch/detekt/rules/style/ObjectLiteralToLambda.kt 81.25% <81.25%> (ø) 23.00 <23.00> (?)
...detekt/rules/bugs/DoubleMutabilityForCollection.kt 88.46% <88.46%> (ø) 4.00 <4.00> (?)
... and 11 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 4495b36...14085ad. Read the comment docs.

* }
* </compliant>
*
* @active since v1.17.0
Copy link
Member

Choose a reason for hiding this comment

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

Don't activate it by default please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Are there any criteria to set the rule to be enabled by default?

Copy link
Member

Choose a reason for hiding this comment

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

We never enable a rule by default the first time we introduced it because it can have false positives and we will impact to a lot of users. We prefer to enable it later if we think that its a rule that nearly all our user base will like to have it. But we don't have any defined rule here.

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.

Nice test coverage 👍

@zmunm zmunm requested a review from cortinico March 24, 2021 04:00
Debt.FIVE_MINS
)

private val KotlinType.isSamInterface
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private val KotlinType.isSamInterface
private val KotlinType.couldBeSamInterface

According to its original documentation, isDefinitelyNotSamInterface may return falsely false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* An anonymous object that does nothing other than the implementation of a single method
* can be used as a lambda.
*
* See https://kotlinlang.org/docs/fun-interfaces.html
Copy link
Member

Choose a reason for hiding this comment

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

Would this rule detect findings when the consuming code is Kotlin 1.3 or 1.3-?

Copy link
Member

Choose a reason for hiding this comment

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

(It may be good at least to call it out in the documentation)

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 wandering looking for a way to test. Can you give me a hint? @chao2zhang

Copy link
Member

Choose a reason for hiding this comment

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

I believe this requires us to set up a sample project and set its kotlin version to 1.3.*

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When running detektMain with java.util.Runnable in 1.3.72, it was confirmed that it is detected as a test case.

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we get that information from CompilerResources?

About how to test that if... we don't have any way to make it right now. But maybe we can create an issue to fix that and accept that if untested for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Everything is new, so I'm grasping it. :loading:
Please let me know if you have any misunderstandings.

From what I checked, in CompileResource, We can only see that the version is 1.3, and it seems that We can't actually compile with 1.3. Is that right?

What we want to check is the latter, and in java-interop there is nothing we can do with LanguageVersion.KOTLIN_1_3 because the code we need to detect is exactly the same.

When I tested fun interface in 1.3, there is nothing else to do because it fails in the :compileKotlin task.

If I make an issue, I think I will have to rewrite the case in the module implementing 1.3, but I wonder if this falls within the scope of the project's support.

Copy link
Member

Choose a reason for hiding this comment

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

The bottom line is that we do not crash when compiling Kotlin 1.3.
What's better is that even if the code does not compile, detekt findings should be easy to understand.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, the default configured detekt task does not depend on compileKotlin. If the error message is confusing, we should consider adding a LanguageVersion.KOTLIN_1_3 version check, otherwise, this PR should be good to go.

@chao2zhang
Copy link
Member

Actually, I also find that converting to SAM may indicate anonymous object converted to singleton: JetBrains/kotlin@99a6bde

I guess this is an edge case but we should at least call it out. (We can soon have @Controverial annotation)

@zmunm zmunm marked this pull request as draft March 29, 2021 11:14
@zmunm
Copy link
Contributor Author

zmunm commented Mar 29, 2021

In the process of adding the case, there have been many modifications to the code and JetBrains/kotlin@99a6bde is still under research.
So I changed PR to a draft and I'll request again as soon as it is resolved. Thank you

@zmunm zmunm marked this pull request as ready for review April 7, 2021 14:04
* An anonymous object that does nothing other than the implementation of a single method
* can be used as a lambda.
*
* See https://kotlinlang.org/docs/fun-interfaces.html
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't we get that information from CompilerResources?

About how to test that if... we don't have any way to make it right now. But maybe we can create an issue to fix that and accept that if untested for now.

@chao2zhang chao2zhang added this to the 1.17.0 milestone Apr 11, 2021
@zmunm
Copy link
Contributor Author

zmunm commented Apr 12, 2021

@chao2zhang Please change the linked issue to #3528

@chao2zhang
Copy link
Member

Should Detekt support primarily the current and the immediate previous feature release?
As Kotlin 1.5 is approaching, adding checks for Kotlin 1.4 language features should be okay to merge and we can loose our compatibility requirement for Kotlin 1.3-.

@zmunm
Copy link
Contributor Author

zmunm commented May 12, 2021

Is there something else I need to add for this?

@chao2zhang
Copy link
Member

Kotlin 1.5 has arrived already (Not in Detekt yet), we could say that Detekt at maximum will be supporting 2 major releases of Kotlin.

@chao2zhang chao2zhang merged commit 4d80329 into detekt:main May 12, 2021
@zmunm zmunm deleted the single-abstract-method branch May 12, 2021 03:44
@zmunm zmunm mentioned this pull request May 12, 2021
chao2zhang pushed a commit to chao2zhang/detekt that referenced this pull request May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

suggest Single Abstract Method
5 participants