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

Rule options from implicitly included plugins are not validated #11559

Closed
jarrodldavis opened this issue Mar 28, 2019 · 6 comments · Fixed by #11546
Closed

Rule options from implicitly included plugins are not validated #11559

jarrodldavis opened this issue Mar 28, 2019 · 6 comments · Fixed by #11546
Assignees
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

Comments

@jarrodldavis
Copy link
Sponsor

jarrodldavis commented Mar 28, 2019

Tell us about your environment

  • ESLint Version: v5.15.3
  • Node Version: v11.12.0
  • npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using? default (Espree)

Please show your full configuration:

Configuration
{
    "root": true,
    "env": {
        "commonjs": true,
        "es6": true,
        "node": true
    },
    "extends": ["eslint:recommended", "plugin:node/recommended"],
    "globals": {
        "Atomics": "readonly",
        "SharedArrayBuffer": "readonly"
    },
    "parserOptions": {
        "ecmaVersion": 2018
    },
    "rules": {
        "node/exports-style": ["error", "invalid-option"]
    }
}

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

const none = require("./doesnt-exist");
$ ./node_modules/.bin/eslint .

What did you expect to happen?

I expected ESLint to report a validation error like so:

$ ./node_modules/.bin/eslint .
Error: /Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/.eslintrc.json:
	Configuration for rule "node/exports-style" is invalid:
	Value "invalid-option" should be equal to one of the allowed values.

    at validateRuleOptions (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config/config-validator.js:133:19)
    at Object.keys.forEach.id (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config/config-validator.js:176:9)
    at Array.forEach (<anonymous>)
    at validateRules (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config/config-validator.js:175:30)
    at Object.validate (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config/config-validator.js:262:5)
    at loadFromDisk (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config/config-file.js:544:19)
    at Object.load (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config/config-file.js:587:20)
    at Config.getLocalConfigHierarchy (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config.js:240:44)
    at Config.getConfigHierarchy (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config.js:192:43)
    at Config.getConfigVector (/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/explicit-plugin/node_modules/eslint/lib/config.js:299:21)

What actually happened? Please include the actual, raw output from ESLint.

The validation error is ignored and ESLint continues as if the plugin option is valid

$ ./node_modules/.bin/eslint .

/Users/jarrodldavis/source/repos/repro-eslint-validate-issue/implicit-plugin/index.js
  1:7   error  'none' is assigned a value but never used  no-unused-vars
  1:22  error  "./doesnt-exist" is not found              node/no-missing-require

✖ 2 problems (2 errors, 0 warnings)

I've created a reproduction repository that demonstrates the difference in plugin rule validation behavior when implicitly vs explicitly including a plugin.

I believe I have narrowed it down to this code:

const ruleMap = configContext.linterContext.getRules();
// validate the configuration before continuing
validator.validate(config, ruleMap.get.bind(ruleMap), configContext.linterContext.environments, resolvedPath.configFullName);
/*
* If an `extends` property is defined, it represents a configuration file to use as
* a "parent". Load the referenced file and merge the configuration recursively.
*/
if (config.extends) {
config = applyExtends(config, configContext, resolvedPath.filePath, dirname);
}

function validateRules(rulesConfig, ruleMapper, source = null) {
if (!rulesConfig) {
return;
}

It validates the local config (including rule options) before applying config extensions, so the plugin and its rules aren't loaded into the linter until after the rules enabled by the local config are validated. Since the rules aren't present at the time of validation, validation is silently skipped.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Mar 28, 2019
@not-an-aardvark
Copy link
Member

Thanks for the report. This seems like a serious issue, I'm surprised no one had noticed this before now.

I think this is probably already fixed by #11546 (cc @mysticatea).

@jarrodldavis
Copy link
Sponsor Author

jarrodldavis commented Mar 28, 2019

Thanks for the quick response!

I pulled down the issue11510 branch from #11546 to npm link into my reproduction repository and can confirm that the validation works as expected in the implicit-plugin directory.

@platinumazure platinumazure added bug ESLint is working incorrectly core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Mar 28, 2019
@mysticatea mysticatea added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Mar 29, 2019
@mysticatea
Copy link
Member

mysticatea commented Mar 29, 2019

I confirmed that this reproduced.

And yes, #11546 fixed this bug because it has separated config schema validation and rule option validation. In #11546, ESLint validates config schema when each config file was read and validates rule options and environments when the configuration was determined for each source file (i.e. after all cascading config files, shareable configs, plugins, and parsers are loaded).

@fvhockney
Copy link

I've pulled this into a vue project where I have been having problems with eslint and their plugin for months. I can confirm that this fixes the error I was having. Is this going to be merged in before version 6.0.0?

@mysticatea
Copy link
Member

We have published the final release of ESLint 5.x in the last week. We have a plan to release the first 6.0.0 pre-release in the next week and #11546 will be included in that.

Unfortunately, we have not backported bug fixes to old major versions because backporting makes complex our release process and we don't have enough human resource for that. And, #11546 changes our codebase about configuration from almost scratch and that fixes some bugs which are considered breaking changes, so it's hard to backport.

At a glance, the current --fix-type implementation doesn't support plugin rules because it caches rules before finding any configs... :(

@fvhockney
Copy link

Great. #11546 seems to fix the issue for --fix-type as well. I'll keep my eyes peeled for the releases.

@mysticatea mysticatea self-assigned this Apr 13, 2019
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Nov 7, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Nov 7, 2019
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants