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

Bug: context.parserOptions becomes empty with flat config and custom parser #16559

Closed
1 task done
ryym opened this issue Nov 18, 2022 · 11 comments
Closed
1 task done

Bug: context.parserOptions becomes empty with flat config and custom parser #16559

ryym opened this issue Nov 18, 2022 · 11 comments
Assignees
Labels
bug ESLint is working incorrectly repro:needed This issue should include a reproducible example

Comments

@ryym
Copy link
Contributor

ryym commented Nov 18, 2022

Environment

Environment Info:

Node version: v18.10.0
npm version: v8.19.2
Local ESLint version: v8.27.0 (Currently used)
Global ESLint version: Not found
Operating System: darwin 21.6.0

What parser are you using?

@typescript-eslint/parser

What did you do?

I used new ESLint configuration file eslint.config.js with some plugins, and noticed that some plugin rules do not work correctly when used with a non-default parser. For example, the no-default-export rule in eslint-plugin-import does not report errors at all only when used with eslint.config.js and non-default parser such as @typescript-eslint/parser .

I found that this is because parserOptions in context object passed to rule creator functions becomes empty in that case. The no-default-export rule checks parserOptions.sourceType on rule creation, and skips running if the sourceType is not module.
https://github.com/import-js/eslint-plugin-import/blob/48e8130a9f33afd0f9d06635c25d4f1df4d63340/src/rules/no-default-export.js#L15-L18

I created a reproduction repository: https://github.com/ryym/eslint-flat-config-ts-parser-issue

In this repo I created a local ESLint rule that just prints context.parserOptions and used it in four patterns:

  1. npm run old-config ... .eslintrc.js with default parser
  2. npm run old-config-ts ... .eslintrc.js with @typescript-plugin/parser parser
  3. npm run new-config ... eslint.config.js with default parser
  4. npm run new-config-ts ... eslint.config.js with @typescript-plugin/parser parser

You can see the output is an empty object only in new-config-ts.

reproduction flow and output
  1. clone the repo
  2. npm install
  3. npm run check
% npm run check

> check
> npm run old-config && npm run old-config-ts && npm run new-config && npm run new-config-ts


> old-config
> ESLINT_USE_FLAT_CONFIG=false npx eslint a.js

{
  ecmaVersion: 11,
  sourceType: 'module',
  ecmaFeatures: { globalReturn: false }
}

> old-config-ts
> ESLINT_USE_FLAT_CONFIG=false TS_PARSER=true npx eslint a.js

{
  ecmaVersion: 11,
  sourceType: 'module',
  ecmaFeatures: { globalReturn: false }
}

> new-config
> ESLINT_USE_FLAT_CONFIG=true npx eslint a.js

{ sourceType: 'module' }

> new-config-ts
> ESLINT_USE_FLAT_CONFIG=true TS_PARSER=true npx eslint a.js

{}

I also confirmed that this occurs with @babel/eslint-parser as well.

What did you expect to happen?

ESLint rules using context.parserOptions internally work in any combination of config file format and parser.

What actually happened?

As I wrote before, a rule using context.parserOptions do not work with new flat config eslint.config.js with non-default parser such-as @typescript-eslint/parser, @babel-eslint/parser . This is because parserOptions becomes an empty object only in that case.

Participation

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

Additional comments

No response

@ryym ryym added bug ESLint is working incorrectly repro:needed This issue should include a reproducible example labels Nov 18, 2022
@mdjermanovic
Copy link
Member

Hi @ryym, thanks for the issue!

This is intentional. Some rules will need to be changed to be compatible with flat config.

In particular:

  • context.languageOptions.sourceType should be used instead of context.parserOptions.sourceType.
  • context.languageOptions.ecmaVersion should be used instead of context.parserOptions.ecmaVersion.
  • context.languageOptions.parser should be used instead of context.parserPath.

I believe this will be included in #13481 Phase 3: Compatibility testing, so I'm not sure if we should keep this open as a separate issue. @nzakas what do you think?

@nzakas
Copy link
Member

nzakas commented Nov 18, 2022

You’re correct, this is intentional. When using flat config, all of the options are enclosed in languageOptions, so you need to use context.languageOptions to access them. This will also work with eslintrc configs because that mapping is easy. It’s not as easy to go in the opposite direction, so changing to always use context.languageOptions is the preferred choice.

We can keep this open for phase 3, as I’m sure this question will come up in the future again, and maybe we will figure out a better way to map these two approaches.

@nzakas nzakas self-assigned this Nov 18, 2022
@ryym
Copy link
Contributor Author

ryym commented Nov 19, 2022

Thank you for your responses!
Let me check if I understood correctly. I understood the rule should access languageOptions instead of parserOptions , but what I wondered was why parserOptions is empty only when using a non-default parser. Without specifying a parser, parserOptions.sourceType still be set even if a flat config is used. Is this also intentional?

As an example, if you run ESLint with the following rule, parserOptions will be empty only when parser is specified.

// rule js
module.exports = {
  create(context) {
    console.log(context.parserOptions);
    return {};
  },
};
// eslint.config.js
const tsParser = require("@typescript-eslint/parser");
module.exports = [
  {
    // console output: { sourceType: 'module' }
    languageOptions: { sourceType: 'module' },

    // console output: {}
    languageOptions: { sourceType: 'module', parser: tsParser },
  },
];

@nzakas
Copy link
Member

nzakas commented Nov 23, 2022

Because context.parserOptions is filled by a top-level parserOptions in the config. When a top-level parserOptions doesn't exist, then there is no value to fill in there. Additionally, some of the previous properties in parserOptions are now in languageOptions (such as ecmaVersion and sourceType), so there's no direct mapping. Hope this help.

@nzakas nzakas moved this to Blocked in Triage Jan 3, 2023
@nzakas nzakas added this to Triage Jan 3, 2023
@morganney
Copy link

morganney commented Jul 19, 2023

Given this file:

top-level-await.js

import { glob } from 'glob'

await glob('*.js', { ignore: 'node_modules/**' })

With this config:

export default [
  // This very well could be an extended plugin config, such as one from eslint-plugin-n
  {
    languageOptions: {
      parserOptions: {
        ecmaVersion: 2021,
        sourceType: 'module'
      }
    }
  },
  // Say this is my own custom config
  {
    languageOptions: {
      ecmaVersion: 2023,
      sourceType: 'module'
    },
    files: ['top-level-await.js'],
    rules: {
      'no-console': 'error'
    }
  }
]

Running eslint top-level-await.js will result in:

/path/to/top-level-await.js
  3:1  error  Parsing error: Cannot use keyword 'await' outside an async function

✖ 1 problem (1 error, 0 warnings)

I would have though that this statement

In particular:
context.languageOptions.sourceType should be used instead of context.parserOptions.sourceType.
context.languageOptions.ecmaVersion should be used instead of context.parserOptions.ecmaVersion.
context.languageOptions.parser should be used instead of context.parserPath.

means I do not need to include an additional parserOptions, with it's own nested ecmaVersion to make the expected override work. However, the nested parserOptions is necessary:

export default [
  {
    languageOptions: {
      parserOptions: {
        ecmaVersion: 2021,
        sourceType: 'module'
      }
    }
  },
  {
    languageOptions: {
      ecmaVersion: 2023,
      sourceType: 'module',
      parserOptions: {
        ecmaVersion: 2023
      }
    },
    files: ['top-level-await.js'],
    rules: {
      'no-console': 'error'
    }
  }
]

So, is this expected behavior? Can parserOptions be defined outside of languageOptions? The example doesn’t indicate that.

@nzakas
Copy link
Member

nzakas commented Jul 19, 2023

If languageOptions.parserOptions is supplied, then it will be passed to the parser and will overrides anything set in languageOptions.

In your first example, you'll end up with a config that looks like this:

  {
    languageOptions: {
      ecmaVersion: 2023,
      sourceType: 'module',
      parserOptions: {
        ecmaVersion: 2021,
        sourceType: 'module'
      }
    },
    files: ['top-level-await.js'],
    rules: {
      'no-console': 'error'
    }
  }

So even though you have languageOptions.ecmaVersion: 2023 the languageOptions.parserOptions.ecmaVersion: 2021 is what is passed to the parser. (Again: parserOptions will override languageOptions when the key name is the same.)

In your second example, the nested parserOptions is necessary to reset languageOptions.parserOptions.ecmaVersion.

Note: If you have further questions about this, please do so in a discussion; this is not related to this issue.

@morganney
Copy link

Ah, there are Discussions 👌 , noted.

Thanks for your prompt feedback. This helped confirm what I was experiencing:

If languageOptions.parserOptions is supplied, then it will be passed to the parser and will overrides anything set in languageOptions.

However, I didn't see any documentation related to that behavior in the docs I linked to in my previous comment (unless I overlooked something), leaving me to learn about it only through trial and error.

@nzakas
Copy link
Member

nzakas commented Jul 20, 2023

@morganney we are still working on the documentation for flat config. That's a big task for v9.0.0.

@wycats
Copy link

wycats commented Oct 12, 2023

@nzakas It seems like it ought to be possible to allow users to specify a parserPath as a transitional strategy. In the long term, of course what we want is for plugins to stop using parserPath. But it doesn't seem problematic (unless I'm missing something) to allow users to opt in to compatibility with context.parserPath (by explicitly specifying one).

For reference, I hit this issue with the no-commented-out-code rule from eslint-plugin-etc, not one of the more banner cases like eslint-plugin-import.

@nzakas
Copy link
Member

nzakas commented Oct 13, 2023

@wycats we don't have the data to provide parserPath when flat config is used...we don't have the path to the parser. context.languageOptions.parser should work in both eslintrc and flat config modes.

@nzakas
Copy link
Member

nzakas commented Jun 17, 2024

Revisiting this now that flat config has shipped. Unfortunately, there just isn't an easy way to ensure that context.parserOptions will have the correct values regardless of the config file format used.

The recommended approach is to always use context.languageOptions and context.languageOptions.parserOptions, as these are present and correct in both flat config and eslintrc formats for both ESLint v8.x and v9.x. If you need to support versions older than that, you'll probably want to do something like this:

const parserOptions = context.languageOptions?.parserOptions ?? context.parserOptions;

@nzakas nzakas closed this as not planned Won't fix, can't repro, duplicate, stale Jun 17, 2024
@github-project-automation github-project-automation bot moved this from Blocked to Complete in Triage Jun 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ESLint is working incorrectly repro:needed This issue should include a reproducible example
Projects
Archived in project
Development

No branches or pull requests

5 participants