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

overrides with processor works unexpectedly #14207

Closed
JounQin opened this issue Mar 12, 2021 · 7 comments · Fixed by #14227
Closed

overrides with processor works unexpectedly #14207

JounQin opened this issue Mar 12, 2021 · 7 comments · Fixed by #14227
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features repro:yes
Projects

Comments

@JounQin
Copy link
Contributor

JounQin commented Mar 12, 2021

Tell us about your environment

  • ESLint Version: 7.21.0
  • Node Version: v14.16.0
  • npm Version: 7.5.6
  • Operating System: GNU/Linux 5.4.0-1024-gcp

What parser (default, @babel/eslint-parser, @typescript-eslint/parser, etc.) are you using?

Default

Please show your full configuration:

https://github.com/JounQin/test/blob/eslint/.eslintrc.js

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

https://github.com/JounQin/test/blob/eslint/test.cjs

yarn eslint test.cjs

What did you expect to happen?

No error because prettier is intended to be disabled

What actually happened? Please copy-paste the actual, raw output from ESLint.

Oops! Something went wrong! :(

ESLint: 7.21.0

Error: ENOTDIR: not a directory, stat '/workspace/test/test.cjs/0_.cjs'
Occurred while linting /workspace/test/test.cjs/0_.cjs:1
    at Object.statSync (fs.js:1086:3)
    at isTypeSync (/workspace/test/node_modules/prettier/third-party.js:9748:46)
    at getDirectorySync (/workspace/test/node_modules/prettier/third-party.js:9802:62)
    at ExplorerSync.searchSync (/workspace/test/node_modules/prettier/third-party.js:9950:66)
    at _resolveConfig (/workspace/test/node_modules/prettier/index.js:25592:50)
    at Function.resolveConfig.sync (/workspace/test/node_modules/prettier/index.js:25621:42)
    at Program (/workspace/test/node_modules/eslint-plugin-prettier/eslint-plugin-prettier.js:167:40)
    at /workspace/test/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/workspace/test/node_modules/eslint/lib/linter/safe-emitter.js:45:38)

Steps to reproduce this issue:

  1. clone https://github.com/JounQin/test/tree/eslint or open https://turquoise-limpet-v0as7arg.ws-us03.gitpod.io/
  2. run yarn eslint test.cjs, there will be an error
  3. toggle the comment in .eslintrc.js and run yarn eslint test.cjs again, there will be no error, but it is not expected, I want to check test.cjs with prettier but disable it on code block

Are you willing to submit a pull request to fix this bug?

I don't know how to fix it easily.

@JounQin JounQin added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Mar 12, 2021
@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features and removed triage An ESLint team member will look at this issue soon labels Mar 17, 2021
@mdjermanovic mdjermanovic added this to Needs Triage in Triage via automation Mar 17, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Triaging in Triage Mar 17, 2021
@mdjermanovic mdjermanovic moved this from Triaging to Evaluating in Triage Mar 18, 2021
@mdjermanovic
Copy link
Member

I can confirm this behavior.

When preprocess() returns a filename with the same extension as the original file, that virtual file will be linted with the configuration for the original file (except the "processor" option).

eslint/lib/linter/linter.js

Lines 1311 to 1326 in ebd7026

// Resolve configuration again if the file extension was changed.
if (configForRecursive && path.extname(blockName) !== originalExtname) {
debug("Resolving configuration again because the file extension was changed.");
return this._verifyWithConfigArray(
blockText,
configForRecursive,
{ ...options, filename: blockName }
);
}
// Does lint.
return this._verifyWithoutProcessors(
blockText,
config,
{ ...options, filename: blockName }
);

I believe we assumed that if a processor used for *.foo files returns a .foo filename, then it's the same content, and calling this._verifyWithConfigArray again would cause infinite recursion. But, as a side effect of using the same config, overrides for *.foo/*.foo do not apply.

I'm not sure what's the intended behavior in this case. For users, it might make more sense to see this as the original file and thus make a configuration for *.foo (not *.foo/*.foo). On the other hand, it looks inconsistent with the behavior when processors return different extensions.

@JounQin
Copy link
Contributor Author

JounQin commented Mar 18, 2021

I believe we assumed that if a processor used for *.foo files returns a .foo filename, then it's the same content, and calling this._verifyWithConfigArray again would cause infinite recursion.

That's not true for markdown/mdx in markdown/mdx.

@JounQin
Copy link
Contributor Author

JounQin commented Mar 18, 2021

I think we should change the following

eslint/lib/linter/linter.js

Lines 1311 to 1319 in ebd7026

// Resolve configuration again if the file extension was changed.
if (configForRecursive && path.extname(blockName) !== originalExtname) {
debug("Resolving configuration again because the file extension was changed.");
return this._verifyWithConfigArray(
blockText,
configForRecursive,
{ ...options, filename: blockName }
);
}

to

// Resolve configuration again if the file content or extension was changed.
if (configForRecursive && (text !== blockText || path.extname(blockName) !== originalExtname)) {
  debug('Resolving configuration again because the file content or extension was changed.')
  return this._verifyWithConfigArray(blockText, configForRecursive, {
    ...options,
    filename: blockName,
  })
}

@mdjermanovic I'd like to raise a PR if you like it.

@mdjermanovic
Copy link
Member

@eslint/eslint-tsc thoughts about this, is it a bug or the intended behavior?

If it's a bug, does the fix in #14227 makes sense, and would it be a breaking change now?

@nzakas
Copy link
Member

nzakas commented Mar 26, 2021

I think this is a bug. We probably did this for performance reasons, since calculating configs is expensive.

I’m also guessing this use case is fairly uncommon so fixing this bug doesn’t seem like a breaking change to me.

@btmills
Copy link
Member

btmills commented Mar 27, 2021

I agree this is a bug that we can fix in a semver-minor change.

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Mar 31, 2021
@nzakas
Copy link
Member

nzakas commented Mar 31, 2021

Marking as accepted. There is already a PR open.

@nzakas nzakas moved this from Evaluating to Pull Request Opened in Triage Mar 31, 2021
Triage automation moved this from Pull Request Opened to Complete Apr 13, 2021
btmills pushed a commit that referenced this issue Apr 13, 2021
…4227)

* Fix: lint code block with same extension but different content

close #14207

* Chore: apply review suggestions
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Oct 11, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Oct 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly core Relates to ESLint's core APIs and features repro:yes
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

4 participants