track fasta parser begin/end event pairs

track fasta parser begin/end event pairs

This is a first cut at tracking fasta parser events
to ensure that I don't break them when improving parser recovery
and integrating the fasta parser into analyzer.

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/6e87f881cfe0a0a8729cb4030179ea4c2b6eab78

[danrubel, 2017-07-12 22:40:00]

danrubel@google.com changed reviewers:
+ paulberry@google.com

[Paul Berry, 2017-07-13 02:52:58]

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/parser_fasta_listener.dart (right):

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:30: _expectBegin(String
beginEvent, {bool optional: true, bool pop: true}) {
It reads weird to see calls to "_expectBegin" in methods named "end...". 
Consider renaming to "_end".  It's a less descriptive name, but I think it will
result in less confusion.

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:45:
_begin('beginArguments');
Consider changing the convention so that the word "begin" is not included in the
text passed to _begin.  If you take this suggestion as well as the one above,
then the code would read like this:

@override
void beginArguments(...) {
  listener.beginArguments(...);
  _begin('Arguments');
}

@override
void endArguments(...) {
  listener.endArguments(...);
  _end('Arguments');
}

Which would be nicely intuitive.

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:105: // There is no
corresponding endClassOrNamedMixinApplication
However, every call to beginClassOrNamedMixinApplication is followed by a call
to either: (a) beginClassDeclaration followed by endClassDeclaration, or (b)
beginNamedMixinApplication followed by endNamedMixinDeclaration.

So we could address this by uncommenting line 106, and adding
"expectBegin('beginClassOrNamedMixinApplication')" to the end of both
endClassDeclaration and endNamedMixinDeclaration.

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:190: // There is no
corresponding endExpression
AFAICT, no listener actually does anything in response to this event.  It used
to be used by the BodyBuilder to tell whether an identifier should be looked up
in the current scope or not (identifiers at the beginning of an expression
should; identifiers after a `.` should not).  But that was changed in
e37d55d111cea3af26659e744beffa461cbd9264 when we started making use of
IdentifierContext.

No action needed for this CL, but maybe in a separate CL we should just get rid
of this event entirely.

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:559: // TODO: implement
discardTypeReplacedWithCommentTypeAssign
This TODO doesn't have a name, and I don't understand it.  Was it left in by
accident?

There are many similar TODOs elsewhere in the file.

[danrubel, 2017-07-13 14:46:00]

Comments addressed. PTAL.

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/parser_fasta_listener.dart (right):

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:30: _expectBegin(String
beginEvent, {bool optional: true, bool pop: true}) {
On 2017/07/13 02:52:58, Paul Berry wrote:
> It reads weird to see calls to "_expectBegin" in methods named "end...". 
> Consider renaming to "_end".  It's a less descriptive name, but I think it
will
> result in less confusion.

Done.

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:45:
_begin('beginArguments');
On 2017/07/13 02:52:58, Paul Berry wrote:
> Consider changing the convention so that the word "begin" is not included in
the
> text passed to _begin.  If you take this suggestion as well as the one above,
> then the code would read like this:
> 
> @override
> void beginArguments(...) {
>   listener.beginArguments(...);
>   _begin('Arguments');
> }
> 
> @override
> void endArguments(...) {
>   listener.endArguments(...);
>   _end('Arguments');
> }
> 
> Which would be nicely intuitive.

Yes, that makes the code read more easily. Thanks!

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:105: // There is no
corresponding endClassOrNamedMixinApplication
On 2017/07/13 02:52:58, Paul Berry wrote:
> However, every call to beginClassOrNamedMixinApplication is followed by a call
> to either: (a) beginClassDeclaration followed by endClassDeclaration, or (b)
> beginNamedMixinApplication followed by endNamedMixinDeclaration.
> 
> So we could address this by uncommenting line 106, and adding
> "expectBegin('beginClassOrNamedMixinApplication')" to the end of both
> endClassDeclaration and endNamedMixinDeclaration.

Good suggestion. Done.

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:190: // There is no
corresponding endExpression
On 2017/07/13 02:52:58, Paul Berry wrote:
> AFAICT, no listener actually does anything in response to this event.  It used
> to be used by the BodyBuilder to tell whether an identifier should be looked
up
> in the current scope or not (identifiers at the beginning of an expression
> should; identifiers after a `.` should not).  But that was changed in
> e37d55d111cea3af26659e744beffa461cbd9264 when we started making use of
> IdentifierContext.
> 
> No action needed for this CL, but maybe in a separate CL we should just get
rid
> of this event entirely.

Yup, I've already started this conversation with Peter and hope to have future
CL(s) addressing this and other event issues.

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:559: // TODO: implement
discardTypeReplacedWithCommentTypeAssign
On 2017/07/13 02:52:58, Paul Berry wrote:
> This TODO doesn't have a name, and I don't understand it.  Was it left in by
> accident?
> 
> There are many similar TODOs elsewhere in the file.

These are auto generated by the IDE quick-fix and I've left them there
because... well... this is a first cut and I need TODO something about them. :-)

[Paul Berry, 2017-07-13 15:40:47]

lgtm assuming TODO comments are changed to TODO(danrubel)

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/parser_fasta_listener.dart (right):

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:559: // TODO: implement
discardTypeReplacedWithCommentTypeAssign
On 2017/07/13 14:46:00, danrubel wrote:
> On 2017/07/13 02:52:58, Paul Berry wrote:
> > This TODO doesn't have a name, and I don't understand it.  Was it left in by
> > accident?
> > 
> > There are many similar TODOs elsewhere in the file.
> 
> These are auto generated by the IDE quick-fix and I've left them there
> because... well... this is a first cut and I need TODO something about them.
:-)

In that case would you mind changing them to "TODO(danrubel)" to follow our
usual coding convention?

[danrubel, 2017-07-13 15:48:10]

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
File pkg/analyzer/test/generated/parser_fasta_listener.dart (right):

https://codereview.chromium.org/2975123002/diff/20001/pkg/analyzer/test/gener...
pkg/analyzer/test/generated/parser_fasta_listener.dart:559: // TODO: implement
discardTypeReplacedWithCommentTypeAssign
On 2017/07/13 15:40:47, Paul Berry wrote:
> On 2017/07/13 14:46:00, danrubel wrote:
> > On 2017/07/13 02:52:58, Paul Berry wrote:
> > > This TODO doesn't have a name, and I don't understand it.  Was it left in
by
> > > accident?
> > > 
> > > There are many similar TODOs elsewhere in the file.
> > 
> > These are auto generated by the IDE quick-fix and I've left them there
> > because... well... this is a first cut and I need TODO something about them.
> :-)
> 
> In that case would you mind changing them to "TODO(danrubel)" to follow our
> usual coding convention?

Oh yeah. Right. Done :-)

[danrubel, 2017-07-13 15:49:13]

Description was changed from

==========
track fasta parser begin/end event pairs

This is a first cut at tracking fasta parser events
to ensure that I don't break them when improving parser recovery
and integrating the fasta parser into analyzer.
==========

to

==========
track fasta parser begin/end event pairs

This is a first cut at tracking fasta parser events
to ensure that I don't break them when improving parser recovery
and integrating the fasta parser into analyzer.

R=paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6e87f881cfe0a0a8729cb4030179ea4c2b6eab78
==========

[danrubel, 2017-07-13 15:49:15]

Committed patchset #4 (id:60001) manually as
6e87f881cfe0a0a8729cb4030179ea4c2b6eab78 (presubmit successful).