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

Allow soaks and prototype shorthands in object splats #5293

Merged

Conversation

helixbass
Copy link
Collaborator

Fixes #5291

This PR expands the grammar for allowable object splats to include prototype shorthands (eg a = {b::c...}) and soaks (eg a = {b?.c..., d?()...})

o '. Property', -> new Access $2
o 'INDEX_START IndexValue INDEX_END', -> $2
o 'INDEX_START INDENT IndexValue OUTDENT INDEX_END', -> $3
o 'SimpleObjAssignable Accessor', -> (new Value $1).add $2
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems like there was no need to have a specific ObjSpreadAccessor rule, it worked to just use the existing Accessor rule (which includes prototype shorthands and soaks)

o 'DYNAMIC_IMPORT Arguments', -> new DynamicImportCall LOC(1)(new DynamicImport), $2
o 'SimpleObjAssignable Arguments', -> new Call (new Value $1), $2
o 'ObjSpreadExpr Arguments', -> new Call $1, $2
o 'SUPER OptFuncExist Arguments', -> new SuperCall LOC(1)(new Super), $3, $2.soak, $1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This adds support for soaked calls as object splats by mimicking the existing Invocation rules (which include normal soaked calls)

The general question would be "why are we duplicating all of these existing grammar rules here for object spreads? why not just use the existing Splat rule?"

As far as I could tell, there are two reasons:

  1. since we allow "computed shorthand" object syntax (eg {[a]}), there would be a grammar ambiguity if we also allowed arrays as object splat targets (eg {[a]...}). Personally I don't see the value in the computed shorthand object syntax (which generates an object with a key that matches its value eg {[a]: a})

  2. In order to avoid a grammar ambiguity, our object splat grammar rule (ObjRestValue) has to use SimpleObjAssignable as a "building block" (since that's what AssignObj uses for parsing shorthand object properties/object keys) (?)

But so there are still various expressions that are allowed as array/argument splats but not object splats eg:

[null...] # ok
f(null...) # ok
{null...} # parse error

[a + b...] # ok
f(a + b...) # ok
{a + b...} # parse error

[a = b...] # ok
f(a = b...) # ok
{a = b...} # parse error

None of these examples necessarily make a ton of sense/are good ideas, and you can always achieve the object splat version by wrapping the splat expression in parens, but it's still not great to have inconsistency between array vs argument vs object splats in terms of what's syntactically allowed

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can make all of those ok examples you cite parse errors if you want, they’re all invalid JavaScript:

> [...null]
// TypeError: object null is not iterable (cannot read property Symbol(Symbol.iterator))
> fn = () => {}; fn(...null)
// TypeError: fn is not iterable (cannot read property Symbol(Symbol.iterator))
> [a + ...b]
// SyntaxError: Unexpected token '...'
> fn = () => {}; fn(a + ...b)
// SyntaxError: Unexpected token '...'
> [a = ...b]
// SyntaxError: Unexpected token '...'
> fn = () => {}; fn(a = ...b)
// SyntaxError: Unexpected token '...'

Personally I don't see the value in the computed shorthand object syntax

Perhaps, but I find myself using the shorthand object syntax a lot, especially as the “function options object” pattern becomes more common. I feel like someone out there has used {[a]}, and we shouldn’t make a breaking change if all that we achieve is simplifying grammar.coffee a little. Does this PR continue to allow the computed shorthand object syntax?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can make all of those ok examples you cite parse errors if you want, they’re all invalid JavaScript

Ya they're not parse errors in JS, just will fail at runtime (your 3rd-6th example should be phrased differently to be the equivalent JS: a + ...b -> ...a + b, a = ...b -> ...a = b). So I don't think there's a need to make them parse errors in Coffeescript

And it would force a more complex grammar (compared to Splat currently just being eg ... Expression) to make some splat targets allowed but not others

That's basically what's going on here is object splats sadly can't use that simple Splat grammar, which is what's resulting in this gap between legal array/argument splats vs object splats

I feel like someone out there has used {[a]}, and we shouldn’t make a breaking change if all that we achieve is simplifying grammar.coffee a little

Ya I agree. Just in hindsight I wouldn't have allowed {[a]}. The thing we lose (unless there's some fancy grammar/rewriter trick to avoid the grammar ambiguity) is the ability to allow eg {[a]...}, which while not common/super useful (a) is valid in JS (eg ({...[a]}) (b) is allowed for array/argument splats (eg [[a]...], f([a]...))

Does this PR continue to allow the computed shorthand object syntax?

Yup it doesn't disallow any existing constructs, only adds newly allowed ones

@GeoffreyBooth
Copy link
Collaborator

Thanks, I’ll release 2.5.1 soon unless you have any more fixes incoming.

@helixbass
Copy link
Collaborator Author

@GeoffreyBooth I started working on a fix for #5292. If you don't mind holding off for a day or two let me see if I can get that working

@helixbass helixbass deleted the iss5291-object-spread-grammar branch January 27, 2020 23:54
@GeoffreyBooth GeoffreyBooth mentioned this pull request Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Parser: soaks/prototype shorthands in object spreads
2 participants