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

Don't run rules annotated with @RequiresTypeResolution when running plain detekt #5176

Merged
merged 5 commits into from Sep 8, 2022

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented Aug 3, 2022

This is an alternative for #5130. To be more precise this was proposed by @marschwar here: #5130 (comment)

I implemented this in :detekt-core because I think it should be the one that decides if a rule should or should not be runned. But to be more consistent with the current code we could move this check to the Rule inside :detekt-api. What do you think?


This PR isn't finish yet. I need to remove all the if (bindingContext == BindingContext.EMPTY) return that we have in our code now but I want feedback to know if this is what we want.

@github-actions github-actions bot added api build core dependencies Pull requests that update a dependency file labels Aug 3, 2022
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #5176 (5c1a3b9) into main (97fd12b) will increase coverage by 0.31%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #5176      +/-   ##
============================================
+ Coverage     84.93%   85.24%   +0.31%     
+ Complexity     3716     3700      -16     
============================================
  Files           514      514              
  Lines         12068    12004      -64     
  Branches       2271     2222      -49     
============================================
- Hits          10250    10233      -17     
+ Misses          698      696       -2     
+ Partials       1120     1075      -45     
Impacted Files Coverage Δ
...turbosch/detekt/rules/complexity/NamedArguments.kt 86.20% <ø> (+2.87%) ⬆️
...ch/detekt/rules/complexity/NestedScopeFunctions.kt 89.58% <ø> (-0.22%) ⬇️
...kt/rules/complexity/ReplaceSafeCallChainWithRun.kt 94.11% <ø> (+5.22%) ⬆️
...etekt/rules/coroutines/RedundantSuspendModifier.kt 64.28% <ø> (-0.84%) ⬇️
...sch/detekt/rules/coroutines/SleepInsteadOfDelay.kt 75.86% <ø> (-0.81%) ⬇️
...coroutines/SuspendFunWithCoroutineScopeReceiver.kt 81.48% <ø> (+2.91%) ⬆️
...t/rules/coroutines/SuspendFunWithFlowReturnType.kt 83.33% <ø> (-0.67%) ⬇️
...osch/detekt/rules/bugs/AvoidReferentialEquality.kt 95.83% <ø> (+3.83%) ⬆️
...gitlab/arturbosch/detekt/rules/bugs/Deprecation.kt 86.66% <ø> (+5.41%) ⬆️
...h/detekt/rules/bugs/DontDowncastCollectionTypes.kt 78.37% <ø> (+4.01%) ⬆️
... and 53 more

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

Copy link
Contributor

@marschwar marschwar left a comment

Choose a reason for hiding this comment

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

To me this would be a good solution. As it was mentioned your other PR this would still require a custom rule to verify that a rule that is not annotated with @RequiresTypeResolution does not in fact access the BindingContext.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

I think putting the filter in core is a good idea.
I want to get rid of the mixed rule behavior rather sooner than later.
This is for sure a breaking change and we should be careful with activating this new behavior.
To avoid confusion, I'd rather change this PR to a draft in the meantime.

@BraisGabin
Copy link
Member Author

I'm moved this to Draft and I removed all the if (bindingContext == BindingContext.EMPTY) return that were not needed anymore. But this PR should not be consider a breaking change. This PR doesn't change any behaviour of any rule. The mixed rules doesn't have the @RequiresTypeResolution annotation so they are not disabled.

This PR just makes the plain-detekt faster. This should be just the first step for #3577 and it is an alternative over #5130.

@BraisGabin BraisGabin marked this pull request as ready for review August 7, 2022 10:13
@BraisGabin
Copy link
Member Author

I'm moving this back to "ready for review" because I don't see how this PR could be a breaking change.

@github-actions
Copy link

github-actions bot commented Aug 7, 2022

Messages
📖

Thank you very much for making our website better ❤️!

Generated by 🚫 dangerJS against 5c1a3b9

@cortinico
Copy link
Member

I don't see how this PR could be a breaking change.

Not technically a breaking change, but more like a behavioural change. We should make clear that @RequiresTypeResolution is now considered by detekt-core. Previously was just metadata.

@cortinico cortinico added the notable changes Marker for notable changes in the changelog label Aug 7, 2022
@BraisGabin
Copy link
Member Author

@RequiresTypeResolution is inside the internal package so if someone uses it we shouldn't care too much. I agree, we should note it but it shouldn't be a big deal.

@cortinico
Copy link
Member

so if someone uses it we shouldn't care too much

We should. As if a user wrote a rule with type resolution and haven't used the @RequiresTypeResolution we won't be running it.

The fact that lives inside the .internal package now is a bit concerning, as I believe it should be moved to io.gitlab.arturbosch.detekt.api instead (maybe we can deprecate and create a new annotation also).

@BraisGabin
Copy link
Member Author

We should. As if a user wrote a rule with type resolution and haven't used the @RequiresTypeResolution we won't be running it.

It is be the other way around. If a third party rule was written and it has the annotation @RequiresTypeResolution but it also work without it, then we should not be running it.

To me that seems a strange case, and we should be free to introduce that breaking change because no one should be using our internal api.

And I agree, we should think about moving this annotation and friends to our public api. Because of this PR and the one by @VitalyVPinchuk about creating the config file for third party. But we can do that on other pr.

@cortinico
Copy link
Member

It is be the other way around. If a third party rule was written and it has the annotation @RequiresTypeResolution but it also work without it, then we should not be running it.
To me that seems a strange case, and we should be free to introduce that breaking change because no one should be using our internal api.

Yup in this case, it makes sense.
Should we decide to implement the iif behavior (do not offer BindingContext to rules without @RequiresTypeResolution then we should move the annotation probably (and we can do it safely).

And I agree, we should think about moving this annotation and friends to our public api. Because of this PR and the one by @VitalyVPinchuk about creating the config file for third party. But we can do that on other pr.

Yup. Still worth a note in the release notes.

@BraisGabin
Copy link
Member Author

Is there something that I need to do here to move this PR forward? I think it's ready to merge but maybe I'm missunderstanding some comments that require me to change something.

@schalkms
Copy link
Member

schalkms commented Sep 4, 2022

@BraisGabin no comments or requests for change from my side

@BraisGabin BraisGabin added this to the 1.22.0 milestone Sep 4, 2022
@BraisGabin BraisGabin enabled auto-merge (squash) September 8, 2022 21:20
@BraisGabin BraisGabin merged commit 5e46099 into main Sep 8, 2022
@BraisGabin BraisGabin deleted the use-requires-type-resolution branch September 8, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api build core dependencies Pull requests that update a dependency file documentation notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants