Various errors on constant expressions.

Various errors on constant expressions.

R=karlklose@google.com

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

[ahe, 2017-03-22 14:01:13]

Patchset #1 (id:1) has been deleted

[ahe, 2017-03-22 14:06:29]

Patchset #1 (id:20001) has been deleted

[ahe, 2017-03-22 14:08:20]

ahe@google.com changed reviewers:
+ karlklose@google.com, paulberry@google.com

[ahe, 2017-03-22 14:08:21]

I may have to update the status files.

[ahe, 2017-03-22 14:20:49]

The preliminary results from running tests are looking good. There's a lot more
tests passing.

However, I'm not correctly marking local variables as const, so there are also
some new unexpected compile-time errors.

Regardless, I think this CL can be landed, and I'd appreciate your review.

[karlklose, 2017-03-22 14:25:53]

LGTM

[Paul Berry, 2017-03-22 16:18:05]

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:187: "Type variable can't
be used as a constant expression $type.");
I have two concerns about how this technique for detecting compile time errors
is going to integrate with analyzer:

1. Analyzer (and other Google-internal tools which consume analyzer output)
assumes that each error is associated with a pseudo-enum ErrorCode (see
pkg/front_end/lib/src/base/errors.dart) and that the error message is formed
using a template stored in ErrorCode.  If we encode the error message as a
simple string here, we aren't going to be able to translate it to analyzer's
representation later.

2. If we put error detection logic here in BodyBuilder, we will have to have
duplicate error detection logic in AstBuilder (or in a later analysis step). 
Not only does this mean duplicate coding work, it also increases the risk of
unintended differences between analyzer-reported errors and compiler-reported
errors.

Can we talk about this next time we meet, and brainstorm ways to do the error
reporting that will work well with analyzer integration?

Until then it's ok with me if you want to go ahead with this CL--I don't want to
block you from fixing tests :)

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:542:
addCompileTimeError(charOffset, "Not a constant expression.");
Idea: when we report a "Not a constant expression" compile time error, set
constantExpressionRequired to false so that the user gets a single error rather
than a pile of errors.

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:752: if (context !=
IdentifierContext.namedArgumentReference &&
Rather than a bunch of != checks, we should add a getter to IdentifierContext so
that this can be something like:

if (constantExpressionRequired && !context.allowedInConstantExpression) {
  addCompileTimeError(...);
}

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2404:
addCompileTimeError(charOffset, message);
This seems really scary to me.  Are we sure that all builder-detected warnings
should be treated as errors inside constant expressions?  I can't think of any
counterexamples off the top of my head, but this seems like an accident waiting
to happen.

[ahe, 2017-03-22 16:58:02]

Thank you!

Paul: you have a keen eye for all the things that already trouble me :-)

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:187: "Type variable can't
be used as a constant expression $type.");
On 2017/03/22 16:18:04, Paul Berry wrote:
> I have two concerns about how this technique for detecting compile time errors
> is going to integrate with analyzer:
> 
> 1. Analyzer (and other Google-internal tools which consume analyzer output)
> assumes that each error is associated with a pseudo-enum ErrorCode (see
> pkg/front_end/lib/src/base/errors.dart) and that the error message is formed
> using a template stored in ErrorCode.  If we encode the error message as a
> simple string here, we aren't going to be able to translate it to analyzer's
> representation later.

I'm aware of that, and I've started work on error codes in parallel. It's a cost
I'm pushing ahead of me and I know I have to pay.


> 2. If we put error detection logic here in BodyBuilder, we will have to have
> duplicate error detection logic in AstBuilder (or in a later analysis step). 
> Not only does this mean duplicate coding work, it also increases the risk of
> unintended differences between analyzer-reported errors and compiler-reported
> errors.
> 
> Can we talk about this next time we meet, and brainstorm ways to do the error
> reporting that will work well with analyzer integration?
> 
> Until then it's ok with me if you want to go ahead with this CL--I don't want
to
> block you from fixing tests :)

I share your concern about this, and I only have vague ideas so far.

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:542:
addCompileTimeError(charOffset, "Not a constant expression.");
On 2017/03/22 16:18:04, Paul Berry wrote:
> Idea: when we report a "Not a constant expression" compile time error, set
> constantExpressionRequired to false so that the user gets a single error
rather
> than a pile of errors.

That's probably a good idea.

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:752: if (context !=
IdentifierContext.namedArgumentReference &&
On 2017/03/22 16:18:05, Paul Berry wrote:
> Rather than a bunch of != checks, we should add a getter to IdentifierContext
so
> that this can be something like:
> 
> if (constantExpressionRequired && !context.allowedInConstantExpression) {
>   addCompileTimeError(...);
> }

Good idea.

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2404:
addCompileTimeError(charOffset, message);
On 2017/03/22 16:18:04, Paul Berry wrote:
> This seems really scary to me.  Are we sure that all builder-detected warnings
> should be treated as errors inside constant expressions?  I can't think of any
> counterexamples off the top of my head, but this seems like an accident
waiting
> to happen.

There are a few warnings that don't follow this rule, in particular, const
<Unresolved>[]. So I've been thinking about adding named parameter to this
method to say if this warning is a compile-time error in constants (in most
cases, I think it is).

[ahe, 2017-03-23 12:22:14]

Thank you!

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:542:
addCompileTimeError(charOffset, "Not a constant expression.");
On 2017/03/22 16:58:02, ahe wrote:
> On 2017/03/22 16:18:04, Paul Berry wrote:
> > Idea: when we report a "Not a constant expression" compile time error, set
> > constantExpressionRequired to false so that the user gets a single error
> rather
> > than a pile of errors.
> 
> That's probably a good idea.

I'm going to follow up on this in separate CL.

https://codereview.chromium.org/2765283002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:752: if (context !=
IdentifierContext.namedArgumentReference &&
On 2017/03/22 16:58:02, ahe wrote:
> On 2017/03/22 16:18:05, Paul Berry wrote:
> > Rather than a bunch of != checks, we should add a getter to
IdentifierContext
> so
> > that this can be something like:
> > 
> > if (constantExpressionRequired && !context.allowedInConstantExpression) {
> >   addCompileTimeError(...);
> > }
> 
> Good idea.

I'll do this in a separate CL.

[ahe, 2017-03-23 12:40:31]

Description was changed from

==========
Various errors on constant expressions.
==========

to

==========
Various errors on constant expressions.

R=karlklose@google.com

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

[ahe, 2017-03-23 12:40:33]

Committed patchset #3 (id:80001) manually as
2eca8c58c3e31271a66d2e00e0ff81344baab5a2 (presubmit successful).