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

Implement rule to check the correct usage of @RequiresTypeResolution #5182

Merged
merged 10 commits into from Oct 16, 2022

Conversation

BraisGabin
Copy link
Collaborator

@BraisGabin BraisGabin commented Aug 4, 2022

Fixes #5148

This rule detects all the mixed behaviour rules that we have on our project so I added them to the baseline meanwhile we don't fix them. But at least with this rule we ensure that we don't add any new mixed and we know all the current mixed rules too.


I know that the test are failing but I have no idea why the tests are failing. I debugged and

bindingContext[BindingContext.CLASS, this]
        ?.defaultType
        ?.supertypes()

Only returns a List with a single element: kotlin.Any. But when I run the rule over the code base of detekt it seems to work. Any idea?

@marschwar
Copy link
Contributor

Only returns a List with a single element: kotlin.Any. But when I run the rule over the code base of detekt it seems to work. Any idea?

It seems like that Rule and BaseRule types are not known to the test compiler. I will try to figure out a way to add them.

@BraisGabin
Copy link
Collaborator Author

It seems like that Rule and BaseRule types are not known to the test compiler. I will try to figure out a way to add them.

I thought the same. But they compile successfully.

@github-actions
Copy link

github-actions bot commented Aug 5, 2022

Messages
📖 Thanks for adding a new rule to Detekt ❤️

Generated by 🚫 dangerJS against c4bdd5c

@codecov
Copy link

codecov bot commented Sep 4, 2022

Codecov Report

Merging #5182 (c4bdd5c) into main (6274f21) will increase coverage by 85.97%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             main    #5182       +/-   ##
===========================================
+ Coverage        0   85.97%   +85.97%     
- Complexity      0     3614     +3614     
===========================================
  Files           0      514      +514     
  Lines           0    12056    +12056     
  Branches        0     2162     +2162     
===========================================
+ Hits            0    10365    +10365     
- Misses          0      618      +618     
- Partials        0     1073     +1073     
Impacted Files Coverage Δ
...otlin/io/gitlab/arturbosch/detekt/api/MultiRule.kt 100.00% <ø> (ø)
...bosch/detekt/rules/complexity/LongParameterList.kt 87.75% <ø> (ø)
...urbosch/detekt/rules/bugs/ImplicitDefaultLocale.kt 75.00% <ø> (ø)
...tlab/arturbosch/detekt/rules/bugs/LateinitUsage.kt 92.59% <ø> (ø)
...t/rules/bugs/MapGetWithNotNullAssertionOperator.kt 81.25% <ø> (ø)
...kt/rules/exceptions/InstanceOfCheckForException.kt 79.16% <ø> (ø)
...h/detekt/rules/naming/MemberNameEqualsClassName.kt 87.80% <ø> (ø)
...urbosch/detekt/rules/performance/SpreadOperator.kt 83.33% <ø> (ø)
...etekt/rules/style/FunctionOnlyReturningConstant.kt 93.75% <ø> (ø)
...sch/detekt/rules/style/UnnecessaryAbstractClass.kt 77.35% <ø> (ø)
... and 520 more

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

@BraisGabin
Copy link
Collaborator Author

This should be ready to review :)

config/detekt/baseline.xml Outdated Show resolved Hide resolved

override fun visitKtFile(file: KtFile) {
super.visitKtFile(file)
klasses.forEach { klass ->
Copy link
Member

Choose a reason for hiding this comment

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

is it possible to move the checking logic inside visitClass(klass: KtClass) to avoid maintaining klass: MutableList<KtClass>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The problem is that we have rules that uses the BindingContext inside private extension functions. So if I move it to visitClass we would hava false positives. For that reason I moved it to visitClass. I don't like the MutableList either but i don't know other way to implement it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if the check here is too sophisticated and we should just do a syntactic check on whether BindingContext or bindingContext are present as string in the body of the rule.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was my initial implementation but I got this comment: #5182 (comment) so I made it more sophisticated. I think it's better like it's now.

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.

Let's make sure we ship this inside RC2 otherwise we'll end up shipping an empty ruleset :)

config/detekt/baseline.xml Outdated Show resolved Hide resolved
import kotlin.reflect.KClass

/**
* If a rule uses `bindingContext` should be annotated with `@RequiresTypeResolution`. And if it doesn't it shouldn't
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
* If a rule uses `bindingContext` should be annotated with `@RequiresTypeResolution`. And if it doesn't it shouldn't
* If a rule uses [BindingContext] should be annotated with `@RequiresTypeResolution`. And if it doesn't it shouldn't

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here I'm talking about the bindingContext property. No the type BindingContext. I rephrased it to be more clear.


override fun visitKtFile(file: KtFile) {
super.visitKtFile(file)
klasses.forEach { klass ->
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if the check here is too sophisticated and we should just do a syntactic check on whether BindingContext or bindingContext are present as string in the body of the rule.

@BraisGabin
Copy link
Collaborator Author

@cortinico can you re-review this? I adressed some of the issues you pointed out and comment in other that I'm not 100% sure if they should be implemented.

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.

Minor typo in the rule name. Other than that, looks good to me 👍

@@ -271,3 +271,8 @@ style:
WildcardImport:
active: true
excludeImports: []

ruleauthors:
ViolateTypeResolutionRequirements:
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
ViolateTypeResolutionRequirements:
ViolatesTypeResolutionRequirements:

Copy link
Member

Choose a reason for hiding this comment

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

We also have UseX as imperative sentences though, so I don't think it is a typo.

Copy link
Member

Choose a reason for hiding this comment

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

But we also use Has (HasPlatformType) which is a plural.

To me the UseX rules (like UseRequire) should be interpreted in a sentence like "You should use require here".

While the one with plurarls should be interpreted in sentences where the subject is "the code" so like "your code has platform type" or "your code violates type resolution".

Also this is a nit, so feel free to skip it :)

@cortinico
Copy link
Member

@BraisGabin can we finalize this as I'd like to prepare RC2 tomorrow 👍

@BraisGabin
Copy link
Collaborator Author

I don't have a computer near this weekend. I'm OK with keeping the current name or adding the s.

If you want to release tomorrow feel free to change the name yourself.

@cortinico cortinico enabled auto-merge (squash) October 16, 2022 14:35
@cortinico cortinico merged commit 3f9e95a into main Oct 16, 2022
23 checks passed
@cortinico cortinico deleted the requires-type-resolution branch October 16, 2022 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New rule: check for @RequiresTypeResolution when a rule accesses BindingContext
4 participants