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

MemberExpressions cannot be used as Patterns #161

Closed
wants to merge 2 commits into from

Conversation

jamiebuilds
Copy link

let {a, b} // << valid pattern
let a.b // << invalid pattern

```js
let {a, b} // << valid pattern
let a.b // << invalid pattern
```
```js
var obj = { prop: 'foo' };
for (obj.prop in { bar: 1, baz: 2 }) {}
console.log(obj.prop); // "baz"
```
@jamiebuilds
Copy link
Author

Also modified ForInStatement because it accepts an Expression:

var obj = { prop: 'foo' };
for (obj.prop in { bar: 1, baz: 2 }) {}
console.log(obj.prop); // "baz"

@RReverser
Copy link
Member

Declaration case is special, ESTree's type system just doesn't cover all the possible errors, need more types to express relationships correctly. In other places Pattern is accepted, MemberExpression can be used as well (ForInStatement, ForOfStatement, AssignmentExpression, sub-pattern of ObjectPattern, ArrayPattern, RestElement, AssignmentPattern etc.).

@jamiebuilds
Copy link
Author

Err, well actually it accepts only valid LHS assignable expressions. But there is no such node type, and AssignmentExpression accepts an Expression as well.

@RReverser
Copy link
Member

But there is no such node type

That's exactly what we express with Pattern (assignable patterns). It's declaration patterns that would need to be somehow specially subtyped from each regular pattern type (ideally in a way that wouldn't bloat entire spec with very similar types).

@jamiebuilds
Copy link
Author

Oh my, well I guess I need to diverge further in Babylon...

@jamiebuilds jamiebuilds deleted the patch-1 branch March 31, 2017 00:51
@RReverser
Copy link
Member

shrug This isn't a precise type system, rather a representational spec.

@michaelficarra
Copy link
Member

@thejameskyle You probably want something like Shift.

@mikesherov
Copy link
Contributor

@thejameskyle, that's certainly your prerogative as to how much Babylon's AST differs from ESTree. As @michaelficarra alludes to, Shift is a much safer format in terms of representing valid trees.

My hope would be that we'd all accept the deficiencies in ESTree for the sake of max interop. I still remember your talk at the last jQuery conference about being able to pass ASTs around, and would love to keep that notion alive without having to do Babylon -> ESTree conversions first. But of course, that's not up to me! That's a tradeoff for you and the rest of the Babylon maintainers to consider.

@mikesherov
Copy link
Contributor

@thejameskyle but I know y'all just recently added the ability to output to ESTree anyway, so shrug, do what ya gotta!

@jamiebuilds
Copy link
Author

My views on interop have changed since that talk. ESTree is an outdated specification and the community should drop it in favor of a better Babylon AST or the Shift AST for a better path forward for all tools. Although I'd be more interested in Shift if it adopted broadly community accepted language extensions (JSX/Flow/TS).

ESTree interop should be treated as legacy system support.

@michaelficarra
Copy link
Member

I have given similar talks encouraging people to adopt estree (then SpiderMonkey AST) despite its numerous flaws. And I have also since changed my opinion on that matter due to the crippling nature of some of the flaws as well as the huge investment of time/effort already spent on an alternative (Shift).

@mikesherov
Copy link
Contributor

Also reasonable to move away. We could work towards documenting that too, or not! ESTree isn't a spec as much as it's documenting what's there in the community.

Maybe it is time for an ESTree 2, or whatever it gets called.

I am of the opinion that we could fix lots of stuff without going full Shift, and therefore making minimal breaking changes. Babylon is proof.

Of course, that would require folks to be interested in that endeavor, at least two out of the 3 of Babylon, Acorn, and Esprima.

@thejameskyle, thoughts? I know @nzakas and others have argued for an ESTree 2 in the past.

Or we could also just say fuck it and everyone lives their own lives too. But I'm interested in interop either way.

@jamiebuilds
Copy link
Author

My interest with Babylon right now is to incrementally migrate it towards the Shift spec. I could see it being an extension in the future. I've actually begun documenting the full Babylon AST (with some minor consistency updates and changes to Flow types since there was no standard previously).

I think it's a mistake to move in any direction other than a more provably correct AST. I would prefer adopting the specific changes Shift has already made into Babylon and ESTree (unless of course there is something that is clearly better than what Shift has in which case I would encourage Shift to change too).

I will say that I don't find interop a very convincing argument to avoid moving Babylon (or any other AST) in the direction that is independently better.

@RReverser
Copy link
Member

RReverser commented Mar 31, 2017

@thejameskyle This starts looking confusing as @hzoo who is also actively maintaining Babylon, said that he wants to make a move towards ESTree. Although this is probably best left for your own internal discussions on Babylon repo and not on this issue.

@jamiebuilds
Copy link
Author

I think that would be a pretty big mistake for the project, we'll be discussing it more though.

@mikesherov
Copy link
Contributor

Ok cool. So sounds like there's no interest in having the suite of parsers move the community towards Shift from your end. I'll drop it then!

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.

None yet

4 participants