Better crash reporting

[alexzherdev]

The version of ESLint you are using.
5.12

The problem you want to solve.
When a rule crashes for any reason, it can be difficult for the users to determine which source file caused the crash - especially if they are running ESLint from the command line on a big project. We're encountering this very often after releases in eslint-plugin-react and frequently have to ask for reproduction code.

I'm filing this to double-check if anything can be done in ESLint core to improve on this.

Your take on the correct solution to problem.
It'd be ideal if before the stack trace we could output the source file name and line number that caused the crash, although I imagine at that point it wouldn't be too hard to output the actual code on that line. I'm not sure if this can be achieved within ESLint's architecture, which I'm not familiar with.

Are you willing to submit a pull request to implement this change?
Yes, although again I'm not familiar with the ESLint codebase.

[not-an-aardvark]

Hi, thanks for the proposal. Have you tried running ESLint with the --debug flag? I think this adds some debugging information about which source file caused the crash. (Outputting the relevant line number of the source file seems difficult because I'm not sure how we would determine which part of the source file broke the rule's assumptions, given that a rule can operate on any part of the AST at any given time.)

[alexzherdev]

Just tried that flag - here's what it looks like on ESLint 5.12.1:

  eslint:linter Linting code for /Users/.../file.js (pass 1) +0ms
  eslint:linter An error occurred while traversing +2s
  eslint:linter Filename: /Users/.../file.js +0ms
  eslint:linter Parser Options: { ecmaFeatures:
   { globalReturn: false,
     ...
     experimentalObjectRestSpread: true },
  ecmaVersion: 7,
  sourceType: 'module' } +0ms
  eslint:linter Parser Path: /Users/.../node_modules/babel-eslint/lib/index.js +1ms
  eslint:linter Settings: { 'import/resolver':
   { node: { extensions: [Array] },
     webpack: { config: 'webpack.dev.config.js' } },
  'import/extensions': [ '.js', '.mjs', '.jsx' ],
  ...
  propWrapperFunctions: [ 'forbidExtraProps', 'exact', 'Object.freeze' ] } +0ms
Cannot read property 'split' of undefined
MBP15:src admin$

On ESLint 4.19 it's just

  eslint:linter Linting code for /Users/.../file.js (pass 1) +0ms
Cannot read property 'split' of undefined
MBP15:src admin$

So it does output the file name, which is good. I guess we could direct users to use the --debug flag, although it'd be nice to output the file name without the flag if we're going to crash, to avoid the back-and-forth.
cc @ljharb

[ljharb]

Basically i don’t see any reason why the:

• package containing the rule that crashed
• which rule crashed, on which line
• the file and line it crashed on

aren’t always reported - i can’t conceive of any reason why that isnt relevant every single time.

[not-an-aardvark]

Isn't all of that information already inferable from the stack trace?

[ljharb]

The trace is from where it threw; it tells you nothing about which code it threw on.

[not-an-aardvark]

Sorry, I misunderstood (I thought by "the file and line it crashed on" you meant the rule file).

I think it would be reasonable to always include the target filepath that caused the crash in the error message. However, I don't think it's possible to determine the line of the target file that caused the crash.

[ljharb]

Surely the ast node’s location, for the visitor that crashed, is available?

[not-an-aardvark]

We could report the AST node's location, which would help in some cases. But it could often turn out that the crash was a result of some corrupted state from some previous point in the file, which only resulted in an error being thrown at a normal-looking AST node.

For example, if a rule's logic is "given some variable reference, iterate over all places where the variable was assigned", and one of the assignments does something that causes the rule to crash, then the crash might occur in the visitor where the variable is read, even though the actual cause of the crash is in the node where the variable is assigned.

[ljharb]

That doesn't matter; the point in the source code at which the crash occurred is invaluable in determine why the rule code crashed.

[not-an-aardvark]

Sounds reasonable, I'd be fine with improving the error message to include that info if anyone wants to create a PR. (Since this just modifies an error message for debugging and doesn't have semver impact, I don't think this would need TSC approval.)

It would probably be better to add this information to the error object that gets thrown from Linter (rather than adding console.error statements), to ensure that the info can be used and doesn't spam the console when running ESLint from an integration.

[alexzherdev]

Ok, I'm taking a stab at this. A couple of clarifying questions:

• is it enough to just add the line number of the node's starting location? (we already have the file name in the output)
• do we still want to output this information only when --debug is set? I feel like this flag is not widely known, but I also don't fully understand the consequences of outputting it without that flag. Perhaps as long as it's not --quiet, it could be fine?

[ljharb]

I would prefer to see both the code’s filename and line number always, without exception.

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[ljharb]

It’d be great to reopen this; this impacts the entire plugin ecosystem.

[not-an-aardvark]

@alexzherdev Sorry, I missed your questions. Thanks for working on this!

is it enough to just add the line number of the node's starting location? (we already have the file name in the output)

I agree with @ljharb that outputting both the line number and the filename is worthwhile in the event of a crash, regardless of whether the --debug flag is enabled.

do we still want to output this information only when --debug is set? I feel like this flag is not widely known, but I also don't fully understand the consequences of outputting it without that flag. Perhaps as long as it's not --quiet, it could be fine?

Given that this only applies when something has crashed, I think it's fine to add lots of debugging information regardless of whether a flag like --quiet is in use.

---

To reiterate #11304 (comment), I think this would involve adding information to a thrown error object rather than adding something like console.error, so I don't think it should be conditional on whether debug mode is enabled.

[alexzherdev]

Sounds good. I understand this is impactful and I’ll try to find time to complete this 👍
I think I had questions around testing this when I first looked into it, but to start I’ll do a PR when I have something to show.