Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2676083002:
Connect fasta's scanner to the analyzer-derived scanner tests in front_end. (Closed)
|
Created:
3 years, 10 months ago by Paul Berry Modified:
3 years, 10 months ago CC:
reviews_dartlang.org, dart-fe-team+reviews_google.com Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionConnect fasta's scanner to the analyzer-derived scanner tests in front_end.
Failing tests are currently marked with "@failingTest" to prevent
breaking buildbots.
I've marked the issues I'm aware of with "TODO(paulberry,ahe)". Peter
and I need to triage these issues--some of them may not be necessary
to fix due to the fact that fasta replaces the parser as well. We
will follow up with CLs that either fix the issues or remove the TODO
comments as appropriate.
R=ahe@google.com
Committed: https://github.com/dart-lang/sdk/commit/04f19f6bfb458daebbd7e4015ca1c53bc6b3e43e
Patch Set 1 #
Total comments: 19
Messages
Total messages: 11 (3 generated)
paulberry@google.com changed reviewers: + ahe@google.com
https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... File pkg/front_end/test/scanner_test.dart (right): https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:23: defineReflectiveTests(ScannerTest); Note that this test file now tests both the analyzer and fasta implementations of the scanner. If you want to exercise a single test in the debugger, an easy way to do so is to temporarily change the name of that test from "test_foo" to "solo_test_foo" and temporarily comment out this line, so that only the fasta version is tested. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:191: @failingTest Note: these overrides indicate to the test_reflective_loader infrastructure that the test is expected to fail. Once the failure is fixed, the overrides can be removed. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:788: void test_comment_multi_consecutive_2() { Note: during this process I discovered that consecutive comments weren't adequately tested, so I added these tests.
Exciting stuff! I've added a few notes, they are not really comments about the code, just information. I "accidentally" opened my email before getting breakfast on a Saturday morning, so I think I'll just get back to that and wait until Monday for a full review. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... File pkg/front_end/test/scanner_test.dart (right): https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:249: // TODO(paulberry,ahe): "[]" should be parsed as a single token. Note: [] and []= are tokenized as a single token only after the built-in identifier "operator". See: https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:447: // ScannerErrorCode.UNTERMINATED_MULTI_LINE_COMMENT Absolutely. Just this week, I was thinking about cleaning this up. It's already a little bit better as I added ErrorToken.errorCode, and the API should probably be good enough for testing, but I do want to simplify UnterminatedToken.errorCode. See: https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... Another thought: perhaps I should move errorCode to Token. This way we don't have to rely on type tests of tokens.
No need to respond to this before Monday--I just wanted to put my thoughts down ASAP to reduce the number of round trip delays in this cross-Atlantic code review :) https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... File pkg/front_end/test/scanner_test.dart (right): https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:249: // TODO(paulberry,ahe): "[]" should be parsed as a single token. On 2017/02/04 09:48:19, ahe wrote: > Note: [] and []= are tokenized as a single token only after the built-in > identifier "operator". > > See: > > https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... Interesting. The behavior of the analyzer scanner is to lex "[]" and "[]=" as single tokens wherever they appear. On the downside, this complicates the analyzer parser slightly, since it means that it must recognize both `[]` and `[` `]` as empty list expressions. On the upside, it makes it easier for the analyzer parser to recover from errors like this: class Foo { int [](int index) => 0; } (Analyzer reports the error "Operator declarations must be preceeded by the keyword 'operator'."). If we wanted to get this error recovery behavior with fasta's lexer, there would be more parser work. On the other hand, maybe that's ok, because the extra parser work would be all along an error handling code path anyway. And admittedly, this particular error recovery corner case is probably extremely rare in practice. In my mind this issue is simple enough that it's probably not worth discussing, but since it's the first one that came up, let's use it as a practice run and see how the discussion goes. Once we get into a groove we shouldn't need to talk through every single detail. I can see a few possible ways to resolve this: (1) Change analyzer's parser and scanner to be like Fasta's, and implement extra logic in analyzer's parser to keep the error recovery behavior. This would carry little benefit (it would make the unit test behavior line up, but not change the user experience other than a possible tiny performance improvement). And it would all be wasted effort assuming we switch to Fasta in the long term. (2) Change analyzer's parser and scanner to be like Fasta's, and don't worry about the error recovery behavior. This is also wasted effort, though probably less effort than (1). (3) Change fasta's parser and scanner to be like analyzer's. This gets us the nice error recovery behavior, at the expense of some coding effort and a possible performance drop (though my intuition is that it would unmeasurably small). This is probably not worth it either, given how rare the error recovery corner case is. (4) Keep fasta's scanner as is, and implement logic in its parser to handle the error recovery corner case. Again, probably not worth it, given how rare the error recovery corner case. (5) Change nothing. Fasta code stays simple and efficent, and the only user-visible effect is that this particular error recovery case stops being addressed. I personally lean toward (5). In which case I think the right thing to do is (a) keep the "@failingTest" override, but change the comment to explain in a sentence or two why we're ok with Fasta failing this test. (b) add a test (or tests) validating Fasta's behavior, along with a "@failingTest" override for analyzer's scanner. (c) after we've transitioned to Fasta and removed analyzer's scanner, we can remove the failing test if we choose to. I don't care which one of us does (a) and (b)--it should only take a few minutes. As a point of process: since it's probably going to take some time to resolve all the TODOs in this CL, I'd like to land it as is first (assuming that you don't come up with blocking issues in your review) and then address the TODOs with follow-up CLs. So I'll postpone doing (a) and (b) until after this CL lands. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:447: // ScannerErrorCode.UNTERMINATED_MULTI_LINE_COMMENT On 2017/02/04 09:48:19, ahe wrote: > Absolutely. Just this week, I was thinking about cleaning this up. > > It's already a little bit better as I added ErrorToken.errorCode, and the API > should probably be good enough for testing, but I do want to simplify > UnterminatedToken.errorCode. > > See: > https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... Aha, thank you. I failed to notice the presence of ErrorToken.errorCode. I'll rework this code to use it. > > Another thought: perhaps I should move errorCode to Token. This way we don't > have to rely on type tests of tokens. Interesting. We would still have to do a single megamorphic dispatch for token.info, but thereafter we would avoid "is" checks and implicit downcasts, and we could probably make everything else monomorphic. I'm a little wary of making too many changes to Fasta right now, since it's not hooked up to golem or to analyzer tests yet, but I suppose moving errorCode to Token is pretty safe as changes go.
First of all: LGTM! Generally, I expect replies to code reviews within a business day or two for the reviewer. I believe it's important to respect days off, for example, weekends. I generally strive to not read emails during the weekend, but I sometimes fail So if I send you something during the weekends or a holiday, it's just because I was bored or particularly excited about something. Don't feel you have to respond until you're back at work. If I have something that's urgent, I'll let you know, but in that case, I'll often try to find a local reviewer and address comments from other timezones in a follow-up CL. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... File pkg/front_end/test/scanner_test.dart (right): https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:207: // TODO(paulberry,ahe): Fasta doesn't support generic method comment syntax. Do you think that would be necessary to support in Fasta? https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:221: // TODO(paulberry,ahe): see UnimplementedError("distinguish unterminated errors") Long line here and a few below. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:249: // TODO(paulberry,ahe): "[]" should be parsed as a single token. On 2017/02/04 14:57:55, Paul Berry wrote: > On 2017/02/04 09:48:19, ahe wrote: > > Note: [] and []= are tokenized as a single token only after the built-in > > identifier "operator". > > > > See: > > > > > https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... > > Interesting. The behavior of the analyzer scanner is to lex "[]" and "[]=" as > single tokens wherever they appear. On the downside, this complicates the > analyzer parser slightly, since it means that it must recognize both `[]` and > `[` `]` as empty list expressions. On the upside, it makes it easier for the > analyzer parser to recover from errors like this: > > class Foo { > int [](int index) => 0; > } > > (Analyzer reports the error "Operator declarations must be preceeded by the > keyword 'operator'."). If we wanted to get this error recovery behavior with > fasta's lexer, there would be more parser work. On the other hand, maybe that's > ok, because the extra parser work would be all along an error handling code path > anyway. And admittedly, this particular error recovery corner case is probably > extremely rare in practice. I agree. A corner case with the dart2js/fasta approach is that if you write something like this: main() { var operator = ["Hello, World!"]; print(operator[]); // Forgot 0 here. } The fasta/dart2js parser won't recover well. My gut feeling is that the analyzer's approach is superior here. > In my mind this issue is simple enough that it's probably not worth discussing, > but since it's the first one that came up, let's use it as a practice run and > see how the discussion goes. Once we get into a groove we shouldn't need to > talk through every single detail. > > I can see a few possible ways to resolve this: > > (1) Change analyzer's parser and scanner to be like Fasta's, and implement extra > logic in analyzer's parser to keep the error recovery behavior. This would > carry little benefit (it would make the unit test behavior line up, but not > change the user experience other than a possible tiny performance improvement). > And it would all be wasted effort assuming we switch to Fasta in the long term. > > (2) Change analyzer's parser and scanner to be like Fasta's, and don't worry > about the error recovery behavior. This is also wasted effort, though probably > less effort than (1). > > (3) Change fasta's parser and scanner to be like analyzer's. This gets us the > nice error recovery behavior, at the expense of some coding effort and a > possible performance drop (though my intuition is that it would unmeasurably > small). This is probably not worth it either, given how rare the error recovery > corner case is. I agree, I don't even think the performance of fasta is necessarily better in this case. > (4) Keep fasta's scanner as is, and implement logic in its parser to handle the > error recovery corner case. Again, probably not worth it, given how rare the > error recovery corner case. > > (5) Change nothing. Fasta code stays simple and efficent, and the only > user-visible effect is that this particular error recovery case stops being > addressed. > > I personally lean toward (5). In which case I think the right thing to do is > (a) keep the "@failingTest" override, but change the comment to explain in a > sentence or two why we're ok with Fasta failing this test. (b) add a test (or > tests) validating Fasta's behavior, along with a "@failingTest" override for > analyzer's scanner. (c) after we've transitioned to Fasta and removed > analyzer's scanner, we can remove the failing test if we choose to. > > I don't care which one of us does (a) and (b)--it should only take a few > minutes. I think I see a slightly different option: create a subclass of `fasta.StringScanner` that overrides tokenizeOpenSquareBracket to behave as the analyzer expects. This should have no impact on fasta at all, so we can just do this in general, and later look at deciding which approach is the best long term (in this case, I think it's the dartanalyzer's). > As a point of process: since it's probably going to take some time to resolve > all the TODOs in this CL, I'd like to land it as is first (assuming that you > don't come up with blocking issues in your review) and then address the TODOs > with follow-up CLs. So I'll postpone doing (a) and (b) until after this CL > lands. I like this approach, and is what I'm used to. Normally, my comments are comments, and if I feel something is important, I'll often say "l*tm, provided you address so and so." Sometimes addressing my comments is explaining to me that I misunderstood something :-) https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:447: // ScannerErrorCode.UNTERMINATED_MULTI_LINE_COMMENT On 2017/02/04 14:57:55, Paul Berry wrote: > On 2017/02/04 09:48:19, ahe wrote: > > Absolutely. Just this week, I was thinking about cleaning this up. > > > > It's already a little bit better as I added ErrorToken.errorCode, and the API > > should probably be good enough for testing, but I do want to simplify > > UnterminatedToken.errorCode. > > > > See: > > > https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... > > Aha, thank you. I failed to notice the presence of ErrorToken.errorCode. I'll > rework this code to use it. > > > > > Another thought: perhaps I should move errorCode to Token. This way we don't > > have to rely on type tests of tokens. > > Interesting. We would still have to do a single megamorphic dispatch for > token.info, but thereafter we would avoid "is" checks and implicit downcasts, > and we could probably make everything else monomorphic. We should be careful about polymorphism, but in this case, errorCode is only used *after* we know that the tokens contains an error. This is because I set a bit on the scanner object whenever I create an ErrorToken. Another point is that one of the experiments I've considered for a long time is to have only one token class. This would be the ultimate way to reduce polymorphism of tokens. I think could potentially make the parser a little faster, but we'll see. Right now, I feel it's more important to get the compiler good enough wrt feature completeness to replace dartk. > I'm a little wary of making too many changes to Fasta right now, since it's not > hooked up to golem or to analyzer tests yet, but I suppose moving errorCode to > Token is pretty safe as changes go. I think it should be safe. As for benchmarking, the only thing I've been tracking is compile-time of dart2js. How to compile dart2js is described in pkg/front_end/lib/src/fasta/README.md, so if you run it a few times with and without your changes, you should be able to convince yourself that there's no impact. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:655: // single-line comments? Causes a failure in test_comment_single(). I'd just look at the first two characters of the comment: bool get isMultiline => value.startsWith("/*"); But we could just as well distinguish these kinds in the scanner. Since tokenizing comments is optional and not normally used by fasta, it can't affect performance in any way.
https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... File pkg/front_end/test/scanner_test.dart (right): https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:207: // TODO(paulberry,ahe): Fasta doesn't support generic method comment syntax. On 2017/02/06 11:01:20, ahe wrote: > Do you think that would be necessary to support in Fasta? It's not clear at this point. I asked Dan Grove at Friday's analyzer meeting, and he said that there is likely to be a long tail of internal code within Google that will continue using generic comment syntax until Dart 2.0. So if we wanted to cut over to Fasta prior to the Dart 2.0 fork, we would need generic comment support. I don't know when we are going to want to cut over. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:221: // TODO(paulberry,ahe): see UnimplementedError("distinguish unterminated errors") On 2017/02/06 11:01:20, ahe wrote: > Long line here and a few below. Done. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:249: // TODO(paulberry,ahe): "[]" should be parsed as a single token. On 2017/02/06 11:01:20, ahe wrote: > On 2017/02/04 14:57:55, Paul Berry wrote: > > On 2017/02/04 09:48:19, ahe wrote: > > > Note: [] and []= are tokenized as a single token only after the built-in > > > identifier "operator". > > > > > > See: > > > > > > > > > https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... > > > > Interesting. The behavior of the analyzer scanner is to lex "[]" and "[]=" as > > single tokens wherever they appear. On the downside, this complicates the > > analyzer parser slightly, since it means that it must recognize both `[]` and > > `[` `]` as empty list expressions. On the upside, it makes it easier for the > > analyzer parser to recover from errors like this: > > > > class Foo { > > int [](int index) => 0; > > } > > > > (Analyzer reports the error "Operator declarations must be preceeded by the > > keyword 'operator'."). If we wanted to get this error recovery behavior with > > fasta's lexer, there would be more parser work. On the other hand, maybe > that's > > ok, because the extra parser work would be all along an error handling code > path > > anyway. And admittedly, this particular error recovery corner case is > probably > > extremely rare in practice. > > I agree. A corner case with the dart2js/fasta approach is that if you write > something like this: > > main() { > var operator = ["Hello, World!"]; > print(operator[]); // Forgot 0 here. > } > > The fasta/dart2js parser won't recover well. > > My gut feeling is that the analyzer's approach is superior here. > > > In my mind this issue is simple enough that it's probably not worth > discussing, > > but since it's the first one that came up, let's use it as a practice run and > > see how the discussion goes. Once we get into a groove we shouldn't need to > > talk through every single detail. > > > > I can see a few possible ways to resolve this: > > > > (1) Change analyzer's parser and scanner to be like Fasta's, and implement > extra > > logic in analyzer's parser to keep the error recovery behavior. This would > > carry little benefit (it would make the unit test behavior line up, but not > > change the user experience other than a possible tiny performance > improvement). > > And it would all be wasted effort assuming we switch to Fasta in the long > term. > > > > (2) Change analyzer's parser and scanner to be like Fasta's, and don't worry > > about the error recovery behavior. This is also wasted effort, though > probably > > less effort than (1). > > > > (3) Change fasta's parser and scanner to be like analyzer's. This gets us the > > nice error recovery behavior, at the expense of some coding effort and a > > possible performance drop (though my intuition is that it would unmeasurably > > small). This is probably not worth it either, given how rare the error > recovery > > corner case is. > > I agree, I don't even think the performance of fasta is necessarily better in > this case. > > > (4) Keep fasta's scanner as is, and implement logic in its parser to handle > the > > error recovery corner case. Again, probably not worth it, given how rare the > > error recovery corner case. > > > > (5) Change nothing. Fasta code stays simple and efficent, and the only > > user-visible effect is that this particular error recovery case stops being > > addressed. > > > > I personally lean toward (5). In which case I think the right thing to do is > > (a) keep the "@failingTest" override, but change the comment to explain in a > > sentence or two why we're ok with Fasta failing this test. (b) add a test (or > > tests) validating Fasta's behavior, along with a "@failingTest" override for > > analyzer's scanner. (c) after we've transitioned to Fasta and removed > > analyzer's scanner, we can remove the failing test if we choose to. > > > > I don't care which one of us does (a) and (b)--it should only take a few > > minutes. > > I think I see a slightly different option: create a subclass of > `fasta.StringScanner` that overrides tokenizeOpenSquareBracket to behave as the > analyzer expects. This should have no impact on fasta at all, so we can just do > this in general, and later look at deciding which approach is the best long term > (in this case, I think it's the dartanalyzer's). SGTM. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:447: // ScannerErrorCode.UNTERMINATED_MULTI_LINE_COMMENT On 2017/02/06 11:01:20, ahe wrote: > On 2017/02/04 14:57:55, Paul Berry wrote: > > On 2017/02/04 09:48:19, ahe wrote: > > > Absolutely. Just this week, I was thinking about cleaning this up. > > > > > > It's already a little bit better as I added ErrorToken.errorCode, and the > API > > > should probably be good enough for testing, but I do want to simplify > > > UnterminatedToken.errorCode. > > > > > > See: > > > > > > https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... > > > > Aha, thank you. I failed to notice the presence of ErrorToken.errorCode. > I'll > > rework this code to use it. > > > > > > > > Another thought: perhaps I should move errorCode to Token. This way we don't > > > have to rely on type tests of tokens. > > > > Interesting. We would still have to do a single megamorphic dispatch for > > token.info, but thereafter we would avoid "is" checks and implicit downcasts, > > and we could probably make everything else monomorphic. > > We should be careful about polymorphism, but in this case, errorCode is only > used *after* we know that the tokens contains an error. This is because I set a > bit on the scanner object whenever I create an ErrorToken. Aha, nice. When I do my rework of this code (which I think I'll save for a follow-up CL) I'll make use of that bit. > > Another point is that one of the experiments I've considered for a long time is > to have only one token class. This would be the ultimate way to reduce > polymorphism of tokens. I think could potentially make the parser a little > faster, but we'll see. Yeah, I had a similar thought. > Right now, I feel it's more important to get the compiler > good enough wrt feature completeness to replace dartk. Agreed :) https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:655: // single-line comments? Causes a failure in test_comment_single(). On 2017/02/06 11:01:20, ahe wrote: > I'd just look at the first two characters of the comment: > > bool get isMultiline => value.startsWith("/*"); > > But we could just as well distinguish these kinds in the scanner. Since > tokenizing comments is optional and not normally used by fasta, it can't affect > performance in any way. Ok. I'll make a follow-up CL that adjusts this code to use value.startsWith("/*") as a short-term solution.
Description was changed from ========== Connect fasta's scanner to the analyzer-derived scanner tests in front_end. Failing tests are currently marked with "@failingTest" to prevent breaking buildbots. I've marked the issues I'm aware of with "TODO(paulberry,ahe)". Peter and I need to triage these issues--some of them may not be necessary to fix due to the fact that fasta replaces the parser as well. We will follow up with CLs that either fix the issues or remove the TODO comments as appropriate. ========== to ========== Connect fasta's scanner to the analyzer-derived scanner tests in front_end. Failing tests are currently marked with "@failingTest" to prevent breaking buildbots. I've marked the issues I'm aware of with "TODO(paulberry,ahe)". Peter and I need to triage these issues--some of them may not be necessary to fix due to the fact that fasta replaces the parser as well. We will follow up with CLs that either fix the issues or remove the TODO comments as appropriate. R=ahe@google.com Committed: https://github.com/dart-lang/sdk/commit/04f19f6bfb458daebbd7e4015ca1c53bc6b3e43e ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) manually as 04f19f6bfb458daebbd7e4015ca1c53bc6b3e43e (presubmit successful).
Message was sent while issue was closed.
sigmund@google.com changed reviewers: + sigmund@google.com
Message was sent while issue was closed.
very exciting to see this go by and to see the general discussion! I had a small DBC below. https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... File pkg/front_end/test/scanner_test.dart (right): https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:249: // TODO(paulberry,ahe): "[]" should be parsed as a single token. On 2017/02/06 13:46:54, Paul Berry wrote: > On 2017/02/06 11:01:20, ahe wrote: > > On 2017/02/04 14:57:55, Paul Berry wrote: > > > On 2017/02/04 09:48:19, ahe wrote: > > > > Note: [] and []= are tokenized as a single token only after the built-in > > > > identifier "operator". > > > > > > > > See: > > > > > > > > > > > > > > https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... > > > > > > Interesting. The behavior of the analyzer scanner is to lex "[]" and "[]=" > as > > > single tokens wherever they appear. On the downside, this complicates the > > > analyzer parser slightly, since it means that it must recognize both `[]` > and > > > `[` `]` as empty list expressions. On the upside, it makes it easier for > the > > > analyzer parser to recover from errors like this: > > > > > > class Foo { > > > int [](int index) => 0; > > > } > > > > > > (Analyzer reports the error "Operator declarations must be preceeded by the > > > keyword 'operator'."). If we wanted to get this error recovery behavior > with > > > fasta's lexer, there would be more parser work. On the other hand, maybe > > that's > > > ok, because the extra parser work would be all along an error handling code > > path > > > anyway. And admittedly, this particular error recovery corner case is > > probably > > > extremely rare in practice. > > > > I agree. A corner case with the dart2js/fasta approach is that if you write > > something like this: > > > > main() { > > var operator = ["Hello, World!"]; > > print(operator[]); // Forgot 0 here. > > } > > > > The fasta/dart2js parser won't recover well. > > > > My gut feeling is that the analyzer's approach is superior here. > > > > > In my mind this issue is simple enough that it's probably not worth > > discussing, > > > but since it's the first one that came up, let's use it as a practice run > and > > > see how the discussion goes. Once we get into a groove we shouldn't need to > > > talk through every single detail. > > > > > > I can see a few possible ways to resolve this: > > > > > > (1) Change analyzer's parser and scanner to be like Fasta's, and implement > > extra > > > logic in analyzer's parser to keep the error recovery behavior. This would > > > carry little benefit (it would make the unit test behavior line up, but not > > > change the user experience other than a possible tiny performance > > improvement). > > > And it would all be wasted effort assuming we switch to Fasta in the long > > term. > > > > > > (2) Change analyzer's parser and scanner to be like Fasta's, and don't worry > > > about the error recovery behavior. This is also wasted effort, though > > probably > > > less effort than (1). > > > > > > (3) Change fasta's parser and scanner to be like analyzer's. This gets us > the > > > nice error recovery behavior, at the expense of some coding effort and a > > > possible performance drop (though my intuition is that it would unmeasurably > > > small). This is probably not worth it either, given how rare the error > > recovery > > > corner case is. > > > > I agree, I don't even think the performance of fasta is necessarily better in > > this case. > > > > > (4) Keep fasta's scanner as is, and implement logic in its parser to handle > > the > > > error recovery corner case. Again, probably not worth it, given how rare > the > > > error recovery corner case. > > > > > > (5) Change nothing. Fasta code stays simple and efficent, and the only > > > user-visible effect is that this particular error recovery case stops being > > > addressed. > > > > > > I personally lean toward (5). In which case I think the right thing to do > is > > > (a) keep the "@failingTest" override, but change the comment to explain in a > > > sentence or two why we're ok with Fasta failing this test. (b) add a test > (or > > > tests) validating Fasta's behavior, along with a "@failingTest" override for > > > analyzer's scanner. (c) after we've transitioned to Fasta and removed > > > analyzer's scanner, we can remove the failing test if we choose to. > > > > > > I don't care which one of us does (a) and (b)--it should only take a few > > > minutes. > > > > I think I see a slightly different option: create a subclass of > > `fasta.StringScanner` that overrides tokenizeOpenSquareBracket to behave as > the > > analyzer expects. This should have no impact on fasta at all, so we can just > do > > this in general, and later look at deciding which approach is the best long > term > > (in this case, I think it's the dartanalyzer's). > > SGTM. A small question: could we annotate somehow these tests that highlights a difference we expect to be visible in error recovery? Once we have a way of running error recovery tests, I'd suggest just adding a test there, but for now, maybe a small comment with a grep-able text? or filing a short bug with a new label (e.g. fasta-recovery)?
Message was sent while issue was closed.
https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... File pkg/front_end/test/scanner_test.dart (right): https://codereview.chromium.org/2676083002/diff/1/pkg/front_end/test/scanner_... pkg/front_end/test/scanner_test.dart:249: // TODO(paulberry,ahe): "[]" should be parsed as a single token. On 2017/02/06 23:51:29, Siggi Cherem (dart-lang) wrote: > On 2017/02/06 13:46:54, Paul Berry wrote: > > On 2017/02/06 11:01:20, ahe wrote: > > > On 2017/02/04 14:57:55, Paul Berry wrote: > > > > On 2017/02/04 09:48:19, ahe wrote: > > > > > Note: [] and []= are tokenized as a single token only after the built-in > > > > > identifier "operator". > > > > > > > > > > See: > > > > > > > > > > > > > > > > > > > > https://github.com/dart-lang/sdk/blob/master/pkg/front_end/lib/src/fasta/scan... > > > > > > > > Interesting. The behavior of the analyzer scanner is to lex "[]" and > "[]=" > > as > > > > single tokens wherever they appear. On the downside, this complicates the > > > > analyzer parser slightly, since it means that it must recognize both `[]` > > and > > > > `[` `]` as empty list expressions. On the upside, it makes it easier for > > the > > > > analyzer parser to recover from errors like this: > > > > > > > > class Foo { > > > > int [](int index) => 0; > > > > } > > > > > > > > (Analyzer reports the error "Operator declarations must be preceeded by > the > > > > keyword 'operator'."). If we wanted to get this error recovery behavior > > with > > > > fasta's lexer, there would be more parser work. On the other hand, maybe > > > that's > > > > ok, because the extra parser work would be all along an error handling > code > > > path > > > > anyway. And admittedly, this particular error recovery corner case is > > > probably > > > > extremely rare in practice. > > > > > > I agree. A corner case with the dart2js/fasta approach is that if you write > > > something like this: > > > > > > main() { > > > var operator = ["Hello, World!"]; > > > print(operator[]); // Forgot 0 here. > > > } > > > > > > The fasta/dart2js parser won't recover well. > > > > > > My gut feeling is that the analyzer's approach is superior here. > > > > > > > In my mind this issue is simple enough that it's probably not worth > > > discussing, > > > > but since it's the first one that came up, let's use it as a practice run > > and > > > > see how the discussion goes. Once we get into a groove we shouldn't need > to > > > > talk through every single detail. > > > > > > > > I can see a few possible ways to resolve this: > > > > > > > > (1) Change analyzer's parser and scanner to be like Fasta's, and implement > > > extra > > > > logic in analyzer's parser to keep the error recovery behavior. This > would > > > > carry little benefit (it would make the unit test behavior line up, but > not > > > > change the user experience other than a possible tiny performance > > > improvement). > > > > And it would all be wasted effort assuming we switch to Fasta in the long > > > term. > > > > > > > > (2) Change analyzer's parser and scanner to be like Fasta's, and don't > worry > > > > about the error recovery behavior. This is also wasted effort, though > > > probably > > > > less effort than (1). > > > > > > > > (3) Change fasta's parser and scanner to be like analyzer's. This gets us > > the > > > > nice error recovery behavior, at the expense of some coding effort and a > > > > possible performance drop (though my intuition is that it would > unmeasurably > > > > small). This is probably not worth it either, given how rare the error > > > recovery > > > > corner case is. > > > > > > I agree, I don't even think the performance of fasta is necessarily better > in > > > this case. > > > > > > > (4) Keep fasta's scanner as is, and implement logic in its parser to > handle > > > the > > > > error recovery corner case. Again, probably not worth it, given how rare > > the > > > > error recovery corner case. > > > > > > > > (5) Change nothing. Fasta code stays simple and efficent, and the only > > > > user-visible effect is that this particular error recovery case stops > being > > > > addressed. > > > > > > > > I personally lean toward (5). In which case I think the right thing to do > > is > > > > (a) keep the "@failingTest" override, but change the comment to explain in > a > > > > sentence or two why we're ok with Fasta failing this test. (b) add a test > > (or > > > > tests) validating Fasta's behavior, along with a "@failingTest" override > for > > > > analyzer's scanner. (c) after we've transitioned to Fasta and removed > > > > analyzer's scanner, we can remove the failing test if we choose to. > > > > > > > > I don't care which one of us does (a) and (b)--it should only take a few > > > > minutes. > > > > > > I think I see a slightly different option: create a subclass of > > > `fasta.StringScanner` that overrides tokenizeOpenSquareBracket to behave as > > the > > > analyzer expects. This should have no impact on fasta at all, so we can just > > do > > > this in general, and later look at deciding which approach is the best long > > term > > > (in this case, I think it's the dartanalyzer's). > > > > SGTM. > > A small question: could we annotate somehow these tests that highlights a > difference we expect to be visible in error recovery? > > Once we have a way of running error recovery tests, I'd suggest just adding a > test there, but for now, maybe a small comment with a grep-able text? or filing > a short bug with a new label (e.g. fasta-recovery)? I've filed an issue: https://github.com/dart-lang/sdk/issues/28665 I'll follow up with a CL that links to the issue from this test. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
