Implement new constant expressions

Implement new constant expressions

R=paulberry@google.com

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

[Brian Wilkerson, 2014-09-17 20:48:43]

brianwilkerson@google.com changed reviewers:
+ paulberry@google.com

[Brian Wilkerson, 2014-09-17 20:48:44]

Two new expressions were/are being added to the language spec: concatenation of
two strings using the "+" operator and getting the length of a constant string.

[Paul Berry, 2014-09-18 00:05:07]

Are there shared tests for computing the length of a constant string?  If not,
we should add some.

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

https://codereview.chromium.org/581773002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantVisitor.java:401:
if (accessor.getName().equals("length")) {
I'm confused.  It seems to me that the case we need to handle is something like
this (where we're evaluating x):

  const String s = "foo";
  const int x = s.length;

In which case, when we get here, prefixNode, prefixElement, and accessor refer
to "s", not "length".  So it seems like the thing we should be comparing to
"length" is node.getIdentifier().

https://codereview.chromium.org/581773002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantVisitor.java:403:
if (parent instanceof ClassElement && ((ClassElement)
parent).getName().equals("String")) {
It's not sufficient to check whether the class's name is "String", because it
could be a class String declared elsewhere.

E.g. this is legal (albeit contrived) dart code:

  import 'dart:core' as core;
  class String {
    ...
  }

We need to check whether parent is the "String" class from dart:core.

https://codereview.chromium.org/581773002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantVisitor.java:408:
} else if (!(prefixElement instanceof PrefixElement)) {
I'm not convinced that "else" is correct here.  It seems like we need a brand
new "if" statement, so that if the above code fails to return for any reason,
we'll still do this "if" test.

https://codereview.chromium.org/581773002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantVisitor.java:444:
if (parent instanceof ClassElement && ((ClassElement)
parent).getName().equals("String")) {
As above, it's not sufficient to check whether the class's name is "String".

https://codereview.chromium.org/581773002/diff/1/tests/language/language_anal...
File tests/language/language_analyzer.status (right):

https://codereview.chromium.org/581773002/diff/1/tests/language/language_anal...
tests/language/language_analyzer.status:174: compile_time_constant_c_test/02:
Fail, OK
IMHO, leaving the test wrong and marking it as "Fail, OK" defeats much of the
purpose of having shared tests.  The strategy I've had luck with in past CL's
is: fix the test to agree with analyzer's (now correct) behavior.  If dart2js
and/or the VM don't have correct behavior yet, then file bugs and update their
status files to document that they don't work properly yet.

[Brian Wilkerson, 2014-09-18 20:29:26]

PTAL

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

https://codereview.chromium.org/581773002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantVisitor.java:401:
if (accessor.getName().equals("length")) {
> I'm confused.

No, I was. Reworked.

https://codereview.chromium.org/581773002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantVisitor.java:403:
if (parent instanceof ClassElement && ((ClassElement)
parent).getName().equals("String")) {
Thanks! I forgot to come back to this.

https://codereview.chromium.org/581773002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantVisitor.java:408:
} else if (!(prefixElement instanceof PrefixElement)) {
I agree. Done.

https://codereview.chromium.org/581773002/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/constant/ConstantVisitor.java:444:
if (parent instanceof ClassElement && ((ClassElement)
parent).getName().equals("String")) {
Done.

[Paul Berry, 2014-09-18 20:39:25]

lgtm, thanks.  (I'm assuming you're planning to file issues and update the
.status files before committing)

https://codereview.chromium.org/581773002/diff/20001/tests/language/language....
File tests/language/language.status (right):

https://codereview.chromium.org/581773002/diff/20001/tests/language/language....
tests/language/language.status:63: compile_time_constant_c_test: Fail # Issue
Need an issue number

https://codereview.chromium.org/581773002/diff/20001/tests/language/language_...
File tests/language/language_analyzer.status (right):

https://codereview.chromium.org/581773002/diff/20001/tests/language/language_...
tests/language/language_analyzer.status:174: # compile_time_constant_c_test/02:
Fail, OK
Delete?

https://codereview.chromium.org/581773002/diff/20001/tests/language/language_...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/581773002/diff/20001/tests/language/language_...
tests/language/language_dart2js.status:6: compile_time_constant_c_test: Fail #
Issue
Need an issue number

[Brian Wilkerson, 2014-09-18 20:48:44]

> I'm assuming you're planning to file issues and update the
> .status files before committing

Yes. I just didn't want to file the issues until I knew which tests to mention.

https://codereview.chromium.org/581773002/diff/20001/tests/language/language....
File tests/language/language.status (right):

https://codereview.chromium.org/581773002/diff/20001/tests/language/language....
tests/language/language.status:63: compile_time_constant_c_test: Fail # Issue
Done.

https://codereview.chromium.org/581773002/diff/20001/tests/language/language_...
File tests/language/language_analyzer.status (right):

https://codereview.chromium.org/581773002/diff/20001/tests/language/language_...
tests/language/language_analyzer.status:174: # compile_time_constant_c_test/02:
Fail, OK
Done.

https://codereview.chromium.org/581773002/diff/20001/tests/language/language_...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/581773002/diff/20001/tests/language/language_...
tests/language/language_dart2js.status:6: compile_time_constant_c_test: Fail #
Issue
Done.

[Brian Wilkerson, 2014-09-18 20:50:45]

Committed patchset #2 (id:20001) manually as 40462 (presubmit successful).