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: @eslint/compat fixupPluginRules() does not work with rule options #127

Closed
1 of 7 tasks
ehmicky opened this issue Oct 31, 2024 · 5 comments · Fixed by #128
Closed
1 of 7 tasks

Bug: @eslint/compat fixupPluginRules() does not work with rule options #127

ehmicky opened this issue Oct 31, 2024 · 5 comments · Fixed by #128
Assignees
Labels
accepted bug Something isn't working repro:yes Issues with a reproducible example

Comments

@ehmicky
Copy link

ehmicky commented Oct 31, 2024

Which packages are affected?

  • @eslint/compat
  • @eslint/config-array
  • @eslint/core
  • @eslint/migrate-config
  • @eslint/object-schema
  • @eslint/plugin-kit

Environment

Node version: 23.1.0
npm version: 10.9.0
ESLint version: 9.13.0
Operating System: Ubuntu 24.04

What did you do?

// eslint.config.js
import {fixupPluginRules} from '@eslint/compat';
import fp from 'eslint-plugin-fp';

export default [
  {
    plugins: {
      fp: fixupPluginRules(fp),
    },
    rules: {
      'fp/no-mutating-methods': ['error', {allowedObjects: ['state']}]
    },
  },
];
// index.js
const state = [];
state.push(3);
$ eslint index.js

Oops! Something went wrong! :(

ESLint: 9.13.0

Error: Key "rules": Key "fp/no-mutating-methods":
	Value [{"allowedObjects":["state"]}] should NOT have more than 0 items.

    at RuleValidator.validate (/home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/eslint/lib/config/rule-validator.js:170:27)
    at new Config (/home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/eslint/lib/config/config.js:243:27)
    at [finalizeConfig] (/home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/eslint/lib/config/flat-config-array.js:216:16)
    at FlatConfigArray.getConfigWithStatus (/home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/@eslint/config-array/dist/cjs/index.cjs:1102:55)
    at FlatConfigArray.getConfig (/home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/@eslint/config-array/dist/cjs/index.cjs:1120:15)
    at /home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/eslint/lib/eslint/eslint.js:764:40
    at async Promise.all (index 0)
    at async ESLint.lintFiles (/home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/eslint/lib/eslint/eslint.js:759:25)
    at async Object.execute (/home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/eslint/lib/cli.js:498:23)
    at async main (/home/ether/Desktop/repos/eslint-rewrite-bug/node_modules/eslint/bin/eslint.js:158:22)

What did you expect to happen?

The fixupPluginRules() from @eslint/compat should fix the fp/no-mutating-methods rule.

What actually happened?

It crashed it instead.

Link to Minimal Reproducible Example

https://github.com/ehmicky/eslint-rewrite-bug

Participation

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

Additional comments

eslint-plugin-fp is unmaintained, so I cannot expect an upgrade to ESLint 9, and need to rely on @eslint/compat instead.

However, their rule seems to be correct: https://github.com/jfmengels/eslint-plugin-fp/blob/205861e874a4db8caedfbe2cc8af599d8d45b475/rules/no-mutating-methods.js#L70-L80. The above configuration worked fine with ESLint 8.

Please note that:

  • If I pass an empty object as options to that rule, it fails with the same error message
  • But if I pass no rule options, the rule works
@ehmicky ehmicky added bug Something isn't working repro:needed This issue should include a reproducible example labels Oct 31, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in Triage Oct 31, 2024
@ehmicky ehmicky changed the title Bug: (fill in) Bug: @eslint/compat fixupPluginRules() does not work with rule options Oct 31, 2024
@mdjermanovic mdjermanovic added repro:yes Issues with a reproducible example and removed repro:needed This issue should include a reproducible example labels Oct 31, 2024
@mdjermanovic
Copy link
Member

The problem is that fp/no-mutating-methods specifies schema as a top-level property instead of a property of the meta object:

https://github.com/jfmengels/eslint-plugin-fp/blob/205861e874a4db8caedfbe2cc8af599d8d45b475/rules/no-mutating-methods.js#L82-L92

Top-level schema property in object-style rules was never officially supported, I believe, but was working in ESLint v8 by chance because it was picked up by code that was supposed to handle schema property of function-style rules.

I think we could update @eslint/compat to copy top-level schema into meta.schema. @nzakas what do you think?

@mdjermanovic mdjermanovic moved this from Needs Triage to Feedback Needed in Triage Oct 31, 2024
@nzakas
Copy link
Member

nzakas commented Oct 31, 2024

I think we could update @eslint/compat to copy top-level schema into meta.schema. @nzakas what do you think?

That seems like the easiest solution to me. 👍

@nzakas nzakas moved this from Feedback Needed to Ready to Implement in Triage Oct 31, 2024
@mdjermanovic
Copy link
Member

I can work on this.

@mdjermanovic mdjermanovic self-assigned this Oct 31, 2024
@ehmicky
Copy link
Author

ehmicky commented Oct 31, 2024

Thanks @mdjermanovic!

I was having the same problem with eslint-plugin-filenames, and it turns out it also incorrectly sets module.exports.schema.

I am not familiar with old plugins, but eslint-plugin-filenames seems quite different:

  • it exports the create function directly, instead of an object with a create function
  • it does not export a meta property
  • it does not always export a schema property, even when the rule has some options

@nzakas
Copy link
Member

nzakas commented Nov 1, 2024

@ehmicky yes, that was the original format of rules that we deprecated and removed in v9.

mdjermanovic added a commit that referenced this issue Nov 2, 2024
@mdjermanovic mdjermanovic moved this from Ready to Implement to Implementing in Triage Nov 2, 2024
@github-project-automation github-project-automation bot moved this from Implementing to Complete in Triage Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted bug Something isn't working repro:yes Issues with a reproducible example
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants