Closed BraisGabin closed 3 weeks ago
and that class is declared on
:detekt-psi-utils
. That have little sense, that class should be defined on:detekt-api
. But once you move that class to:detekt-api
a lot of:detekt-psi-utils
doesn't compile because they useFilePath
Which class are we talking about?
I'd love to get rid of that class. We have basePath, which should be required from tooling, or have a sensible default. If other paths are absolute, we can always generate the relative path from the absolute path and basePath. No need to store all versions of the path, nor do we need to convert anything to a FilePath instead of just using a Path.
Relative paths are only needed when outputting results. Absolute paths should always be used internally.
That's true! And it shouldn't be difficult to change. I'm going to give it a try.
My findings:
Right now on a KtFile
we store as UserDataProperty
3 different paths:
relativePath
absolutePath
basePath
For sure we don't need 3 because with 2 of them we can calculate the other. So we could reduce it to:
relativePath
basePath
But we can do it even better. basePath
is ALWAYS the same for ALL the KtFile
s. We don't need to store this value on every single KtFile
. So we can just store relativePath
and then store basePath
somewhere else.
Note: because we just have one path now on KtFile
we could rename it to just path
. But I'm going to keep calling it relativePath
here to avoid confusions.
The problem with relativePath
is that it is a Path
and if you call relativePath.absolute()
you expect to get the absolute path of it. But that's not true if basePath != System.getProperty("user.dir")
. And it is way to easy to fall on that error.
My proposal to avoid this is to change System.getProperty("user.dir")
to ensure that it is always the same as basePath
. This way we fixed two problems:
relativePath
behaves as expected. When you call absolute
it really returns the correct path.basePath
anywhere.What do you think? I'm going to implement it to see how it looks like (or at least try it).
We should actually go further, but I don't think it's possible to solve while we support autocorrect in the way we do now.
If we created each KtFile
the proper way the absolute file path would be available at KtFile.virtualFile.path
and we wouldn't need to store it again. The relative path can be generated from this & the base path. This is actually how we get the absolute path with the compiler plugin:
https://github.com/detekt/detekt/blob/68b949f3ed9965a03797d2951cce68fd7dfc87a2/detekt-psi-utils/src/main/kotlin/io/github/detekt/psi/KtFiles.kt#L33-L37
Why is it available in compiler plugin, but not when we generate them? Because we use a different method to create each KtFile
. detekt uses createPhysicalFile
here:
https://github.com/detekt/detekt/blob/68b949f3ed9965a03797d2951cce68fd7dfc87a2/detekt-parser/src/main/kotlin/io/github/detekt/parser/KtCompiler.kt#L34-L37
While the Kotlin compiler will add the source files to the KotlinCoreEnvironment then use KotlinCoreEnvironment.getSourceFiles()
which returns List<KtFile>
.
Why can't we do the same? Because KtFile
created by the compiler are not writeable, but files created by the KtPsiFileFactory are. And files have to be writeable for autocorrect to work, at least how it's currently implemented, otherwise we would get errors like:
Caused by: org.jetbrains.kotlin.com.intellij.util.IncorrectOperationException: Cannot modify a read-only file 'D:\IdeaProjects\detekt\detekt-api\src\main\kotlin\io\gitlab\arturbosch\detekt\api\internal\PathFilters.kt'.
at org.jetbrains.kotlin.com.intellij.psi.impl.CheckUtil.checkWritable(CheckUtil.java:33)
Since we now say autocorrect support is not available in detekt itself, but can be implemented in the rules or rule providers themselves, we could switch to using KotlinCoreEnvironment.getSourceFiles()
and say that rule implementers have to create writeable versions of files themselves if they want to support autocorrect.
I personally think that's a reasonable tradeoff.
We can easily get rid of FilePath
then :D
Once my PRs are merged, it's trivial to create a PR to remove the last couple of detekt-psi-utils usages from detekt-api.
However the question becomes, do we want to keep it as an api
dependency on detekt-api? Removing it means adding it as a dependency to nearly every detekt rule set, but it seems more correct to remove it.
Draft PR: #7260
do we want to keep it as an api dependency on detekt-api?
No, we don't want to keep it as a dependency. It's better to declare it on every rule module.
do we want to keep it as an api dependency on detekt-api?
No, we don't want to keep it as a dependency. It's better to declare it on every rule module.
Agree on this 👍 I'd rather have explicit dependency on each ruleset rather than having api
dependencies around
Expected Behavior
:detekt-api
shouldn't depend on:detekt-psi-utils
.Current Behavior
It does
Context
This idea comes from this comment: https://github.com/detekt/detekt/pull/7105#issuecomment-2034125369
I did a fast check and the main reason to have this dependency is because
:detekt-api
usesFilePath
and that class is declared on:detekt-psi-utils
. That have little sense, that class should be defined on:detekt-api
. But once you move that class to:detekt-api
a lot of:detekt-psi-utils
doesn't compile because they useFilePath
. So maybe the solution is to invert the dependency, I'm not sure.Also this could help to unblock #7123.