Betterer stores issueColumn as -1 for failing ESLint rules

[annaet]

Describe the bug

When an ESLint rule fails for code at the start of the line the issueColumn is stored as a -1 instead of 0.

This results in Betterer not being able to identify a reoccurring error when lines are added above the erroring code.

To Reproduce

Here is a simple repo with a reproduction of the issue: https://github.com/annaet/betterer-demo

In this branch I have applied a change that causes an update to the results file. However when you run npm run betterer it results in this error:

💥 Betterer (213ms): 1 test done! 1 test errored!
🔥 Enable eslint rules: "Enable eslint rules" got worse. (1 issue) 😔
・ 1 fixed issue in "/Users/anna.thomas/Development/betterer-demo/src/App.tsx".
・ New issue in "/Users/anna.thomas/Development/betterer-demo/src/App.tsx"!
・
・   /Users/anna.thomas/Development/betterer-demo/src/App.tsx
・    9 |  */
・   10 |
・ > 11 | /* eslint-disable */
・      | ^^^^^^^^^^^^^^^^^^^^^ Unexpected unlimited 'eslint-disable' comment. Specify some rule names to disable.
・   12 | function App() {
・   13 |   const [count, setCount] = useState(0)
・   14 |
・

Error: "Enable eslint rules" got worse. (1 issue) 😔
      at task (/Users/anna.thomas/Development/betterer-demo/node_modules/@betterer/reporter/src/components/suite/tasks.ts:66:13)
      at async /Users/anna.thomas/Development/betterer-demo/node_modules/@betterer/tasks/src/tasks/TaskLogger.tsx:73:24

1 test got checked. 🤔
1 test got worse. 😔

It reports that 1 issue was fixed and a new error was identified, instead of understanding they're the same error.

Expected behavior

I'd expect the issueColumn to be saved as 0 instead of -1 and the results file to be updated correctly when new code is added above the erroring code.

Versions (please complete the following information):

• OS: MacOS
• Betterer Version: 5.4.0
• Node Version: 20.11.1

Additional context

I was able to get the results file to updated as expected when adding new code by removing the minus 1 from the following code:
packages/eslint/src/eslint.ts#L81

const startColumn = message.column; // - 1 was removed

From what I can tell eslint indexes it's line numbers from 1, but its column numbers from 0. Which is a bit confusing! 🤷‍♀️

[phenomnomnominal]

Hey @annaet thanks for this! Looks like your repro/branch are private! Any chance you can give me access?

[annaet]

@phenomnomnominal Ha woops - should be public now!

[phenomnomnominal]

So I had a quick look at this and it's a bit confusing due to different things using different indexes for positions in a line!

If we use an "official" ESLint rulle, by adding "no-debugger": "error" to your @betterer/eslint config, add a debugger; on the line above the eslint disable comment, and running betterer --update, you'll see that it stores 0 for the start column index of the issue. This is intentional, and I believe I made the decision because TypeScript uses 0 indexed positions for it's warnings/errors. The - 1 in const startColumn = message.column - 1; is to make these things consistent.

I thought that meant that the bug must be in eslint-plugin-eslint-comments, but then I realised that the squiggle underline in VSCode is working correctly for both, so the VSCode ESLint extension must handle both somehow?

But then I looked at their code, and noticed this:

toForceLocation(location) {
    return {
        start: {
            line: location.start.line,
            column: -1,
        },
        end: location.end,
    }
},

which is used in the no-unlimited-disable rule...

ESLint then has the following:

function createProblem(options) {
    const problem = {
        ruleId: options.ruleId,
        severity: options.severity,
        message: options.message,
        line: options.loc.start.line,
        column: options.loc.start.column + 1,
        nodeType: options.node && options.node.type || null
    };

So the rule sets the start column to -1, and then ESLint sets it back to 0.
I guess ESLint interprets 0 as the warning/error being on the whole line of text, which I guess we can confirm with the ESLint extension:

Betterer then does the - 1, and stores that position in the results file (which I think is generally the correct behavior). The issue happens when it tries to convert the issue back to the real text to compare the content of the issue:

function getIssueFromPositions(
  lc: LinesAndColumns,
  issueOverride: BettererIssueOverride
): BettererIssueLineColLength | null {
  if (!isPositions(issueOverride)) {
    return null;
  }
  const [line, column, endLine, endColumn, message, overrideHash] = issueOverride;
  const start = lc.indexForLocation({ line, column }) || 0; // <---- here
  const end = lc.indexForLocation({ line: endLine, column: endColumn) }) || 0;
  const length = end - start;
  return [line, column, length, message, overrideHash];
}

That makes Betterer think that the issue starts at the first character of the file not the line, so we get different hashes for the issue, which is why they're treated as one new and one fixed issue. So I guess the fix would be:

const start = lc.indexForLocation({ line, column: Math.max(0, column) }) || 0;
const end = lc.indexForLocation({ line, column: Math.max(0, endColumn) }) || 0;

To add to all of this, there was some weirdness going on with the --cache which I had to turn off to even validate any of this. Fun!

[annaet]

Can confirm fix adding the Math.max works on my main repo 🎉

It results in me having to regenerate the results file to update the line numbers, but once that's done I am able to add new code above without any new errors being picked up.

Will you be able to publish a new version with the fix?

[phenomnomnominal]

Will close when actually released