Detekt's 2.0 architecture #2680
Replies: 37 comments
-
Nice idea to start collecting/discussing further architecture changes. A interface DetektCliWrapper {
fun execute(args: Array<String>): ExitCode
} interface RunnerProvider {
fun load(args: Array<String>, out: Appendable, error: Appendable): Runner
} interface DetektFacadeProvider {
fun load(settings: ProcessingSettings, classloader: ClassLoader?): DetektFacade
} interface DetektFacade {
fun run(files: Collection<KtFile>, context: BindingContext?): Result
fun run(file: KtFile, context: BindingContext?): Result
fun run(path: Path): Result
fun run(paths: Collection<Path>): Result
fun run(content: String, filename: String): Result
} A minimal |
Beta Was this translation helpful? Give feedback.
-
I also feel that this issue is a good starting point for collecting ideas. Thanks for that! @BraisGabin
That's the biggest pain point. For me this has the highest priority. The rest is IMHO nice to have. As a user, I'd like to have a long term stable detekt API. Users don't want to change their plugins and 3rd party rules every few months. Regarding the visibility of the API functions, I think Artur has done a good job here. Since detekt is extensible in many ways, many interfaces are exposed. |
Beta Was this translation helpful? Give feedback.
-
Probably we can address this point without any breaking change. But I don't know if it's worth it to make a refactor like that forcing us to compatibility
I agree. We should wait to have a really good reason. Or multiple medium reasons.
I agree. Detekt is really extensible. My point is that we should create clear separations between public api and private api so we can refactor all we want easier. |
Beta Was this translation helpful? Give feedback.
-
When further investing into metrics, a general interface Metric {
val type: String,
val value: Int,
val component: Component,
val threshold: Int? = null,
val isDouble: Boolean = false,
val conversionFactor: Int = DEFAULT_FLOAT_CONVERSION_FACTOR
} enum class Component {
FUNCTION,
METHOD,
CLASS,
FILE,
DIRECTORY,
PROJECT
} |
Beta Was this translation helpful? Give feedback.
-
We have two concepts to suppress issues: We could mark an issue as enum class SuppressionKind {
ANNOTATION,
COMMENT,
BASELINE
} Filtering issues could be generalized as: interface ResultMapping {
val priority: Int = Int.MAX
fun map(result: Result): Result
} Some kind of |
Beta Was this translation helpful? Give feedback.
-
To minimize changes across different modules when upgrading the kotlin compiler dependency, I propose to introduce a new module This module may include |
Beta Was this translation helpful? Give feedback.
-
Many classes/functions inside As @BraisGabin and @schalkms have pointed out a core module which can be consumed by the cli and gradle plugin is desirable. |
Beta Was this translation helpful? Give feedback.
-
Open questions (I don't know the answers):
|
Beta Was this translation helpful? Give feedback.
-
It's basically the name of the metric.
We could use it to identify |
Beta Was this translation helpful? Give feedback.
-
We should use more immutable classes. For example And then, even the private api should be immutable by default. I don't know all the cases and we can have good reasons to have some mutable ones, but in general I think that immutable is less error prune. |
Beta Was this translation helpful? Give feedback.
-
interface RuleSetProvider : Extension {
/**
* Every rule set must be pre-configured with an ID to validate if this rule set
* must be created for current analysis.
*/
val ruleSetId: String
/**
* This function must be implemented to provide custom rule sets.
* Make sure to pass the configuration to each rule to allow rules
* to be self configurable.
*/
fun rules(config: Config): Collection<Rule>
} |
Beta Was this translation helpful? Give feedback.
-
object DefaultConfig {
const val RESOURCE_NAME = "default-detekt-config.yml" |
Beta Was this translation helpful? Give feedback.
-
We should unify the package names too. |
Beta Was this translation helpful? Give feedback.
-
A signature should always include the whole function or class header to be unambiguous. The signature algorithm is in fact wrong as it does not consider the whole signature of the parents of the reported elements. See example from the testcase: Test.kt\$C\$private fun memberFun(): Int This signature includes the whole function header but not the package or extended types of test.Test.kt\$C : Any\$private fun memberFun(): Int In 99.5% of all cases this will be no problem for our users but we should consider fixing this in a major release. |
Beta Was this translation helpful? Give feedback.
-
We should think about hiding console-reports behind a I cross-reference this to #2833 |
Beta Was this translation helpful? Give feedback.
-
I expended some time trying to move filter, aliases, active and suppressing to core last week. I didn't create any PR because I didn't find any good way to do it without breaking changes. It is possible, but the code is really complex because the API is designed for that. My idea was the same as the one proposed by Artur: Create a new API for the rules. My ideas:
|
Beta Was this translation helpful? Give feedback.
-
We should consider allowing easy access to |
Beta Was this translation helpful? Give feedback.
-
We should consider removing some rules that are already handled by ktlint.
You can read a thread about this here: #2962 |
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
Yes it does. I'll open today an issue to ktlint so they can fix it. |
Beta Was this translation helpful? Give feedback.
-
I think that the interface Rule : (KtFile, CompilerInfo) -> List<Issue> or we can go even further: typealias Rule = (KtFile, CompilerInfo) -> List<Issue> |
Beta Was this translation helpful? Give feedback.
-
We need to see how far the "new" compiler frontend goes. As far as I see the PSI is still the main first representation and I think we should think about a different way to "visit" the detekt-core:
val cache: Map<T : NodeType, Set<Rule>>
for file in ktFiles:
for node in file:
val rules = cache.get(node)
rules.accept(node, context) That way we safe thousands of complete file visits and get rid of the |
Beta Was this translation helpful? Give feedback.
-
Cross-referencing #3002 (comment) |
Beta Was this translation helpful? Give feedback.
-
@arturbosch could you explaint this a bit more: "A move from rules being visitors to registering to ast nodes can help reduce analysis time too." My understanding was that if we have the visitor as a different part we could register all the desired visitors to a single KtFile and then it will run the "visiting code" once. And this way we would reduce analysis time. But I don't see how can we accomplish that using the KtFile API. I see that we use |
Beta Was this translation helpful? Give feedback.
-
Hey @BraisGabin , sorry for the late response! class MyRule : Rule {
fun visitFunction() {
// do stuff only with a KtNamedFunction node type
}
... Even so we are only visiting function the whole KtFile and all it's nodes need to be traversed. One possible improvement would be: val registar: Map<KCLass<KtElement>, Rule>
rules.forEach { registrar.put(it.node, it) }
for file in ktFiles {
file.visit(object : Visitor {
fun visitElement(elem: KtElement) {
registrar.get(elem::class)?.forEach { it.run(element) }
}
})
} The rules need to be immutable so we can parallelize the |
Beta Was this translation helpful? Give feedback.
-
Which is the slow part? The visit or calling a lot of empty functions? The problem of your approuch are the subtypes. What do you think about this: interface KtElementVisitor {
fun visit(item: KtElement) = Unit
} This way in your rule you can do something like: class MyRule : KtElementVisitor {
override visit(item: KtAnnotation) {
// Whatever
}
} And in the core we would do something like: val fastRules = rules.filter { it is KtElementVisitor }
for file in ktFiles {
file.visit(object : Visitor {
fun visitElement(elem: KtElement) {
rules.forEach { it.visit(element) }
}
})
} As in your example, this is not final code, is just to illustrate the concept. |
Beta Was this translation helpful? Give feedback.
-
I am working in a team of around 100~ Android developers. I recevied several complaints about autoCorrect is not working for all rules. It seems that autoCorrect is only working for ktlint rules, but I believe a fairly large number of rules under Given that we have not written autoCorrect rules in detekt, I couldn't safely assert that autoCorrect support is backward compatible. Therefore I can create a new issue and assign it myself to work on it. |
Beta Was this translation helpful? Give feedback.
-
Thanks for the good detailed feedback! There’s already an open issue. Please take a look at #2560 . |
Beta Was this translation helpful? Give feedback.
-
As discussed in #3274 our severity model is way to complex and brings little benefit. For 2.0 we should remove the old |
Beta Was this translation helpful? Give feedback.
-
I would recommend us to covert this to a discussion. There are a few structures annotated with |
Beta Was this translation helpful? Give feedback.
-
There are PRs and issues talking about how to arrange the code in the project for the 2.0. Some PRs moving classes and creating aliases to don't break the compatibility for "1.0" but preparing the 2.0 version.
I'm not saying that we are near to create the 2.0 version. I think that there is not a big reason right now to release a major release with breaking changes. But, anyway, we can start talking about how should be the architecture of detekt. Right now I'm mainly thinking about the gradle modules, but we can talk about other topics too.
Ideas:
For example. Right now
:detekt-gradle-plugin
depends on:detekt-cli
. I think that we should break that dependency. Both should be clients of/depend on the same module that exposes all the functionality.Should we expose all our public api in a single module or split it? Rules/post processors/metrics?/something else?
Could we hide the implementation details from that modules? I mean, could we make does modules without depend on our "core"? This way we reduce the accidental use of our private API.
Other option to avoid this problem is to create a custom rule so the third parties can use that rule and warn them if they are using our private API.
Beta Was this translation helpful? Give feedback.
All reactions