Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1567603005:
Set up rewriting triggers (Closed)
DescriptionSet up rewriting triggers
This patch implements eager expression rewriting when parsing. It will
be used for desugaring spreads but may have other uses in the future.
We call Traits::RewriteExpression as soon as we realise that something
parsed as an expression is actually used as an expression (and not as
a pattern). This patch adds a dummy implementation for this function,
doing no rewriting at all, and adds the trigers in the right places of
the parser.
R=rossberg@chromium.org
BUG=
Committed: https://crrev.com/2b90397d670a2dcbd280aca4c93181dd0480a36a
Cr-Commit-Position: refs/heads/master@{#33300}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Rebase #Patch Set 3 : Merge RewriteExpression and ValidateExpression #
Total comments: 19
Patch Set 4 : Clean up and rename to RewriteNonPattern #Patch Set 5 : Add some more forgotten triggers #Patch Set 6 : Rebase #
Messages
Total messages: 37 (10 generated)
https://codereview.chromium.org/1567603005/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:1598: expression = Traits::RewriteExpression(expression); Do we actually need to do this here? My thinking was that we should only have to rewrite in places where we resolve an ambiguous parse (i.e., all places that do ValidateExpression). In the case here (and most others below), there was no ambiguity, so we can assume that the expression is already in the right shape. If we invoke this everywhere, wouldn't we end up traversing expressions exponentially in the worst case? (Maybe you don't even need a separate RewriteExpression trait function, but can simply trigger the rewrite in ValidateExpression?)
https://codereview.chromium.org/1567603005/diff/1/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/1/src/parsing/parser-base.h#n... src/parsing/parser-base.h:1598: expression = Traits::RewriteExpression(expression); On 2016/01/08 12:22:27, rossberg wrote: > Do we actually need to do this here? My thinking was that we should only have to > rewrite in places where we resolve an ambiguous parse (i.e., all places that do > ValidateExpression). In the case here (and most others below), there was no > ambiguity, so we can assume that the expression is already in the right shape. > If we invoke this everywhere, wouldn't we end up traversing expressions > exponentially in the worst case? You can look at this as a "non-pattern rewriter". For spread rewriting, it only traverses pattern-looking things that are not patterns. The idea is that such rewriters should only do local work and not go arbitrarily deep in the tree. The general invariant is that a rewriter for an expression tree should avoid visiting subtrees that have already been rewritten as expressions (unless of course it is required to rewrite them anew). I thought that this should be the rewriters' responsibility. If this invariant is not obeyed, even if you restrict this to the places where ValidateExpression is called, you'll still have too many unnecessary traversals. If we really want to combine this with ValidateExpression, then the spread rewriter would need to go deeper in the tree. I'm not sure if in this way it would be possible to avoid traversing some subtrees twice (which now is indeed avoided), without keeping track of what has been rewritten and what not. > (Maybe you don't even need a separate RewriteExpression trait function, but can > simply trigger the rewrite in ValidateExpression?) Yes, depending on the verdict of the previous question. An alternative way to combine this with ValidateExpression would be to do it the other way round. Would it make sense to call ValidateExpression in all places where RewriteExpression is called?
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1040: #if 0 // TODO(nikolaos): is this needed? Yes, this is needed -- ParseAndClassifyIdentifier may set expression errors in some cases (e.g. using `arguments` in strong mode). https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? Isn't this needed to catch syntax errors like "(());"? Why is Rewrite not needed here? https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:2056: #if 0 // TODO(nikolaos): is this needed? Hm, why is it not needed? Don't you even have to do RewriteExpression here? Can't this arise as "x = ()", for example? https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser.h#ne... src/parsing/parser.h:654: // Rewrite expressions Nit: can you clarify in the comment that this is used to resolve expressions that were parsed using the cover grammar?
caitpotter88@gmail.com changed reviewers: + caitpotter88@gmail.com
https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 13:47:52, rossberg wrote: > Isn't this needed to catch syntax errors like "(());"? Why is Rewrite not needed > here? This is needed to catch things like CoverInitializedNames in things that didn't turn out to be Arrow formal parameters
On 2016/01/12 13:50:39, caitp wrote: > https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h > File src/parsing/parser-base.h (right): > > https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... > src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? > On 2016/01/12 13:47:52, rossberg wrote: > > Isn't this needed to catch syntax errors like "(());"? Why is Rewrite not > needed > > here? > > This is needed to catch things like CoverInitializedNames in things that didn't > turn out to be Arrow formal parameters An example that AFAIK should do the wrong thing with this line commented out like this is `x = ({ y = 10 })`, which per spec is a syntax error
On 2016/01/12 13:54:33, caitp wrote: > On 2016/01/12 13:50:39, caitp wrote: > > > https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h > > File src/parsing/parser-base.h (right): > > > > > https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... > > src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? > > On 2016/01/12 13:47:52, rossberg wrote: > > > Isn't this needed to catch syntax errors like "(());"? Why is Rewrite not > > needed > > > here? > > > > This is needed to catch things like CoverInitializedNames in things that > didn't > > turn out to be Arrow formal parameters > > An example that AFAIK should do the wrong thing with this line commented out > like this is `x = ({ y = 10 })`, which per spec is a syntax error Looks like one of the other changes does make it unnecessary, though.
https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1040: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 13:47:52, rossberg wrote: > Yes, this is needed -- ParseAndClassifyIdentifier may set expression errors in > some cases (e.g. using `arguments` in strong mode). Acknowledged. I'm adding a test for this. https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 13:50:39, caitp wrote: > On 2016/01/12 13:47:52, rossberg wrote: > > Isn't this needed to catch syntax errors like "(());"? Why is Rewrite not > needed > > here? > > This is needed to catch things like CoverInitializedNames in things that didn't > turn out to be Arrow formal parameters Acknowledged. I'm adding a test for this. https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:2056: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 13:47:52, rossberg wrote: > Hm, why is it not needed? Don't you even have to do RewriteExpression here? > Can't this arise as "x = ()", for example? I have a RewriteExpression at the place where the RHS of the assignment is parsed, so now it will be validated there. Probably the rhs flag is not needed any more, @caitp? https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser.h File src/parsing/parser.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser.h#ne... src/parsing/parser.h:654: // Rewrite expressions On 2016/01/12 13:47:52, rossberg wrote: > Nit: can you clarify in the comment that this is used to resolve expressions > that were parsed using the cover grammar? We agreed to rename this as RewriteNonPattern which is self-explanatory.
https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:2056: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 14:12:32, nickie wrote: > On 2016/01/12 13:47:52, rossberg wrote: > > Hm, why is it not needed? Don't you even have to do RewriteExpression here? > > Can't this arise as "x = ()", for example? > > I have a RewriteExpression at the place where the RHS of the assignment is > parsed, so now it will be validated there. Probably the rhs flag is not needed > any more, @caitp? It looks like you can remove this block, and the flag, but it's possible that I've missed something
On 2016/01/12 14:25:55, caitp wrote: > https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h > File src/parsing/parser-base.h (right): > > https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... > src/parsing/parser-base.h:2056: #if 0 // TODO(nikolaos): is this needed? > On 2016/01/12 14:12:32, nickie wrote: > > On 2016/01/12 13:47:52, rossberg wrote: > > > Hm, why is it not needed? Don't you even have to do RewriteExpression here? > > > Can't this arise as "x = ()", for example? > > > > I have a RewriteExpression at the place where the RHS of the assignment is > > parsed, so now it will be validated there. Probably the rhs flag is not > needed > > any more, @caitp? > > It looks like you can remove this block, and the flag, but it's possible that > I've missed something (non-owner) lgtm with the commented out/unneeded bits removed, it seems to do okay
https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1040: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 14:12:32, nickie wrote: > Acknowledged. I'm adding a test for this. Cannot find a test for this. This is the "else" case, when the use of restricted identifiers is allowed. Is there something to validate in this case? https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 14:12:32, nickie wrote: > Acknowledged. I'm adding a test for this. Cannot find a test for this either. It seems to me that errors like "(())" will be caught when this whole thing is rewritten and validated. This may be at the place where somebody called ParsePrimaryExpression, or indeed at the place where somebody called a chain that is a suffix of: ParseExpression ParseAssignmentExpression ParseConditionalExpression ParseBinaryExpression ParseUnaryExpression ParsePostfixExpression ParseLeftHandSideExpression ParseMemberExpressionWithNewPrefixesExpression ParseMemberExpression ParsePrimaryExpression https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:2056: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 14:25:55, caitp wrote: > On 2016/01/12 14:12:32, nickie wrote: > > On 2016/01/12 13:47:52, rossberg wrote: > > > Hm, why is it not needed? Don't you even have to do RewriteExpression here? > > > Can't this arise as "x = ()", for example? > > > > I have a RewriteExpression at the place where the RHS of the assignment is > > parsed, so now it will be validated there. Probably the rhs flag is not > needed > > any more, @caitp? > > It looks like you can remove this block, and the flag, but it's possible that > I've missed something Acknowledged.
https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? On 2016/01/12 13:50:39, caitp wrote: > On 2016/01/12 13:47:52, rossberg wrote: > > Isn't this needed to catch syntax errors like "(());"? Why is Rewrite not > needed here? > > This is needed to catch things like CoverInitializedNames in things that didn't > turn out to be Arrow formal parameters After Caitlin's patch for parenthesized patterns, I believe this is not needed anymore. It doesn't hurt much to have a validate-and-rewrite here, but notice that this expression will certainly be (validated and) rewritten again, so we're sort of violating our invariant that this non-pattern rewriting phase should be kept as "shallow" as possible.
LGTM, although I still feel somewhat uncomfortable about the slightly fuzzy rewriting points for parenthesised expressions. https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1040: #if 0 // TODO(nikolaos): is this needed? Yes, I don't understand why this is the else case either. Shouldn't it occur in the then case? https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? Hm, I wonder if it isn't a broader shortcoming that parenthesised get rewritten again (also in other cases). Shouldn't RewriteNonPattern stop when it hits an expression with the is_parenthesised flag?
https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1040: #if 0 // TODO(nikolaos): is this needed? On 2016/01/14 13:28:40, rossberg wrote: > Yes, I don't understand why this is the else case either. Shouldn't it occur in > the then case? In the "then" case there is some validation. But this thing is just an identifier and not yet found to be a non-pattern, so no rewriting must take place there (and I believe that calling ValidateExpression would be wrong). On top of everything, we're talking about rewriting an identifier which will probably never be non-trivial... https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? On 2016/01/14 13:28:40, rossberg wrote: > Hm, I wonder if it isn't a broader shortcoming that parenthesised get rewritten > again (also in other cases). Shouldn't RewriteNonPattern stop when it hits an > expression with the is_parenthesised flag? There's two approaches, as I understand it. Either we remove the validation from here and change nothing else, or we add the validation here and we have RewriteNonPattern stop when it hits expressions with the is_parenthesised flag set. I have taken the first approach, see here: https://codereview.chromium.org/1567603005/diff/100001/src/parsing/parser-bas... but if you prefer the second, I can change it.
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caitpotter88@gmail.com, rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1567603005/#ps120001 (title: "Add some more forgotten triggers")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567603005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567603005/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_avx2_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel/builds/...) v8_linux64_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_rel_ng/builds/46) v8_linux_arm_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel/builds/12386) v8_linux_dbg_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/45) v8_linux_mips64el_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mips64el_compile_r...) v8_linux_mipsel_compile_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_mipsel_compile_rel...) v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...) v8_presubmit on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/9762)
https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base.h File src/parsing/parser-base.h (right): https://codereview.chromium.org/1567603005/diff/80001/src/parsing/parser-base... src/parsing/parser-base.h:1359: #if 0 // TODO(nikolaos): is this needed? On 2016/01/14 14:47:14, nickie wrote: > On 2016/01/14 13:28:40, rossberg wrote: > > Hm, I wonder if it isn't a broader shortcoming that parenthesised get > rewritten > > again (also in other cases). Shouldn't RewriteNonPattern stop when it hits an > > expression with the is_parenthesised flag? > > There's two approaches, as I understand it. Either we remove the validation > from here and change nothing else, or we add the validation here and we have > RewriteNonPattern stop when it hits expressions with the is_parenthesised flag > set. I have taken the first approach, see here: > > https://codereview.chromium.org/1567603005/diff/100001/src/parsing/parser-bas... > > but if you prefer the second, I can change it. Just a note about why the ValidateExpression was originally added, it was because the ParseExpression method was changed from one that did its own ValidateExpression(), to one that doesn't (because arrow formals needed to be validated differently). For whatever reason, it seems that the correct validation is happening elsewhere. So, so long as you can't write `({ x = 10 });` or `({ x: { y = 10 } });` without a SyntaxError, it should be fine to just remove the validation. This is unrelated to the parenthesized flag, it's all making sure ExpressionErrors are only emitted in an Expression context, and at this point it's ambiguous whether it's an Expression context or not, until the `=>` is seen or not seen
The CQ bit was checked by nikolaos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from caitpotter88@gmail.com, rossberg@chromium.org Link to the patchset: https://codereview.chromium.org/1567603005/#ps140001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1567603005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1567603005/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Set up rewriting triggers This patch implements eager expression rewriting when parsing. It will be used for desugaring spreads but may have other uses in the future. We call Traits::RewriteExpression as soon as we realise that something parsed as an expression is actually used as an expression (and not as a pattern). This patch adds a dummy implementation for this function, doing no rewriting at all, and adds the trigers in the right places of the parser. R=rossberg@chromium.org BUG= ========== to ========== Set up rewriting triggers This patch implements eager expression rewriting when parsing. It will be used for desugaring spreads but may have other uses in the future. We call Traits::RewriteExpression as soon as we realise that something parsed as an expression is actually used as an expression (and not as a pattern). This patch adds a dummy implementation for this function, doing no rewriting at all, and adds the trigers in the right places of the parser. R=rossberg@chromium.org BUG= Committed: https://crrev.com/2b90397d670a2dcbd280aca4c93181dd0480a36a Cr-Commit-Position: refs/heads/master@{#33300} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/2b90397d670a2dcbd280aca4c93181dd0480a36a Cr-Commit-Position: refs/heads/master@{#33300}
Message was sent while issue was closed.
adamk@chromium.org changed reviewers: + adamk@chromium.org
Message was sent while issue was closed.
Just noticed this patch as I was reviewing something else. This seems like a pretty big change to the design of the parser. Is there a design doc somewhere for this approach? It looks like we're now walking over every expression during parsing? Are we not worried about performance issues?
Message was sent while issue was closed.
On 2016/01/19 22:33:39, adamk wrote: > Just noticed this patch as I was reviewing something else. This seems like a > pretty big change to the design of the parser. Is there a design doc somewhere > for this approach? It looks like we're now walking over every expression during > parsing? Are we not worried about performance issues? I think this came off slightly harsher than I intended. But my main issue is that this seems like it significantly changes the programming model for the parser, and I'd like to understand the intended current (and future) model.
Message was sent while issue was closed.
On 2016/01/19 23:18:53, adamk wrote: > On 2016/01/19 22:33:39, adamk wrote: > > Just noticed this patch as I was reviewing something else. This seems like a > > pretty big change to the design of the parser. Is there a design doc somewhere > > for this approach? It looks like we're now walking over every expression > > during parsing? Are we not worried about performance issues? > > I think this came off slightly harsher than I intended. But my main issue is > that this seems like it significantly changes the programming model for the > parser, and I'd like to understand the intended current (and future) model. In principle, you're right. In practice, however, we talked about this with Andreas and, as long as the traversal performed by RewriteNonPattern is "shallow" (in the sense that [a] it does not go deeper than the pattern grammar, and [b] it makes sure not traverse the same subtree twice), performance should not be an issue. This is what the NonPatternRewriter does now, and it should be respected as an invariant in the future, in case other rewritings are found to be useful. Do we have evidence of performance deterioration, so far? An alternative implementation would involve queueing possible rewritable non-patterns (in the same way that Caitlin queues rewritable assignment expressions, in order to rewrite their patterns) and traverse the queue instead of traversing the AST. My impression was that we preferred the "eager" traversal. I'm happy to rewrite this, if we decide otherwise, it shouldn't be too hard.
Message was sent while issue was closed.
On 2016/01/20 08:36:17, nickie wrote: > On 2016/01/19 23:18:53, adamk wrote: > > On 2016/01/19 22:33:39, adamk wrote: > > > Just noticed this patch as I was reviewing something else. This seems like a > > > pretty big change to the design of the parser. Is there a design doc > somewhere > > > for this approach? It looks like we're now walking over every expression > > > during parsing? Are we not worried about performance issues? > > > > I think this came off slightly harsher than I intended. But my main issue is > > that this seems like it significantly changes the programming model for the > > parser, and I'd like to understand the intended current (and future) model. > > In principle, you're right. In practice, however, we talked about this with > Andreas and, as long as the traversal performed by RewriteNonPattern is > "shallow" (in the sense that [a] it does not go deeper than the pattern grammar, > and [b] it makes sure not traverse the same subtree twice), performance should > not be an issue. This is what the NonPatternRewriter does now, and it should be > respected as an invariant in the future, in case other rewritings are found to > be useful. Do we have evidence of performance deterioration, so far? > > An alternative implementation would involve queueing possible rewritable > non-patterns (in the same way that Caitlin queues rewritable assignment > expressions, in order to rewrite their patterns) and traverse the queue instead > of traversing the AST. My impression was that we preferred the "eager" > traversal. I'm happy to rewrite this, if we decide otherwise, it shouldn't be > too hard. I don't have evidence of performance degradation, no, and thanks for explaining the "shallow" design. When I mentioned the "programming model" one thing I meant was that the idiom went from being a call to ValidateExpression() to RewriteNonPattern() without any announcement or explanation. And in fact ValidateExpression() is still called directly in preparser.cc, which is rather confusing when comparing parser code to the preparser. At a higher level, I think my question here is: is there a concise explanation somewhere (besides the code) of how this new machinery works, how it should be used, and what the plan is for moving more code into the rewriter?
Message was sent while issue was closed.
On 2016/01/20 19:12:27, adamk wrote: > When I mentioned the "programming model" one thing I meant was that the idiom > went from being a call to ValidateExpression() to RewriteNonPattern() without > any announcement or explanation. And in fact ValidateExpression() is still > called directly in preparser.cc, which is rather confusing when comparing parser > code to the preparser. Yes, Andreas and I have also discussed about this. The preparser could call RewriteNonPattern, for symmetry, which only calls ValidateExpression in its case. For some reason, we turned this down. > At a higher level, I think my question here is: is there a concise explanation > somewhere (besides the code) of how this new machinery works, how it should be > used, and what the plan is for moving more code into the rewriter? There's not, yet. I can write something up about the changes I implemented. I'm not sure about the general plan, though.
Message was sent while issue was closed.
@adamk, there isn't more of a design doc for this than for any other implementation change we ever made to the system. :) But I agree that it would be good to add documentation in the form of suitable code comments somewhere that e.g. state the ambiguous intersection subgrammar, and note that both RewritePattern and RewriteNonPattern get executed at the point where they are disambiguated. Also, that these rewriters only traverses the intersection grammar, so nothing is traversed twice. The advantage of this approach is that the rewrites are in sync with validates (and Niko already discovered several validation bugs that way). I don't have strong feelings about adapting the preparser, though it would feel a bit weird if it explicitly called something called Rewrite when it doesn't even have an AST.
Message was sent while issue was closed.
On 2016/01/21 12:05:27, rossberg wrote: > @adamk, there isn't more of a design doc for this than for any other > implementation change we ever made to the system. :) Sure, but this touched 10s of callsites throughout the parser, and presumably affects future desugaring plans (say, of ObjectLiterals). And to be fair I would've been happier if, say, ExpressionClassifier had an accompanying doc too...I just think this is an area we could improve. > But I agree that it would be good to add documentation in the form of suitable > code comments somewhere that e.g. state the ambiguous intersection subgrammar, > and note that both RewritePattern and RewriteNonPattern get executed at the > point where they are disambiguated. Also, that these rewriters only traverses > the intersection grammar, so nothing is traversed twice. > > The advantage of this approach is that the rewrites are in sync with validates > (and Niko already discovered several validation bugs that way). I don't have > strong feelings about adapting the preparser, though it would feel a bit weird > if it explicitly called something called Rewrite when it doesn't even have an > AST. Well, the ParserBase parts already call "rewrite" on PreParserExpressions, so I'm not sure it's much weirder. I think there's a danger in leaving calls to ValidateExpression() sitting around that someone might accidentally call that instead of RewriteNonPattern and silently fail to handle the appropriate rewriting, causing bad behavior or crashes downstream. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
