Evaluate constant instance creation expressions in ConstantValueComputer.

Evaluate constant instance creation expressions in ConstantValueComputer.

Previously, we evaluated them in ConstantVisitor; as a result we
couldn't take advantage of the ConstantValueComputer's depedendency
graph, so we couldn't detect cycles involving constant constructor
declarations.  This will become important soon, when we expand our
constant evaluation logic to traverse constructor field initializers.

With this change, ConstantValueComputer stores the computed value of
each constant instance creation expression in the AST (similar to how
it stores the computed value of each constant variable in the element
model), and ConstantVisitor simply looks up the already-computed
value.

R=brianwilkerson@google.com

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

[Brian Wilkerson, 2014-05-08 22:12:48]

LGTM

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantFinder.java
(right):

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantFinder.java:46:
private HashSet<InstanceCreationExpression> constructorInvocations = new
HashSet<InstanceCreationExpression>();
I don't think there's any reason for this to be a set, because we're already
guaranteed to never be adding the same node twice. Not making it a set would
probably be a performance win, in part because it would be easier to replace
some for-each loops with regular for loops, which are faster in both Java and
Dart.

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantValueComputer.java
(right):

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantValueComputer.java:226:
ArrayList<AnalysisError> errors = new ArrayList<AnalysisError>();
Am I missing something? It looks like we're populating the list and then not
doing anything with it.

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantValueComputer.java:229:
Source source = element.getAncestor(CompilationUnitElement.class).getSource();
Can the source be computed outside the loop?

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/constant/ConstantValueComputerTest.java
(right):

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/constant/ConstantValueComputerTest.java:211:
// x depends on "const A()"
I don't see how we're testing that the dependencies are being computed correctly
(in this and several other tests).

[Paul Berry, 2014-05-08 23:12:45]

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantFinder.java
(right):

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantFinder.java:46:
private HashSet<InstanceCreationExpression> constructorInvocations = new
HashSet<InstanceCreationExpression>();
On 2014/05/08 22:12:49, Brian Wilkerson wrote:
> I don't think there's any reason for this to be a set, because we're already
> guaranteed to never be adding the same node twice. Not making it a set would
> probably be a performance win, in part because it would be easier to replace
> some for-each loops with regular for loops, which are faster in both Java and
> Dart.

Good point.  I've changed it to an ArrayList.

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantValueComputer.java
(right):

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantValueComputer.java:226:
ArrayList<AnalysisError> errors = new ArrayList<AnalysisError>();
On 2014/05/08 22:12:49, Brian Wilkerson wrote:
> Am I missing something? It looks like we're populating the list and then not
> doing anything with it.

Wow, you're right.  It looks like the entire if-block has been effectively a NOP
ever since it was introduced all the way back in r20451.  I've removed it.

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/constant/ConstantValueComputerTest.java
(right):

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/constant/ConstantValueComputerTest.java:211:
// x depends on "const A()"
On 2014/05/08 22:12:49, Brian Wilkerson wrote:
> I don't see how we're testing that the dependencies are being computed
correctly
> (in this and several other tests).

It's by the same mechanism that I introduced in r35935: assertProperDependencies
uses the ValidatingConstantValueComputer class, which instruments
ConstantValueComputer and ConstantVisitor to add the following actions:

- When ConstantValueComputer finishes building the dependency graph, it takes a
snapshot of it.

- When ConstantValueComputer asks ConstantVisitor to compute the value of a
constant, it makes a note of which node is being evaluated (this should
correspond to a "head" in the dependency graph).

- When ConstantValueComputer uses getEvaluationResult() to get the value of a
previously computed constant (a "tail" in the dependency graph), it double
checks that the dependency graph included an arc from head to tail.

So, rather than explicitly hand-coding the expected dependency graph (which
would be error prone), what we're doing is verifying that the dependency graph
is consistent with the true dependencies that are revealed by watching the
behavior of ConstantVisitor.

[Brian Wilkerson, 2014-05-08 23:53:40]

LGTM

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/constant/ConstantValueComputerTest.java
(right):

https://codereview.chromium.org/270813004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/constant/ConstantValueComputerTest.java:211:
// x depends on "const A()"
I can live with that; the trade-off is that if ConstantVisitor doesn't work
correctly we'll miss something. But as long as ConstantVisitor is tested
thoroughly, we should be fine.

[Paul Berry, 2014-05-09 12:51:41]

Committed patchset #1 manually as r35969 (presubmit successful).