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: syntax errors created by object-shorthand autofix (fixes #7574) #7575

Merged
merged 2 commits into from Nov 11, 2016
Merged

Fix: syntax errors created by object-shorthand autofix (fixes #7574) #7575

merged 2 commits into from Nov 11, 2016

Conversation

not-an-aardvark
Copy link
Member

@not-an-aardvark not-an-aardvark commented Nov 10, 2016 •
edited

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

[ ] Documentation update
[x] 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
[ ] Other, please explain:

See #7574

What changes did you make? (Give an overview)

This fixes several bugs in object-shorthand's autofixing.

  • Previously, if an object had a computed key that was anything other than an identifier corresponding to a generator function property, the fixer would output unbalanced square brackets with undefined as the property name.

    ({ [foo.bar]: function*() {} });

// was previously fixed to:

({ [*[undefined]() {} }); // unexpected `undefined`

  • Previously, the fixer would output invalid syntax if there was a space between a computed property and its closing bracket.

    ({ [ foo ]: function() {} });

// was previously fixed to:

({ [ foo () {} }); // no closing ]

  • Previously, the fixer would output invalid syntax for generator functions if there was a space before the star.

    ({ foo: function *() {} });

// was previously fixed to:

({ *foo*() {} }); // extra star

  • Previously, the fixer would output incorrect fixes for async functions (Linting with auto-fix with rule 'object-shorthand' breaks properties with async/await functions #7574).

    ({ foo: async function() {} });

// was previously fixed to:

({ foonction() {} }); // lol `foonction`

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

This is a fairly bad bug, since the resulting code after autofixing an async function is still syntactically valid (so the user won't notice the issue), but it does something completely different than before. I'm hoping we can expedite this PR to get the fix into the upcoming release.

However, it is a semver-patch change, so it could also go into a patch release if we decide to create one for this release.

@not-an-aardvark not-an-aardvark added accepted There is consensus among the team that this change meets the criteria for inclusion bug ESLint is working incorrectly rule Relates to ESLint's core rules labels Nov 10, 2016
@mention-bot
Copy link

@not-an-aardvark, thanks for your PR! By analyzing the history of the files in this pull request, we identified @NickHeiner, @platinumazure and @martijndeh to be potential reviewers.

@eslintbot
Copy link

LGTM

vitorbal
vitorbal approved these changes Nov 10, 2016
View reviewed changes
Copy link
Member

@vitorbal vitorbal left a comment

Choose a reason for hiding this comment

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

Nice job on the quick turnaround time for this bug. Thanks a lot! I just have a few suggestion for additional tests, but otherwise LGTM

tests/lib/rules/object-shorthand.js
{ code: "({ foo: async function () {} })", output: "({ async foo () {} })", parserOptions: { ecmaVersion: 8 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ 'foo': async function() {} })", output: "({ async 'foo'() {} })", parserOptions: { ecmaVersion: 8 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ [foo]: async function() {} })", output: "({ async [foo]() {} })", parserOptions: { ecmaVersion: 8 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ [foo.bar]: function*() {} })", output: "({ *[foo.bar]() {} })", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test for the case when a generator function has a space before the star as well? Like in your PR overview example:

({ foo: function *() {} });

tests/lib/rules/object-shorthand.js
@@ -144,6 +144,11 @@ ruleTester.run("object-shorthand", rule, {
{ code: "doSomething({y: function() {}})", output: "doSomething({y() {}})", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "doSomething({[y]: function() {}})", output: "doSomething({[y]() {}})", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "doSomething({['y']: function() {}})", output: "doSomething({['y']() {}})", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ foo: async function () {} })", output: "({ async foo () {} })", parserOptions: { ecmaVersion: 8 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ 'foo': async function() {} })", output: "({ async 'foo'() {} })", parserOptions: { ecmaVersion: 8 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ [foo]: async function() {} })", output: "({ async [foo]() {} })", parserOptions: { ecmaVersion: 8 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
Copy link
Member

Choose a reason for hiding this comment

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

Just in case, how about an additional test for async and spaces inside the square brackets combined:

({ [ foo ]: async function() {} })

tests/lib/rules/object-shorthand.js
{ code: "({ 'foo': async function() {} })", output: "({ async 'foo'() {} })", parserOptions: { ecmaVersion: 8 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ [foo]: async function() {} })", output: "({ async [foo]() {} })", parserOptions: { ecmaVersion: 8 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ [foo.bar]: function*() {} })", output: "({ *[foo.bar]() {} })", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
{ code: "({ [foo ]: function() {} })", output: "({ [foo ]() {} })", parserOptions: { ecmaVersion: 6 }, errors: [{ message: "Expected method shorthand.", type: "Property" }] },
Copy link
Member

Choose a reason for hiding this comment

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

I guess I'm being a bit picky (please don't hate me) but could we add a couple more tests, just in case, for when there are spaces only after the opening bracket, and for when there are spaces after the opening and the closing bracket?

({ [  foo   ]: function() {} })
({ [  foo]: function() {} })

@eslintbot
Copy link

LGTM

@not-an-aardvark
Copy link
Member Author

Updated to add the requested tests.

platinumazure
platinumazure approved these changes Nov 10, 2016
View reviewed changes
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, but would like someone else to take a look and make sure there are enough test cases.

@vitorbal vitorbal merged commit b8d6e48 into master Nov 11, 2016
@vitorbal vitorbal deleted the object-shorthand-autofix-async branch November 11, 2016 16:26
@not-an-aardvark not-an-aardvark mentioned this pull request Dec 12, 2016
Closed
@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.
Reviewers

@platinumazure platinumazure platinumazure approved these changes

@vitorbal vitorbal vitorbal approved these changes

@ilyavolodin ilyavolodin ilyavolodin approved these changes

Assignees
No one assigned
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 bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@not-an-aardvark @mention-bot @eslintbot @platinumazure @vitorbal @ilyavolodin