detekt icon indicating copy to clipboard operation
detekt copied to clipboard

Introduce "analysis mode" flag to CLI

Open 3flex opened this issue 2 years ago • 9 comments

Expected Behavior

I can use rules that use type resolution without passing a classpath explicitly.

Current Behavior

Type resolution is only enabled when a classpath is passed in.

Context

detekt assumes that if no classpath is passed in, that type resolution & full analysis isn't possible. This isn't true though - we call configureJdkClasspathRoots unconditionally: https://github.com/JetBrains/kotlin/blob/v1.9.23/compiler/cli/cli-base/src/org/jetbrains/kotlin/cli/jvm/config/JvmContentRoots.kt#L91-L103

It's possible to have a module that has no Kotlin dependencies at all, but depends on the JRE, or it might not have any dependencies at all. This is rare but detekt should properly handle this.

We could have a switch or flag to enable or disable full analysis mode so the intention is clear.

3flex avatar Apr 12 '24 22:04 3flex

For discussion @detekt/maintainers:

  1. Should we have a separate mode to enable autocorrect? It would replace the "auto-correct" flag. That probably will mean that third party rule sets can't have a rule that requires type resolution and also auto corrects... but do we care about that? We don't for any of our rules.
  2. What are the values that should be passed to enable the 2 (or 3) different analysis modes? Maybe "fast", "complete" (and "autocorrect")?
  3. What's the default mode for detekt 2.0? I can't see any good reason to change from today's behaviour i.e. it doesn't use type resolution by default but let's all be clear on this. The Gradle plugin can do its own thing and use type resolution mode since it can configure everything properly easily, but CLI requires a lot more effort to get the right command.

3flex avatar Jun 23 '24 06:06 3flex

  1. I would not disable a useful feature (I wrote rules which needs type resolution AND supports auto-correct).
  2. Does it make sense to conflate auto-correct with type resolution at all? They seem orthogonal.
  3. Keep it as simple as possible, document the rest :)

TWiStErRob avatar Jun 23 '24 12:06 TWiStErRob

I agree with Rob

  1. I would not disable a useful feature (I wrote rules which needs type resolution AND supports auto-correct).
  2. Does it make sense to conflate auto-correct with type resolution at all? They seem orthogonal.
  3. Keep it as simple as possible, document the rest :)

schalkms avatar Jun 23 '24 18:06 schalkms

Noted - last thing is, what do we call the two analysis modes?

3flex avatar Jun 23 '24 21:06 3flex

"Type resolution" seems like an obvious choice. Would it make sense to just make it a boolean flag? Considering we have no clue what a 3rd option might be, it might be also orthogonal, so YAGNI?

If that's not an option, we talked about swapping the default last year, maybe full and lite are good enough? CLI defaults to lite, Gradle defaults to full, both which makes sense given the amount of info available.

TWiStErRob avatar Jun 24 '24 00:06 TWiStErRob

"Type resolution" seems like an obvious choice.

Only if you know what type resolution is... IMHO we could simplify the language and terminology around that, most probably don't care to know what type resolution actually means. There's only one reference to that term in official Kotlin docs (How KSP models Kotlin code) so it's not something users are familiar with.

That's why fast/complete or light/full seem like better options, then we can explain the tradeoffs in docs and only mention type resolution then.

3flex avatar Jun 24 '24 01:06 3flex

I agree with no use "type resolution" exactly for the same reason.

I don't like fast. It doesn't explain that the check will be not as good as the other. "full/light" sound good.

I remember that @cortinico had some good ideas about these names too. But we talked about this topic in so many issues that I have no idea where to find them.

BraisGabin avatar Jun 24 '24 10:06 BraisGabin

Makes sense, full/lite naming is good then. Throwing in an alternative: strong/weak (to stay with typing terms). Maybe the ideas/names are in the meeting notes doc from last year?

Note: my boolean question still stands regardless of name.

TWiStErRob avatar Jun 24 '24 12:06 TWiStErRob

I prefer different names instead of a Boolean. They are more descriptive, I think.

BraisGabin avatar Jun 24 '24 14:06 BraisGabin