Support for modifiers.

Support for modifiers.

R=paulberry@google.com, ahe@google.com
BUG=

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

[Paul Berry, 2017-02-21 23:09:11]

lgtm assuming you take my suggestions below.

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1073: // TODO(scheglov):
Report error.
The TODO is good, but for safety, let's also use "internalError" here so that if
we forget to come back and fix this, we won't ever accept invalid programs.

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1150: // TODO(scheglov):
Report error.
Ditto.

[scheglov, 2017-02-22 03:08:24]

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1073: // TODO(scheglov):
Report error.
On 2017/02/21 23:09:11, Paul Berry wrote:
> The TODO is good, but for safety, let's also use "internalError" here so that
if
> we forget to come back and fix this, we won't ever accept invalid programs.

Done.

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1150: // TODO(scheglov):
Report error.
On 2017/02/21 23:09:11, Paul Berry wrote:
> Ditto.

Done.

[scheglov, 2017-02-22 03:08:36]

Description was changed from

==========
Support for modifiers.

R=ahe@google.com, paulberry@google.com
BUG=
==========

to

==========
Support for modifiers.

R=paulberry@google.com, ahe@google.com
BUG=

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

[scheglov, 2017-02-22 03:08:37]

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

[ahe, 2017-02-22 09:08:17]

lgtm

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1073: // TODO(scheglov):
Report error.
On 2017/02/21 23:09:11, Paul Berry wrote:
> The TODO is good, but for safety, let's also use "internalError" here so that
if
> we forget to come back and fix this, we won't ever accept invalid programs.

Alternatively, try this:

printUnexpected(uri, modifier.charOffset, "Unexpected modifier: '${value}'.");

Later on, I plan to run through all these text strings and convert them to
proper error messages.

[scheglov, 2017-02-22 16:32:46]

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1073: // TODO(scheglov):
Report error.
On 2017/02/22 09:08:17, ahe wrote:
> On 2017/02/21 23:09:11, Paul Berry wrote:
> > The TODO is good, but for safety, let's also use "internalError" here so
that
> if
> > we forget to come back and fix this, we won't ever accept invalid programs.
> 
> Alternatively, try this:
> 
> printUnexpected(uri, modifier.charOffset, "Unexpected modifier: '${value}'.");
> 
> Later on, I plan to run through all these text strings and convert them to
> proper error messages.

There are 331 usage of internalError() and just 2 usages of printUnexpected() in
the codebase. So, it seems that internalError() is more popular.

[ahe, 2017-02-27 08:39:26]

FYI

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart (right):

https://codereview.chromium.org/2709843004/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/analyzer/ast_builder.dart:1073: // TODO(scheglov):
Report error.
On 2017/02/22 16:32:46, scheglov wrote:
> There are 331 usage of internalError() and just 2 usages of printUnexpected()
in
> the codebase. So, it seems that internalError() is more popular.

We talked about the differences between internalError and printUnexpected in a
meeting, but as my memory sometimes plays tricks on me, I've realized that
there's an even better way to report compile-time errors.

internalError is used when the compiler is broken. Here's its documentation:

/// Used to report an internal error.
///
/// Internal errors should be avoided as best as possible, but are preferred
/// over assertion failures. Favor error messages that starts with "Internal
/// error: " and a short description that may help a developer debug the issue.
/// This method should be called instead of using `throw`, as this allows us to
/// ensure that there are no throws anywhere in the codebase.

So that might not be the right thing to use here as it seems you've identified a
compile-time error.

I haven't documented [printUnexpected], and I'll do so, but I've documented
[inputError] which is kinda similar. The latter stops the compilation, that is,
a fatal compile-time error that the compiler can't recover from. Obviously, this
isn't ideal, so I added [printUnexpected], which just prints a compile-time
error.

However, when you're in BodyBuilder or AstBuilder, you always have access to the
current library builder, so you can use LibraryBuilder.addCompileTimeError.

So finally, I can conclude that LibraryBuilder.addCompileTimeError would
probably be the right method to use here :-)

I'll create a CL where I update the documentation.