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

Improve binding context management #5130

Closed

Conversation

BraisGabin
Copy link
Collaborator

This is the first step to work on #3577

We had different ways/places to disable a rule to don't run when it doesn't have a proper bindingContext. But it wasn't a very efficient way because we were checking it while we where traversing the PSI tree. detekt has this visitCondition that let to any rule to decide if it should be executed or not. This should be way more performant because we disable the rule before we start traversing the PSI tree.

And the new rule ensures that we make this in a consistent way.

This PR uses the new detekt-authors module implemented on #5129

@BraisGabin
Copy link
Collaborator Author

🤔 ArrayPrimitive is an "interesting" case. It's annotated with @RequiresTypeSolving but actually is a rule with mixed behavior (#2994). Here we have two options: Or we remove the annotation and keep the rule with its mixed behaviour or we remove the mixed behaviour and pass it to a true @RequiresTypeSolving rule.

@marschwar
Copy link
Contributor

I like to idea of removing this boilerplate check, but:

  • Are you sure there is only one rule with mixed behavior. We had so many bug reports where this was the cause and it feels like we complained about this much more often than just for one rule.
  • If we do this, I propose to change the retention of the RequiresTypeResolution annotation so that we can extract it from the class file and remove the rule in the Analyzer or Rule#visitCondition

@BraisGabin
Copy link
Collaborator Author

  • Are you sure there is only one rule with mixed behavior. We had so many bug reports where this was the cause and it feels like we complained about this much more often than just for one rule.

No, there are more rules with mixed behavior. But those are not annotated with RequiresTypeResolution because those rules doesn't require it. The problem is this rule that doesn't require it but it claims so.

  • If we do this, I propose to change the retention of the RequiresTypeResolution annotation so that we can extract it from the class file and remove the rule in the Analyzer or Rule#visitCondition

I was thinking about that too. But that would force us to use reflection apis. I'm not against. It would be a way cleaner approach if we are fine with the usage of reflection.

@marschwar
Copy link
Contributor

Why would we not use reflection? Because of a runtime performance penalty?

@BraisGabin
Copy link
Collaborator Author

Why would we not use reflection? Because of a runtime performance penalty?

To be honest, because I'm an Android developer and reflection is the devil there. But you are completely right, I don't see a good reason to don't use reflection here. That would make this rule completely useless and that's good! The code that doesn't exist doesn't have bugs :) I'm keeping this open for now but I'll open other PR implementing the reflection idea. If we merge the other one I'll close this one.

*
* <noncompliant>
* @@RequiresTypeResolution
* class MyRule(config: Config = Config.empty) : Rule(config) {
Copy link
Member

Choose a reason for hiding this comment

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

My 2 cents here: shouldn't we just instead have a Rule subclass or a parameter to the Rule constructor, or a property inside Rule that users can override?

While a generic visitCondition works, it does not prevent from rules accidentally using TR and forgetting to add the @RequiresTypeResolution annotation.

Copy link
Contributor

Choose a reason for hiding this comment

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

While a generic visitCondition works, it does not prevent from rules accidentally using TR and forgetting to add the @RequiresTypeResolution annotation.

You are right. That is why #5148 is still needed

@BraisGabin
Copy link
Collaborator Author

I'm closing this one. #5176 is clearly a better solution.

@BraisGabin BraisGabin closed this Aug 4, 2022
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.

None yet

3 participants