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

Update: Include debugging information when rule throws error #9742

Merged
merged 8 commits into from Mar 30, 2018

Conversation

@pfhayes
Contributor

pfhayes commented Dec 20, 2017

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

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

What changes did you make? (Give an overview)

When developing eslint rules (including for plugins), tracking down
errors within rules is difficult and requires manual updating of the
code to track down where the error is coming from. This adds additional
diagnostic information which can help

  eslint:traverser An error occurred while traversing +736ms
  eslint:traverser Filename: /Users/patrick/dev/test.js +0ms
  eslint:traverser Parser Options: { ecmaFeatures: { globalReturn: false, jsx: true },
  codeFrame: false,
  ecmaVersion: 6,
  sourceType: 'module' } +0ms
  eslint:traverser Parser Path: /Users/patrick/dev/node_modules/babel-eslint/index.js +2ms
  eslint:traverser Settings: { react: { pragma: 'React' } } +0ms

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

I opted for debug logging to make this minimally disruptive, but I
would also advocate for promoting this information to being always logged to
stderr. This information will be valuable to developers working on
rules, but may also be valuable to the end developer to help them track
down the source of their error

Update: Include debugging information when rule throws error
When developing eslint rules (including for plugins), tracking down
errors within rules is difficult and requires manual updating of the
code to track down where the error is coming from. This adds additional
diagnostic information which can help

```
  eslint:traverser An error occurred while traversing +736ms
  eslint:traverser Filename: /Users/patrick/dev/test.js +0ms
  eslint:traverser Parser Options: { ecmaFeatures: { globalReturn: false, jsx: true },
  codeFrame: false,
  ecmaVersion: 6,
  sourceType: 'module' } +0ms
  eslint:traverser Parser Path: /Users/patrick/dev/node_modules/babel-eslint/index.js +2ms
  eslint:traverser Settings: { react: { pragma: 'React' } } +0ms
```

I opted for `debug` logging to make this minimally disruptive, but I
would also advocate for promoting this information to being always logged to
stderr. This information will be valuable to developers working on
rules, but may also be valuable to the end developer to help them track
down the source of their error
@platinumazure

Should we include node information as well? If we could get at the RuleContext object, we could use context.getSourceCode().getText(node) to show the node source text. Maybe it would be good to show only the first 50-100 characters or so in case it's a large node, though.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Jan 6, 2018

TSC Summary: When a rule crashes, there isn't a lot of good debugging information available. This pull request uses the debug module to log the currently linted file, parser options, parser path, and shared plugin settings object when a rule throws an unhandled exception, and could be augmented to include information on the node in question.

TSC Question: Should we accept this enhancement?

@wmertens

This comment has been minimized.

wmertens commented Jan 21, 2018

Would it be an option to not re-throw the error, instead letting the rules that did not crash still do their thing?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Jan 21, 2018

Would it be an option to not re-throw the error, instead letting the rules that did not crash still do their thing?

As long as we can figure out a way to report the error (including stack trace, etc.) while still reporting other role messages, I'm certainly not opposed, but I think we would need a concrete proposal. I could also see a CLI option to basically turn thrown errors into brief rule messages, trading debug information for the ability to keep running other rules.

I'm also okay with implementing this PR as a start and revisiting afterwards, as well.

@kaicataldo

This comment has been minimized.

Member

kaicataldo commented Feb 1, 2018

This issue was accepted at today's TSC meeting.

@platinumazure

Could you please add some unit tests, and maybe a functional/integration test where a (fake) rule actually crashes and we test for proper debugging output? Thanks!

@pfhayes

This comment has been minimized.

Contributor

pfhayes commented Feb 3, 2018

@platinumazure i added a test. I wasn't sure what the best practices around a more involved integration test would be, as I didn't see any similar tests in the repo. I'm happy to add further tests if any guidance is available

@pfhayes pfhayes changed the title from Include debugging information when rule throws error to Update: Include debugging information when rule throws error Feb 3, 2018

@platinumazure

LGTM, thanks!

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Feb 3, 2018

Thanks for the PR! Was there a reason you added this logic to traverser rather than adding it to Linter directly? It seems like that could avoid the need to pass around sharedTraversalContext. (From a code cleanliness perspective, I'm a bit averse to passing around sharedTraversalContext because it's an implementation detail of Linter, and doesn't have all of the properties that one might expect from dealing with context objects in rules.)

To answer your question, there are a few integration tests in tests/bin/eslint.js, although in this case I'm not sure whether that would be any better because our debugging messages aren't considered to be a public API. I think the way you did it is generally fine.

@pfhayes

This comment has been minimized.

Contributor

pfhayes commented Feb 7, 2018

I've moved the relevant code into the Linter class

});
} catch (err) {
this._logger("An error occurred while traversing");
if (this._context) {

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Feb 7, 2018

Member

Was this a typo? I don't think this._context exists on Linter.

The filename, parser options, etc. should be in scope here already, so it's probably not necessary to pull them from context.

This comment has been minimized.

@pfhayes

pfhayes Feb 7, 2018

Contributor

fixed, cc @not-an-aardvark

@pfhayes

This comment has been minimized.

Contributor

pfhayes commented Feb 7, 2018

Good catch - I just updated them

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Feb 7, 2018

@platinumazure I'm a bit worried about the need to expose the logger functionality to users of Linter for testing purposes -- we've had issues in the past with people monkeypatching Linter to get unintended functionality, which prevented us from changing things afterwards. I'm wondering if a test is actually necessary here, given that we don't have tests for any of our other debugging lines. (I'm also thinking this should have been labeled as chore and not needed TSC approval, since the TSC generally doesn't need to approve debug lines when they're added as part of other features.)

@platinumazure

This comment has been minimized.

Member

platinumazure commented Feb 7, 2018

Okay, I can buy that we don't want to expose things on Linter and that we don't need a test.

I think whether this needed TSC discussion is debatable. I don't see this as a chore. To me, this was an enhancement that deserved some discussion on whether the benefit was worth the maintenance overhead, like any other enhancement to core. I'm glad we decided it was.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Feb 7, 2018

I suppose my thinking is that this isn't a public API, so if we decided later that it wasn't worth the maintenance overhead, we could just remove it.

@platinumazure platinumazure requested a review from not-an-aardvark Mar 27, 2018

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 27, 2018

Hi @pfhayes, sorry we lost track of this issue.

Would you mind rebasing to fix the merge conflict currently in this PR? Then hopefully @not-an-aardvark can take a look at your changes since his last review and then hopefully we can merge this in. Thanks!

@platinumazure platinumazure self-assigned this Mar 27, 2018

pfhayes added some commits Mar 27, 2018

@pfhayes

This comment has been minimized.

Contributor

pfhayes commented Mar 27, 2018

@@ -908,6 +908,7 @@ module.exports = class Linter {
this.rules = new Rules();
this.environments = new Environments();
this._logger = debug;

This comment has been minimized.

@kaicataldo

kaicataldo Mar 29, 2018

Member

Just for my own understanding - is the reasoning for creating this property to make it testable?

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 29, 2018

@kaicataldo

This comment has been minimized.

Member

kaicataldo commented Mar 29, 2018

Agreed - my preference would be to use debug directly here and not add another property.

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 29, 2018

I agree, I think it would be better to not add an additional property to linter.

@platinumazure

Based on team feedback, two things:

  1. Could we remove yarn.lock?
  2. Could we remove the _logger property (and, as necessary, any unit tests that might have led to that property being created)?

Thanks! And sorry for the confusion from my original request.

@ljharb

This comment has been minimized.

Contributor

ljharb commented Mar 29, 2018

@platinumazure i'd recommend gitignoring all three common lockfile formats, to prevent them from being accidentally included in PRs and breaking CI builds.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 29, 2018

@ljharb Good call. yarn.lock, package-lock.json, what's the third? npm-shrinkwrap.json?

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 29, 2018

Would ignoring those files cause an issue if someone has a local version which is out of date? I had been thinking we should just add a test that fails if those files exist.

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 29, 2018

@not-an-aardvark I don't think so. Adding to .gitignore just means the files aren't in the repository and can't be git added, but the filesystem/npm/yarn/other programs can do whatever they want with the files. Seems pretty low-risk to me. Worst case, we could unignore them in a semver-patch release.

@ljharb

This comment has been minimized.

Contributor

ljharb commented Mar 29, 2018

@platinumazure yep, that's all three. Also add an .npmrc to the repo with package-lock=false in it.

@not-an-aardvark it would mean that anyone who wasn't freshly rebased might miss out on the ignores, but they'll catch up eventually.

Separately, yarn.lock and package.lock.json only apply if you're running npm install directly inside the eslint project; and npm-shrinkwrap.json is ignored by yarn, and would prevent deduping if it were ever published. Gitignoring it is a good way to prevent it from being created (and it's less likely than the others to be accidentally created).

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 29, 2018

To clarify, I mean that if an eslint contributor has a package-lock.json/npm-shrinkwrap.json file locally, and then we make changes to the dependencies upstream, and the contributor pulls those changes without manually editing their lockfile, it seems like they could mess up their local development environment as a result of having that file, because the packages in npm-shrinkwrap.json and package.json would be incompatible.

This scenario seems more likely and more difficult to debug if we add the files to .gitignore, because the contributor might not notice that they have a lockfile locally, so it would be hard to figure out what's wrong with their local environment. On the other hand, if we add a test that fails when those files are present, then we would be able to prevent the problem entirely because the contributor would notice their lockfile and delete it.

I'm in agreement that we should prevent these files from being committed. I'm proposing that we should loudly warn a contributor about the files rather than ignoring them if the files could cause any issues by being present locally.

@pfhayes

This comment has been minimized.

Contributor

pfhayes commented Mar 30, 2018

@not-an-aardvark @platinumazure thanks, I've made the requested updates

@platinumazure

This comment has been minimized.

Member

platinumazure commented Mar 30, 2018

LGTM, thanks!

@not-an-aardvark @kaicataldo Does this look good on your end?

@not-an-aardvark

LGTM, thanks!

@kaicataldo

LGTM. Thanks for contributing!

@not-an-aardvark not-an-aardvark merged commit 462b058 into eslint:master Mar 30, 2018

5 checks passed

commit-message PR title follows commit message guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Mar 30, 2018

Thanks for contributing!

@eslint eslint bot locked and limited conversation to collaborators Sep 27, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.