Switch order of correctable rules and other rules

[chao2zhang]

Interestingly, the previous code caused a crash in (#2693). The problem is that we run Ktlint/CommentSpacing before running LongMethod. Because CommentSpacing mutates the ktfile.text, but does not update
Ktfile.viewerProvider.document, resulting in an inconsistent state and eventually crash (#2693 (comment))

Note that this commit is merely a short-term fix. In the long term, we should be more careful in rule running order, while leveraging parallelism to optimize the runtime.

Test

I verified that the correct violations CommentSpacing are reported in the sample project (https://github.com/andrey-bolduzev/detekt_issue_2693) after the change. Before this change, it will crash with java.lang.IndexOutOfBoundsException: Wrong offset: 834. Should be in range: [0, 833]

[chao2zhang]

Fun fact: I almost spent my day debugging this. I initially thought this is a Kotlin compiler bug and started by refactoring the code to extract PsiWhitespace, then scratched my head on why PsiWhitespace is by default not provided in the parsed KtFile. Luckily I was able to debug through the stack and find the root cause.

[codecov]

Codecov Report

Merging #3317 (4b91fa4) into master (c6a971f) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #3317   +/-   ##
=========================================
  Coverage     80.27%   80.27%           
  Complexity     2676     2676           
=========================================
  Files           443      443           
  Lines          8142     8142           
  Branches       1548     1548           
=========================================
  Hits           6536     6536           
  Misses          775      775           
  Partials        831      831           

Impacted Files	Coverage Δ	Complexity Δ
...otlin/io/gitlab/arturbosch/detekt/core/Analyzer.kt	81.08% <100.00%> (ø)	9.00 <0.00> (ø)

---

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 c6a971f...4b91fa4. Read the comment docs.

[BraisGabin]

We can't merge this because it will break the html report and the position of a lot of issues. If you first run the no correctable and then the correctable ones all the issues that you found before could have change its position.

We need to find other way to fix this.

The problem is that KtFile doesn't update itself correctly. Right? So we could open an issue to jetbrains. But that could be really slow so, how could we fix it in our end? Who is using that value that is not updated? Our rules? Or the ktlint rules?

[chao2zhang]

That's a good point. Let me summarize the dilemma:

• Run non-correctable rules before running correctable ones: The position reported in the non-correctable rules may be outdated when correctable ones were mutating the KtFile content
• Run correctable rules before running non-correctable ones: Ktlint rules mutates the ASTNode directly. Therefore there can be stale references that were not updated. In our case, LinesOfCode depends on DiagnosticUtils.getLineAndColumnInPsiFile, which internally uses the class Document to find line and column in the original value before correction. In fact DiagnosticUtils is eventually invoked by many of our rules. (This was also discovered in Detekt crash on autoCorrect property #3250 (comment))

There are several ways to fix them:

1. Keep with option 1 - If the KtFile content is mutated, we announce that locations might be offsetted.
2. Keep with option 1 - Find a smart way to handle ASTNode modification.
3. Keep with option 2 - Find a smart way to update all the stale references: Although this effort might be difficult to be exhaustive, and we need to keep maintaining them when we upgrade kotlin versions.
4. Keep with option 2 - Find a smart way to refactor off methods using the stale references.

There is a collection of tangential issues related to this:
a. KtlintMultiRule is regarded as either all auto-correctible or non-autocorrectible. But in fact, it mixes auto-correctible with non-autocorrectible rules. Thus we need to split KtlintMultiRule into two sets, at least.
b. KtLint rules have certain modifiers to impose orders as MaxLineLengthRule runs as the Last rule. Some long lines may not actually exceed the line length limit after mutation from other rules. I am pretty confident that this issue exists for Detekt as well.
c. Parallelism is my perception of the intention of creating MultiRule, but I believe we could do much better here to figure out the strategy across the board. The argument here would be a Rule can report Multiple issues.

Overall autocorrection is a difficult problem. I am wondering if Ktlint owners can shed some lights (The ordering marker interface was added in pinterest/ktlint#101 and modified in pinterest/ktlint#158)

[BraisGabin]

Other option is to run the correctable ones first and, if there is any change, regenerate the KtFile. This one would be slower.

[chao2zhang]

I may not have the privilege, but is it possible to convert this PR to a discussion? If not, let's create a new discussion and close this PR

[BraisGabin]

I may not have the privilege, but is it possible to convert this PR to a discussion? If not, let's create a new discussion and close this PR

It seems that we can't convert PRs in conversations. But I think that this is better suited in an issue. This is a bug.