Compile Time Constants cycle check

Compile Time Constants cycle check

http://code.google.com/p/dart/issues/detail?id=120

This adds a check to compile time constants to make sure there
are no cycles.  This turned out to be a bit more difficult than
I had hoped, due to interaction with incremenatal compile, when
only a diet parse tree was available for modules that did not
need a full compile. To fix this, I added a lite resolution step
(CompileTimeConstantResolver) to be performed on all units.

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

[zundel, 2011-11-16 19:25:01]

http://codereview.chromium.org/8523034/diff/20/compiler/java/com/google/dart/...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java
(left):

http://codereview.chromium.org/8523034/diff/20/compiler/java/com/google/dart/...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstVisitor.java:39:
public class CompileTimeConstVisitor extends DartNodeTraverser<Void> {
Renamed to CompileTimeConstVisitor.

http://codereview.chromium.org/8523034/diff/20/compiler/java/com/google/dart/...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java
(right):

http://codereview.chromium.org/8523034/diff/20/compiler/java/com/google/dart/...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:146:
Type type = inferredTypes.get(node);
First, consult the local hash table.

http://codereview.chromium.org/8523034/diff/20/compiler/java/com/google/dart/...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:180:
private void setType(DartNode x, Type type) {
This method used to set the type on the node, but now it only sets it in a local
hash table to avoid interfering with the Dart type analyzer.

http://codereview.chromium.org/8523034/diff/20/compiler/java/com/google/dart/...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (left):

http://codereview.chromium.org/8523034/diff/20/compiler/java/com/google/dart/...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:367:
checkConstantExpression(expression);
these checks were moved to CompileTimeConstantAnalyzer.java

http://codereview.chromium.org/8523034/diff/20/compiler/javatests/com/google/...
File
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java
(left):

http://codereview.chromium.org/8523034/diff/20/compiler/javatests/com/google/...
compiler/javatests/com/google/dart/compiler/resolver/CompileTimeConstantTest.java:14:
// TODO(zundel) This test should pass, but the compiler doesn't currently
This looks like a lot of churn, but all that really happened was that I chopped
these tests into smaller test cases.

[srdjan, 2011-11-16 22:03:11]

DBC

http://codereview.chromium.org/8523034/diff/8001/tests/language/src/ConstInit...
File tests/language/src/ConstInit5NegativeTest.dart (right):

http://codereview.chromium.org/8523034/diff/8001/tests/language/src/ConstInit...
tests/language/src/ConstInit5NegativeTest.dart:10: int main() {
You must invoke C getter to guarantee a compile time error.

[zundel, 2011-11-17 14:56:45]

http://codereview.chromium.org/8523034/diff/8001/tests/language/src/ConstInit...
File tests/language/src/ConstInit5NegativeTest.dart (right):

http://codereview.chromium.org/8523034/diff/8001/tests/language/src/ConstInit...
tests/language/src/ConstInit5NegativeTest.dart:10: int main() {
On 2011/11/16 22:03:11, srdjan wrote:
> You must invoke C getter to guarantee a compile time error.

Done.

[mmendez, 2011-11-17 15:32:10]

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/resolver/ClassElementImplementation.java
(right):

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/ClassElementImplementation.java:24:
public class ClassElementImplementation extends AbstractElement implements
ClassElement {
I don't think that this needs to be public.  Or was this part of the previous
version of this patch?

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java
(right):

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:54:
* Given an expression, Determines if the expression matches all the rules for a
Nit: Determines -> determines

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:62:
public class CompileTimeConstantAnalyzer extends DartNodeTraverser<Void> {
Nit: not sure if you meant for this to be a visitor itself or whether the
visitor should be a nested class.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:151:
switch (ElementKind.of(element)) {
Can't you have the FIELD, METHOD, VARIABLE and CONSTRUCTOR cases share the same
code (type = element.getType()).

I'm not sure about the ICE since it is a larger issue and every author on the
compiler has done things slightly differently.  Would it be better to just
return dynamic and let a compilation error be generated?

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:170:
throw new InternalCompilerException("Unexpected element "
I think that this will ICE if you access a constant from a library and them
import uses a prefix.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:175:
return node.getType();
Could you add a comment explaining under which circumstances this happens?

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:180:
private void setType(DartNode x, Type type) {
Rename to cache type for node?  It doesn't actually set the type of the node.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:269:
public Void visitBooleanLiteral(DartBooleanLiteral x) {
Nit: Not part of your patch, but I would have a expected the literal to have the
boolType set on it already.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:339:
switch (ElementKind.of(element)) {
Won't this reset the value set on line 332?  Maybe add a comment explaining why
this is necessary?

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java
(right):

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java:31:
* type analysis and cycle detection done in {@link CompileTimeConstantAnalysis}.
CompileTimeConstantAnalysis -> CompileTimeConstantAnalyzer?

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java:94:
private boolean isStatic = true;
Nit: should this be a static final or simply inline the constant into
isStaticContext?

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java
(right):

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:34:
CIRCULAR_REFERENCE("Circular reference"),
Nit: Maybe reword to explain that this is due to the restrictions on consts? 
"Compile time constants cannot depend on themselves" or something that clues
them to the compile-time const restrictions.

[zundel, 2011-11-17 16:26:42]

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/resolver/ClassElementImplementation.java
(right):

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/ClassElementImplementation.java:24:
public class ClassElementImplementation extends AbstractElement implements
ClassElement {
On 2011/11/17 15:32:10, mmendez wrote:
> I don't think that this needs to be public.  Or was this part of the previous
> version of this patch?
I had at one point tried to access it outside the package but throught better of
it.  Reverted.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java
(right):

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:62:
public class CompileTimeConstantAnalyzer extends DartNodeTraverser<Void> {
On 2011/11/17 15:32:10, mmendez wrote:
> Nit: not sure if you meant for this to be a visitor itself or whether the
> visitor should be a nested class.

Created a nested class.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:151:
switch (ElementKind.of(element)) {
On 2011/11/17 15:32:10, mmendez wrote:
> Can't you have the FIELD, METHOD, VARIABLE and CONSTRUCTOR cases share the
same
> code (type = element.getType()).
> 
> I'm not sure about the ICE since it is a larger issue and every author on the
> compiler has done things slightly differently.  Would it be better to just
> return dynamic and let a compilation error be generated?

Done.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:170:
throw new InternalCompilerException("Unexpected element "
On 2011/11/17 15:32:10, mmendez wrote:
> I think that this will ICE if you access a constant from a library and them
> import uses a prefix.

I don't think so. that happens all the time in the Dart core library.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:175:
return node.getType();
On 2011/11/17 15:32:10, mmendez wrote:
> Could you add a comment explaining under which circumstances this happens?

It was happening when diet parsed files didn't have symbols resolved in compile
time constant expressions.  Since that is fixed, I can greatly simplify.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:180:
private void setType(DartNode x, Type type) {
On 2011/11/17 15:32:10, mmendez wrote:
> Rename to cache type for node?  It doesn't actually set the type of the node.
renamed

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:269:
public Void visitBooleanLiteral(DartBooleanLiteral x) {
On 2011/11/17 15:32:10, mmendez wrote:
> Nit: Not part of your patch, but I would have a expected the literal to have
the
> boolType set on it already.

Doesn't get set until Resolver is run.  That is never run on diet parse files.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:339:
switch (ElementKind.of(element)) {
On 2011/11/17 15:32:10, mmendez wrote:
> Won't this reset the value set on line 332?  Maybe add a comment explaining
why
> this is necessary?

There is a tree walk in the middle.  The type was set beforehand in case the
current symbol was null BUT that shouldnt happen now that I've added the
CompileTimeConstantResolver, so we can get rid of it.

Afterward, we want to set the value of this identifier to any assigment we found
for the symbol while traversing the expression.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java
(right):

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java:31:
* type analysis and cycle detection done in {@link CompileTimeConstantAnalysis}.
On 2011/11/17 15:32:10, mmendez wrote:
> CompileTimeConstantAnalysis -> CompileTimeConstantAnalyzer?

Done.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java:94:
private boolean isStatic = true;
On 2011/11/17 15:32:10, mmendez wrote:
> Nit: should this be a static final or simply inline the constant into
> isStaticContext?

Done.

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java
(right):

http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:34:
CIRCULAR_REFERENCE("Circular reference"),
On 2011/11/17 15:32:10, mmendez wrote:
> Nit: Maybe reword to explain that this is due to the restrictions on consts? 
> "Compile time constants cannot depend on themselves" or something that clues
> them to the compile-time const restrictions.

Done.

[mmendez, 2011-11-17 18:18:22]

I think that this is looking good, but I see and ICE when I try the following
code:

// File - dart.dart
#import("mylib.dart", prefix:"mylib");

var A = mylib.B;

main() {
}

// File - mylib.dart
#library("mylib");

var B = 1;

On 2011/11/17 16:26:42, zundel wrote:
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
> File
>
compiler/java/com/google/dart/compiler/resolver/ClassElementImplementation.java
> (right):
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/ClassElementImplementation.java:24:
> public class ClassElementImplementation extends AbstractElement implements
> ClassElement {
> On 2011/11/17 15:32:10, mmendez wrote:
> > I don't think that this needs to be public.  Or was this part of the
previous
> > version of this patch?
> I had at one point tried to access it outside the package but throught better
of
> it.  Reverted.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
> File
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java
> (right):
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:62:
> public class CompileTimeConstantAnalyzer extends DartNodeTraverser<Void> {
> On 2011/11/17 15:32:10, mmendez wrote:
> > Nit: not sure if you meant for this to be a visitor itself or whether the
> > visitor should be a nested class.
> 
> Created a nested class.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:151:
> switch (ElementKind.of(element)) {
> On 2011/11/17 15:32:10, mmendez wrote:
> > Can't you have the FIELD, METHOD, VARIABLE and CONSTRUCTOR cases share the
> same
> > code (type = element.getType()).
> > 
> > I'm not sure about the ICE since it is a larger issue and every author on
the
> > compiler has done things slightly differently.  Would it be better to just
> > return dynamic and let a compilation error be generated?
> 
> Done.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:170:
> throw new InternalCompilerException("Unexpected element "
> On 2011/11/17 15:32:10, mmendez wrote:
> > I think that this will ICE if you access a constant from a library and them
> > import uses a prefix.
> 
> I don't think so. that happens all the time in the Dart core library.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:175:
> return node.getType();
> On 2011/11/17 15:32:10, mmendez wrote:
> > Could you add a comment explaining under which circumstances this happens?
> 
> It was happening when diet parsed files didn't have symbols resolved in
compile
> time constant expressions.  Since that is fixed, I can greatly simplify.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:180:
> private void setType(DartNode x, Type type) {
> On 2011/11/17 15:32:10, mmendez wrote:
> > Rename to cache type for node?  It doesn't actually set the type of the
node.
> renamed
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:269:
> public Void visitBooleanLiteral(DartBooleanLiteral x) {
> On 2011/11/17 15:32:10, mmendez wrote:
> > Nit: Not part of your patch, but I would have a expected the literal to have
> the
> > boolType set on it already.
> 
> Doesn't get set until Resolver is run.  That is never run on diet parse files.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantAnalyzer.java:339:
> switch (ElementKind.of(element)) {
> On 2011/11/17 15:32:10, mmendez wrote:
> > Won't this reset the value set on line 332?  Maybe add a comment explaining
> why
> > this is necessary?
> 
> There is a tree walk in the middle.  The type was set beforehand in case the
> current symbol was null BUT that shouldnt happen now that I've added the
> CompileTimeConstantResolver, so we can get rid of it.
> 
> Afterward, we want to set the value of this identifier to any assigment we
found
> for the symbol while traversing the expression.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
> File
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java
> (right):
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java:31:
> * type analysis and cycle detection done in {@link
CompileTimeConstantAnalysis}.
> On 2011/11/17 15:32:10, mmendez wrote:
> > CompileTimeConstantAnalysis -> CompileTimeConstantAnalyzer?
> 
> Done.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
>
compiler/java/com/google/dart/compiler/resolver/CompileTimeConstantResolver.java:94:
> private boolean isStatic = true;
> On 2011/11/17 15:32:10, mmendez wrote:
> > Nit: should this be a static final or simply inline the constant into
> > isStaticContext?
> 
> Done.
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
> File compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java
> (right):
> 
>
http://codereview.chromium.org/8523034/diff/6001/compiler/java/com/google/dar...
> compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:34:
> CIRCULAR_REFERENCE("Circular reference"),
> On 2011/11/17 15:32:10, mmendez wrote:
> > Nit: Maybe reword to explain that this is due to the restrictions on consts?

> > "Compile time constants cannot depend on themselves" or something that clues
> > them to the compile-time const restrictions.
> 
> Done.

[zundel, 2011-11-17 18:55:01]

Updated patch with new test.

[mmendez, 2011-11-17 19:34:30]

On 2011/11/17 18:55:01, zundel wrote:
> Updated patch with new test.

LGTM

It would be good to confirm that the following code should or should not
generate a type warning:

final A = "Hello";
final int B = A;

I assume not since A is dynamic, but it could not hurt.