linter: consistent-type-definitions rule does not skip over comments and will highlight comments instead of keywords

[bradzacher]

What version of Oxlint are you using?

playground

What command did you run?

https://playground.oxc.rs/#eNptUk2P00AM/SuVj6uuFpCQUBEnlkor8SWEuBAOsxMnzDIZj2yn7SrKf8eTNlMOnJJ5/np+zxN42EGLPjrGzd3NRp8zbm7uzt/vm3ebaX7bpCbBFgh2E/CYyieGpLBTHnELHfHgVJFh17ko/yIPV0zZJSmBigSh6BTb+2W600BJavD4OyhKdh4rNLjUx+vT05AZ5VoinrKFz6TkeXikuL5811/S5i1kx1LIToAnxSQ21zRQOdmOLkY6fkMdOX0ZVUKL+zH5Qm1tVWYiH/CrCVb4ntGl7sebh6QcrKOvAUGjrcF/YCa+oMah6Hfm4Cl1wdjB1ECOY2/lDex+NlAcEM8hawO/tg1406onDljiluyJGb0mk8CABqjrGpgtkcd4zTEyYkvqbWl322IXUliUXmqOjpMVzVZ3NejMSx33aB4DyqsXL1+bNqPgfanHPfH76ET2AWNbN8VTRg6DzXLRLCVztG5s0SFoRT+hutapq3L8/xQmEJMzm6jIyZmZq4OeWuxxOcRUTq36/CQt2UFfPEmJdOm1IhH7NXfpsh7QZK/1umxzyh/xgLXpH8T82Q24XtpSakZT3JvppeKA/EiynmbZJz2ZNSVU/2aDF/mXGfVvLniRed1p/gsblUdw

What does your .oxlintrc.json config file look like?

consistent-type-definitions: 'error'

What happened?

Input:

declare /* type */ type T = {};

Expected:

declare /* type */ type T = {};
                   ^^^^ error here

Actual:

declare /* type */ type T = {};
           ^^^^ error here

Because oxc/oxlint doesn't work with tokens it is unable to distinguish between comment text and code text.

---

Side note that I suspect that if I had a bit of a poke around I could find a few bugs like this. It's the major weaknesses of relying on String::find on top of source code instead of having a CST to inspect.

[MichaReiser]

Something we're doing in Ruff, which works fine and doesn't require a whole CST, is that we store the tokens alongside the parsed AST, and we can poke into the tokens when the information in the AST isn't enough.

We also have a simplified lexer that can lex most tokens except strings. It allows us to do ad-hoc lexing when needed (e.g. what's the first token after offset XX, but skipping over comments.

A CST would obviously be nice and has some other benefits, e.g. that it enables edits that preserve source formatting and comments.

[bradzacher]

Yeah eslint doesn't use a full CST - just the tokens. It was why I began poking around here.

I've had a few people at work ask when we could look into migrating our internal eslint rules to oxlint and I knew there was a dozen or so that wouldn't work due to not having the tokens.

So I was curious how they had worked around the lack of tokens when they migrated some rules from typescript-eslint and that's when I found this bug.

The fixer for the rule also does lose comments in some locations due to the fact that the fixer replaces the entire node's range with the new type. For comparison the typescript-eslint fixer picks individual tokens to change or delete to avoid mutating the source code too much to help preserve comments.

Tokens would be a boon to making rules safer and more resilient to such bugs.

[overlookmotel]

I can see access to tokens or a CST would be useful, but our rationale is this:

Lint errors should be rare. If you get errors/warnings in a lint run, you'll tend to fix them (or disable them with an // oxlint-disable comment). So on the next run, no errors.

Therefore, the hot path in linter is "no errors in this file". Storing tokens, or constructing a CST, when none of that data will be used for the vast majority of files being linted would likely be a large drag on performance.

However, conversely, once you do hit an error, it's no problem to do a bunch of work - because that expensive branch is rarely taken. So we could look at providing better APIs to lazily tokenize (or similar), in order to make it easier to write accurate diagnostic/fix code.

[bradzacher]

For sure - the rationale makes sense.

But definitely there must be APIs that make it trivial to grab the tokens so that rule authors reach for that instead of doing things like reaching for substring matches cos it's easier.

Substring matches against code will always lead to buggy rule reports and even worse - to buggy fixers which produce broken code.

An incorrect report location isn't terrible - it's a minor quibble a user can mostly ignore.
But a broken fixer is lethal - it means a user can't trust autofix-on-save or --fix runs because they're scared their code will get broken.

From my experience working on linters - losing that trust is really hard to get back. Cos once you've broken someone's code once they can never ever be 100% sure that you won't do it again.

As an example at FB for a long time they purposely did two things:

1. forcibly disabled autofix-on-save for everyone
2. made their linter CLI show the user each fix and force them to accept it manually

They did this because they had years (in the early days of linters) where so many fixers were outright broken.
It took a lot of effort to rebuild that trust and even then I was only able to enable autofix-on-save with a small allowlist of rules.