Move scanner into pkg/front_end/lib/src/scanner.

Move scanner into pkg/front_end/lib/src/scanner.

Several support classes also had to move to front_end along with the
scanner.  Some of these support classes arguably don't belong to the
scanner itself, since they have other uses (e.g. SyntacticEntity,
ErrorCode, ErrorSeverity, ErrorType, and StringUtilities).  They will
be reorganized into a more appropriate location in future CLs, at the
time that they become needed by other components of the front end.

In order to avoid dragging in a lot of dependencies, the following
changes were made:

1. Scanner no longer reports errors through Source and
AnalysisErrorListener objects passed to the constructor.  Instead, it
provides an abstract reportError() method which clients may override
to perform error reporting in any way they wish.  Analyzer contains an
override of Scanner that mimics the old behavior in order to maintain
compatibility.

2. Static members of ErrorCode (`values` and `byUniqueName`) have been
moved to top level, and remain in analyzer.  To maintain
compatibility, these static members remain in ErrorCode (as deprecated
members that simply wrap the implementations in analyzer).  This means
we have a reverse dependency (front_end depends on analyzer), but this
dependency will go away as soon as we publish the next breaking change
release of analyzer.

R=brianwilkerson@google.com, scheglov@google.com

Committed: https://github.com/dart-lang/sdk/commit/0a1fb8d248b1a2ca4cfe145a204ce4c7c3ef0206

[Paul Berry, 2016-11-08 19:34:39]

paulberry@google.com changed reviewers:
+ brianwilkerson@google.com, scheglov@google.com

[scheglov, 2016-11-08 20:07:05]

LGTM

[Brian Wilkerson, 2016-11-08 20:53:24]

I pointed this out in a couple of explicit comments, but then decided to provide
more general feedback. I think it's a mistake to put everything that was pulled
over into the 'scanner' directory. Many of these classes are not part of the
scanner, even though they're used by it. Classes such as AnalysisError are a
fairly obvious example of that. As a more subtle case, I liked the previous
organizational separation between the data structures that were being produced
and the class that was producing them. In my (often unique) view of the world,
the fact that the scanner produces tokens doesn't mean that tokens are part of
the scanner. The latter might be too subtle for anyone's taste but my own, but I
think the general comment still applies in many places.

Also, I noticed that this CL doesn't pull over any of the scanner tests. Was
that intentional? I'm guessing that we don't yet have a bot set up to run tests
in front_end, so moving the tests at this point would mean that they didn't
actually get run, which would be bad.

Other than that, and the in-line comments, lgtm.

https://codereview.chromium.org/2486873003/diff/1/pkg/analyzer/lib/error/erro...
File pkg/analyzer/lib/error/error.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/analyzer/lib/error/erro...
pkg/analyzer/lib/error/error.dart:33: //       lib/src/error/codes.dart |
Can you update this to include the new location for scanner errors?

https://codereview.chromium.org/2486873003/diff/1/pkg/analyzer/lib/error/erro...
pkg/analyzer/lib/error/error.dart:480:
StaticTypeWarningCode.WRONG_NUMBER_OF_TYPE_ARGUMENTS,
Unfortunately, it looks like this file was changed after you started editing it,
so you'll have to check carefully when you merge. (There are four (I think) new
error codes in the latest definition.)

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/errors.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/errors.dart:5: library
front_end.src.scanner.errors;
Is there any value to having library directives (in the absence of parts)?

Also, consider adding doc comments to all libraries in the process.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/errors.dart:69: abstract class ErrorCode {
I would recommend putting the abstract classes in a non-scanner-specific
library.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/interner.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/interner.dart:1: // Copyright (c) 2016, the Dart
project authors.  Please see the AUTHORS file
We should be consistent about updating or not updating the copyright. (I don't
see any value in updating it, but I don't have a strong preference.)

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/reader.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/reader.dart:10: abstract class CharacterReader {
Seems odd to not have a concrete implementation in the front-end.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/string_utilities.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/string_utilities.dart:12: static bool
endsWith3(String str, int c1, int c2, int c3) {
We might want to verify that these methods are still more performant than
String.endsWith and String.startsWith before pulling them over.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/syntactic_entity.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/syntactic_entity.dart:11: abstract class
SyntacticEntity {
This isn't a scanner-specific class, so it seems wrong to have it in a scanner
library.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/token.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/token.dart:5: library front_end.src.scanner.token;
Not sure why we dropped the doc comment.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/token.dart:16: class BeginToken extends
SimpleToken {
As discussed off-line, I liked the previous separation between the Token
interface, which was public, and the specific implementations of tokens, which
were not public. Your call, though.

[Paul Berry, 2016-11-08 21:46:55]

On 2016/11/08 20:53:24, Brian Wilkerson wrote:
> I pointed this out in a couple of explicit comments, but then decided to
provide
> more general feedback. I think it's a mistake to put everything that was
pulled
> over into the 'scanner' directory. Many of these classes are not part of the
> scanner, even though they're used by it. Classes such as AnalysisError are a
> fairly obvious example of that.

Agreed.  I plan to address this in a follow-up CL.

> As a more subtle case, I liked the previous
> organizational separation between the data structures that were being produced
> and the class that was producing them. In my (often unique) view of the world,
> the fact that the scanner produces tokens doesn't mean that tokens are part of
> the scanner. The latter might be too subtle for anyone's taste but my own, but
I
> think the general comment still applies in many places.

Acknowledged.  There is definitely room for reasonable people to disagree here,
and I'm still forming my own opinion.  I consider the file organization within
front_end/lib/src to be provisional at this point, and I anticipate that we'll
continue making changes as more pieces are pulled into the front end.

> 
> Also, I noticed that this CL doesn't pull over any of the scanner tests. Was
> that intentional? I'm guessing that we don't yet have a bot set up to run
tests
> in front_end, so moving the tests at this point would mean that they didn't
> actually get run, which would be bad.

It's intentional, but only because I wanted to kick start the process with the
smallest CL I could manage.  I plan to move over tests in the next CL.

We don't yet have a bot to run front_end tests, however there is no gap in
coverage (the tests are being run on one of the overworked "pub" bots).  On
Thursday I put in a request to Bill to test the front end on the analyzer bots
in the short term, and on their own bots in the long term; he indicated that he
should be able to get to that this week.  I will ping him and make sure this
doesn't get forgotten about.

[Paul Berry, 2016-11-08 21:47:06]

https://codereview.chromium.org/2486873003/diff/1/pkg/analyzer/lib/error/erro...
File pkg/analyzer/lib/error/error.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/analyzer/lib/error/erro...
pkg/analyzer/lib/error/error.dart:33: //       lib/src/error/codes.dart |
On 2016/11/08 20:53:23, Brian Wilkerson wrote:
> Can you update this to include the new location for scanner errors?

Done.

https://codereview.chromium.org/2486873003/diff/1/pkg/analyzer/lib/error/erro...
pkg/analyzer/lib/error/error.dart:480:
StaticTypeWarningCode.WRONG_NUMBER_OF_TYPE_ARGUMENTS,
On 2016/11/08 20:53:23, Brian Wilkerson wrote:
> Unfortunately, it looks like this file was changed after you started editing
it,
> so you'll have to check carefully when you merge. (There are four (I think)
new
> error codes in the latest definition.)

Thanks for the heads up.  I manually merged in the new codes.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/errors.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/errors.dart:5: library
front_end.src.scanner.errors;
On 2016/11/08 20:53:23, Brian Wilkerson wrote:
> Is there any value to having library directives (in the absence of parts)?

IMHO, not really.  I've removed them.

> Also, consider adding doc comments to all libraries in the process.

Acknowledged.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/errors.dart:69: abstract class ErrorCode {
On 2016/11/08 20:53:23, Brian Wilkerson wrote:
> I would recommend putting the abstract classes in a non-scanner-specific
> library.

Acknowledged.  I will address this in later CLs.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/interner.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/interner.dart:1: // Copyright (c) 2016, the Dart
project authors.  Please see the AUTHORS file
On 2016/11/08 20:53:23, Brian Wilkerson wrote:
> We should be consistent about updating or not updating the copyright. (I don't
> see any value in updating it, but I don't have a strong preference.)

The only value I see in updating it is that as files get moved from the analyzer
to the front end, there won't always be a simple 1-to-1 relationship between the
old file and the new file, so if we want to keep the old copyright dates, it
won't always be obvious which date is the "old" one.  So I opted to consistently
update all the copyright dates.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/reader.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/reader.dart:10: abstract class CharacterReader {
On 2016/11/08 20:53:23, Brian Wilkerson wrote:
> Seems odd to not have a concrete implementation in the front-end.

Acknowledged.  Concrete implementations will be moved in the next CL, when I
move scanner tests into the front end.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/syntactic_entity.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/syntactic_entity.dart:11: abstract class
SyntacticEntity {
On 2016/11/08 20:53:24, Brian Wilkerson wrote:
> This isn't a scanner-specific class, so it seems wrong to have it in a scanner
> library.

Acknowledged.  I will address this in a follow-up CL.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/token.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/token.dart:5: library front_end.src.scanner.token;
On 2016/11/08 20:53:24, Brian Wilkerson wrote:
> Not sure why we dropped the doc comment.

Re-added.

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/token.dart:16: class BeginToken extends
SimpleToken {
On 2016/11/08 20:53:24, Brian Wilkerson wrote:
> As discussed off-line, I liked the previous separation between the Token
> interface, which was public, and the specific implementations of tokens, which
> were not public. Your call, though.

Acknowledged.

[Paul Berry, 2016-11-08 21:47:20]

Description was changed from

==========
Move scanner into pkg/front_end/lib/src/scanner.

Several support classes also had to move to front_end along with the
scanner.  Some of these support classes arguably don't belong to the
scanner itself, since they have other uses (e.g. SyntacticEntity,
ErrorCode, ErrorSeverity, ErrorType, and StringUtilities).  They will
be reorganized into a more appropriate location in future CLs, at the
time that they become needed by other components of the front end.

In order to avoid dragging in a lot of dependencies, the following
changes were made:

1. Scanner no longer reports errors through Source and
AnalysisErrorListener objects passed to the constructor.  Instead, it
provides an abstract reportError() method which clients may override
to perform error reporting in any way they wish.  Analyzer contains an
override of Scanner that mimics the old behavior in order to maintain
compatibility.

2. Static members of ErrorCode (`values` and `byUniqueName`) have been
moved to top level, and remain in analyzer.  To maintain
compatibility, these static members remain in ErrorCode (as deprecated
members that simply wrap the implementations in analyzer).  This means
we have a reverse dependency (front_end depends on analyzer), but this
dependency will go away as soon as we publish the next breaking change
release of analyzer.
==========

to

==========
Move scanner into pkg/front_end/lib/src/scanner.

Several support classes also had to move to front_end along with the
scanner.  Some of these support classes arguably don't belong to the
scanner itself, since they have other uses (e.g. SyntacticEntity,
ErrorCode, ErrorSeverity, ErrorType, and StringUtilities).  They will
be reorganized into a more appropriate location in future CLs, at the
time that they become needed by other components of the front end.

In order to avoid dragging in a lot of dependencies, the following
changes were made:

1. Scanner no longer reports errors through Source and
AnalysisErrorListener objects passed to the constructor.  Instead, it
provides an abstract reportError() method which clients may override
to perform error reporting in any way they wish.  Analyzer contains an
override of Scanner that mimics the old behavior in order to maintain
compatibility.

2. Static members of ErrorCode (`values` and `byUniqueName`) have been
moved to top level, and remain in analyzer.  To maintain
compatibility, these static members remain in ErrorCode (as deprecated
members that simply wrap the implementations in analyzer).  This means
we have a reverse dependency (front_end depends on analyzer), but this
dependency will go away as soon as we publish the next breaking change
release of analyzer.

R=brianwilkerson@google.com, scheglov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/0a1fb8d248b1a2ca4cfe145a204ce4c7c3ef0206
==========

[Paul Berry, 2016-11-08 21:47:21]

Committed patchset #1 (id:1) manually as
0a1fb8d248b1a2ca4cfe145a204ce4c7c3ef0206 (presubmit successful).

[Brian Wilkerson, 2016-11-08 22:01:23]

lgtm

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/interner.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/interner.dart:1: // Copyright (c) 2016, the Dart
project authors.  Please see the AUTHORS file
In that case, you missed two files. :-) (Hence the observation.)

[Paul Berry, 2016-11-08 22:06:03]

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/interner.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/interner.dart:1: // Copyright (c) 2016, the Dart
project authors.  Please see the AUTHORS file
On 2016/11/08 22:01:23, Brian Wilkerson wrote:
> In that case, you missed two files. :-) (Hence the observation.)

Whoops, thank you.  Please see https://codereview.chromium.org/2483313003/.

[kevmoo, 2016-11-08 22:26:02]

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

[kevmoo, 2016-11-08 22:26:03]

DBQ

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/errors.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/errors.dart:7: import
'package:analyzer/error/error.dart'
We try to avoid circular dependencies between packages (at least in the lib
directory, tests are fine)

Is this short-term?

[Paul Berry, 2016-11-08 22:28:15]

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
File pkg/front_end/lib/src/scanner/errors.dart (right):

https://codereview.chromium.org/2486873003/diff/1/pkg/front_end/lib/src/scann...
pkg/front_end/lib/src/scanner/errors.dart:7: import
'package:analyzer/error/error.dart'
On 2016/11/08 22:26:03, kevmoo wrote:
> We try to avoid circular dependencies between packages (at least in the lib
> directory, tests are fine)
> 
> Is this short-term?

Yes.  This will be removed when the "@deprecated" members below are removed,
which we should be able to do next time we publish a breaking change to
analyzer.