Check const map literal keys and switch case exprs using type of constant.

Check const map literal keys and switch case exprs using type of constant.

The following type rules are defined based on the actual type of the
const object:

- CONST_MAP_KEY_EXPRESSION_TYPE_IMPLEMENTS_EQUALS
- INCONSISTENT_CASE_EXPRESSION_TYPES
- CASE_EXPRESSION_TYPE_IMPLEMENTS_EQUALS

Previously, we were using the propagated type for the case expression
checks, and the static type for the const map check.

BUG=dartbug.com/11697
R=brianwilkerson@google.com, scheglov@google.com

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

[Paul Berry, 2014-05-03 13:33:06]

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

https://codereview.chromium.org/267923004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:269:
// TODO(jwren) Revisit this algorithm, should there up to n-1 errors?
Jaime: is this still an open issue?

[Brian Wilkerson, 2014-05-03 15:03:34]

LGTM

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

https://codereview.chromium.org/267923004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:269:
// TODO(jwren) Revisit this algorithm, should there up to n-1 errors?
Yes, it's still an issue. I think what we really want to do, when there are
conflicting types, is to report the smallest number of errors (which we
currently don't). For example, given

  switch(...) {
    case 3: ...
    case "a": ...
    case "b": ...
    case "c": ...
  }

we will produce 3 errors (on all but the first case) when I think we should
produce one error (on the first case).

https://codereview.chromium.org/267923004/diff/1/tests/language/const_map3_te...
File tests/language/const_map3_test.dart (right):

https://codereview.chromium.org/267923004/diff/1/tests/language/const_map3_te...
tests/language/const_map3_test.dart:17: confuse(x) {
I don't think the method 'confuse' adds anything to this test, and should
probably be removed.

[scheglov, 2014-05-03 16:47:43]

lgtm

[Paul Berry, 2014-05-03 17:14:17]

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

[Paul Berry, 2014-05-03 17:14:56]

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

https://codereview.chromium.org/267923004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:269:
// TODO(jwren) Revisit this algorithm, should there up to n-1 errors?
On 2014/05/03 15:03:34, Brian Wilkerson wrote:
> Yes, it's still an issue. I think what we really want to do, when there are
> conflicting types, is to report the smallest number of errors (which we
> currently don't). For example, given
> 
>   switch(...) {
>     case 3: ...
>     case "a": ...
>     case "b": ...
>     case "c": ...
>   }
> 
> we will produce 3 errors (on all but the first case) when I think we should
> produce one error (on the first case).

Ah, ok.  I've changed the comment to "TODO(paulberry): to minimize error
messages, it would be nice to compare all types with the most popular type
rather than the first type."

https://codereview.chromium.org/267923004/diff/1/tests/language/const_map3_te...
File tests/language/const_map3_test.dart (right):

https://codereview.chromium.org/267923004/diff/1/tests/language/const_map3_te...
tests/language/const_map3_test.dart:17: confuse(x) {
On 2014/05/03 15:03:34, Brian Wilkerson wrote:
> I don't think the method 'confuse' adds anything to this test, and should
> probably be removed.

Done.

[jwren, 2014-05-03 23:03:59]

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

https://codereview.chromium.org/267923004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:269:
// TODO(jwren) Revisit this algorithm, should there up to n-1 errors?
Sounds good, although as far as priority this is a nice to have, not a
defect-bug.



On 2014/05/03 17:14:56, Paul Berry wrote:
> On 2014/05/03 15:03:34, Brian Wilkerson wrote:
> > Yes, it's still an issue. I think what we really want to do, when there are
> > conflicting types, is to report the smallest number of errors (which we
> > currently don't). For example, given
> > 
> >   switch(...) {
> >     case 3: ...
> >     case "a": ...
> >     case "b": ...
> >     case "c": ...
> >   }
> > 
> > we will produce 3 errors (on all but the first case) when I think we should
> > produce one error (on the first case).
> 
> Ah, ok.  I've changed the comment to "TODO(paulberry): to minimize error
> messages, it would be nice to compare all types with the most popular type
> rather than the first type."

https://codereview.chromium.org/267923004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:363:
if (type == null || type.equals(intType) ||
type.equals(typeProvider.getStringType())) {
What about other nums such as the num type, double, etc.?

[Brian Wilkerson, 2014-05-04 04:39:12]

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

https://codereview.chromium.org/267923004/diff/1/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ConstantVerifier.java:363:
if (type == null || type.equals(intType) ||
type.equals(typeProvider.getStringType())) {
> What about other nums such as the num type, double, etc.?

The specification explicitly states:

It is a compile-time error if the class C has an implementation of the operator
== other than the one inherited from Object unless the value of the expression
is a string or integer.

num and double are not explicitly mentioned. (And actually (a) we need to handle
double specially in next condition and (b) num is an abstract class so there
won't ever be an instance of it.)