Break the dependency of :detekt-api with :detekt-psi-utils

[BraisGabin]

Expected Behavior

:detekt-api shouldn't depend on :detekt-psi-utils.

Current Behavior

It does

Context

This idea comes from this comment: #7105 (comment)

I did a fast check and the main reason to have this dependency is because :detekt-api uses FilePath 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 use FilePath. So maybe the solution is to invert the dependency, I'm not sure.

Also this could help to unblock #7123.

[cortinico]

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 use FilePath

Which class are we talking about?

[BraisGabin]

FilePath

[3flex]

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.

[BraisGabin]

That's true! And it shouldn't be difficult to change. I'm going to give it a try.

[BraisGabin]

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 KtFiles. 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:

1. relativePath behaves as expected. When you call absolute it really returns the correct path.
2. We don't need to store basePath anywhere.

What do you think? I'm going to implement it to see how it looks like (or at least try it).

[3flex]

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:

detekt/detekt-psi-utils/src/main/kotlin/io/github/detekt/psi/KtFiles.kt

Lines 33 to 37 in 68b949f

	/*
	absolutePath will be null when the Kotlin compiler plugin is used. The file's path can be obtained from the virtual file
	instead.
	*/
	fun PsiFile.absolutePath(): Path = absolutePath ?: Path(virtualFile.path)

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:

detekt/detekt-parser/src/main/kotlin/io/github/detekt/parser/KtCompiler.kt

Lines 34 to 37 in 68b949f

	val psiFile = psiFileFactory.createPhysicalFile(
	normalizedAbsolutePath.name,
	StringUtilRt.convertLineSeparators(content)
	)

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

[3flex]

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

[BraisGabin]

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.

[cortinico]

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