fasta.Keyword implement analyzer.Keyword

fasta.Keyword implement analyzer.Keyword

* update fasta.Keyword to implement analyzer.Keyword
* update analyzer.Keyword to have fasta.Keyword consts
* make analyzer.Keyword abstract

R=paulberry@google.com

Committed: https://github.com/dart-lang/sdk/commit/4c050a6a6a8ad024c625657ff23addc5078c731b

[danrubel, 2017-03-14 02:39:08]

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

[Paul Berry, 2017-03-14 03:27:21]

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

[Paul Berry, 2017-03-14 03:27:21]

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/keyword.dart (right):

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:184: bool get isPseudoKeyword
=> isPseudo;
Two problems with this:

1. Analyzer's notion of "pseudo-keyword" corresponds to Fasta's notion of
"built-in keyword".  Fasta's terminology is more consistent with the spec, but
unfortunately we're stuck with analyzer's terminology (at least for now) because
we don't want to introduce a breaking change with zero notice.

So I would recommend changing this to a block-bodied function so that we can put
in an explanatory comment.  E.g. something like:

@override
bool get isPseudoKeyword {
  // The term "pseudo-keyword" doesn't exist in the spec, and
  // Analyzer and Fasta have different notions of what it means.
  // Analyzer's notion of "pseudo-keyword" corresponds with Fasta's
  // notion of "built-in keyword".
  return isBuiltIn;
}

2. There is one difference between Fasta's notion of "built-in identifier" and
analyzer's notion of "pseudo-keyword": Analyzer considers "deferred" to be in
this category; Fasta doesn't.  Based on my reading of the spec, this is a bug in
Fasta).  If we land this CL now (even with the fix I mention above), we will
regress some analyzer functionality due to the change in interpretation of
"deferred".  Either we should fix Fasta's handling of "deferred" before landing
this CL, or we should add a hack into this getter to avoid regressing analyzer.

[danrubel, 2017-03-14 12:46:17]

Updated. PTAL.

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/keyword.dart (right):

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:184: bool get isPseudoKeyword
=> isPseudo;
Thank you for the detailed explanation.

On 2017/03/14 03:27:21, Paul Berry wrote:
> Two problems with this:
> 
> 1. Analyzer's notion of "pseudo-keyword" corresponds to Fasta's notion of
> "built-in keyword".  Fasta's terminology is more consistent with the spec, but
> unfortunately we're stuck with analyzer's terminology (at least for now)
because
> we don't want to introduce a breaking change with zero notice.
> 
> So I would recommend changing this to a block-bodied function so that we can
put
> in an explanatory comment.  E.g. something like:
> 
> @override
> bool get isPseudoKeyword {
>   // The term "pseudo-keyword" doesn't exist in the spec, and
>   // Analyzer and Fasta have different notions of what it means.
>   // Analyzer's notion of "pseudo-keyword" corresponds with Fasta's
>   // notion of "built-in keyword".
>   return isBuiltIn;
> }

I have revised the implementation and added a test so that we don't break the
existing Analyzer API.

> 2. There is one difference between Fasta's notion of "built-in identifier" and
> analyzer's notion of "pseudo-keyword": Analyzer considers "deferred" to be in
> this category; Fasta doesn't.  Based on my reading of the spec, this is a bug
in
> Fasta).  If we land this CL now (even with the fix I mention above), we will
> regress some analyzer functionality due to the change in interpretation of
> "deferred".  Either we should fix Fasta's handling of "deferred" before
landing
> this CL, or we should add a hack into this getter to avoid regressing
analyzer.

I've updated the adapter method to "return isBuiltIn || this == DEFERRED" so
that I can continue adapting the fasta scanner to produce analyzer tokens. Is
there a issue already open for this fasta bug?

[Paul Berry, 2017-03-14 13:01:47]

lgtm

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/keyword.dart (right):

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:184: bool get isPseudoKeyword
=> isPseudo;
On 2017/03/14 12:46:16, danrubel wrote:
> Thank you for the detailed explanation.
> 
> On 2017/03/14 03:27:21, Paul Berry wrote:
> > Two problems with this:
> > 
> > 1. Analyzer's notion of "pseudo-keyword" corresponds to Fasta's notion of
> > "built-in keyword".  Fasta's terminology is more consistent with the spec,
but
> > unfortunately we're stuck with analyzer's terminology (at least for now)
> because
> > we don't want to introduce a breaking change with zero notice.
> > 
> > So I would recommend changing this to a block-bodied function so that we can
> put
> > in an explanatory comment.  E.g. something like:
> > 
> > @override
> > bool get isPseudoKeyword {
> >   // The term "pseudo-keyword" doesn't exist in the spec, and
> >   // Analyzer and Fasta have different notions of what it means.
> >   // Analyzer's notion of "pseudo-keyword" corresponds with Fasta's
> >   // notion of "built-in keyword".
> >   return isBuiltIn;
> > }
> 
> I have revised the implementation and added a test so that we don't break the
> existing Analyzer API.
> 
> > 2. There is one difference between Fasta's notion of "built-in identifier"
and
> > analyzer's notion of "pseudo-keyword": Analyzer considers "deferred" to be
in
> > this category; Fasta doesn't.  Based on my reading of the spec, this is a
bug
> in
> > Fasta).  If we land this CL now (even with the fix I mention above), we will
> > regress some analyzer functionality due to the change in interpretation of
> > "deferred".  Either we should fix Fasta's handling of "deferred" before
> landing
> > this CL, or we should add a hack into this getter to avoid regressing
> analyzer.
> 
> I've updated the adapter method to "return isBuiltIn || this == DEFERRED" so
> that I can continue adapting the fasta scanner to produce analyzer tokens. Is
> there a issue already open for this fasta bug?

Good point.  I just opened one: https://github.com/dart-lang/sdk/issues/29069

https://codereview.chromium.org/2746023003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/scanner/keyword.dart (right):

https://codereview.chromium.org/2746023003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:188: // notion of "built-in
keyword".
Let's expand this comment to mention
https://github.com/dart-lang/sdk/issues/29069.  E.g.:

// TODO: remove `this == DEFERRED` once dartbug.com/29069 is fixed.

[ahe, 2017-03-14 14:28:34]

These are just some preliminary comments, you don't need to wait for my complete
review.

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/keyword.dart (right):

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:181: //===================
analyzer.Token compatibility ==================
These kinds of sectioning comments tend to grow stale, so I normally avoid them.

https://codereview.chromium.org/2746023003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/scanner/keyword.dart (right):

https://codereview.chromium.org/2746023003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:19: static const ASSERT = const
Keyword("assert");
Why do we need to declare these constants?

[danrubel, 2017-03-14 15:23:48]

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/scanner/keyword.dart (right):

https://codereview.chromium.org/2746023003/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:181: //===================
analyzer.Token compatibility ==================
On 2017/03/14 14:28:34, ahe wrote:
> These kinds of sectioning comments tend to grow stale, so I normally avoid
them.

Fair enough. Removed.

https://codereview.chromium.org/2746023003/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/scanner/keyword.dart (right):

https://codereview.chromium.org/2746023003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:19: static const ASSERT = const
Keyword("assert");
On 2017/03/14 14:28:34, ahe wrote:
> Why do we need to declare these constants?

I want analyzer.Keyword consts to be identical to fasta.Keyword consts. For this
to be true, each const in analyzer.Keyword must reference the corresponding
fasta.Keyword const, thus I need to list each const individually.

https://codereview.chromium.org/2746023003/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/scanner/keyword.dart:188: // notion of "built-in
keyword".
On 2017/03/14 13:01:46, Paul Berry wrote:
> Let's expand this comment to mention
> https://github.com/dart-lang/sdk/issues/29069.  E.g.:
> 
> // TODO: remove `this == DEFERRED` once dartbug.com/29069 is fixed.

Good idea. Done.

[danrubel, 2017-03-14 15:26:37]

Description was changed from

==========
fasta.Keyword implement analyzer.Keyword

* update fasta.Keyword to implement analyzer.Keyword
* update analyzer.Keyword to have fasta.Keyword consts
* make analyzer.Keyword abstract
==========

to

==========
fasta.Keyword implement analyzer.Keyword

* update fasta.Keyword to implement analyzer.Keyword
* update analyzer.Keyword to have fasta.Keyword consts
* make analyzer.Keyword abstract

R=paulberry@google.com

Committed:
https://github.com/dart-lang/sdk/commit/4c050a6a6a8ad024c625657ff23addc5078c731b
==========

[danrubel, 2017-03-14 15:26:38]

Committed patchset #5 (id:80001) manually as
4c050a6a6a8ad024c625657ff23addc5078c731b (presubmit successful).