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: fix rule schemas #16879

Closed
3 tasks done
mho22 opened this issue Feb 10, 2023 · 13 comments
Closed
3 tasks done

Bug: fix rule schemas #16879

mho22 opened this issue Feb 10, 2023 · 13 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible bug ESLint is working incorrectly repro:yes rule Relates to ESLint's core rules
Projects

Comments

@mho22
Copy link
Contributor

mho22 commented Feb 10, 2023

Environment

Node version: 18.8.0
npm version: 8.18.0
Local ESLint version: 8.26.0
Global ESLint version: 8.26.0
Operating System: macOS 10.15.7

What parser are you using?

Default (Espree)

What did you do?

Configuration
{
	"root": true,
	"env": {
    	"node": true,
		"browser": true
	},
	"extends": [ "eslint:recommended" ],
	"ignorePatterns": [ "**/node_modules/**/*" ],
  	"rules": {
		"no-undef" : "off",
		"no-unused-vars" : "off",
		"array-element-newline": [ "error", "always", { "minItems": 2 } ]
	}
}
var c = [1];
var d = [1, 2];
var e = [1, 2, 3
];
var f = [ 1, 2, 3 ];
var g = [
    function foo() {
        dosomething();
    }, function bar() {
        dosomething();
    }
];

What did you expect to happen?

It should probably return a configuration error because { "minItems" : 2 } shouldn't be allowed.

What actually happened?

it doesn't take the option into account, the rule works like it should have been : "array-element-newline": [ "error", "always" ] without any configuration error reported.

Participation

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

Additional comments

Checklist

@mho22 mho22 added bug ESLint is working incorrectly repro:needed labels Feb 10, 2023
@mdjermanovic mdjermanovic added this to Need Discussion in v9.0.0 Feb 10, 2023
@mdjermanovic mdjermanovic added breaking This change is backwards-incompatible rule Relates to ESLint's core rules accepted There is consensus among the team that this change meets the criteria for inclusion labels Feb 10, 2023
@mdjermanovic
Copy link
Member

Thanks for the issue!

Per our semver policies, this is a breaking change so the fix will have to wait for the next major version.

@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 21, 2023

Also, schema for nonblock-statement-body-position is missing type : "object" in two places. These configurations should be invalid ("foo" should be an object):

"nonblock-statement-body-position": ["error", "any", "foo"]
"nonblock-statement-body-position": ["error", "any", { "overrides": "foo" }]

@mdjermanovic
Copy link
Member

mdjermanovic commented Feb 21, 2023

TODO: also check changes from #13911

Edit: checked, nothing to add here.

@mho22
Copy link
Contributor Author

mho22 commented Feb 21, 2023

Also, schema for no-sequences is missing type : "object" in one place. The configuration should be invalid("foo" should be an object):

"no-sequences": ["error", "foo"]

@mdjermanovic
Copy link
Member

In no-constructor-return, instead of schema: {} it should be schema: [].

For example, "no-constructor-return": ["error", "foo"] should cause a validation error.

@mdjermanovic mdjermanovic changed the title Bug: array-element-newline missing additionalItems property Bug: fix rule schemas Apr 2, 2023
@mdjermanovic
Copy link
Member

@itemshopp

It seems that rule comma-dangle accepts an array but works only on enum or object . AJV validates an array but ESLint rejects it. Probably a misinterpretation of the schema, and maybe not relevant. If it is, it may be :

[
    {
        oneOf: [
                {
                    $ref: "#/definitions/value"
                },
                {
                    type: "object",
                    properties: {
                    arrays: { $ref: "#/definitions/valueWithIgnore" },
                    objects: { $ref: "#/definitions/valueWithIgnore" },
                    imports: { $ref: "#/definitions/valueWithIgnore" },
                    exports: { $ref: "#/definitions/valueWithIgnore" },
                    functions: { $ref: "#/definitions/valueWithIgnore" }
                },
                additionalProperties: false
            }
        ]
    }
]

instead of the actual one :

{
    type: "array",
    items: [
        {
            oneOf: [
                 {
                     $ref: "#/definitions/value"
                 },
                 {
                     type: "object",
                     properties: {
                         arrays: { $ref: "#/definitions/valueWithIgnore" },
                         objects: { $ref: "#/definitions/valueWithIgnore" },
                         imports: { $ref: "#/definitions/valueWithIgnore" },
                         exports: { $ref: "#/definitions/valueWithIgnore" },
                         functions: { $ref: "#/definitions/valueWithIgnore" }
                    },
                        additionalProperties: false
                    }
                ]
            }
        ],
        additionalItems: false
    }
}

If I'm incorrect, don't hesitate to indicate it to me, I'll delete the comment

Can you provide an example where the current schema doesn't work as expected for this rule? I think these two schemas are equivalent but the array shorthand cannot be used with references.

https://eslint.org/docs/latest/extend/custom-rules#options-schemas

Note: Currently you need to use full JSON Schema object rather than array in case your schema has references ($ref), because in case of array format ESLint transforms this array into a single schema without updating references that makes them incorrect (they are ignored).

@mdjermanovic
Copy link
Member

I updated the original post with a checklist.

@mho22
Copy link
Contributor Author

mho22 commented Apr 3, 2023

@mdjermanovic I tried both schemas and you're indeed right, the suggested schema does not work. I was actually focused on the type: array thing. I should have noticed its importance for $refs and $defs, I will delete my comment to keep the issue clean. Apologies.

@mho22
Copy link
Contributor Author

mho22 commented Apr 4, 2023

in new-parens , schema is an object. it should be an array depending on documentation :

schema (array) specifies the [options] so ESLint can prevent invalid [rule configurations]

Schema works like a charm even if it doesn't respect the mentioned type. But it should behave like a simple enum case :

schema: [
    {
        enum : [ "always", "never" ]
    }
],

I can't produce any error on this, I juste saw a particular schema type and reported it. If it is incorrect I'll delete the comment.

@mdjermanovic
Copy link
Member

in new-parens , schema is an object. it should be an array depending on documentation

The schema describes arrays and is technically correct for this rule: { type: "array" } in the schema means that instances should be arrays. However, I have no idea why it uses anyOf. Feel free to submit a refactor: PR to simplify this schema as you suggested.

@mdjermanovic
Copy link
Member

array-element-newline and nonblock-statement-body-position are now deprecated, so we're not making any further changes there.

no-constructor-return will be fixed by #17792.

@mho22 would you like to submit a PR for no-sequences?

@mho22
Copy link
Contributor Author

mho22 commented Dec 19, 2023

@mdjermanovic I will. I am on it.

@mdjermanovic
Copy link
Member

no-sequences has been fixed by #17878

no-constructor-return has been fixed by #17792.

The other two rules are deprecated, so this task is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion breaking This change is backwards-incompatible bug ESLint is working incorrectly repro:yes rule Relates to ESLint's core rules
Projects
Archived in project
Status: Done
v9.0.0
Need Discussion
Development

No branches or pull requests

2 participants