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

Change Request: Include ruleId in error message of crash #15037

Closed
1 task done
AriPerkkio opened this issue Sep 8, 2021 · 3 comments · Fixed by #15053
Closed
1 task done

Change Request: Include ruleId in error message of crash #15037

AriPerkkio opened this issue Sep 8, 2021 · 3 comments · Fixed by #15053
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects

Comments

@AriPerkkio
Copy link
Contributor

AriPerkkio commented Sep 8, 2021

ESLint version

7.32.0, 8.0.0-beta.1

What problem do you want to solve?

When ESLint rule encounters unexpected error and crashes, the error log should indicate which rule caused the crash. There is already some custom error handling done:

eslint/lib/linter/linter.js

Lines 1213 to 1226 in 143a598

} catch (err) {
err.message += `\nOccurred while linting ${options.filename}`;
debug("An error occurred while traversing");
debug("Filename:", options.filename);
if (err.currentNode) {
const { line } = err.currentNode.loc.start;
debug("Line:", line);
err.message += `:${line}`;
}
debug("Parser Options:", parserOptions);
debug("Parser Path:", parserName);
debug("Settings:", settings);
throw err;

It is important for users to identify which rule is causing linter to crash. This information is used when temporarily disabling the erroneous rule. It is also essential when reproducing and debugging the issue. Users and plugin developers are most interested in the root cause.

Most of the time the stack trace already displays the erroneous rule but there are many cases where it does not. Below are some real examples from stable releases of various eslint plugins.

Rule code is minified
TypeError: Cannot read property 'type' of undefined
Occurred while linting <text>:45
    at analyzePropertyChain (/<removed>/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js:2235:12)
    at analyzePropertyChain (/<removed>/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js:2264:20)
    at /<removed>/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js:1297:34
    at Array.forEach (<anonymous>)
    at visitFunctionWithDependencies (/<removed>/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js:1277:43)
    at visitCallExpression (/<removed>/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js:1737:11)
    at /<removed>/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/<removed>/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/<removed>/eslint/lib/linter/node-event-generator.js:254:26)
Recursion fills stack trace
RangeError: Maximum call stack size exceeded
Occurred while linting <removed>/oaf-project/oaf-react-final-form/src/validation/index.ts:125
    at <removed>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:109:66
    at Array.every (<anonymous>)
    at isTypeReadonlyRecurser (<removed>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:109:55)
    at <removed>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:36:43
    at Array.some (<anonymous>)
    at checkTypeArguments (<removed>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:36:27)
    at isTypeReadonlyArrayOrTuple (<removed>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:48:16)
    at isTypeReadonlyRecurser (<removed>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:123:29)
    at <removed>/node_modules/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:109:66
    at Array.every (<anonymous>)

Rule is using utilities and third party helpers
TypeError: Cannot read property 'flags' of undefined
Occurred while linting /<removed>/querycap/webappkit/@querycap/reactutils/must.tsx:7
    at Object.isUnionType (/<removed>/tsutils/typeguard/2.8/type.js:69:18)
    at unionTypeParts (/<removed>/tsutils/util/type.js:92:19)
    at isPropertyReadonlyInType (/<removed>/tsutils/util/type.js:151:21)
    at isReadonlyPropertyFromMappedType (/<removed>/tsutils/util/type.js:197:12)
    at /<removed>/tsutils/util/type.js:179:21
    at someTypePart (/<removed>/tsutils/util/type.js:100:52)
    at isReadonlyPropertyIntersection (/<removed>/tsutils/util/type.js:172:12)
    at Object.isPropertyReadonlyInType (/<removed>/tsutils/util/type.js:161:43)
    at isTypeReadonlyObject (/<removed>/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:72:28)
    at isTypeReadonlyRecurser (/<removed>/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:127:30)
Not sure what's going on here
TypeError: Cannot destructure property `object` of 'undefined' or 'null'.
Occurred while linting <text>:21
    at CallExpression[callee.property.name=/toBe(Truthy|Falsy)?|toEqual/][callee.object.callee.name='expect'] (/<removed>/ci/node_modules/eslint-plugin-jest-dom/dist/createBannedAttributeRule.js:34:21)
    at /<removed>/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/<removed>/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/<removed>/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/<removed>/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/<removed>/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (/<removed>/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /<removed>/node_modules/eslint/lib/linter/linter.js:952:32
    at Array.forEach (<anonymous>)

What do you think is the correct solution?

ESLint should include the erroneous rule id in the error logs.

Example of ideal error log. Line 7 is added by this feature. Line numbers are only for demo purpose here.

 1 | Oops! Something went wrong! :(
 2 |
 3 | ESLint: 8.0.0-beta.1
 4 |
 5 | TypeError: Cannot read property 'flags' of undefined
 6 | Occurred while linting /<removed>/querycap/webappkit/@querycap/reactutils/must.tsx:7
 7 | Rule: "some-plugin/some-rule"
 8 |     at Object.isUnionType (/<removed>/tsutils/typeguard/2.8/type.js:69:18)
 9 |     at unionTypeParts (/<removed>/tsutils/util/type.js:92:19)
10 |     at isPropertyReadonlyInType (/<removed>/tsutils/util/type.js:151:21)
11 |     at isReadonlyPropertyFromMappedType (/<removed>/tsutils/util/type.js:197:12)
12 |     at /<removed>/tsutils/util/type.js:179:21
13 |     at someTypePart (/<removed>/tsutils/util/type.js:100:52)
14 |     at isReadonlyPropertyIntersection (/<removed>/tsutils/util/type.js:172:12)
15 |     at Object.isPropertyReadonlyInType (/<removed>/tsutils/util/type.js:161:43)
16 |     at isTypeReadonlyObject (/<removed>/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:72:28)
17 |     at isTypeReadonlyRecurser (/<removed>/@typescript-eslint/eslint-plugin/dist/util/isTypeReadonly.js:127:30)

I have required changes ready for this feature. I'll setup PR if this gets accepted.

Participation

  • I am willing to submit a pull request for this change.
@AriPerkkio AriPerkkio added core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint triage An ESLint team member will look at this issue soon labels Sep 8, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Sep 8, 2021
AriPerkkio added a commit to AriPerkkio/eslint that referenced this issue Sep 8, 2021
AriPerkkio added a commit to AriPerkkio/eslint that referenced this issue Sep 8, 2021
@mdjermanovic mdjermanovic removed the triage An ESLint team member will look at this issue soon label Sep 8, 2021
@mdjermanovic mdjermanovic moved this from Needs Triage to Feedback Needed in Triage Sep 8, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 8, 2021

Thanks for the proposal, this seems like something that could be very useful 👍

@nzakas
Copy link
Member

nzakas commented Sep 9, 2021

I like it. 👍

AriPerkkio added a commit to AriPerkkio/eslint that referenced this issue Sep 9, 2021
AriPerkkio added a commit to AriPerkkio/eslint that referenced this issue Sep 12, 2021
@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Sep 15, 2021
@nzakas
Copy link
Member

nzakas commented Sep 15, 2021

Marking as accepted.

@nzakas nzakas moved this from Feedback Needed to Pull Request Opened in Triage Sep 15, 2021
AriPerkkio added a commit to AriPerkkio/eslint that referenced this issue Sep 16, 2021
Triage automation moved this from Pull Request Opened to Complete Sep 21, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Mar 21, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Mar 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint
Projects
Triage
Complete
Development

Successfully merging a pull request may close this issue.

3 participants