Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diktat produces wrong line:column numbers in the "fix" mode if multiple rules are applied #1538

Closed
0x6675636b796f75676974687562 opened this issue Oct 28, 2022 · 2 comments · Fixed by #1769
Assignees
Labels
bug Something isn't working
Milestone

Comments

@0x6675636b796f75676974687562
Copy link
Member

Diktat 1.2.3, Ktlint 0.46.1.
Consider this code fragment:

@file:Suppress(
    "MISSING_KDOC_TOP_LEVEL",
    "MISSING_KDOC_CLASS_ELEMENTS",
    "EMPTY_BLOCK_STRUCTURE_ERROR",
)

package org.cqfn.diktat

class                                        Example{fun memberFunction(){}}

Diktat (e.g. when run from the command line) reports the following errors, all on the same line:

$ diktat --relative Example.kt
Example.kt:9:6: [TOO_MANY_CONSECUTIVE_SPACES] too many consecutive spaces: found: 40. need to be: 1 (diktat-ruleset:too-many-spaces)
Example.kt:9:53: [WRONG_WHITESPACE] incorrect usage of whitespaces for code separation: there should be a whitespace before '{' (diktat-ruleset:horizontal-whitespace)
Example.kt:9:54: [BRACES_BLOCK_STRUCTURE_ERROR] braces should follow 1TBS style: incorrect same line after opening brace (diktat-ruleset:block-structure)
Example.kt:9:54: [MISSING_KDOC_ON_FUNCTION] all public, internal and protected functions should have Kdoc with proper tags: memberFunction (cannot be auto-corrected) (diktat-ruleset:kdoc-methods)
Example.kt:9:74: [WRONG_WHITESPACE] incorrect usage of whitespaces for code separation: there should be a whitespace before '{' (diktat-ruleset:horizontal-whitespace)
Example.kt:9:76: [BRACES_BLOCK_STRUCTURE_ERROR] braces should follow 1TBS style: no newline before closing brace (diktat-ruleset:block-structure)

Now, when run in the "fix" mode, Diktat will correct all of the above errors except for the MISSING_KDOC_ON_FUNCTION:

$ diktat --relative -F Example.kt
Example.kt:9:21: [MISSING_KDOC_ON_FUNCTION] all public, internal and protected functions should have Kdoc with proper tags: memberFunction (cannot be auto-corrected) (diktat-ruleset:kdoc-methods)

As you can see, the line:column numbers will no longer be 9:54 but 9:21, because some of the rules have been already applied and some of the errors fixed. Yet, 9:21 is absolutely misleading, because the resulting (formatted) file will look like this:

@file:Suppress(
    "MISSING_KDOC_TOP_LEVEL",
    "MISSING_KDOC_CLASS_ELEMENTS",
    "EMPTY_BLOCK_STRUCTURE_ERROR",
)

package org.cqfn.diktat

class Example {
    fun memberFunction() {}
}

If we run Diktat again (now against the reformatted file), there will be 10:5 instead of 9:21:

$ diktat --relative Example.kt
Example.kt:10:5: [MISSING_KDOC_ON_FUNCTION] all public, internal and protected functions should have Kdoc with proper tags: memberFunction (cannot be auto-corrected) (diktat-ruleset:kdoc-methods)

The problem becomes even more evident after the upgrade to Ktlint 0.47.x (API Changes & RuleSet providers), because it has changed the way AST elements are traversed.

Consider there're multiple rules, ordered in a specific way:

graph LR;
    A-->B;
    B-->C;
Loading

Assuming there're multiple AST syntax nodes enclosed in each other:

graph TD;
    File-->Node;
    Node-->Leaf;
Loading

Ktlint 0.46 used to recursively apply the above rules in the following order:

flowchart TB;
  subgraph File
    direction LR;
    A1["A"]-->B1["B"];
    B1["B"]-->C1["C"];
  end
  subgraph Node
    direction LR;
    A2["A"]-->B2["B"];
    B2["B"]-->C2["C"];
  end
  subgraph Leaf
    direction LR;
    A3["A"]-->B3["B"];
    B3["B"]-->C3["C"];
  end
    File-->Node;
    Node-->Leaf;
Loading

As of Ktlint 0.47, the traversal order is different:

flowchart TB;
  subgraph A
    direction LR;
    File1["File"]-->Node1["Node"]
    Node1["Node"]-->Leaf1["Leaf"]
  end
  subgraph B
    direction LR;
    File2["File"]-->Node2["Node"]
    Node2["Node"]-->Leaf2["Leaf"]
  end
  subgraph C
    direction LR;
    File3["File"]-->Node3["Node"]
    Node3["Node"]-->Leaf3["Leaf"]
  end
    A-->B;
    B-->C;
Loading

In practice, when Diktat is run in the "fix" mode, this change in Ktlint behaviour results in the same error (e.g.: MISSING_KDOC_ON_FUNCTION) reported multiple times against different (incorrect) locations within a file.

Probably, we should change our strategy to first running KtLint.format() ignoring any errors followed by KtLint.lint(), and only report errors produced by the last lint() call.

In particular, this blocks saveourtool/benedikt#3.

@0x6675636b796f75676974687562
Copy link
Member Author

@akuleshov7, @nulls, WDYT?

@nulls
Copy link
Member

nulls commented Oct 28, 2022

@0x6675636b796f75676974687562, agree. We need to split fix and warn parts: we need to fix everything that possible in first run and then warn about left errors.

But suggestion to make this change with old version of ktlint in first phase: I think it should work for the old version too

0x6675636b796f75676974687562 added a commit that referenced this issue Oct 28, 2022
### What's done:

* ktlint 0.47: migrate to the new Baseline API.
* ktlint 0.47: make sure rule ids are always fully-qualified (pre-0.47 behaviour broken).
* ktlint 0.47: suppress deprecation warnings (`RuleSet` and `RuleSetProvider` are now deprecated, scheduled for removal in 0.48).
* ktlint 0.47: ignore the corrected errors when in "fix" mode (pre-0.47 behaviour broken).
* ktlint 0.47, test code: allow lint errors to appear in any order (because of the new AST traversal order, see #1538).
* Configure Surefire and Failsafe plugins for better reporting.
* GutHub Actions: print out stack traces (`-X`) when running the snapshot version of the Maven plug-in.
* Smoke tests: enable extra diagnostics when running `diktat` from `save-cli`.
* Smoke tests: enable extra `save-cli` verbosity (`--log all`) when running it on Windows, too.
* Smoke tests: when running `save-cli`, override the temporary directory on Windows (`TMP` and `TEMP`), because of pinterest/ktlint#1608.

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Andrey Shcheglov <[email protected]>
@0x6675636b796f75676974687562 0x6675636b796f75676974687562 added this to the 1.2.5 milestone Nov 9, 2022
@0x6675636b796f75676974687562 0x6675636b796f75676974687562 removed this from the 1.2.5 milestone Mar 17, 2023
@nulls nulls added this to the 2.0.1 milestone Nov 3, 2023
@nulls nulls modified the milestones: 2.1.0, 2.0.0 Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants