Change Request: Add "did you mean" for slightly incorrect rule option keys

[JoshuaKGoldberg]

ESLint version

8.44.0

What problem do you want to solve?

Right now, if a user slightly typos a rule option name, the error message is the same as if they used a completely unknown option. Example: typescript-eslint/typescript-eslint#7217

Error: .eslintrc.js:
        Configuration for rule "@typescript-eslint/no-confusing-void-expression" is invalid:
        Value {"ignoredArrowShorthand":true} should NOT have additional properties.

(the rule option is ignoreAllowShorthand - no d after ignore)

I know I've made this mistake plenty of times before and spent multiple minutes squinting at the screen. It would be nice if ESLint used something like didyoumean2 to detect when there's a small edit distance to an existing option. It could then give an improved message like, maybe...

Error: .eslintrc.js:
        Configuration for rule "@typescript-eslint/no-confusing-void-expression" is invalid:
        Value {"ignoredArrowShorthand":true} should NOT have additional properties.
        Did you mean "ignoreArrowShorthand"?

What do you think is the correct solution?

Seems like a straightforward win to me? See #17056 for another "did you mean?" change request - though that one's on options shapes, not string keys.

Participation

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

Additional comments

I applied a similar fix to pnpm: pnpm/pnpm#6492 -> pnpm/pnpm#6496

[nzakas]

We've been actively trying to remove extra dependencies from ESLint, and I'm not sure there's a big enough value-add here to take on another dependency. I'll leave this open to get feedback from others on the team.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[Rec0iL99]

Not stale. Waiting for feedback.

cc @eslint/eslint-team

[nzakas]

@mdjermanovic looking for your feedback here.

[snitin315]

I'd be okay with this change as this improves DX. We can do either of the following:

1. There is another popular package fastest-levenshtein which we can leverage and it's also smaller in size didyoumean2.

2. If we don't want to add a new dependency, we can simply improve the error message by listing all the valid options. That should help too:

   Error: .eslintrc.js:
           Configuration for rule "@typescript-eslint/no-confusing-void-expression" is invalid:
           Value {"ignoredArrowShorthand":true} should NOT have additional properties.
           
           Expected properties are: "ignoreArrowShorthand", "ignoreVoidOperator"
   

[mdjermanovic]

I think it would be very helpful to show the additional (i.e., the unexpected) property name and the list of expected property names, even if we implement "did you mean". That alone would be a great improvement over the existing message.

With that, I think "did you mean" would be a small improvement on top, so I'm not sure if it warrants adding a dependency, though I wouldn't be against adding a small dependency if other team members are in favor.

[mdjermanovic]

I was curious if Ajv validation actually provides the data we need for this feature. By validateRule.errors for a test configuration "array-callback-return": ["error", { "foo": true }], it seems doable:

[
    {
        "keyword": "additionalProperties", // <- indicates a problem with additional properties
        "dataPath": "[0]",
        "schemaPath": "#/items/0/additionalProperties",
        "params": {
            "additionalProperty": "foo" // <- unexpected property name
        },
        "message": "should NOT have additional properties",
        "schema": false, // <- to be sure that the problem is caused by an unexpected property name
        "parentSchema": {
            "type": "object",
            "properties": { // <- keys of this object are expected property names
                "allowImplicit": {
                    "type": "boolean",
                    "default": false
                },
                "checkForEach": {
                    "type": "boolean",
                    "default": false
                }
            },
            "additionalProperties": false
        },
        "data": {
            "foo": true,
            "allowImplicit": false,
            "checkForEach": false
        }
    }
]

[nzakas]

@mdjermanovic it sounds like you're in favor of just listing potential properties rather than using a dependency to calculate the possible correct property?

[mdjermanovic]

@mdjermanovic it sounds like you're in favor of just listing potential properties rather than using a dependency to calculate the possible correct property?

Yes. I think we should list potential properties either way, and that alone will be a significant improvement in these messages. Additionally suggesting which of them is probably the right one would then be a small addition that maybe isn't worth adding a new dependency.

[nzakas]

That sounds good to me. Maybe we can make it a bit "smart" and suggest any properties that begin with the same letter, and otherwise, just list out all of the properties?

[mdjermanovic]

Maybe we can make it a bit "smart" and suggest any properties that begin with the same letter

Sounds good to me.

and otherwise, just list out all of the properties?

Just to clarify, I think we should always list out all of the properties.

[nzakas]

If we're always going to list out all properties then I don't see any reason to also show properties beginning with the same letter. My goal was to shorten and simplify the output, but outputting everything all the time negates that benefit.

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[nzakas]

@mdjermanovic what do you think of my last comment?

[mdjermanovic]

I think that "did you mean" is generally more useful for a large number of options, while these will usually be small.

Also, listing all properties can be useful when the schema uses oneOf/anyOf as it shows what exactly failed matching.

For example, consider the schema for prefer-destructuring. It allows either { VariableDeclarator, AssignmentExpression } or { array, object }.

For a config like this:

{
    rules: {
        "prefer-destructuring": ["error", {
            VariableDeclarator: {
                object: true
            },
            array: false
        }]
    }
}

I think it would be most useful to show an error like this:

Error: Key "rules": Key "prefer-destructuring":
        Value {"VariableDeclarator":{"object":true},"array":false} should NOT have additional properties.
                Unexpected property "array". Expected properties: "VariableDeclarator", "AssignmentExpression".
        Value {"VariableDeclarator":{"object":true},"array":false} should NOT have additional properties.
                Unexpected property "VariableDeclarator". Expected properties: "array", "object".
        Value {"VariableDeclarator":{"object":true},"array":false} should match exactly one schema in oneOf.

My opinion is that the error should always show the config property that failed to match (the additional property). Then, if we are choosing between all properties of the schema object and "did you mean", I think that listing all properties would be generally more useful, though I wouldn't mind showing both all properties and "did you mean".

[nzakas]

So it sounds like what you're proposing is not a "did you mean" but rather just displaying all of the errors we can get from Ajv?

[mdjermanovic]

No, in this case we're already displaying multiple errors.

This is what we're currently displaying for the config from my previous comment:

Error: Key "rules": Key "prefer-destructuring":         Value {"VariableDeclarator":{"object":true},"array":false} should NOT have additional properties.
        Value {"VariableDeclarator":{"object":true},"array":false} should NOT have additional properties.
        Value {"VariableDeclarator":{"object":true},"array":false} should match exactly one schema in oneOf.

Now, I'll just add a linebreak for diff clarity (we should probably format these messages better regardless of this issue):

Error: Key "rules": Key "prefer-destructuring":
        Value {"VariableDeclarator":{"object":true},"array":false} should NOT have additional properties.
        Value {"VariableDeclarator":{"object":true},"array":false} should NOT have additional properties.
        Value {"VariableDeclarator":{"object":true},"array":false} should match exactly one schema in oneOf.

The proposal is to display additional info below each error that relates to additional properties (I believe this is what @JoshuaKGoldberg is proposing too, just different data):

Error: Key "rules": Key "prefer-destructuring":
        Value {"VariableDeclarator":{"object":true},"array":false} should NOT have additional properties.
+               Unexpected property "array". Expected properties: "VariableDeclarator", "AssignmentExpression".
        Value {"VariableDeclarator":{"object":true},"array":false} should NOT have additional properties.
+               Unexpected property "VariableDeclarator". Expected properties: "array", "object".
        Value {"VariableDeclarator":{"object":true},"array":false} should match exactly one schema in oneOf.

[nzakas]

Ah, I see. So we'll have an additional line that we'll generate by looking at the schema and figuring out which properties should be there instead. Is there some complexity there if we have a schema using anyOf? Or is that easy enough to work through?

[github-actions]

Oops! It looks like we lost track of this issue. What do we want to do here? This issue will auto-close in 7 days without an update.

[Rec0iL99]

Ping @mdjermanovic.

[mdjermanovic]

Is there some complexity there if we have a schema using anyOf? Or is that easy enough to work through?

There's no additional complexity for anyOf. error.parentSchema is the subschema for which additionalProperties check failed.

[mdjermanovic]

Note that "did you mean" would also have to look at the schema to figure out which properties are expected. I'm only proposing that we simply list all of them instead of picking just one.

[nzakas]

Okay, I think we are agreed on this proposal so marking as accepted.