On Tue, Nov 10, 2015 at 11:41 AM, Caitlin Potter <[email protected]> wrote:
> 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 > Could we do something similar to what Dan did for sloppy block-scoped functions and keep a list of the needing-rewrite nodes in the parser? That'd at least avoid a full program tree walk to find them. See https://codereview.chromium.org/1332873003 for that example. > 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 <[email protected]> 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 <[email protected]> > 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 <[email protected]> >> 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 >>> [email protected] >>> 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 [email protected]. >>> For more options, visit https://groups.google.com/d/optout. >>> >> >> > > -- -- v8-dev mailing list [email protected] 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 [email protected]. For more options, visit https://groups.google.com/d/optout.
