Change Request: Provide a way for rules to apply suggestions to other files

[JoshuaKGoldberg]

ESLint version

v8.56.0

What problem do you want to solve?

Porting JoshuaKGoldberg/eslint-plugin-expect-type#115 over to ESLint core: at least one community plugin -eslint-plugin-expect-type has a rule whose fixer operates on a separate "snapshot" file in the file system, not the file being linted. That rule has no native way of knowing whether ESLint is being run in --fix mode. Because fixers run even if ESLint isn't in fix mode, the fixer can't reliably know whether it should update the file snapshot.

What do you think is the correct solution?

Two thoughts:

• Can we avoid running fix() functions when not in fix mode?
• Alternately, can a rule's context object contain info on whether the fixer is being run? Or the fixer passed to the fix() function?

Edit (Jan 9): #17881 (comment) shows the current proposal of enabling suggestions to specify changes to other files in limited cases:

• Limiting this option to suggestions, not fixes?
• Add a new meta property like meta.allowSuggestingFileChanges (but with a better name)?
• Having these out-of-file changes go through fixer methods that limit the allowed actions in some way?

Participation

• I am willing to submit a pull request for this change.

Additional comments

A rules equivalent of what I proposed a year ago in eslint/rfcs#102, perhaps? 🙂

Edit (Jan 9): Later comments have filled in use cases:

• eslint-plugin-expect-type's expect-type/expect rule's $ExpectTypeSnapshot feature (Change Request: Provide a way for rules to apply suggestions to other files #17881 (comment))
• CSS-in-JS styling systems such as Panda CSS and Vanilla Extract being able to suggest moving css(...) calls to a new file (Change Request: Provide a way for rules to apply suggestions to other files #17881 (comment))
• Spell-checkers such as the cspell spellchecker project's @cspell/eslint ESLint plugin being able to suggest adding to a user dictionary (Change Request: Provide a way for rules to apply suggestions to other files #17881 (comment))

[mdjermanovic]

a rule whose fixer operates on a separate "snapshot" file in the file system, not the file being linted.

I think this is similar to #16143 (RFC eslint/rfcs#93). It didn't seem feasible because ESLint doesn't always know when the fixes are applied. The issue/RFC was about suggestion fixes, but I think the same applies to auto-fixes.

• Can we avoid running fix() functions when not in fix mode?

fix() functions are run even when not in fix mode because the API consumer might want to apply the fixes. For example, in VS Code, when user clicks "Fix this arrow-body-style problem" action, VS Code applies that individual fix, based on the data from EditInfo that was provided on the lint message.

• Alternately, can a rule's context object contain info on whether the fixer is being run? Or the fixer passed to the fix() function?

We could add info about whether ESLint is run in fix mode, but that still doesn't guarantee that the fixes will or will not be applied.

[JoshuaKGoldberg]

that still doesn't guarantee that the fixes will or will not be applied

I think that's fine for rules like expect-type/expect. Regardless of whether providing a fix() in a rule report means its fixer logic may or will run, we just need a way to make sure that the fixer logic won't run. Knowing to not provide a fix() at all would satisfy this use case.

[mdjermanovic]

In which cases the fixer logic should run? If I understand JoshuaKGoldberg/eslint-plugin-expect-type#115 correctly, the goal is to enable fixes in editors (for example, "Fix this expect-type/expect problem" popup in VS Code)?

[JoshuaKGoldberg]

Hmm, at first I was thinking that the extension could re-run ESLint, or since per eslint/rfcs#93 that seems unlikely, switching to a suggestion instead of a fix. But maybe the right solution is just to always provide a suggestion? And a rule wanting to control when a "fix" is applied generally means that logic should be a suggestion?

[mdjermanovic]

Rules don't control if and when either fixes or suggestions will be applied. What use cases currently don't work for expect-type/expect?

[JoshuaKGoldberg]

Ok, got it - yes, I can verify that both fixes and suggestions are applied immediately. I put a more standalone repro in https://github.com/JoshuaKGoldberg/repros/tree/repro-eslint-expect-type-and-fix. Both trying the rule as-is and switching what's currently a fix() to a suggestion[0].fix() result in real time updating of the snapshot file in VS Code:

Now I'm starting to think: maybe the root request here is the ability to mark fixes and suggestion fixers as having side effects? As in, marking that some logic should be run when the fix/suggestion is executed, not when generated?

Per eslint/rfcs#93 (comment):

Actually letting ESLint know that a fix / suggestion was used will be hard. VS Code doesn't provide any callback when a code action is applied. The VS Code ESLint extension basically converts fixes into VS Code's text edits and hands them over to VS Code. The user then selects one from a list and VS Code applies it. All that the extension currently gets is a change event on the document as a result.

I can see a couple potential ways around this:

• Allowing rules to indicate changes to files other than the one being linted
• Sending a feature request to VS Code to provide that "when a code action is applied" callback

[nzakas]

The design of rules intentionally doesn't let rules know when they are running in fix mode -- rules are intended to not be modal, as they should always do the same thing. Otherwise, we could get hard-to-track-down errors because they occur in fix mode and not in non-fix mode. (There's a lot of isolation designed into rules to prevent whole classes of problems.) So I'm 👎 to letting rules know that ESLint is being run in fix mode.

It seems like the larger issue is much more about fixes being applied to a snapshot rather than the file being linted which, to me, is not a problem that ESLint can or should attempt to solve. Rules were never intended to have filesystem access and definitely not to edit other files in relation to what ESLint is doing.

I think a better question to ask here is whether what the rule is currently doing actually makes sense? What is the actual problem that it's trying to solve and why is writing a separate file the answer vs. something else?

To that end, it would help to have an explanation of what the rule currently does and why.

[JoshuaKGoldberg]

What is the actual problem that it's trying to solve and why is writing a separate file the answer vs. something else?

The feature in question is eslint-plugin-expect-type's expect-type/expect rule's $ExpectTypeSnapshot feature. It allows you to place comments like // $ExpectTypeSnapshot SomeName in code, then have those comments be turned into snapshots of the corresponding TypeScript type a __type-snapshots__/*.snap.json file`:

// file.ts
declare const getTextLength: (text: string) => number;

// $ExpectTypeSnapshot FunctionIdentifier
getTextLength;

// __type-snapshots__/file.ts.snap.json
{
	"FunctionIdentifier": "(text: string) => number"
}

This is preferred in some situations over inline snapshots (// ^?, // $ExpectType because TypeScript types can get quite large & verbose. Having a separate snapshot file means they don't take up more than a single line in the source file.

The problem comes from the rule's auto-fixer. When it was first written, it was able to take advantage of ESLint not reading the text property of fixes until they're written. At some point that stopped being true: Running eslint without --fix still updates snapshots (eslint-plugin-expect-type issue #14). I worked around that no longer being usable in fix: only update type snapshots if process.argv includes --fix (eslint-plugin-expect-type PR #113). But the process.argv.includes("--fix") solution is a hacky bandaid that doesn't work in all conditions: 🐛 Bug: Type snapshots can't infer ESLint fix mode without a process.argv --fix (eslint-plugin-expect-type issue #115).

[JoshuaKGoldberg]

Ah, apologies for the double post, but I came across another use case for applying fixes or suggestions to other files.

CSS-in-JS styling systems such as Panda CSS and Vanilla Extract will typically expose a function like css to generate a CSS class name from an object. Some frameworks allow those can be inline (e.g. Panda CSS) while others require them to be in a *.css.(j|t)s file (e.g. Vanilla Extract). Some projects may desire to enforce always putting them in .css.ts -file rather than inline and re-evaluated each component render- as a de-cluttering/stylistic preference and/or as a performance optimization.

ESLint plugins for these styling systems will want to be able to enforce only placing css() calls in the separate file. Writing a fix/suggestion for those rules would therefore necessitate being able to remove from one file (e.g. MyComponent.tsx) and write to another file (e.g. MyComponent.css.ts). This is not doable in ESLint right now.

Before --fix

import { css } from "styled-system/css";

export const MyComponent = () => (
  <p className={css({ color: "foreground" })}>Hello, world!</p>
  //            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  // Put css() styles into a ./*.css.ts file. (eslint/eslint-plugin-...)
);

After --fix

// MyComponent.tsx
import * as styles from "./MyComponent.css.ts";

export const MyComponent = () => (
  <p className={styles.myComponent}>Hello, world!</p>
);

// MyComponent.css.ts
import { css } from "styled-system/css";

export const p = css({
  color: "foreground",
});

[nzakas]

Thanks, that context is very helpful.

So, I think writing to a different file from a rule is a dangerous use case -- one of the implicit guarantees of ESLint is that rules won't have side effects. Not only will this not work in runtimes other than Node.js (in the future, we'd like ESLint to run on Deno and in the browser), but there's no way a user could infer that enabling a rule could cause filesystem access outside of the file being linted.

For expect-type, fundamentally this doesn't seem like a linting operation at all -- it's code generation. Which, to me, is not what rules are for, and further, not what ESLint is for. This seems like a completely separate process that is only incidentally related to ESLint in that you want to know if the file has changed. And so it seems like what you really need is some kind of utility that can notice when a file has changed and regenerate the associated type snapshots. Does that seem like an accurate description?

[JoshuaKGoldberg]

dangerous use case -- one of the implicit guarantees of ESLint is that rules won't have side effects

Agreed. I think in my mind the danger of being able to write to separate files is worth it in these cases. But maybe there are ways to make a relatively safer way for rules to do this?

• Limiting this option to suggestions, not fixes?
• Add a new meta property like meta.allowSuggestingFileChanges (but with a better name)?
• Having these out-of-file changes go through fixer methods that limit the allowed actions in some way?

... it seems like what you really need is some kind of utility that can notice when a file has changed and regenerate the associated type snapshots. Does that seem like an accurate description?

I don't think so, no. Users of the type snapshots use them as assertions - akin to test snapshots. Sometimes folks want snapshots to update automatically (e.g. Vitest's -u CLI parameter). Sometimes folks want them to update only on-demand (e.g. Vitest's pressing u during watch mode). Sometimes folks want them to need manual editing.

Which reminds me of another use case for this: spell-checkers such as the cspell spellchecker project's @cspell/eslint ESLint plugin. Typical editor extensions for spell checkers often provide suggestions to add unknown words to one of several dictionaries. Implementing these suggestions in an ESLint rule would require being able to suggest writes to arbitrary files.

I've also edited the OP to summarize these use cases & link to their comments, so they're easier to see all together.

[coderaiser]

So, I think writing to a different file from a rule is a dangerous use case -- one of the implicit guarantees of ESLint is that rules won't have side effects...

...but there's no way a user could infer that enabling a rule could cause filesystem access outside of the file being linted.

What if Filesystem was represented as JavaScript AST? And all manipulations on files was stubbed during development and only works when user provides a flag. In this case implementing of both examples provided by @JoshuaKGoldberg would be just manipulation with JavaScript file:

Before:

({
    "filepath": "/",
    "type": "directory"
    "files": [{
        "type": "file",
        "content": "declare const getTextLength: (text: string) => number;",
        "filepath": "/file.ts"
    }]
})

After:

({
    "filepath": "/",
    "type": "directory"
    "files": [{
        "type": "file",
        "content": "declare const getTextLength: (text: string) => number;",
        "filepath": "/file.ts"
    }, {
       "type": "directory",
       "filepath": "/__type-snapshots__",
       "files": [{
           "type": "file",
           "filepath": "__type-snapshots__/file.ts.snap.json"
       }]
    }]
})

To manipulate files a new API can be made to dial with Filesystem like: create file, put the data to the file, remove directory etc...

Maybe it is a bit out of scope of ESLint, but the main idea is AST based manipulation - field ESLint operates on all the time.

[nzakas]

@coderaiser that's an interesting idea, but I think is overcomplicating the problem.

@JoshuaKGoldberg I'll need to think about this some more. As I said, this is definitely not what rules were designed to support, and I still feel tension between the implicit promise that rules don't have side effects and what you're attempting to do here. At a minimum, I'd want there to be some way for users to know that a rule is doing unexpected things to their filesystem. I know we can't actually prevent people from doing filesystem calls inside of rules but perhaps there could be some blessed way to do so. In any case, I don't think piggybacking on top of the fix() method, which is only intended to fix the currently linted file, is the correct approach.

[nzakas]

fix() functions are run even when not in fix mode because the API consumer might want to apply the fixes. For example, in VS Code, when user clicks "Fix this arrow-body-style problem" action, VS Code applies that individual fix, based on the data from EditInfo that was provided on the lint message.

@mdjermanovic is this true for both fixes and suggestions? It seems like we should provide a flag that people need to pass in order to generate one or both, as that's an unnecessary perf hit if that info isn't being used. (I seem to recall at one point that fixes were only generated when running in fix mode, but maybe that changed?)

[mdjermanovic]

@mdjermanovic is this true for both fixes and suggestions? It seems like we should provide a flag that people need to pass in order to generate one or both, as that's an unnecessary perf hit if that info isn't being used. (I seem to recall at one point that fixes were only generated when running in fix mode, but maybe that changed?)

Yes, ESLint always generates all fixes and all suggestions since we don't know if they are needed or not.

Related issue: #14637.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[Rec0iL99]

@nzakas do you have an update on this? (#17881 (comment)) What are the next steps here?

[nzakas]

I haven't had any time to revisit exploring this yet. I'm still not sure this is something ESLint should be encouraging, so I'd like more feedback from @eslint/eslint-team as well.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[nzakas]

TSC Summary: This issue requests to formalize the ability of ESLint rules to make changes to files other than the file currently being linted. This can happen when a plugin is managing a separate file, such as eslint-plugin-expect-type, which generates type information and stores it in a snapshot file.

TSC Question: Is this something we want to explore?

[nzakas]

Some prior art:
#16143

RFC:
eslint/rfcs#93

Clinching RFC comment:
eslint/rfcs#93 (comment)

[fasttime]

It seems that RFC 93 was closed because of limitations in the VS Code ESLint extension, one of the "interactive environments" mentioned in the motivation section, which would not have been able to benefit from the change.

Now, the problem of updating file snapshots described here looks like a different use case that would still benefit from the solution proposed in the RFC, and it may be worth to reconsider it in this discussion.

I'm not in favor of adding the ability to apply changes to different files directly in ESLint, because that is a specific requirement that would be hard to generalize in a way to make it useful to other implementations. It would also shift the burden of maintenance for this feature and its many details onto ESLint. A more rationale approach would be to notify rules of a fix and let them take appropriate action.

[sam3k]

Per 2024-04-04 TSC meeting, we've decided to table the discussion until we have time to review it more thoroughly.

[nzakas]

I was thinking about this some more, and I'm wondering if we are focusing too much on the "write to other files" aspect of this proposal. We may benefit from thinking about this more abstractly. Specifically: what would it mean for a rule to be able to store data in between ESLint sessions?

In both of the examples (eslint-plugin-expect-type and cspell/eslint), the real goal is to have a database that is built up over time and used over multiple ESLint sessions. The way this is working right now is through saving to files, but I'm curious if saving to files is really the only solution to this problem?

As a thought experiment, what if ESLint provided a persistent key-value store to rules? It would be up to ESLint to determine how best to persist that data, which would mean it could write to files or, if in an environment where filesystem access was not present (i.e. a browser), it could store the info in memory or maybe using localStorage.

The question would then become: how necessary is it for users to be able to check-in and version control that data?

[JoshuaKGoldberg]

how necessary is it for users to be able to check-in and version control that data?

For both of those two examples, very necessary in file system uses.

• cspell/eslint: its cspell.json file is a general dictionary used by all cspell integrations. The lint rule must save to it so CSpell's APIs can pick up on it.
• eslint-plugin-expect-type: snapshots should be reviewable alongside their file changes.

In general for both dictionaries and snapshots, I'd expect to be able to include their changes alongside any other file changes in Git. If a pull request touches something that incurs a change to either, it'll likely need to be reviewed alongside the other changes.

I do like the thought experiment & abstraction of the persistence. One common use case is playgrounds such as eslint.org/play and typescript-eslint.io/play. I could see the playgrounds providing functions that do playground-specific actions instead of file system actions.

Tangentially relevant: https://www.typescriptlang.org/dev/typescript-vfs is a good reference for virtual file systems. We've been wanting to use it on typescript-eslint.io/play for a while.

[nzakas]

Hmm okay, back to the drawing board.