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

Detect type resolving rules run without type resolution classpath set #3577

Closed
chao2zhang opened this issue Mar 19, 2021 · 12 comments · Fixed by #5226
Closed

Detect type resolving rules run without type resolution classpath set #3577

chao2zhang opened this issue Mar 19, 2021 · 12 comments · Fixed by #5226

Comments

@chao2zhang
Copy link
Member

This is raised from #3537 (comment).

  1. If the type resolution classpath is not set, but there are active type resolving rules, we should print out warnings to remind users.
  2. We can also skip type resolving rules if type resolution classpath is not set.
  3. We probably need to wait for Use actual Kotlin annotation class to provide metadata to rules #3562 to be implemented.
@cortinico
Copy link
Member

We can also skip type resolving rules if type resolution classpath is not set.

We're basically already doing it, no?

@chao2zhang
Copy link
Member Author

What I was looking for Rule.isActive() should be false for those type resolving rules without classpath set. In SARIF output, we should show the active rules run.

@github-actions
Copy link

github-actions bot commented Nov 4, 2021

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Nov 4, 2021
@cortinico
Copy link
Member

  1. we should print out warnings to remind users.

I'm unsure about this. We might have users that are fine having rules enabled only on detektMain and not on detekt. Perhaps the warning should be configurable?

I'm also wondering what was the original use case of this feature request.

@github-actions github-actions bot removed the stale label Nov 5, 2021
@BraisGabin
Copy link
Member

I think that here we have two different issues:

  • One related with sarif. But right now we don't list any rule at all (SARIF report should include active rules for ReportingDescriptor #3554). So I think that we should care about that first. Then we can filter the ones that are active but couldn't run.
  • The other issue is related with performance. But I'm not sure if we want to expend time improving the performance of "plain detekt" because it is already fast.

So, to me we could close this issue for now. What do you think @chao2zhang?

@github-actions
Copy link

github-actions bot commented Feb 4, 2022

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

@github-actions github-actions bot added the stale label Feb 4, 2022
@cortinico
Copy link
Member

  1. If the type resolution classpath is not set, but there are active type resolving rules, we should print out warnings to remind users.

Following up here: we could add a warning when we do the if bindingContext is empty check that instead of just returning it's printing something like "Rule xyz was skipped because Type Resolution is not available".

@github-actions github-actions bot removed the stale label Feb 5, 2022
@github-actions
Copy link

github-actions bot commented May 6, 2022

This issue is stale because it has been open 90 days with no activity. Please comment or this will be closed in 7 days.

@BraisGabin
Copy link
Member

I agree with cortinico. Maybe we could create an iniline function that would make that. Check, print the warning and stop the rule (as it does now)

@eygraber
Copy link
Contributor

I'm unsure about this. We might have users that are fine having rules enabled only on detektMain and not on detekt. Perhaps the warning should be configurable?

Yes please 😄

My detekt runs just got very noisy with 1.22.0-RC1. I have one config file that contains all the enabled rules for my project, but I sometimes run detekt without type resolution.

@BraisGabin
Copy link
Member

Could you open this as a new issue? You can link there this issue to haver better context.

@eygraber
Copy link
Contributor

#5334

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants