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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from Jul 2, 2022
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
53250b4
fix: throw helpful exception when rule has wrong return type
bmish Jun 30, 2022
c8c4cdc
do not throw when rule returns array
bmish Jul 1, 2022
b4cd77c
Update lib/linter/linter.js
bmish Jul 1, 2022
2245d82
Update lib/linter/linter.js
bmish Jul 1, 2022
4e509f0
update tests
bmish Jul 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/linter/linter.js
View file
Open in desktop
Expand Up @@ -1119,6 +1119,10 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserName, languageO
};
}

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

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

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

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

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

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

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

Choose a reason for hiding this comment

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

Opened issue: #16086


// add all the selectors from the rule as listeners
Object.keys(ruleListeners).forEach(selector => {
const ruleListener = timing.enabled
Expand Down
22 changes: 22 additions & 0 deletions tests/lib/linter/linter.js
View file
Open in desktop
Expand Up @@ -7000,6 +7000,28 @@ var a = "test2";

assert(ok);
});

it("should throw when rule's create() function does not return an object", () => {
const config = { rules: { checker: "error" } };

linter.defineRule("checker", () => null); // returns null

assert.throws(() => {
linter.verify("abc", config, filename);
}, "The create() function for rule checker did not return an object.");

linter.defineRule("checker", () => {}); // returns undefined

assert.throws(() => {
linter.verify("abc", config, filename);
}, "The create() function for rule checker did not return an object.");

linter.defineRule("checker", () => []); // returns an array

assert.throws(() => {
linter.verify("abc", config, filename);
}, "The create() function for rule checker did not return an object.");
});
});

describe("Custom parser", () => {
Expand Down