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
Change Request: Provide a way for rules to apply suggestions to other files #17881
Comments
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.
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. |
I think that's fine for rules like |
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)? |
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? |
Rules don't control if and when either fixes or suggestions will be applied. What use cases currently don't work for |
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 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?
I can see a couple potential ways around this:
|
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. |
The feature in question is // 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 ( 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 |
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 ESLint plugins for these styling systems will want to be able to enforce only placing Before
|
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 |
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?
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 Which reminds me of another use case for this: spell-checkers such as the I've also edited the OP to summarize these use cases & link to their comments, so they're easier to see all together. |
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. |
@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 |
@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. |
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 do you have an update on this? (#17881 (comment)) What are the next steps here? |
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. |
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. |
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 TSC Question: Is this something we want to explore? |
Some prior art: RFC: Clinching RFC comment: |
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. |
Per 2024-04-04 TSC meeting, we've decided to table the discussion until we have time to review it more thoroughly. |
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 ( 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 The question would then become: 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.
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. |
Hmm okay, back to the drawing board. |
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 runningfix()
functions when not in fix mode?Alternately, can a rule'scontext
object contain info on whether the fixer is being run? Or thefixer
passed to thefix()
function?Edit (Jan 9): #17881 (comment) shows the current proposal of enabling suggestions to specify changes to other files in limited cases:
Participation
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:
css(...)
calls to a new file (Change Request: Provide a way for rules to apply suggestions to other files #17881 (comment))The text was updated successfully, but these errors were encountered: