Skip to content
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: Add an /* eslint-ignore-file */ directive #18028

Closed
1 task
jeengbe opened this issue Jan 24, 2024 · 16 comments
Closed
1 task

Change Request: Add an /* eslint-ignore-file */ directive #18028

jeengbe opened this issue Jan 24, 2024 · 16 comments
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed

Comments

@jeengbe
Copy link

jeengbe commented Jan 24, 2024

ESLint version

n/a

What problem do you want to solve?

Many libraries that generate code (such as tsoa, typescript-swagger-api, graphql-codegen, the grpc library, etc.) generate leading /* eslint-disable */ directives at the top of generated files. However, the files are still parsed and validated (see here), which slows down the linting process needlessly because potentially large and complex files are processed.

This is not an issue if the file is ignored by e.g. .eslintignore.

What do you think is the correct solution?

Add an /* eslint-ignore-file */ (or similar) that gives files the ability to ignore themselves. This is useful for the above mentioned automatically generated source files in whose linting errors the user very seldom has interest.

The directive would error if it appeared anywhere other than the top of a file.

Participation

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

Additional comments

No response

@jeengbe jeengbe added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint labels Jan 24, 2024
@mdjermanovic
Copy link
Member

A problem with this is how to find the comment if the file is not parsed.

The directive would error if it appeared anywhere other than the top of a file.

How would we define the top of a file? For example, can some other comments appear before /* eslint-ignore-file */?

@jeengbe
Copy link
Author

jeengbe commented Jan 24, 2024

Just sharing crude ideas: In order to skip parsing entirely, it'd need to work with plaintext, perhaps a good solution would be something like:

for line of source file:
  if line is not empty and does not start with "/":
    parse file()
  if line is "/* eslint-ignore-file */":
    skip file parsing()

I'm not exactly sure of how it works with TypeScript ESLint, but I assume that TSC is going to parse the file anyway, but we can skip ESLint's AST at least. :)

It can't require it to be the first like as there are

@fasttime
Copy link
Member

Could you explain the motivation for this request? Is it to save the time spent by the parser, or to avoid errors with files that cannot be parsed? What is the issue with ignoring those files per .eslintignore or config (which also avoids reading the files in the first place)?

@jeengbe
Copy link
Author

jeengbe commented Jan 26, 2024

The immediate motivation for this issue is this bug in a rule that crashes ESLint when the file is processed. A speed boost is a tiny benefit on the side.

Functionally, there is no difference to using an .eslintignore file. The difference is rather the notion of whose responsibility it is to ignore the file. The change allows file generators to declare their files as "do not check me", instead of the workspace declaring the files as not to be checked.

Practically, this is already done. Most generated source files start with a

/* eslint-ignore */
/* tslint-ignore */
/* istanbul ignore file */

prelude that tries to achieve this.

@fasttime
Copy link
Member

The immediate motivation for this issue is this bug in a rule that crashes ESLint when the file is processed.

It's not unusual for third-party plugins to crash ESLint. If this only happens for a particular file, it's okay to ignore that file until the bug is fixed, for example in .eslintgnore if that's what you're using. The fact that that file contains an /* eslint-disable */ comment is irrelevant here, because that comment never caused the file to be ignored (despite the intentions of the author).

The difference is rather the notion of whose responsibility it is to ignore the file. The change allows file generators to declare their files as "do not check me", instead of the workspace declaring the files as not to be checked.

That's a reasonable point, but I see two problems in the suggested solution:

  1. It's prone to false positives. For example, if such an /* eslint-ignore-file */ comment appears in a template string or somewhere in an .md file, then the whole file would be ignored.
  2. It doesn't stop ESLint from reading any files, which is a performance issue. Users should be still advised to ignore those autogenerated files in their config for best results, so it's not like providing an alternative to what is there.

@jeengbe
Copy link
Author

jeengbe commented Jan 27, 2024

It's prone to false positives. For example, if such an /* eslint-ignore-file */ comment appears in a template string or somewhere in an .md file, then the whole file would be ignored.

I would say it's reasonable to require this comment to appear at the top of the file (see a naive implementation in the above comment). If such a comment appears without any preceding source code, I can't come up with any scenario in which this would result in a false positive.

Or perhaps I'm misunderstanding what you mean, are you thinking about embedded JS in a markdown file that would falsely ignore the whole file? I would consider that a rare edge-case that can be handled separately.

It doesn't stop ESLint from reading any files, which is a performance issue. Users should be still advised to ignore those autogenerated files in their config for best results, so it's not like providing an alternative to what is there.

I'm not sure what you mean with this comment. On one hand, the performance wouldn't get worse: currently, every file is read+parsed, with such a comment, there's at least the possibility for files to be skipped. If the issue is about reading the file from disk twice (once to scan for the comment, twice to parse the code), I would defer that as an implementation detail that can be considered once a general decision about the feature has been made. 🙂

@fasttime
Copy link
Member

fasttime commented Jan 28, 2024

I would say it's reasonable to require this comment to appear at the top of the file (see a naive implementation in the above comment).

I think your pseudocode would match that commant in any line, not just in the first one? Many formats don't even allow JavaScript style block comments at the top (think of HTML). Nor does JavaScript if there is a hashbang.

Or perhaps I'm misunderstanding what you mean, are you thinking about embedded JS in a markdown file that would falsely ignore the whole file?

Yes, eslint-plugin-markdown for example lets you lint JavaScript code in markdown files.

On one hand, the performance wouldn't get worse: currently, every file is read+parsed, with such a comment, there's at least the possibility for files to be skipped. If the issue is about reading the file from disk twice (once to scan for the comment, twice to parse the code), I would defer that as an implementation detail that can be considered once a general decision about the feature has been made. 🙂

Files are only read once as long as they are not cached or ignored. In flat config, you can add a global ignore pattern to your config and all files matching that pattern would be skipped - not read. This is much more performant than accessing the contents of every file because you're not sure if it should be ignored or not. There is no mechanism to mark a file or folder as ignored outside the config. We could add one if we really need it, but I think the indication to ignore a file should not be included in the contents of the file itself.

@jeengbe
Copy link
Author

jeengbe commented Jan 28, 2024

I think your pseudocode would match that commant in any line, not just in the first one? Many formats don't even allow JavaScript style block comments at the top (think of HTML). Nor does JavaScript if there is a hashbang.

That might be on me for making that unclear - the parse file() statement is meant to act like an "ok this file should be parsed" and exit the loop. It's essentially just checking whether an /* eslint-ignore-file */ comment appears before anything that is not an empty line or a comment.

Yes, eslint-plugin-markdown for example lets you lint JavaScript code in markdown files.

I'm not sure how that plugin handles JS code mixed with markdown (maybe virtual files per JS block, dunno?). The way I imagine how /* eslint-ignore-file */ could be used (at the top of automatically generated source code that ESLint should just skip), this directive would never appear in a JS block in a markdown file.


This is just me guessing, but if each JS snippet in a markdown file is treated as a virtual file with its own AST, then the comment would still make sense even in the context of embedded JS in Markdown.


Files are only read once as long as they are not cached or ignored. In flat config, you can add a global ignore pattern to your config and all files matching that pattern would be skipped - not read.

I am again not exactly sure how ESLint implements traverse, read, parse, but an option would be to adjust the parse method from (dummy code:)

export declare function parseFile(filePath: string): AST;

to something like

export function parseFile(filePath: string): AST | null {
  const fileContent = readFileContent(filePath);
  
  if (shouldIgnore(fileContent)) return null;
  return parseFileImpl(fileContent);
}

declare function parseFileImpl(fileContent: string): AST;

In this scenario, the file would be read and a shouldIgnore would look at the first lines to see whether an /* eslint-ignore-file */ is present. The signature of parseFile is changed to return a new data structure that represents either "not parsed" with e.g. null, or else the parsed AST.

@fasttime
Copy link
Member

I'm not sure how that plugin handles JS code mixed with markdown (maybe virtual files per JS block, dunno?).

That plugin uses a processor to extract blocks of JavaScript code from the markdown files. Each block is linted as a separate source. In order to run the processor, the file must have been read, the config for the file must have been resolved, and the plugin that provides the processor must have been loaded (ESLint does come with any built-in processors).

I am again not exactly sure how ESLint implements traverse, read, parse, but an option would be to adjust the parse method from (dummy code:)

I imagine you mean traversing the directory structure, as opposed to traversing the AST. This function is where all the files are looked up before the contents are read here. And this function is what triggers the parsing, which is done in a separate package, depending on the parser. If it interests you, you can dig into that code to see how it's implemented.

Copy link

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.

@github-actions github-actions bot added the Stale label Feb 29, 2024
@Rec0iL99
Copy link
Member

Rec0iL99 commented Mar 1, 2024

ping @eslint/eslint-team

@Rec0iL99 Rec0iL99 removed the Stale label Mar 1, 2024
@nzakas nzakas added the needs design Important details about this change need to be discussed label Mar 1, 2024
@nzakas
Copy link
Member

nzakas commented Mar 1, 2024

I think this is an interesting idea that's worth pursuing, but as this conversation shows, there is a fair amount of detail work needed to figure out if this would work for us. So, I think the most logical next step is to ask someone to write up an RFC to explore the details and concerns with this approach.

Copy link

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.

@github-actions github-actions bot added the Stale label Mar 31, 2024
@nzakas
Copy link
Member

nzakas commented Apr 1, 2024

@jeengbe are you interested in putting together an RFC for this proposal?

@jeengbe
Copy link
Author

jeengbe commented Apr 1, 2024

I will look into it and hand over the task once my limited understanding of ESLint's internals is exhausted 👍🏻

@nzakas
Copy link
Member

nzakas commented Apr 26, 2024

Per the discussion on eslint/rfcs#118, we are choosing not to pursue this any further.

@nzakas nzakas closed this as not planned Won't fix, can't repro, duplicate, stale Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint needs design Important details about this change need to be discussed
Projects
Status: Complete
Development

No branches or pull requests

5 participants