I guess I’m just saying, I’m a bit stuck on how to do this without slowing things down a ton, while increasing the complexity of the parser to great lengths.
There are 2 problems with the current solution: 1. sometimes, something may be a BindingPattern and Initializer, and we can’t rewrite in that case, because the PatternRewriter won’t be able to do its job if the pattern is rewritten as an AssignmentPattern before the PatternRewriter is used 2. sometimes, something may be a BindingPattern, but in fact isn’t, so the PatternRewriter is never used, and you end up with an illegal LHS (so, a TypeError). Better than a crash, but still very incorrect. The first problem can occur with any ObjectLiteralProperty or ArrayLiteral element, and the second problem can occur when parsing parenthesized expressions that may be ArrowFormals. We could rewrite things that occur within a parenthesized expression if the `=>` is not found, but this doesn’t help the ObjectLiteral/ArrayLiteral property/elements issue. I’m not sure how to solve this “well” without hurting perf even more, and without making the Parser much more difficult to reason about. I’ve thought of a couple different approaches: - Extra AST node which contains an Object/ArrayLiteral, and denotes that it needs to be rewritten as a destructuring assignment. This works for compat with BindingPatterns, but doesn’t help when the expression isn’t actually part of a BindingPattern. - Track the number of *DestructuringAssignment nodes in the translation unit (eg, a Function or Script), and walk the entire AST rewriting all of them as DestructuringAssignments lazily once parsing has finished. This potentially degrades performance more than it should The first solution is noted as not solving the whole problem, although it would be incorporated into the 2nd solution most likely. The second solution is sort of terrible. Other opinions on the approach taken would be helpful, and whatever it is could be refactored around something similar later on. > On Nov 10, 2015, at 1:43 PM, Adam Klein <ad...@chromium.org> wrote: > > I think such a rearchitecture is something we definitely should consider. But > I'm not sure we should block destructuring assignment on that project. I'm > going to take a deeper look at this later this week (after BlinkON) and see > what might be possible within the current architecture. > > On Mon, Nov 9, 2015 at 2:18 PM, Daniel Vogelheim <vogelh...@google.com > <mailto:vogelh...@google.com>> wrote: > I think Adam has brought this up before. Scanner::BookmarkScope sort of > implements that kind of logic, but the current implementation isn't suitable > for your use case because it's opportunistic and will just fail to set a > bookmark if the conditions are not quite right. > > I don't think there is any super deep reason why this couldn't be > generalized, but it'd be a good amount of work. The bookmark logic would have > to be implemented for all stream classes (I only implemented it for some) and > it would probably need to support several bookmarks (right now, there can > only be one). It's finicky work: My implementation had several severe bugs, > because I didn't properly handle some edge cases. > > Performance: There were several performance regressions, but I think I > resolved those. There's a cost to setting a bookmark, so there would be > performance issues if the number of bookmarks set would be rather large. > Those should be fixable, too, but that would probably require significant > rework of the current streams. > > Daniel > > > On Mon, Nov 9, 2015 at 7:25 AM, Caitlin Potter <caitpotte...@gmail.com > <mailto:caitpotte...@gmail.com>> wrote: > This has been suggested previously, but shot down on the grounds of > performance (and difficulties with the way the Parser allocates variables, > etc). However, I'm bringing it up again, as it seems to be working well for > other implementations: > > Currently, JavaScriptCore has the novel concept of saving a position in the > token stream, and rewinding back to it if necessary. > > They are currently doing this at the start of every AssignmentExpression, > which begins with a peeked LBRACK or LBRACE, and which is not followed by an > ASSIGN token. It would follow that you'd also want to do this for > parenthesized Expressions that may be Arrow formals, although I don't see > this happening in their current implementation. > > It's been suggested that V8 does this before, but has been shot down each > time --- however, it looks like it potentially solves two problems: > > 1. the cover grammars may be parsed specially, rather than requiring > particular productions handle parsing several different ways simultaneously, > and drastically simplifying the parser > 2. it is possible to correctly ensure that appropriate code is produced. > Since the Parser is performing the desugaring (which does not occur in JSC), > eagerly rewriting AssignmentExpressions as destructuring assignment can > produce an AST which the BindingPattern rewriter simply can't deal with, and > which would be difficult for it to deal with. > > Might be worth coming back to the topic if it works well for other > implementations, which still seem to perfectly adequately compile JS on the > web. > > -- > -- > v8-dev mailing list > v8-dev@googlegroups.com <mailto:v8-dev@googlegroups.com> > http://groups.google.com/group/v8-dev <http://groups.google.com/group/v8-dev> > --- > You received this message because you are subscribed to the Google Groups > "v8-dev" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to v8-dev+unsubscr...@googlegroups.com > <mailto:v8-dev+unsubscr...@googlegroups.com>. > For more options, visit https://groups.google.com/d/optout > <https://groups.google.com/d/optout>. > > -- -- v8-dev mailing list v8-dev@googlegroups.com http://groups.google.com/group/v8-dev --- You received this message because you are subscribed to the Google Groups "v8-dev" group. To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+unsubscr...@googlegroups.com. For more options, visit https://groups.google.com/d/optout.
signature.asc
Description: Message signed with OpenPGP using GPGMail
