Check for compile-time constants in DartCompiler

This adds checks for compile-time constants in places 
where the spec demands it:

  - initializing toplevel finals
  - initializing static final class members
  - initializing named parameters to functions

Committed: https://code.google.com/p/dart/source/detail?r=468

[zundel, 2011-10-14 01:54:28]

After a week long hiatus, I'm bringing back this patch, re-written to use the
visitor pattern and with a long list of test cases.  there are 2 tests that
don't pass the compiler:

  ConstInitTest, which references some variables before they are declared.

  One of the end2end incremental tests, which seems to be reading a stale copy
of a file no matter what I do (not a good sign).

http://codereview.chromium.org/8231031/diff/7001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/ast/DartExpression.java (right):

http://codereview.chromium.org/8231031/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/ast/DartExpression.java:13: Type type;
I was advised to use getSymbol().getType(), but binary expressoins and unary
expressions don't have a corresponding symbol.  Should I go to MemberBuilder and
create some kind of Element to back these types of expressions?

[ngeoffray, 2011-10-14 09:26:56]

Initial comments.

http://codereview.chromium.org/8231031/diff/11001/client/samples/sunflower/Su...
File client/samples/sunflower/Sunflower.dart (right):

http://codereview.chromium.org/8231031/diff/11001/client/samples/sunflower/Su...
client/samples/sunflower/Sunflower.dart:18: PHI = (Math.sqrt(5)+1) / 2;
Nit: spaces around +

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:44:
public class CompileTimeConstVisitor extends DartVisitor {
Please use the DartNodeTraverser instead. I think DartVisitor was supposed to go
away.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:115:
rhsType.getClass(); // fast null check
you're doing the null checks and then checking for null line 117. I think
getMostSpecificType should never return null (expressions should be of type
dynamic), so if you want to do the null check, I would do it in
getMostSpecifictype.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:116:
boolean isConst = true;
No need for this variable if lhsType and rhsType can never be null.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:126:
if (checkNumberBooleanOrStringType(lhs, lhsType) &&
Nit: && should be on a newline, right below check...

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:135:
&& checkBoolean(rhs, rhsType)) {
weird indentation.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:150:
}
ditto.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:265:
}
Maybe inline the following code into a 'default' case? Should it be a case NONE:
? and default should throw an AssertionError?

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:357:
switch (ElementKind.of(element)) {
Please add a comment that factory methods cannot be CONSTRUCTOR.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:390:
checkConstantLiteral(expression);
Could you remove that TODO now? We should be able to call a const constructor
and the constant check visitor will validate it, right?

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart:8:
const A() : this.x = 1;
Why changing this file? I would keep it as is, and copy it if you want to run
the almost same code?

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart:14: final
int globalVar = 42;
ditto.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.dart:6: class
Other1 {
ditto.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java:85:
DartCompilerErrorCode.EXPECTED_CONSTANT_EXPRESSION_NUMBER,
It looks like the error messages are duplicated, right? Maybe we should just
have one?

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/CTConst2...
File tests/language/src/CTConst2Test.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/CTConst2...
tests/language/src/CTConst2Test.dart:100: static final a = new A(); //
Assignment from Constant constructor OK
It should be a const A(); then.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/ConstIni...
File tests/language/src/ConstInitTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/ConstIni...
tests/language/src/ConstInitTest.dart:46: // Compiler doesn't recognize the type
of O, N as int in that case.
The order should not matter. I think the CTC checker should visit O, realize
that it's a field, and visit it, so that it knows it's an int. That means the
CTC checker must detect cycles :(

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/ListLite...
File tests/language/src/ListLiteralTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/ListLite...
tests/language/src/ListLiteralTest.dart:45: Expect.equals(200000.0, e2[3]);
Unrelated change?

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
File tests/language/src/StaticFinalFieldTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
tests/language/src/StaticFinalFieldTest.dart:32: // Compile time constants don't
allow string interpolation.
I have CC'ed Matthias to make sure he's aware of that part of the spec (which is
limiting). If we all agree, please move the string interpolation case in a
negative test.

[zundel, 2011-10-14 10:04:21]

Thanks for slogging through it once. I'll put up another patch later.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:390:
checkConstantLiteral(expression);
On 2011/10/14 09:26:56, ngeoffray wrote:
> Could you remove that TODO now? We should be able to call a const constructor
> and the constant check visitor will validate it, right?

I wasn't sure that non static variables needed to have compile time constant
initializers.  Sounds too strict?

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart:8:
const A() : this.x = 1;
On 2011/10/14 09:26:56, ngeoffray wrote:
> Why changing this file? I would keep it as is, and copy it if you want to run
> the almost same code?

Part of this change is to enforce the initialization of static fields.  You
cannot initialize a static final with a non-const value.  This code was using
the implicit constructor which is not const.  I'm pretty sure this file was
intended to be valid dart code, so I updated it.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart:14: final
int globalVar = 42;
On 2011/10/14 09:26:56, ngeoffray wrote:
> ditto.
I was under the impression you cannot initialize a non final field, but I don't
know if that is really true or just an artifact of the way the code was written
before I started my changes.  I'm scouring the spec now.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.dart:6: class
Other1 {
On 2011/10/14 09:26:56, ngeoffray wrote:
> ditto.

I updated the comment, the old code violates the language.  Florian and I
discusses this initiailization when I was working on default values for function
parameters.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java:85:
DartCompilerErrorCode.EXPECTED_CONSTANT_EXPRESSION_NUMBER,
On 2011/10/14 09:26:56, ngeoffray wrote:
> It looks like the error messages are duplicated, right? Maybe we should just
> have one?

What's happening is that first, A.foo() isn't a const expression when we analyze
that node.  Second, when analyzing the binary expression, the LHS has type
dynamic becuase A.foo()'s return type can't be determined (type analysis hasn't
run yet.).  I am not sure how to get rid of one of these errors in this case
without squelching a legitmate error case.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/CTConst2...
File tests/language/src/CTConst2Test.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/CTConst2...
tests/language/src/CTConst2Test.dart:100: static final a = new A(); //
Assignment from Constant constructor OK
On 2011/10/14 09:26:56, ngeoffray wrote:
> It should be a const A(); then.

I don't understand this comment (const A(); is already there as a constructor).

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/ListLite...
File tests/language/src/ListLiteralTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/ListLite...
tests/language/src/ListLiteralTest.dart:45: Expect.equals(200000.0, e2[3]);
On 2011/10/14 09:26:56, ngeoffray wrote:
> Unrelated change?

I must have merged up since the previous patch.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
File tests/language/src/StaticFinalFieldTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
tests/language/src/StaticFinalFieldTest.dart:32: // Compile time constants don't
allow string interpolation.
On 2011/10/14 09:26:56, ngeoffray wrote:
> I have CC'ed Matthias to make sure he's aware of that part of the spec (which
is
> limiting). If we all agree, please move the string interpolation case in a
> negative test.

For the negative test, see tringInterpolate7NegativeTest.dart

For the String interpolation exclusion in the spec, see section 10.1.  I asked
for it to be changed to allow compile time constant expressions, but never got a
response.

[ngeoffray, 2011-10-14 10:33:42]

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:390:
checkConstantLiteral(expression);
On 2011/10/14 10:04:21, zundel wrote:
> On 2011/10/14 09:26:56, ngeoffray wrote:
> > Could you remove that TODO now? We should be able to call a const
constructor
> > and the constant check visitor will validate it, right?
> 
> I wasn't sure that non static variables needed to have compile time constant
> initializers.  Sounds too strict?

Right now, non-static variables can only have compile-time constants as
initializers. Otherwise we would have execution of code that is not in a
constructor's body. That's what we want to avoid.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart:8:
const A() : this.x = 1;
On 2011/10/14 10:04:21, zundel wrote:
> On 2011/10/14 09:26:56, ngeoffray wrote:
> > Why changing this file? I would keep it as is, and copy it if you want to
run
> > the almost same code?
> 
> Part of this change is to enforce the initialization of static fields.  You
> cannot initialize a static final with a non-const value.  This code was using
> the implicit constructor which is not const.  I'm pretty sure this file was
> intended to be valid dart code, so I updated it.

I'm not sure I follow. This file looks like valid Dart code before your changes.
I don't see why you need to change the constructor to 'const' to make it valid?

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart:14: final
int globalVar = 42;
On 2011/10/14 10:04:21, zundel wrote:
> On 2011/10/14 09:26:56, ngeoffray wrote:
> > ditto.
> I was under the impression you cannot initialize a non final field, but I
don't
> know if that is really true or just an artifact of the way the code was
written
> before I started my changes.  I'm scouring the spec now.  

You can initialize non-static fields (including top-level and instance) with
compile-time constants.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.dart:6: class
Other1 {
On 2011/10/14 10:04:21, zundel wrote:
> On 2011/10/14 09:26:56, ngeoffray wrote:
> > ditto.
> 
> I updated the comment, the old code violates the language.  Florian and I
> discusses this initiailization when I was working on default values for
function
> parameters.

Oh, OK. So the final Function FN = ... was accidental. Nevermind thenm thanks
for fixing it!

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java:85:
DartCompilerErrorCode.EXPECTED_CONSTANT_EXPRESSION_NUMBER,
On 2011/10/14 10:04:21, zundel wrote:
> On 2011/10/14 09:26:56, ngeoffray wrote:
> > It looks like the error messages are duplicated, right? Maybe we should just
> > have one?
> 
> What's happening is that first, A.foo() isn't a const expression when we
analyze
> that node.  Second, when analyzing the binary expression, the LHS has type
> dynamic becuase A.foo()'s return type can't be determined (type analysis
hasn't
> run yet.).  I am not sure how to get rid of one of these errors in this case
> without squelching a legitmate error case.

I understand why you're getting both errors, according to the analysis flow. But
I hope that the user only sees one error message. In general, two error messages
on the same expression somehow confuses me. I don't know what's best for
everyone. Feel free to ignore if you think it's ok.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/CTConst2...
File tests/language/src/CTConst2Test.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/CTConst2...
tests/language/src/CTConst2Test.dart:100: static final a = new A(); //
Assignment from Constant constructor OK
On 2011/10/14 10:04:21, zundel wrote:
> On 2011/10/14 09:26:56, ngeoffray wrote:
> > It should be a const A(); then.
> 
> I don't understand this comment (const A(); is already there as a
constructor).

The code you wrote is not allowed: 'a' should be initialized with a constant
expression. However, "new A()" is not a constant expression.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
File tests/language/src/StaticFinalFieldTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
tests/language/src/StaticFinalFieldTest.dart:32: // Compile time constants don't
allow string interpolation.
On 2011/10/14 10:04:21, zundel wrote:
> On 2011/10/14 09:26:56, ngeoffray wrote:
> > I have CC'ed Matthias to make sure he's aware of that part of the spec
(which
> is
> > limiting). If we all agree, please move the string interpolation case in a
> > negative test.
> 
> For the negative test, see tringInterpolate7NegativeTest.dart
> 
> For the String interpolation exclusion in the spec, see section 10.1.  I asked
> for it to be changed to allow compile time constant expressions, but never got
a
> response.

Apologies for the "never got a response". Gilad is next to me right now, so it's
easy to get one :) So the spec is limiting right now, but we may extend it once
it gets deeply needed. It hasn't yet because it's not high-priority.

[floitsch, 2011-10-14 14:36:58]

FYI.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/DartCompilerErrorCode.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/DartCompilerErrorCode.java:64:
EXPECTED_CONSTANT_EXPRESSION_BOOLEAN("Expected constant expression of type
boolean, got %s"),
why not "bool"?
or why not "integer" below?

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/DartIdentifier.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartIdentifier.java:54: *
TODO(zundel): I'm pretty sure getTargetSymbol() is vestigal: getSymbol() is used
elsewhere.
Cannot comment on this. Please get Fabio (or somebody else knowledgeable) to
have a short look at this part.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:185:
public void endVisit(DartPropertyAccess x, DartContext ctx) {
Does this make sure that PropertyAccess only goes to classes (or prefixes)?
(i.e. can an instance-field have the "isConstant" set?)
"const Point.origin().x" is not allowed (for instance).

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:210:
return false;
I'm not really familiar with the visitor, but why does this return "false"?

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:229:
modifiers = modifiers.makeConstant();
This sounds wrong.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:390:
checkConstantLiteral(expression);
I agree. This should be a checkConstantExpression.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart:8:
const A() : this.x = 1;
On 2011/10/14 10:33:42, ngeoffray wrote:
> On 2011/10/14 10:04:21, zundel wrote:
> > On 2011/10/14 09:26:56, ngeoffray wrote:
> > > Why changing this file? I would keep it as is, and copy it if you want to
> run
> > > the almost same code?
> > 
> > Part of this change is to enforce the initialization of static fields.  You
> > cannot initialize a static final with a non-const value.  This code was
using
> > the implicit constructor which is not const.  I'm pretty sure this file was
> > intended to be valid dart code, so I updated it.
> 
> I'm not sure I follow. This file looks like valid Dart code before your
changes.
> I don't see why you need to change the constructor to 'const' to make it
valid?

Maybe because the "static final A a = new A() (below) is invalid. Please make
sure that the checker catches this case.

[zundel, 2011-10-14 20:59:51]

PTAL

http://codereview.chromium.org/8231031/diff/11001/client/samples/sunflower/Su...
File client/samples/sunflower/Sunflower.dart (right):

http://codereview.chromium.org/8231031/diff/11001/client/samples/sunflower/Su...
client/samples/sunflower/Sunflower.dart:18: PHI = (Math.sqrt(5)+1) / 2;
On 2011/10/14 09:26:56, ngeoffray wrote:
> Nit: spaces around +

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/DartCompilerErrorCode.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/DartCompilerErrorCode.java:64:
EXPECTED_CONSTANT_EXPRESSION_BOOLEAN("Expected constant expression of type
boolean, got %s"),
On 2011/10/14 14:36:58, floitsch wrote:
> why not "bool"?
> or why not "integer" below?

made it consistent with 'bool', 'int', and 'num'

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/DartIdentifier.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartIdentifier.java:59: @Deprecated
Fabio gave me some background so I added a comment.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:44:
public class CompileTimeConstVisitor extends DartVisitor {
On 2011/10/14 09:26:56, ngeoffray wrote:
> Please use the DartNodeTraverser instead. I think DartVisitor was supposed to
go
> away.

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:115:
rhsType.getClass(); // fast null check
On 2011/10/14 09:26:56, ngeoffray wrote:
> you're doing the null checks and then checking for null line 117. I think
> getMostSpecificType should never return null (expressions should be of type
> dynamic), so if you want to do the null check, I would do it in
> getMostSpecifictype.

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:116:
boolean isConst = true;
On 2011/10/14 09:26:56, ngeoffray wrote:
> No need for this variable if lhsType and rhsType can never be null.

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:126:
if (checkNumberBooleanOrStringType(lhs, lhsType) &&
On 2011/10/14 09:26:56, ngeoffray wrote:
> Nit: && should be on a newline, right below check...

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:135:
&& checkBoolean(rhs, rhsType)) {
On 2011/10/14 09:26:56, ngeoffray wrote:
> weird indentation.

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:150:
}
On 2011/10/14 09:26:56, ngeoffray wrote:
> ditto.

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:185:
public void endVisit(DartPropertyAccess x, DartContext ctx) {
On 2011/10/14 14:36:58, floitsch wrote:
> Does this make sure that PropertyAccess only goes to classes (or prefixes)?
> (i.e. can an instance-field have the "isConstant" set?)
> "const Point.origin().x" is not allowed (for instance).

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:210:
return false;
On 2011/10/14 14:36:58, floitsch wrote:
> I'm not really familiar with the visitor, but why does this return "false"?

returns false to stop traversing children.

I switched to DartNodeTraverser

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:265:
}
On 2011/10/14 09:26:56, ngeoffray wrote:
> Maybe inline the following code into a 'default' case? Should it be a case
NONE:
> ? and default should throw an AssertionError?

Done.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:357:
switch (ElementKind.of(element)) {
On 2011/10/14 09:26:56, ngeoffray wrote:
> Please add a comment that factory methods cannot be CONSTRUCTOR.

I'm keying off of the 'const' reserved word now, so the code doesn't even need
to analyze the symbol.

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java (right):

http://codereview.chromium.org/8231031/diff/11001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:229:
modifiers = modifiers.makeConstant();
On 2011/10/14 14:36:58, floitsch wrote:
> This sounds wrong.

I'm still changing the modifiers.  Final and Const mean the same thing but on
different types of elements.  So yes, its a little weird, but much more
convenient than trying to figure out whether to test isFinal or isConstant
everywhere.

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart
(right):

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart:8:
const A() : this.x = 1;
On 2011/10/14 10:33:42, ngeoffray wrote:
> On 2011/10/14 10:04:21, zundel wrote:
> > On 2011/10/14 09:26:56, ngeoffray wrote:
> > > Why changing this file? I would keep it as is, and copy it if you want to
> run
> > > the almost same code?
> > 
> > Part of this change is to enforce the initialization of static fields.  You
> > cannot initialize a static final with a non-const value.  This code was
using
> > the implicit constructor which is not const.  I'm pretty sure this file was
> > intended to be valid dart code, so I updated it.
> 
> I'm not sure I follow. This file looks like valid Dart code before your
changes.
> I don't see why you need to change the constructor to 'const' to make it
valid?

It seems to me that if you add together all the places in the spec where it
talks about compile-time constant in initializers, you can only initialize a
variable with a compile-time constant, whether or not it is static or final or
non-static:

Section 7.4 instance variables
...
If an instance variable declaration has one of the forms T v = e;, var v = e;,
final T v = e; or final nal v = e; then the expression e must be a
compile-time constant (10.1).


Section 5, (Variables)
...
A variable that is both static and final must be initialized to a compile-time
constant (10.1) or a compile-time error occurs.
Why tie together two orthogonal concepts like static and nal by requiring the
use of constants? Because we do not want a language where expensive
initialization computations are dened, causing long application startup times.
This is especially crucial for Dart, which is designed for coding client
applications.  One time initializations using constants should incur negligible
cost at runtime

I have code right now that pulls in the constructor being referenced and sees if
it is marked as const.  

Question:  is it legal to have both a constant and non-constant constructor in a
class?  I would think that you could only have one and it would have to be a
factory method.

class A {
  var x;
  const A(this.x);
  A(this.x) { ... do something ...}
}

http://codereview.chromium.org/8231031/diff/11001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart:8:
const A() : this.x = 1;
On 2011/10/14 14:36:58, floitsch wrote:
> On 2011/10/14 10:33:42, ngeoffray wrote:
> > On 2011/10/14 10:04:21, zundel wrote:
> > > On 2011/10/14 09:26:56, ngeoffray wrote:
> > > > Why changing this file? I would keep it as is, and copy it if you want
to
> > run
> > > > the almost same code?
> > > 
> > > Part of this change is to enforce the initialization of static fields. 
You
> > > cannot initialize a static final with a non-const value.  This code was
> using
> > > the implicit constructor which is not const.  I'm pretty sure this file
was
> > > intended to be valid dart code, so I updated it.
> > 
> > I'm not sure I follow. This file looks like valid Dart code before your
> changes.
> > I don't see why you need to change the constructor to 'const' to make it
> valid?
> 
> Maybe because the "static final A a = new A() (below) is invalid. Please make
> sure that the checker catches this case.

Done.

http://codereview.chromium.org/8231031/diff/11001/samples/clock/ClockNumbers....
File samples/clock/ClockNumbers.dart (right):

http://codereview.chromium.org/8231031/diff/11001/samples/clock/ClockNumbers....
samples/clock/ClockNumbers.dart:6: static final PIXELS = const [
Having to make this change just makes me want to cry.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/CTConst2...
File tests/language/src/CTConst2Test.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/CTConst2...
tests/language/src/CTConst2Test.dart:100: static final a = new A(); //
Assignment from Constant constructor OK
On 2011/10/14 10:33:42, ngeoffray wrote:
> On 2011/10/14 10:04:21, zundel wrote:
> > On 2011/10/14 09:26:56, ngeoffray wrote:
> > > It should be a const A(); then.
> > 
> > I don't understand this comment (const A(); is already there as a
> constructor).
> 
> The code you wrote is not allowed: 'a' should be initialized with a constant
> expression. However, "new A()" is not a constant expression.

?  I thought it would be a constant expression since A() is a constructor with
the const qualifier which I thought meant that it can only return constant
values.

10.1 Constants
... A constant constructor invocation, see 10.10.2

10.10.2 says you have to invoke a constant constructor with:

const A();

I am confused by this.  What is it that const A(); does that new A(); does not? 
Is new A(); even valid or does it do the same thing without canonicalizing the
result?

Answer from side conversation:  const A() is evaluated at compile time and
parameters can only be compile time const expressions, new A() is evaluated at
runtime and may have non-compile-time-const expressions as parameters.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
File tests/language/src/StaticFinalFieldTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
tests/language/src/StaticFinalFieldTest.dart:32: // Compile time constants don't
allow string interpolation.
On 2011/10/14 10:33:42, ngeoffray wrote:
> On 2011/10/14 10:04:21, zundel wrote:
> > On 2011/10/14 09:26:56, ngeoffray wrote:
> > > I have CC'ed Matthias to make sure he's aware of that part of the spec
> (which
> > is
> > > limiting). If we all agree, please move the string interpolation case in a
> > > negative test.
> > 
> > For the negative test, see tringInterpolate7NegativeTest.dart
> > 
> > For the String interpolation exclusion in the spec, see section 10.1.  I
asked
> > for it to be changed to allow compile time constant expressions, but never
got
> a
> > response.
> 
> Apologies for the "never got a response". Gilad is next to me right now, so
it's
> easy to get one :) So the spec is limiting right now, but we may extend it
once
> it gets deeply needed. It hasn't yet because it's not high-priority.

I am going ahead and allowing them since the VM does and I've been given a
glimmer of hope that we won't remain this restrictive forever.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:40:
public class CompileTimeConstVisitor extends DartNodeTraverser<Void> {
changed to DartNodeTraverser.  Discussed with Jason - return values aren't
needed so I parameterized it with Void.

[hausner, 2011-10-14 22:20:24]

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
File tests/language/src/StaticFinalFieldTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
tests/language/src/StaticFinalFieldTest.dart:32: // Compile time constants don't
allow string interpolation.
I was told that string interpolation should not be part of const expressions
after I've implemented it. This is because the toString method is called as a
side effect of every interpolated value, which may have side effects. Someone
some day is going to (ab) use that.

So, I should actually take that functionality out of the VM.


On 2011/10/14 20:59:52, zundel wrote:
> On 2011/10/14 10:33:42, ngeoffray wrote:
> > On 2011/10/14 10:04:21, zundel wrote:
> > > On 2011/10/14 09:26:56, ngeoffray wrote:
> > > > I have CC'ed Matthias to make sure he's aware of that part of the spec
> > (which
> > > is
> > > > limiting). If we all agree, please move the string interpolation case in
a
> > > > negative test.
> > > 
> > > For the negative test, see tringInterpolate7NegativeTest.dart
> > > 
> > > For the String interpolation exclusion in the spec, see section 10.1.  I
> asked
> > > for it to be changed to allow compile time constant expressions, but never
> got
> > a
> > > response.
> > 
> > Apologies for the "never got a response". Gilad is next to me right now, so
> it's
> > easy to get one :) So the spec is limiting right now, but we may extend it
> once
> > it gets deeply needed. It hasn't yet because it's not high-priority.
> 
> I am going ahead and allowing them since the VM does and I've been given a
> glimmer of hope that we won't remain this restrictive forever.

[floitsch, 2011-10-14 22:42:08]

On 2011/10/14 20:59:51, zundel wrote:
> Question:  is it legal to have both a constant and non-constant constructor in
a
> class?  I would think that you could only have one and it would have to be a
> factory method.
Const and non-const constructors are in the same name-space. So it is not
allowed to have a const and non-const constructor of the same name. (We
discussed it, but decided against it).
However it is legal to have construct-constructors and differently named
non-const constructors.

Ex:
class A {
  const A();
  A.named() { with body. }
}
http://codereview.chromium.org/8231031/diff/11001/samples/clock/ClockNumbers....
> samples/clock/ClockNumbers.dart:6: static final PIXELS = const [
> Having to make this change just makes me want to cry.
I agree. We will see if we can fix this at some point.

>
http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StaticFi...
> tests/language/src/StaticFinalFieldTest.dart:32: // Compile time constants
don't
> allow string interpolation.
> On 2011/10/14 10:33:42, ngeoffray wrote:
> > On 2011/10/14 10:04:21, zundel wrote:
> > > On 2011/10/14 09:26:56, ngeoffray wrote:
> > > > I have CC'ed Matthias to make sure he's aware of that part of the spec
> > (which
> > > is
> > > > limiting). If we all agree, please move the string interpolation case in
a
> > > > negative test.
> > > 
> > > For the negative test, see tringInterpolate7NegativeTest.dart
> > > 
> > > For the String interpolation exclusion in the spec, see section 10.1.  I
> asked
> > > for it to be changed to allow compile time constant expressions, but never
> got
> > a
> > > response.
> > 
> > Apologies for the "never got a response". Gilad is next to me right now, so
> it's
> > easy to get one :) So the spec is limiting right now, but we may extend it
> once
> > it gets deeply needed. It hasn't yet because it's not high-priority.
> 
> I am going ahead and allowing them since the VM does and I've been given a
> glimmer of hope that we won't remain this restrictive forever.
As Matthias pointed out: string-interpolation calls 'toString', so it is a
dangerous operation. Let's stick with the spec first. We can later allow more.

[floitsch, 2011-10-14 22:46:15]

Much better. Thanks.
There is still the bug that fields must be written in the right order...
If you find an easy work-around please update and we will take another look.
If it looks difficult, get this code in first (i.e. LGTM).

In any case add a negative test with variables in the wrong order.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StringIn...
File tests/language/src/StringInterpolate7NegativeTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StringIn...
tests/language/src/StringInterpolate7NegativeTest.dart:8: static final String
msg = "Hello $A"; // not allowed in compile time constant
80 chars.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:202:
if (!x.getSymbol().getModifiers().isConstant()) {
from an error-reporting point of view it might be better to check the symbol
first.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:252:
if (!x.isConst()) {
from an error-reporting point of view it might be better to check isConst before
visiting the children.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:304:
if (!x.isConst()) {
again. maybe do this check first for the error message.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:321:
if (!x.isConst()) {
ditto.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:358:
private Type getMostSpecificType(DartExpression expr) {
I'm not sure I understand this.
Say we have
static final y = x + 5;
static final x = "foo";

If I see this correctly the binary expression will call getMostSpecificType. But
what does this return for "x"?
I don't see any recursive visit, so how can you know the type of x?

If I see this correctly the binary expression will c

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:367: // Now, this
constant has a type. Save it for future reference
end sentence with dot.

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart:14: final
int globalVar = 42;
This should not be necessary.

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.change.dart
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.change.dart:6:
// The existence of this final triggers b/5078969.
I'm not on VPN, so I can't verify, but is this comment still valid?

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java:1:
// Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
Please add tests where the variables are in the wrong order.

http://codereview.chromium.org/8231031/diff/16001/tests/language/src/ConstIni...
File tests/language/src/ConstInitTest.dart (right):

http://codereview.chromium.org/8231031/diff/16001/tests/language/src/ConstIni...
tests/language/src/ConstInitTest.dart:45: // TODO(zundel): Was it intentional to
forward reference O, N?
It is allowed.
The compiler needs to be fixed.

http://codereview.chromium.org/8231031/diff/16001/tests/language/src/StringIn...
File tests/language/src/StringInterpolate2Test.dart (right):

http://codereview.chromium.org/8231031/diff/16001/tests/language/src/StringIn...
tests/language/src/StringInterpolate2Test.dart:31: Expect.equals("8",
'${fib.length}');
why simplify this test?

[John Lenz, 2011-10-15 00:55:56]

http://codereview.chromium.org/8231031/diff/16001/tests/language/language.status
File tests/language/language.status (right):

http://codereview.chromium.org/8231031/diff/16001/tests/language/language.sta...
tests/language/language.status:40: StringInterpolationNegativeTest: Fail # Bug
???
Are you filing a bug for this?

[zundel, 2011-10-17 02:56:02]

Incorporated feedback.  I'd like to defer the recursive evaluation of compile
time constants for looking ahead.  I think there is a tricky bit around making
sure we don't get stuck in a loop that was referred to.

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StringIn...
File tests/language/src/StringInterpolate7NegativeTest.dart (right):

http://codereview.chromium.org/8231031/diff/11001/tests/language/src/StringIn...
tests/language/src/StringInterpolate7NegativeTest.dart:8: static final String
msg = "Hello $A"; // not allowed in compile time constant
On 2011/10/14 22:46:15, floitsch wrote:
> 80 chars.

Done.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:304:
if (!x.isConst()) {
On 2011/10/14 22:46:15, floitsch wrote:
> again. maybe do this check first for the error message.

Done.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:321:
if (!x.isConst()) {
On 2011/10/14 22:46:15, floitsch wrote:
> ditto.

Done.

http://codereview.chromium.org/8231031/diff/16001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:358:
private Type getMostSpecificType(DartExpression expr) {
On 2011/10/14 22:46:15, floitsch wrote:
> I'm not sure I understand this.
> Say we have
> static final y = x + 5;
> static final x = "foo";
> 
> If I see this correctly the binary expression will call getMostSpecificType.
But
> what does this return for "x"?
> I don't see any recursive visit, so how can you know the type of x?

This part is currently broken as the CTConstTest.dart shows

No type analysis has been done yet - this is the visitor doing its best to
figure out the type without it.  I'd prefer to address this in a followon CL -
Added a TODO.
> 
> If I see this correctly the binary expression will c

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
File compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother0.dart:14: final
int globalVar = 42;
On 2011/10/14 22:46:15, floitsch wrote:
> This should not be necessary.

Done.

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.change.dart
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/end2end/inc/myother1.change.dart:6:
// The existence of this final triggers b/5078969.
On 2011/10/14 22:46:15, floitsch wrote:
> I'm not on VPN, so I can't verify, but is this comment still valid?

That bug was fixed on Aug. 3, so no, it isn't valid.

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java
(right):

http://codereview.chromium.org/8231031/diff/16001/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java:1:
// Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
On 2011/10/14 22:46:15, floitsch wrote:
> Please add tests where the variables are in the wrong order.
I added a disabled test with a TODO

http://codereview.chromium.org/8231031/diff/16001/tests/language/language.status
File tests/language/language.status (right):

http://codereview.chromium.org/8231031/diff/16001/tests/language/language.sta...
tests/language/language.status:40: StringInterpolationNegativeTest: Fail # Bug
???
On 2011/10/15 00:55:56, John Lenz wrote:
> Are you filing a bug for this?
I thought you were trying to clean out bugs, not make new ones! Seriously, I
created one this afternoon and must not have re-uploaded the patch.  Bug5463856

http://codereview.chromium.org/8231031/diff/16001/tests/language/src/StringIn...
File tests/language/src/StringInterpolate2Test.dart (right):

http://codereview.chromium.org/8231031/diff/16001/tests/language/src/StringIn...
tests/language/src/StringInterpolate2Test.dart:31: Expect.equals("8",
'${fib.length}');
On 2011/10/14 22:46:15, floitsch wrote:
> why simplify this test?

sorry, I thought it was a typo.  It changed from double quotes to single quotes,
I thought that's what it was testing.

[floitsch, 2011-10-17 07:28:34]

http://codereview.chromium.org/8231031/diff/16001/tests/language/src/StringIn...
File tests/language/src/StringInterpolate2Test.dart (right):

http://codereview.chromium.org/8231031/diff/16001/tests/language/src/StringIn...
tests/language/src/StringInterpolate2Test.dart:31: Expect.equals("8",
'${fib.length}');
On 2011/10/17 02:56:02, zundel wrote:
> On 2011/10/14 22:46:15, floitsch wrote:
> > why simplify this test?
> 
> sorry, I thought it was a typo.  It changed from double quotes to single
quotes,
> I thought that's what it was testing.

could be, that it was a typo. But since the line-break is testing more, I prefer
to keep it.

[zundel, 2011-10-17 10:43:49]

http://codereview.chromium.org/8231031/diff/28002/tests/language/src/StringIn...
File tests/language/src/StringInterpolate2Test.dart (right):

http://codereview.chromium.org/8231031/diff/28002/tests/language/src/StringIn...
tests/language/src/StringInterpolate2Test.dart:35: length}');
(here's what I did with the test in question)

[floitsch, 2011-10-17 10:50:39]

Please move failing tests into a separate file and mark them in the status-file
instead.
Otherwise LGTM.

[ngeoffray, 2011-10-17 10:59:38]

LGTM! modulo the modifiers changes. Code looks good, and hopefully the tests
cover most cases.

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(right):

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:179:
x.visitChildren(this);
Do you really need to visit children here?

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:284:
case PARAMETER:
It looks like this will say a library prefix or a class are constant. Since they
have to be a DartPropertyAccess, I think that if you remove the visitChildren in
DartPropertyAccess, you can safely do expectConstant for things other than
FIELD, CONSTRUCTOR, VARIABLE or PARAMETER. Or an AssertionError?

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java (right):

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:233: }
Since all initializations must be constant, do you still need to make these
changes on the modifiers?

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:361:
modifiers.makeStatic();
This is weird. You should not change the modifiers really, especially with a
modifier that depends on the source code. Does that actually work with the Dart
pretty printer?

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/TopLevelElementBuilder.java
(right):

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/TopLevelElementBuilder.java:156:
// final toplevel fields are implicitly compile-time constants.
Again, since they must all be compile-time constants, I think you can drop that.

http://codereview.chromium.org/8231031/diff/28002/compiler/javatests/com/goog...
File
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart
(right):

http://codereview.chromium.org/8231031/diff/28002/compiler/javatests/com/goog...
compiler/javatests/com/google/dart/compiler/backend/js/testFieldAccessExprOpt.dart:10:
static final A a = const A();
Any reason for reordering the lines?

http://codereview.chromium.org/8231031/diff/28002/tests/language/src/ConstIni...
File tests/language/src/ConstInitTest.dart (right):

http://codereview.chromium.org/8231031/diff/28002/tests/language/src/ConstIni...
tests/language/src/ConstInitTest.dart:45: // TODO(zundel): Was it intentional to
forward reference O, N?
Does it matter?

http://codereview.chromium.org/8231031/diff/28002/tests/language/src/ConstIni...
tests/language/src/ConstInitTest.dart:46: // Compiler doesn't recognize the type
of O, N as int in that case.
Maybe file a bug to fix that in dartc?

[zundel, 2011-10-17 14:14:26]

Thanks for the review, committed as r468

I'll start looking at the problems with field ordering now.

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(right):

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:179:
x.visitChildren(this);
On 2011/10/17 10:59:38, ngeoffray wrote:
> Do you really need to visit children here?

a qualifier is a part of a PropertyAccess and is typed a  DartExpression.  I
assume it could be a constant or non-constant expression.

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:284:
case PARAMETER:
On 2011/10/17 10:59:38, ngeoffray wrote:
> It looks like this will say a library prefix or a class are constant. Since
they
> have to be a DartPropertyAccess, I think that if you remove the visitChildren
in
> DartPropertyAccess, you can safely do expectConstant for things other than
> FIELD, CONSTRUCTOR, VARIABLE or PARAMETER. Or an AssertionError?

Added kind CLASS and an ICE for the cases not explicitly spelled out.  This is
the place where I need to hook in some recursive check for a compile time
constant, so I added a NONE case and left it commented out.

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java (right):

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:233: }
On 2011/10/17 10:59:38, ngeoffray wrote:
> Since all initializations must be constant, do you still need to make these
> changes on the modifiers?

This is just for help in evaluating whether an expression is constant.  If the
expression is a const contructor, it will be set to isConstant().  If it is a
field or a variable, it will be set to isFinal().  It just makes it easier to
check isConstant() on the element in CompileTimeConstant Visitor without picking
out what kind of element it is.

http://codereview.chromium.org/8231031/diff/28002/tests/language/src/ConstIni...
File tests/language/src/ConstInitTest.dart (right):

http://codereview.chromium.org/8231031/diff/28002/tests/language/src/ConstIni...
tests/language/src/ConstInitTest.dart:45: // TODO(zundel): Was it intentional to
forward reference O, N?
On 2011/10/17 10:59:38, ngeoffray wrote:
> Does it matter?

As per Florian's request, moved to a separate test CTConst3Test.dart

http://codereview.chromium.org/8231031/diff/28002/tests/language/src/ConstIni...
tests/language/src/ConstInitTest.dart:46: // Compiler doesn't recognize the type
of O, N as int in that case.
On 2011/10/17 10:59:38, ngeoffray wrote:
> Maybe file a bug to fix that in dartc?

Codesite issue 120

[mmendez, 2011-11-16 21:03:10]

I came across the following issues while reviewing the latest compile-time
constant patch, http://codereview.chromium.org/8523034.

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/DartExpression.java (right):

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartExpression.java:13: Type type;
Nit: Should this be private?

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartExpression.java:25: @Override
Nit: Since you added [set|get]Type here you might want to remove them from the
DartLiteral node subtype.

[zundel, 2011-11-16 21:38:24]

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/DartExpression.java (right):

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartExpression.java:13: Type type;
On 2011/11/16 21:03:10, mmendez wrote:
> Nit: Should this be private?

Removed - see http://codereview.chromium.org/8523034

http://codereview.chromium.org/8231031/diff/28002/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartExpression.java:25: @Override
On 2011/11/16 21:03:10, mmendez wrote:
> Nit: Since you added [set|get]Type here you might want to remove them from the
> DartLiteral node subtype.

Removed - see http://codereview.chromium.org/8523034