Change switch to give errors when cases don't follow the newest syntax.

Change switch to give warnings when cases don't follow the newest syntax.

The current implementation still falls back on if-else-if dispatch if
the case expressions are not compile-time constants. It also falls back
if the expressions fail to follow the specification for any other reason.

The specification requires that case expressions:
- Are compile-time constants.
- All have the same type and are instances of the same class.
- Do not override Object.operator==.
For the last one, I've made exceptions for int/double/String.

Function constants are rejected due to overriding operator==.
I'm not sure they do yet, but they will eventually.

Map and List are accepted. All constant Map/List instances are
expected to be instances of the same class. If we later decide to make
them override operator==, we'll have to reject them too. If we decide
to have a special class for some List/Map constants, we'll let it
override runtimeType to hide it.

Type equality is only checked as far as the constants know their type.
Type parameters are not included in the test since I don't think they
are recorded in the compile-time constant (I could be wrong).

The new switch semantics are still on "hold", so this change retains
the old version too (which accepted any expression). When we want to
remove that, it's a matter of making the warnings into errors and
remove the fallback (the current content of visitSwitchStatement after
the call to tryBuildConstantSwitch).

The validation of switch cases should really happen during resolution
if possible. I'll try to figure out how to move it there in a later
CL.

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

[ngeoffray, 2012-08-30 07:29:13]

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/compile_time_constants.dart:27: abstract bool
isSameType(Constant constant);
I'd prefer having something consistant with what we have with HType and Element:
a computeType(compiler) method that returns the type of the constant.

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3306: Element element =
type.element;
The code below probably deserves to be in a helper method like in the Elements
class.

[Lasse Reichstein Nielsen, 2012-08-30 10:54:25]

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/compile_time_constants.dart:27: abstract bool
isSameType(Constant constant);
Seems reasonable if there is a simple way to get a Type corresponding to the
primitive types.

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3306: Element element =
type.element;
Any wish for the abstraction level?
isOverridingEquals?
lookupEqualsImplementation?
lookupOperatorImplementation?

[ngeoffray, 2012-08-30 11:07:13]

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/compile_time_constants.dart:27: abstract bool
isSameType(Constant constant);
On 2012/08/30 10:54:25, Lasse Reichstein Nielsen wrote:
> Seems reasonable if there is a simple way to get a Type corresponding to the
> primitive types.

The Dart "primitive" types? compiler.boolClass.computeType(compiler);

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3306: Element element =
type.element;
On 2012/08/30 10:54:25, Lasse Reichstein Nielsen wrote:
> Any wish for the abstraction level?
> isOverridingEquals?
> lookupEqualsImplementation?
> lookupOperatorImplementation?

I like the three of them :) The first two are easy to implement by calling the
third.

[floitsch, 2012-08-30 14:08:18]

LGTM.
The description seems contradictory:
"Change switch to give errors when cases don't follow the newest syntax".

"The new switch semantics are still on "hold", so ..."

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/compile_time_constants.dart:327: return type ==
listConstant.type;
I would expect this to be always true. Maybe change into an assert?

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/compile_time_constants.dart:394: MapConstant
mapConstant = constant;
ditto, but no assert, since maps containing __proto__ or not will have different
type fields.

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3300: // [Map] and [List] does not
override equals.
do not

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/warnings.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/warnings.dart:139: "Switch cases contain invalid
expressions.");
lowercase switch

[Lasse Reichstein Nielsen, 2012-09-03 09:12:55]

I get some "queue is closed" errors when using .computeType on elements in the
builder.
I'll try moving the tests to the resolver instead.

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/compile_time_constants.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/compile_time_constants.dart:27: abstract bool
isSameType(Constant constant);
On 2012/08/30 11:07:13, ngeoffray wrote:
> On 2012/08/30 10:54:25, Lasse Reichstein Nielsen wrote:
> > Seems reasonable if there is a simple way to get a Type corresponding to the
> > primitive types.
> 
> The Dart "primitive" types? compiler.boolClass.computeType(compiler);

Done.

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/compile_time_constants.dart:327: return type ==
listConstant.type;
It's gone now. Switched everything to having a computeType on the constant, and
ObjectConstant just returns the type.

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/compile_time_constants.dart:394: MapConstant
mapConstant = constant;
Gone too.

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/ssa/builder.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3300: // [Map] and [List] does not
override equals.
On 2012/08/30 14:08:18, floitsch wrote:
> do not

Done.

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/ssa/builder.dart:3306: Element element =
type.element;
I went with
 typeOverridesObjectEquals
and
 lookupOperator

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
File lib/compiler/implementation/warnings.dart (right):

https://chromiumcodereview.appspot.com/10916002/diff/1/lib/compiler/implement...
lib/compiler/implementation/warnings.dart:139: "Switch cases contain invalid
expressions.");
On 2012/08/30 14:08:18, floitsch wrote:
> lowercase switch

Done.

[ngeoffray, 2012-09-06 08:15:59]

Lasse, are you still having problems committing this change, because of the
queue error?

[Lasse Reichstein Nielsen, 2012-09-06 08:20:25]

I haven't gotten back to this. I was trying to move the test to the resolver,
but I think I'll porefer to do that in a different CL. 

I still have open queue errors in my presubmit script, but it's likely the
general error, and not related to this change.

[ngeoffray, 2012-09-06 08:23:54]

On 2012/09/06 08:20:25, Lasse Reichstein Nielsen wrote:
> I haven't gotten back to this. I was trying to move the test to the resolver,
> but I think I'll porefer to do that in a different CL. 
> 
> I still have open queue errors in my presubmit script, but it's likely the
> general error, and not related to this change.

Please update to TOT, and see if you don't have this error anymore. I submitted
a fix yesterday.

[ngeoffray, 2012-09-06 09:28:55]

http://codereview.chromium.org/10916002/diff/6/lib/compiler/implementation/co...
File lib/compiler/implementation/compile_time_constants.dart (right):

http://codereview.chromium.org/10916002/diff/6/lib/compiler/implementation/co...
lib/compiler/implementation/compile_time_constants.dart:27: abstract DartType
computeType(ConstantHandler handler);
Please take a Compiler here instead of the ConstantHandler. All the other
computeType methods take a Compiler.

[Lasse Reichstein Nielsen, 2012-09-06 12:03:36]

http://codereview.chromium.org/10916002/diff/6/lib/compiler/implementation/co...
File lib/compiler/implementation/compile_time_constants.dart (right):

http://codereview.chromium.org/10916002/diff/6/lib/compiler/implementation/co...
lib/compiler/implementation/compile_time_constants.dart:27: abstract DartType
computeType(ConstantHandler handler);
Good point. I was trying to not make Constant depend on Compiler, but since I
extract the compiler in most of the implementations anyway, it's not really
working.

[Lasse Reichstein Nielsen, 2012-09-11 12:20:23]

http://codereview.chromium.org/10916002/diff/6/lib/compiler/implementation/co...
File lib/compiler/implementation/compile_time_constants.dart (right):

http://codereview.chromium.org/10916002/diff/6/lib/compiler/implementation/co...
lib/compiler/implementation/compile_time_constants.dart:27: abstract DartType
computeType(ConstantHandler handler);
Done by someone else. Yey.