fix: throw helpful exception when rule has wrong return type

[bmish]

Prerequisites checklist

• I have read the contributing guidelines.

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Especially when developing a custom rule, it's possible to return the wrong data type in the rule's create(context) function. In particular, this often happens when attempting to bail out of a rule early, as shown below. The user needs to use return {}; to bail out instead of null/undefined.

module.exports = {
    meta: {
        // ...
    },

    create(context) {
        if (foo) {
            // bail out since rule doesn't apply
            return null; // wrong, should actually be return {};
        }
        return {
            BlockStatement(node) {},
        };
    }
};

Before (unhelpful error message):

~/Development/repo * yarn eslint file.js
yarn run v1.22.19

Oops! Something went wrong! :(

ESLint: 8.18.0

TypeError: Cannot convert undefined or null to object
Occurred while linting file.js
    at Function.keys (<anonymous>)
    at /Users/username/Development/eslint/lib/linter/linter.js:1129:16
    at Array.forEach (<anonymous>)

After (helpful error message):

~/Development/repo * yarn eslint file.js
yarn run v1.22.19

Oops! Something went wrong! :(

ESLint: 8.18.0

Error: The create() function for rule my-rule did not return an object.
Occurred while linting file.js
    at /Users/user/Development/eslint/lib/linter/linter.js:1123:19
    at Array.forEach (<anonymous>)

This is not a breaking change because this scenario already resulted in a crash and should only be hit during development.

Is there anything you'd like reviewers to focus on?

This could also be considered a chore/documentation improvement but it does have a public (dev-facing) impact.

[netlify]

✅ Deploy Preview for docs-eslint canceled.

Name	Link
🔨 Latest commit	4e509f0
🔍 Latest deploy log	https://app.netlify.com/sites/docs-eslint/deploys/62bf41000dc0230008a4d15c

[mdjermanovic]

CI is currently failing because of DefinitelyTyped/DefinitelyTyped#61032

[amareshsm]

CI is currently failing because of DefinitelyTyped/DefinitelyTyped#61032

Can we add overrides as fix for now?

  "overrides": {
    "@types/eslint": "8.4.3"
  }

[mdjermanovic]

Can we add overrides as fix for now?

  "overrides": {
    "@types/eslint": "8.4.3"
  }

That would work only with npm 8?

[amareshsm]

Can we add overrides as fix for now?

  "overrides": {
    "@types/eslint": "8.4.3"
  }

That would work only with npm 8?

yes, it will work for npm 8+.

[mdjermanovic]

Then it most likely wouldn't fix CI tests on Node 12.x and others.

This seems to be an issue in npm registry, let's wait to see if it will be fixed soon:

https://status.npmjs.org/incidents/6wr25yb0b2dd

[mdjermanovic]

This is not a breaking change because this scenario already resulted in a crash and should only be hit during development.

I left a comment regarding this.

[mdjermanovic]

Does ESLint currently crash if ruleListeners is an array? If it currently crashes only on undefined and null, maybe we should cover only them.

[nzakas]

I agree. We shouldn’t be adding cases that will fail if they don’t already.

[bmish]

Agree, good catch, updated to not throw when rule returns an array.

[bmish]

Would you folks support me opening an issue to add a breaking change to ESLint v9 to only allow rules to return objects? I suspect it's unintentional that we allow them to return arrays or random literal values right now. This seems small enough to not require an RFC.

[mdjermanovic]

Would you folks support me opening an issue to add a breaking change to ESLint v9 to only allow rules to return objects? I suspect it's unintentional that we allow them to return arrays or random literal values right now. This seems small enough to not require an RFC.

Makes sense to me. We could continue the discussion in the issue and decide there whether an RFC would be necessary.

[nzakas]

I’d say go for it. We can definitely discuss for v9.

As a note: I’m not sure there’s anything wrong with returning an array. Arrays can have other properties which could be visitors. Chances are people won’t mean to do that, but ESLint would work ok.

[bmish]

Opened issue: #16086

[mdjermanovic]

LGTM, thanks!

[mdjermanovic]

I marked this accepted because it improves the error message while not affecting the existing behavior.

[nzakas]

LGTM.