Support relative output paths

[chao2zhang]

This addresses #2492 and a part of #3045.

Out of scope items

• Actually integrate with SARIF in Github actions
• Change the default value for the base path to be the project root
• Move SARIF support from experimental to formal
• Update docs about enabling SARIF support + Github actions
• Update CodeSmell messages where the absolute path is used

Implementation

The implementation was done by putting the relative path and base path as user data of KtFile.
OutputReports read them through the paths stored in Location.

Test

In addition to adding tests in Spek, I also perform manual tests with a local repository.

[codecov]

Codecov Report

Merging #3319 (f887ff9) into master (945f075) will decrease coverage by 0.04%.
The diff coverage is 69.23%.

@@             Coverage Diff              @@
##             master    #3319      +/-   ##
============================================
- Coverage     80.28%   80.24%   -0.05%     
- Complexity     2714     2721       +7     
============================================
  Files           445      445              
  Lines          8196     8240      +44     
  Branches       1558     1564       +6     
============================================
+ Hits           6580     6612      +32     
- Misses          778      785       +7     
- Partials        838      843       +5     

Impacted Files	Coverage Δ	Complexity Δ
.../kotlin/io/gitlab/arturbosch/detekt/api/RuleSet.kt	70.00% <0.00%> (+6.36%)	3.00 <1.00> (-1.00)	⬆️
.../arturbosch/detekt/core/tooling/ParsingStrategy.kt	33.33% <0.00%> (+6.06%)	0.00 <0.00> (ø)
...tlab/arturbosch/detekt/DetektCreateBaselineTask.kt	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
...tlab/arturbosch/detekt/extensions/DetektReports.kt	41.17% <ø> (ø)	6.00 <0.00> (ø)
.../io/gitlab/arturbosch/detekt/invoke/CliArgument.kt	0.00% <0.00%> (ø)	0.00 <0.00> (ø)
.../main/kotlin/io/gitlab/arturbosch/detekt/Detekt.kt	27.95% <25.00%> (-0.14%)	19.00 <1.00> (+1.00)	⬇️
...io/github/detekt/report/sarif/SarifOutputReport.kt	90.62% <25.00%> (-9.38%)	4.00 <0.00> (ø)
...o/gitlab/arturbosch/detekt/internal/SharedTasks.kt	60.86% <50.00%> (-1.04%)	0.00 <0.00> (ø)
...ls/src/main/kotlin/io/github/detekt/psi/KtFiles.kt	75.00% <76.92%> (+30.55%)	0.00 <0.00> (ø)
...io/gitlab/arturbosch/detekt/core/KtTreeCompiler.kt	70.00% <80.00%> (+6.66%)	14.00 <0.00> (ø)
... and 13 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 945f075...f887ff9. Read the comment docs.

[schalkms]

I quickly skimmed through the code. The PR looks good to me.
Thanks for picking this up. Relative paths are a highly requested detekt feature.
I'm contemplating about adding more end-to-end tests in this area of detekt, since you also mentioned that you've done manual tests (thanks btw). For regression purposes it definitely makes sense. Could some of those manual tests be automated?

[chao2zhang]

I'm contemplating about adding more integration tests in this area of detekt

This PR definitely added some integration tests in DetektTaskDslTest.

However, if you indicate end-to-end tests, I only see DetektTaskIntegrationTest in the codebase. And there is no integration test written at the moment.

In my mind, such end-to-end tests should be a separate module or repo that applies detekt gradle plugin, runs against some kotlin code with violations, and validates the error can be reported. This does not exist today but I definitely can help sparkle the discussion

[schalkms]

However, if you indicate end-to-end tests, I only see DetektTaskIntegrationTest in the codebase. And there is no integration test written at the moment.

In my mind, such end-to-end tests should be a separate module or repo that applies detekt gradle plugin, runs against some kotlin code with violations, and validates the error can be reported. This does not exist today but I definitely can help sparkle the discussion

Yes, I definitely meant end-to-end tests. Thanks for the hint. End-to-end tests are a separate topic and as you mentioned outside of the scope for this PR.

[BraisGabin]

🤔 I didn't have time to tests it but if you add the workingDir the console report keeps reporting the absolute path. Right?

And, should we wait to merge #3320 before merging this? To ensure that we don't break compatibity.

[BraisGabin]

🤔 couldn't we get the base path from the CliArgs?

[chao2zhang]

Is AstPrinter considered as output? It sounds like yes, please confirm :)

[BraisGabin]

Yes, this command prints a file's psi tree.

[chao2zhang]

On second thought, this seems to conflict with #3319 (comment). AstPrinter also outputs to console.

[chao2zhang]

🤔 I didn't have time to tests it but if you add the workingDir the console report keeps reporting the absolute path. Right?

And, should we wait to merge #3320 before merging this? To ensure that we don't break compatibity.

I think #3320 is ready to go. I will handle the merge once that one is in.

[chao2zhang]

Apologize for the numerous force-push. I was debugging a CI failure that is not reproducible on local.

[chao2zhang]

There is a new problem that I encountered in this PR.

A) In #3319 (comment), we are suggested to set the default relative path base to the project source dir
B) The implementation goes that the detekt gradle plugin will generate the extra args to invoke DetektCli, which means Main.kt need to be able to recognize the new parameter --report-base-path.
C) However, the tests using GradleRunner/gradle-testkit are actually not dependent on the local versions of detekt, but actually performs resolution through all the repositories.
D) This explains why I was able to pass the test locally because I ran ./gradlew publishToMavenLocal before ./gradlew :detekt-gradle-plugin:test. My maven local contains my change while when we run the pre-merge check on CI, publishToMavenLocal was not run.

So to unblock this PR, we can fix any one of the steps listed here:
A) By default, we still remain the old absolute path behavior. The new gradle extension for reportBasePath remains empty if there is no provided value.
B) We create a PR to add --report-base-path parameter to CliArgs, release the next Detekt version to maven/jcenter, then merge in this PR.
C) I have been juggling with [testkit with classpath injection] (https://docs.gradle.org/current/userguide/test_kit.html#sub:test-kit-classpath-injection), but still cannot get it to work with the current setup. The most hopeful try is simply adding implementation(project(":detekt-cli")) to dependencies.
We should at least separate the GradleRunner/gradle-testkit tests into at least a separate Test task, more to be discussed in #3324
D) Run publishToMavenLocal in pre-merge-checks so that GradleRunner/gradle-testkit tests will run against the jar from maven local.

Among all the options,
A) is a reasonable choice.
C) #3324 is the correct approach but will block this PR for quite some time.
D) feels a short-term patch if #3324 is not resolved.
B) is our last straw if none of A/C/D are agreed and implemented.

Apologize for the long response. I am seeking thoughts from all the maintainers, especially @BraisGabin since A) is per his request.

[chao2zhang]

If the default value adds to much complexity we can merge this witout it and open an issue so we can find the best option to add it. What do you think?

It sounds like we are on the same page. What I probably need to do next is to prototype #3324. Since that would be impacting future changes and testing for detekt-gradle-plugin.

[BraisGabin]

This is good to go for me. My last concern (sorry the PR is big and each time I review it I realiced different things) is the cli flag name: --report-base-path. Right now we use the relative paths just for reporting but this feature is not just about that so, maybe, we could rename it to something more generic as --base-path. What do you think? Any way, this is not a blocking topic.

[chao2zhang]

This is good to go for me. My last concern (sorry the PR is big and each time I review it I realiced different things) is the cli flag name: --report-base-path. Right now we use the relative paths just for reporting but this feature is not just about that so, maybe, we could rename it to something more generic as --base-path. What do you think? Any way, this is not a blocking topic.

I don't know other use cases of base path. The input paths can be both absolute and relative based on user.dir, and internally we are always using absolute paths. If we want to expand this idea, we could choose to add a different option specifically.

[BraisGabin]

This is good to go for me. My last concern (sorry the PR is big and each time I review it I realiced different things) is the cli flag name: --report-base-path. Right now we use the relative paths just for reporting but this feature is not just about that so, maybe, we could rename it to something more generic as --base-path. What do you think? Any way, this is not a blocking topic.

I don't know other use cases of base path. The input paths can be both absolute and relative based on user.dir, and internally we are always using absolute paths. If we want to expand this idea, we could choose to add a different option specifically.

Posted in our slack channel: https://kotlinlang.slack.com/archives/C88E12QH4/p1609854726084000?thread_ts=1609854726.084000&cid=C88E12QH4

A user asking exactly for this feature and it's not related with the reports. So, probably, we should rename the flag.

[sschuberth]

A user asking exactly for this feature and it's not related with the reports. So, probably, we should rename the flag.

That user was me 😀 To clarify, I need to write a custom rule that checks for a project's naming conventions based on the relative path of source files, where the base is the Gradle root project directory. So my assumption would be that after this PR is merged, a call to the PsiFile.relativePath() extension function from the io.github.detekt.psi package would return exactly that relative path when using the detekt plugin for Gradle. Right now, a call to PsiFile.relativePath() just returns the file name (and extension) without any path.

Is my assumption / expectation about the goal of this PR correct?

[BraisGabin]

Yes, your expectations are correct. You will need one extra line of configuration in your gradle because of this #3319 (comment) (it will be implemented in later PR). But you would be able to get the relative path using a extension function over the KtFile.