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

Upgrade: espree to 3.2.0, remove tests with SyntaxErrors (fixes #7169) #7170

Merged
merged 1 commit into from Sep 17, 2016

Conversation

not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Sep 16, 2016

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
[ ] Add something to the core
[x] Other, please explain:

Please check each item to ensure your pull request is ready:

  • I've read the pull request guide
  • I've included tests for my change
  • I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)

This removes tests that were failing due to the update to Espree 3.2.0. (See #7169)

These tests contained code that had a SyntaxError according to the ES spec, but previously had been parsed by Acorn rather than throwing an error. The fix is to just remove the tests.

Downstream users of ESLint will not be affected by this update unless their code contained invalid syntax, in which case this update is a bugfix for them. (Code like this would have thrown an early error at runtime anyway.)

edit: For what it's worth, here's the clause in ecma262:

It is a Syntax Error if IsSimpleParameterList of FormalParameterList is false and BoundNames of FormalParameterList contains any duplicate elements.

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

Also see: #7101

@eslintbot
Copy link

LGTM

@mention-bot
Copy link

@not-an-aardvark, thanks for your PR! By analyzing the annotation information on this pull request, we identified @nzakas, @pmcelhaney and @mysticatea to be potential reviewers

Copy link
Member

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

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

LGTM, as soon as we confirm this is the way we want to fix this issue.

@ljharb
Copy link
Sponsor Contributor

ljharb commented Sep 16, 2016

@bterlson - can you confirm that duplicate args in non-simple parameter lists are a syntax error here, and whether that's true in both strict and sloppy mode?

@bterlson
Copy link

@ljharb: confirmed, and it's true in both sloppy and strict. See: https://tc39.github.io/ecma262/#sec-function-definitions-static-semantics-early-errors.

@michaelficarra
Copy link
Member

LGTM

@mysticatea
Copy link
Member

@not-an-aardvark Could you update package.json as well? I'd like to ensure using new espree. Also, I think proper Upgrade: instead of Chore:.

@eslintbot
Copy link

LGTM

@not-an-aardvark not-an-aardvark changed the title Chore: remove tests that contain SyntaxErrors (fixes #7169) Upgrade: espree to 3.2.0, remove tests with SyntaxErrors (fixes #7169) Sep 17, 2016
Copy link
Member

@mysticatea mysticatea left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@mysticatea
Copy link
Member

Though this does not have 2 days yet, I'll merge this PR since this has gotten enough eyes and other PRs are failing tests.

@mysticatea mysticatea merged commit 35f7be9 into eslint:master Sep 17, 2016
@not-an-aardvark not-an-aardvark deleted the fix-failing-tests branch September 17, 2016 00:21
@mysticatea mysticatea mentioned this pull request Sep 17, 2016
12 tasks
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants