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

fix: throw helpful exception when rule has wrong return type #16075

Merged
merged 5 commits into from Jul 2, 2022

Conversation

bmish
Copy link
Sponsor Member

@bmish bmish commented Jun 30, 2022

Prerequisites checklist

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.

@eslint-github-bot eslint-github-bot bot added bug ESLint is working incorrectly triage An ESLint team member will look at this issue soon labels Jun 30, 2022
@netlify
Copy link

@netlify netlify bot commented Jun 30, 2022

Deploy Preview for docs-eslint canceled.

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

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 30, 2022

CI is currently failing because of DefinitelyTyped/DefinitelyTyped#61032

@amareshsm
Copy link
Member

@amareshsm amareshsm commented Jun 30, 2022

CI is currently failing because of DefinitelyTyped/DefinitelyTyped#61032

Can we add overrides as fix for now?

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

@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 30, 2022

Can we add overrides as fix for now?

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

That would work only with npm 8?

@amareshsm
Copy link
Member

@amareshsm amareshsm commented Jun 30, 2022

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
Copy link
Member

@mdjermanovic mdjermanovic commented Jun 30, 2022

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

Copy link
Member

@mdjermanovic mdjermanovic left a comment

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.

if (typeof ruleListeners !== "object" || Array.isArray(ruleListeners) || ruleListeners === null) {
throw new Error(`The create() function for rule ${ruleId} did not return an object.`);
}
Copy link
Member

@mdjermanovic mdjermanovic Jun 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@nzakas nzakas Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Sponsor Member Author

@bmish bmish Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Sponsor Member Author

@bmish bmish Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@mdjermanovic mdjermanovic Jul 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@nzakas nzakas Jul 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Sponsor Member Author

@bmish bmish Jul 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened issue: #16086

@mdjermanovic mdjermanovic added 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 Jun 30, 2022
lib/linter/linter.js Outdated Show resolved Hide resolved
lib/linter/linter.js Outdated Show resolved Hide resolved
bmish and others added 3 commits Jul 1, 2022
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@mdjermanovic mdjermanovic left a comment

LGTM, thanks!

@mdjermanovic mdjermanovic added core Relates to ESLint's core APIs and features 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 Jul 1, 2022
@mdjermanovic
Copy link
Member

@mdjermanovic mdjermanovic commented Jul 1, 2022

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

nzakas
nzakas approved these changes Jul 2, 2022
Copy link
Member

@nzakas nzakas left a comment

LGTM.

@nzakas nzakas merged commit fc81848 into eslint:main Jul 2, 2022
20 checks passed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Jul 4, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.18.0` -> `8.19.0`](https://renovatebot.com/diffs/npm/eslint/8.18.0/8.19.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.19.0`](https://github.com/eslint/eslint/releases/tag/v8.19.0)

[Compare Source](eslint/eslint@v8.18.0...v8.19.0)

#### Features

-   [`7023628`](eslint/eslint@7023628) feat: add importNames support for patterns in no-restricted-imports ([#&#8203;16059](eslint/eslint#16059)) (Brandon Scott)
-   [`472c368`](eslint/eslint@472c368) feat: fix handling of blockless `with` statements in indent rule ([#&#8203;16068](eslint/eslint#16068)) (Milos Djermanovic)

#### Bug Fixes

-   [`fc81848`](eslint/eslint@fc81848) fix: throw helpful exception when rule has wrong return type ([#&#8203;16075](eslint/eslint#16075)) (Bryan Mishkin)

#### Documentation

-   [`3ae0574`](eslint/eslint@3ae0574) docs: Remove duplicate rule descriptions ([#&#8203;16052](eslint/eslint#16052)) (Amaresh  S M)
-   [`f50cf43`](eslint/eslint@f50cf43) docs: Add base href to each page to fix relative URLs ([#&#8203;16046](eslint/eslint#16046)) (Nicholas C. Zakas)
-   [`ae4b449`](eslint/eslint@ae4b449) docs: make logo link clickable on small width screens ([#&#8203;16058](eslint/eslint#16058)) (Milos Djermanovic)
-   [`280f898`](eslint/eslint@280f898) docs: use only fenced code blocks ([#&#8203;16044](eslint/eslint#16044)) (Milos Djermanovic)
-   [`f5d63b9`](eslint/eslint@f5d63b9) docs: add listener only if element exists ([#&#8203;16045](eslint/eslint#16045)) (Amaresh  S M)
-   [`8b639cc`](eslint/eslint@8b639cc) docs: add missing migrating-to-8.0.0 in the user guide ([#&#8203;16048](eslint/eslint#16048)) (唯然)
-   [`b8e68c1`](eslint/eslint@b8e68c1) docs: Update release process ([#&#8203;16036](eslint/eslint#16036)) (Nicholas C. Zakas)
-   [`6d0cb11`](eslint/eslint@6d0cb11) docs: remove table of contents from markdown text ([#&#8203;15999](eslint/eslint#15999)) (Nitin Kumar)

#### Chores

-   [`e884933`](eslint/eslint@e884933) chore: use `github-slugger` for markdown anchors ([#&#8203;16067](eslint/eslint#16067)) (Strek)
-   [`02e9cb0`](eslint/eslint@02e9cb0) chore: revamp carbon ad style ([#&#8203;16078](eslint/eslint#16078)) (Amaresh  S M)
-   [`b6aee95`](eslint/eslint@b6aee95) chore: remove unwanted comments from rules markdown ([#&#8203;16054](eslint/eslint#16054)) (Strek)
-   [`6840940`](eslint/eslint@6840940) chore: correctly use .markdownlintignore in Makefile ([#&#8203;16060](eslint/eslint#16060)) (Bryan Mishkin)
-   [`48904fb`](eslint/eslint@48904fb) chore: add missing images ([#&#8203;16017](eslint/eslint#16017)) (Amaresh  S M)
-   [`910f741`](eslint/eslint@910f741) chore: add architecture to nav ([#&#8203;16039](eslint/eslint#16039)) (Strek)
-   [`9bb24c1`](eslint/eslint@9bb24c1) chore: add correct incorrect in all rules doc ([#&#8203;16021](eslint/eslint#16021)) (Deepshika S)
-   [`5a96af8`](eslint/eslint@5a96af8) chore: prepare versions data file ([#&#8203;16035](eslint/eslint#16035)) (Nicholas C. Zakas)
-   [`50afe6f`](eslint/eslint@50afe6f) chore: Included githubactions in the dependabot config ([#&#8203;15985](eslint/eslint#15985)) (Naveen)
-   [`473411e`](eslint/eslint@473411e) chore: add deploy workflow for playground ([#&#8203;16034](eslint/eslint#16034)) (Milos Djermanovic)
-   [`a30b66c`](eslint/eslint@a30b66c) chore: fix print style ([#&#8203;16025](eslint/eslint#16025)) (Amaresh  S M)
-   [`f4dad59`](eslint/eslint@f4dad59) chore: add noindex meta tag ([#&#8203;16016](eslint/eslint#16016)) (Milos Djermanovic)
-   [`db387a8`](eslint/eslint@db387a8) chore: fix sitemap ([#&#8203;16026](eslint/eslint#16026)) (Milos Djermanovic)
-   [`285fbc5`](eslint/eslint@285fbc5) chore: remove TOC from printable ([#&#8203;16020](eslint/eslint#16020)) (Strek)
-   [`8e84c21`](eslint/eslint@8e84c21) chore: remove ligatures from fonts ([#&#8203;16019](eslint/eslint#16019)) (Strek)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1444
Reviewed-by: 6543 <6543@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
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 bug ESLint is working incorrectly core Relates to ESLint's core APIs and features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants