Issue #1311: Support destructuring function parameters #1450

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@arai-a
Contributor
arai-a commented Jan 2, 2014

This patch adds support for following destructuring function parameters:

  • toplevel array: ([a, b])
  • elision in array: ([a, , b])
  • parenthesis: ([a, (b)])
@rwaldron rwaldron commented on the diff Jan 2, 2014
tests/unit/parser.js
+ .addError(24, "'arrow function syntax (=>)' is only available in JavaScript 1.7.")
+ .addError(27, "'destructuring expression' is only available in JavaScript 1.7.")
+ .addError(27, "'destructuring expression' is only available in JavaScript 1.7.")
+ .addError(27, "'arrow function syntax (=>)' is only available in JavaScript 1.7.")
+ .addError(28, "'destructuring expression' is only available in JavaScript 1.7.")
+ .addError(28, "'destructuring expression' is only available in JavaScript 1.7.")
+ .addError(28, "'arrow function syntax (=>)' is only available in JavaScript 1.7.")
+ .addError(29, "'destructuring expression' is only available in JavaScript 1.7.")
+ .addError(29, "'destructuring expression' is only available in JavaScript 1.7.")
+ .addError(29, "'arrow function syntax (=>)' is only available in JavaScript 1.7.")
+ .addError(30, "'destructuring expression' is only available in JavaScript 1.7.")
+ .addError(30, "'destructuring expression' is only available in JavaScript 1.7.")
+ .addError(30, "'arrow function syntax (=>)' is only available in JavaScript 1.7.")
+ .test(src, {es3: true, unused: true, undef: true});
+
@rwaldron
rwaldron Jan 2, 2014 Member

Why is "is only available in JavaScript 1.7." an appropriate error message? It's not really true, is it? Destructuring is allowed in both ES6 (with esnext) and JS1.7 (with moz)

@arai-a
Contributor
arai-a commented Jan 2, 2014

Ah, as you wrote, the error message is wrong.

By the way, many other ES6 related features (spread/rest, star generator, class, for of) also use W104: "'{a}' is only available in JavaScript 1.7." error message.
If I understand correctly, all of them (including arrow function, except destructuring) are available only in ES6.
JavaScript 1.7 does not have them.

Do "JavaScript 1.7" and "moz" represent SpiderMonkey's current implementation, or actual JavaScript 1.7?
If formar, we should change the error message to "... is only available in ES6 and JavaScript 1.7",
if latter, "... is only available in ES6".

@arai-a
Contributor
arai-a commented Jan 2, 2014

I've looked at ES6/JS1.7 related messages, and I found that most of them are not appropriate.

Message should be changed in following:
(Assuming that "JavaScript1.7" and "moz" mean SpiderMonekey's current implementation, If I'm wrong, please tell me)

  1. only JS1.7(W104) -> only ES6 and JS1.7
    • arrow function
    • destructuring expression
    • spread/rest
    • destructuring assignment
    • generator functions
    • const
    • let
    • for of
    • yield
  2. only ES6(W119) -> only ES6 and JS1.7
    • array comprehension
    • default parameters
    • function*
  3. only JS1.7(W104) -> only ES6(W119)
    • concise methods
    • class

Besides, following conditions are not appropriate, and should be changed:

  1. inESNext(true) -> inESNext()
    • function*
      (implemented in moz (Firefox 26))
  2. inESNext() -> inESNext(true)
    • concise methods
    • class
    • import
    • export
      (not yet implemented in moz)

I prepared a series of patches for them:
https://github.com/arai-a/jshint/commits/es6message

May I include above in the patch for this issue,
or is it better to open dedicated issue(s)?

@rwaldron
Member
rwaldron commented Jan 2, 2014

This would be very helpful, but here:

Assuming that "JavaScript1.7" and "moz" mean SpiderMonekey's current implementation

...is not quite right. "moz" means "JS1.7/1.8" ie. https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.7 or https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/1.8. The problem is that some things, like destructuring, let and const, are currently implemented incorrectly in SpiderMonkey. The whole thing is understandably confusing. Here's a modified version of your list:

  1. Only ES6
    • arrow function
    • star function (ie. function*, implemented in Firefox 26)
      • yield is restricted to the body of a function*
    • concise methods
    • class
    • import
    • export (not yet implemented in moz)
  2. Only ES6 and JS1.7
    • spread/rest
    • destructuring assignment
    • generator functions (the starless function)
    • const
    • let
    • for of
    • yield
    • array comprehension
    • default parameters (w/ destructuring)
@arai-a
Contributor
arai-a commented Jan 2, 2014

Thank you for letting me know it.

Only ES6 and JS1.7

  • spread/rest

They are only ES6, aren't they?
JS1.7/JS1.8.x does not contain it.

Here is a patch for it, based on destParam branch:
arai-a@ca3c747

By the way, why "moz" option make inESNext() true?

state.option.inESNext = function (strict) {
if (strict) {
return !state.option.moz && state.option.esnext;
}
return state.option.moz || state.option.esnext;
};

@arai-a
Contributor
arai-a commented Jan 2, 2014

Sorry, it was described in following comment, I overlooked:

// Let's assume that chronologically ES3 < ES5 < ES6/ESNext < Moz

But, if "moz" is JS1.7/1.8, I think it's not true that 'ES6/ESNext < Moz'.

So that inESNext should be:

state.option.inESNext = function (strict) {
  if (strict) {
    return !state.option.moz && state.option.esnext;
  }
  return state.option.esnext;
};

and it may be better to make following function for 'Only ES6 and JS1.7' features:

state.option.inESNextOrMoz = function (/* strict */) {
  return state.option.moz || state.option.esnext;
};

Here is a draft patch for it:
arai-a@3935afb

@rwaldron
Member
rwaldron commented Jan 2, 2014

Only ES6 and JS1.7

  • spread/rest
    They are only ES6, aren't they?

You are right, I overlooked that.

I originally agreed with your questioning of the version chronology and hierarchy, but the reason it's in place is due to incompatible features, eg. if I switch on moz because some code is using JS1.7/1.8 generators, JSHint needs to show me errors if I accidentally add an ES6 star function generator

@arai-a
Contributor
arai-a commented Jan 2, 2014

Ah, I see.
If we remove moz from inESNext(), we have to use both moz and esnext for recent Mozilla related code (if it uses arrow function or spread/rest), and it makes inMoz(true) false (so we cannot use let expression etc).
Okay, please ignore the patch for inESNext for now.

Then, I updated the patch for error message (fix spread/rest message), based on destParam branch:
arai-a@a275134

@valueof valueof added a commit that referenced this pull request Jan 26, 2014
@arai-a @valueof arai-a + valueof Fixed #1311: Add support for destructuring function parameters (PR #1450
)

Signed-off-by: Anton Kovalyov <anton@kovalyov.net>
9189c74
@valueof valueof closed this Jan 26, 2014
@arai-a arai-a deleted the arai-a:destParam branch Jan 26, 2014
@ccverg
ccverg commented Jan 26, 2014

@arai-a you should go claim the bounty!

@arai-a
Contributor
arai-a commented Jan 26, 2014

Thank you for letting me know about bounty.
however, this problem is not fixed correctly, and I've filed new issue for this:
#1491
and already sent pull-request for it:
#1492
I'd like to wait until this fix is also merged.

@jugglinmike jugglinmike added a commit to jugglinmike/jshint that referenced this pull request Oct 21, 2014
@arai-a @jugglinmike arai-a + jugglinmike Fixed #1311: Add support for destructuring function parameters (PR #1450
)

Signed-off-by: Anton Kovalyov <anton@kovalyov.net>
990413f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment