Update: Cache files that failed linting (fixes #9948)

[platinumazure]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[x] Other, please explain: Updates --cache to store messages for files that failed linting, as well as those which succeeded in linting.

Fixes #9948.

What changes did you make? (Give an overview)

Building on #10562, this changes the cache logic to cache files that failed linting as well as files that passed linting. The rationale is that, as long as the file contents or ESLint config (including ESLint version) haven't changed, we can assume the lint results will be the same even in the failure case.

Is there anything you'd like reviewers to focus on?

Is there sufficient test coverage?

[platinumazure]

The related issue was accepted in today's TSC meeting.

@eslint/eslint-team Could this get a review? Thanks!

[not-an-aardvark]

This generally looks good, I left a few questions.

[not-an-aardvark]

How does this interact with the --fix flag?

• If the user uses the --fix flag on the initial linting run, I think the results object will have an output property rather than a source property. Should that be omitted from the cache as well?
• If the user uses the --fix-dry-run flag (or fix: true with CLIEngine), the errors returned by Linter will correspond to errors that would apply to the fixed text, even though the fixed text isn't written to the filesystem. If the next invocation does not use --fix-dry-run, then it seems like the cached results would be incorrect because they would apply to the fixed text rather than the original text. Do we need to invalidate the cache in that scenario? (This might already be a problem without this change.)

[not-an-aardvark]

Will this cause problems if the source is an empty string? That will cause this if check to fail even though the source property is present. It might be better to use an explicit hasOwnProperty check instead.

[platinumazure]

@not-an-aardvark Thanks for reviewing!

I've fixed the issue about empty source.

Regarding interaction with autofix: It looks like writing of fixes occurs after caching the lint results in the current implementation. (CLIEngine caches during executeOnFiles, whereas lib/cli.js calls CLIEngine.outputFixes after executeOnFiles returns). So it could be safe to let fixes be written after caching, because the file mtime would change and so the cache would be invalidated.

That said, I'm not super comfortable caching lint results when fixes are written, in case the flow changes.

I'm also not 100% sure yet what happens when fixes are generated but not output. I'll do some testing (in any case I'll need to make some sort of change, either to add cases for results with output or to forbid them from being cached). Hoping to have some more commits pushed today.

[platinumazure]

I've just pushed a commit that simply doesn't cache results with an output property. Maybe we can find a way to cache them safely, but I think releasing this without caching fixed files would still be a win for at least some of our users.

[not-an-aardvark]

LGTM, thanks!

[refack]

The rationale is that, as long as the file contents or ESLint config (including ESLint version) haven't changed, we can assume the lint results will be the same even in the failure case.

So we hit a case where this assumption fails, when a rule depends on a 3rd file - nodejs/node#24202.

Any fix we can apply to our rule: https://github.com/nodejs/node/blob/master/tools/eslint-rules/documented-errors.js

[not-an-aardvark]

@refack To clarify, is the issue that you made modifications to a custom rule and ESLint is returning previously-cached results from before the modification? (That sounds plausible, I just want to make sure I'm understanding correctly.)

[platinumazure]

@refack @not-an-aardvark Could we discuss this in a new issue? Thanks!

[refack]

Ack: #11060