Skip to content
This repository was archived by the owner on Feb 22, 2018. It is now read-only.

Named arguments that are reserved ES6 keywords cannot be destructured #392

Closed
ochafik opened this issue Dec 2, 2015 · 11 comments
Closed

Comments

@ochafik
Copy link
Contributor

ochafik commented Dec 2, 2015

Right now we rename variables only. With destructuring of args (#180), looks like we would hit issues with reserved keywords:

main() {
  void f({delete, function}) {
    print("delete: $delete");
    print("function: $function");
  }
  f(delete: 1, function: 2);
}

Would give some syntax-error-full code:

function main() {
  function f({delete = null, function = null} = {}) {
    core.print(`delete: ${delete}`);
    core.print(`function: ${function}`);
  }
  f({delete: 1, function: 2});
}

(note: compiler currently works fine, issues are only with destructuring of named args)

@jmesserly
Copy link
Contributor

@ochafik we shouldn't rename those. It can be written in a JS friendly way like this:

{'delete': 1, 'function': 2}

The bug here is in js_ast's handling of object literals.

In general, one thing to keep in mind with JS: it's fine to have almost any name as a field of an object. The naming restrictions apply for locals (as you can see, they were both renamed).

We also need to avoid whenever possible renaming top-level things. Those are part of the API, part of the module object, and should keep their names. In other words, renaming is a last resort.

@jmesserly jmesserly self-assigned this Dec 2, 2015
@jmesserly jmesserly changed the title Need to rename named arguments that are reserved ES6 keywords Named arguments that are reserved ES6 keywords are not working Dec 2, 2015
@jmesserly
Copy link
Contributor

actually, wait a minute. The current code works just fine (try this in the console):

(function() { 'use strict'; return {function: 1, delete:2 } })()

The compiler at one point did quote these, (printer.dart, propertyNameOut) but it is not needed in ES6 (and I believe this was true in ES5 as well).

@jmesserly
Copy link
Contributor

oops hit send too soon. Did you see an actual problem? Or it just didn't look right to you?

@jmesserly
Copy link
Contributor

from the spec:
Reserved Words

A reserved word is an IdentifierName that cannot be used as an Identifier.

Object Initializer

[...]
LiteralPropertyName : IdentifierName [...]

@ochafik
Copy link
Contributor Author

ochafik commented Dec 2, 2015

Actually sorry, these identifiers only cause issues in some situations (roughly, as lvalues), one of which would be destructuring of named arguments (#180):

var function = 1; // error
var {function} = {}; // error

// That's where named params might be encountered if we destructure named args:
function foo({function}) {} // error

var x = {function: 1}; // ok
{function: 1} // error when entered in the console (because it's ambiguous w/ destructuring?)

(edited description accordingly)

@ochafik
Copy link
Contributor Author

ochafik commented Dec 2, 2015

So, if we prioritize preservation of named args, this means we can't use param destructuring in those cases.

But note that destructuring solves other issues, like the mix of string & identifier we have right now (which hinders Closure's advanced optimizations #311): let b = opts && 'b' in opts ? opts.b : null;

I'm biased but I would prioritize destructuring and would rename function and other reserved words when used as named params... WDYT? (hopefully, shouldn't happen too often anyway)

@jmesserly
Copy link
Contributor

IMO, we should always prioritize generating code with the right APIs. That way the code does what the developer intended. Otherwise there are some JS argument names you can't express correctly in Dart. Thus far, we try and rename as an absolute last resort (and most of the cases are around local variables, where it doesn't leak into the APIs).

I'm not familiar enough with Closure's advanced opts to know what to do, but surely there's a pattern of some sort that works for JS named arguments (that doesn't rely on just-implemented ES6 destructuring). And if not, we could do more good for the JS world by improving Closure than handicapping DDC's output.

@jmesserly
Copy link
Contributor

regarding:

{function: 1} // error when entered in the console (because it's ambiguous w/ destructuring?)

this works:

function foo() {}
foo({function: 1}); // works at console
({function: 1}); // also works

IIRC, the ambiguity existed in ES5. It's because { foo: ... could be a block with a label on the first statement. So open curly at that level of the grammar is always parsed as the start of a block (http://www.ecma-international.org/ecma-262/6.0/#sec-statements). This comes up in the context of arrow functions too. If you want to return an object literal, you have to write: (x, y) => ({ foo: x, bar: y }) instead of (x, y) => { foo: x, bar: y }, as the latter is a block function body, not an object literal.

@ochafik
Copy link
Contributor Author

ochafik commented Dec 2, 2015

Ah, true, parenthesis save the top-level case, but they're not accepted in function params :-S
(function f(({function})) {}).

I'll just drop out of destructuring when any param has a reserved name then, thanks!

(thanks for the context! starting to get why some people thought they'd be better off writing a new language from scratch ;-))

@jmesserly
Copy link
Contributor

yeah, that makes sense. The object destructuring needs to work as both a local variable, as well as for an object property, since it's sort of like doing let b = opts && 'b' in opts ? opts.b : null; in one step. Except for things like function we were handling that by renaming the local let func = opts && 'function' in opts ? opts.function : null;. I guess destructuring can't express the second one. Some systems can support that, often with an "as" (like how imports can be renamed). So it'd be function foo({function as func} = {}) but I'm guessing ES6 didn't include that feature.

@ochafik ochafik changed the title Named arguments that are reserved ES6 keywords are not working Named arguments that are reserved ES6 keywords cannot be destructured Dec 2, 2015
@ochafik
Copy link
Contributor Author

ochafik commented Dec 2, 2015

The aliasing syntax you suggest would be neat & consistent with the import syntax... (hehe, "just" have to file a request for ES7?)

(closing this question, will link to it from the destructuring drop-out logic)

@ochafik ochafik closed this as completed Dec 2, 2015
ochafik pushed a commit that referenced this issue Dec 2, 2015
# The first commit's message is:
Use destructuring assignments for named parameters (#180)

Before (for `f(a, {b, c: c_default})`):
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter clashes with reserved JS names (see discussion in #392)
- When a parameter clashes with a Object.prototype property, using a clean default opts value (Object.create(null)).
- Passing opts through in aliased constructors, both for speed/concision and correctness (since default param value semantic is weird there)

# This is the 2nd commit message:

Rename Destructuring to BindingPattern

# This is the 3rd commit message:

Revert parseLeftHandSide + parseVariableBinding

# This is the 4th commit message:

Drop out of destructuring when names clash with js reserved names
ochafik pushed a commit that referenced this issue Dec 2, 2015
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter clashes with reserved JS names (see discussion in #392)
- When a parameter clashes with a Object.prototype property, using a clean default opts value (Object.create(null)).
- Passing opts through in aliased constructors, both for speed/concision and correctness (since default param value semantic is weird there)

BUG=
[email protected]

Review URL: https://codereview.chromium.org/1484263002 .
nex3 pushed a commit to dart-lang/sdk that referenced this issue Aug 31, 2016
  function f(a, opts) {
    let b = opts && 'b' in opts ? opts.b : null;
    let c = opts && 'c' in opts ? opts.c : c_default;
    ...

After:
  function f(a, {b = null, c = c_default} = {}) {
    ...

Note:
- Still reverting to old code when any parameter clashes with reserved JS names (see discussion in dart-archive/dev_compiler#392)
- When a parameter clashes with a Object.prototype property, using a clean default opts value (Object.create(null)).
- Passing opts through in aliased constructors, both for speed/concision and correctness (since default param value semantic is weird there)

BUG=
[email protected]

Review URL: https://codereview.chromium.org/1484263002 .
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

2 participants