Implement 'as' operator.

Implement 'as' operator.

This is a quick implementation with room for improvement.
It might work.

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

[Lasse Reichstein Nielsen, 2012-06-07 12:23:33]

Karl and Nicolas: Am I doing the right thing with the type checks? It seems to
differ from the "is" operator.

[ahe, 2012-06-07 13:12:58]

Initial comments.

http://codereview.chromium.org/10540048/diff/3001/tests/language/cast_test.dart
File tests/language/cast_test.dart (right):

http://codereview.chromium.org/10540048/diff/3001/tests/language/cast_test.da...
tests/language/cast_test.dart:52: c = oc;  // 09: static type warning
Use three slashes. Some problem elsewhere in this file.

[ahe, 2012-06-08 08:14:57]

http://codereview.chromium.org/10540048/diff/5001/lib/compiler/implementation...
File lib/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/10540048/diff/5001/lib/compiler/implementation...
lib/compiler/implementation/lib/js_helper.dart:955: class CastException extends
TypeError {
Why is this not shared?

http://codereview.chromium.org/10540048/diff/5001/lib/compiler/implementation...
lib/compiler/implementation/lib/js_helper.dart:971: if (value === null) throw
new NullPointerException();
I consider this a show stopper. The specification is broken.

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

Ping non-Peters.
I suggest we commit this as the spec says, get some experience with using it,
and then change it if the spec changes.
Or in other words, it's the spec, live with it or change the spec :P

[ahe, 2012-06-20 08:47:45]

On 2012/06/20 08:09:54, Lasse Reichstein Nielsen wrote:
> Ping non-Peters.
> I suggest we commit this as the spec says, get some experience with using it,
> and then change it if the spec changes.
> Or in other words, it's the spec, live with it or change the spec :P

Gilad has said the specification will change. It is wasted effort reviewing and
implementing one thing when you know will have to change in a week or two. Last
night Gilad said:

"Relax, the spec has already been changed. Thank Peter Ahe for pointing the
issue out last week." about http://code.google.com/p/dart/issues/detail?id=3455.

The "living" doc that Gilad updates ongoing currently says:

"The expression e is evaluated to a value v. Then, if the interface of the class
of v is a subtype of T, the cast-expression evaluates to v. Otherwise, if v is
null, the cast-expression evaluates to v.  In all other cases,  a CastException
is thrown."

[Lasse Reichstein Nielsen, 2012-06-20 09:32:36]

Excellent!

[Lasse Reichstein Nielsen, 2012-06-25 07:54:45]

Please take another look.
Some tests still fail due to an existing bug
(https://code.google.com/p/dart/issues/detail?id=3857).

[ahe, 2012-06-25 10:36:56]

Everything but the SSA looks fine (I'm not sure I understand it). I would like
to understand "callTypeCast" before I sign of on this.

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dart
File corelib/src/exceptions.dart (right):

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:143: class CastException implements Exception {
We generally put fields before constructors which come before methods.

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:144: // TODO(lrn): Change expectedType to "Type"
when reified types are in.
This comment applies to the field below.

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:148: return "$exceptionName : Casting $actualValue
to $expectedType";
You don't put a space after the exception. The above exceptions are a mess and
needs to be cleaned up. No need to repeat that.

What is actualValue? Is that the object that you're trying to cast? That seems
less helpful than the name of the type that is being cast.

Also, the error message is somewhat indirect. The problem is that $actualValue
is not a subtype of $expectedType.

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:154: String get exceptionName() => "CastException";
What is this for? Localization doesn't work well with concatenated strings like
above.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1073: propertyTypeCast(Object
value, Object property) {
Remove types from signature.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1092: callTypeCast(value,
property) {
A comment would be nice here.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1114: if (JS('bool', '!!#[#]',
value, property)) return value;
This could be: return propertyTypeCast(value, property);

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1127: if (JS('bool', '#[#]()',
value, property)) return value;
Should this call callTypeCast?

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1127: if (JS('bool', '#[#]()',
value, property)) return value;
Don't you need to check for typeof value === 'object'?

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1155: if (JS('bool', '!!#[#]',
value, property)) return value;
Call propertyTypeCast?

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1168: if (JS('bool', '#[#]()',
value, property)) return value;
Call callTypeCast?

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1362: if (optional('is', token)
|| optional('as', token)) {
Please put stringValue in a local variable instead of calling optional twice on
the same token.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1375: if (optional('is', token)
|| optional('as', token)) {
Ditto.

http://codereview.chromium.org/10540048/diff/10001/tests/language/language.st...
File tests/language/language.status (right):

http://codereview.chromium.org/10540048/diff/10001/tests/language/language.st...
tests/language/language.status:19: cast_test: Fail # Issue 3401
3401 is for the analyzer.

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

Thanks for the comments.
Adding Florian for the SSA part.

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dart
File corelib/src/exceptions.dart (right):

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:143: class CastException implements Exception {
I know. I was following the convention of the file.

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:144: // TODO(lrn): Change expectedType to "Type"
when reified types are in.
True. I put the comment at the first instance of the name, but it should go
below.

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:148: return "$exceptionName : Casting $actualValue
to $expectedType";
The actualValue is the value being cast, yes. As discussed offline, I'll change
it to the actualType instead, and change the message to "Cannot cast $actualType
to $expectedType".
And remove the space.

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:154: String get exceptionName() => "CastException";
Again I'm following the surrounding code. I assume it is to allow sub-classes to
specialize the name.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1073: propertyTypeCast(Object
value, Object property) {
Why?

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1092: callTypeCast(value,
property) {
Will do.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1114: if (JS('bool', '!!#[#]',
value, property)) return value;
Well spotted.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1127: if (JS('bool', '#[#]()',
value, property)) return value;
I think it should follow the tringSuperNativeTypeCheck above. I don't claim to
understand what it does.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1155: if (JS('bool', '!!#[#]',
value, property)) return value;
Will do.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1168: if (JS('bool', '#[#]()',
value, property)) return value;
That should work. And ditto for the check.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/scanner/parser.dart (right):

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1362: if (optional('is', token)
|| optional('as', token)) {
Done.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/scanner/parser.dart:1375: if (optional('is', token)
|| optional('as', token)) {
On 2012/06/25 10:36:56, ahe wrote:
> Ditto.

Done.

http://codereview.chromium.org/10540048/diff/10001/tests/language/language.st...
File tests/language/language.status (right):

http://codereview.chromium.org/10540048/diff/10001/tests/language/language.st...
tests/language/language.status:19: cast_test: Fail # Issue 3401
Fixed to refer to the VM.

[ahe, 2012-06-25 11:57:03]

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dart
File corelib/src/exceptions.dart (right):

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:154: String get exceptionName() => "CastException";
On 2012/06/25 11:20:06, Lasse Reichstein Nielsen wrote:
> Again I'm following the surrounding code. I assume it is to allow sub-classes
to
> specialize the name.

That the surrounding code is broken is no excuse for copying it.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1073: propertyTypeCast(Object
value, Object property) {
On 2012/06/25 11:20:06, Lasse Reichstein Nielsen wrote:
> Why?

Why are they there?

[Lasse Reichstein Nielsen, 2012-06-25 12:51:10]

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dart
File corelib/src/exceptions.dart (right):

http://codereview.chromium.org/10540048/diff/10001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:154: String get exceptionName() => "CastException";
I don't think the style guide makes any recommendations about the order of
members in classes. I think being consistent with the surrounding code is more
important than being consistent with code in other files. If the current
ordering isn't good enough, we should reorder the entire file/directory. That'll
be easier if ordering is at least internally consistent.

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1073: propertyTypeCast(Object
value, Object property) {
For the same reasons we use them everywhere else (except, apparently, this file,
which I hear is not a reason to follow suit) - to document the expected argument
types.
I'm aware that using "Object" as opposed to "var" (or nothing) will not make a
difference in checked mode, but it will document that this code doesn't expect
any other properties of the arguments.

I should add a "void" return too.

[ahe, 2012-06-25 12:56:32]

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

http://codereview.chromium.org/10540048/diff/10001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:1073: propertyTypeCast(Object
value, Object property) {
On 2012/06/25 12:51:10, Lasse Reichstein Nielsen wrote:
> For the same reasons we use them everywhere else (except, apparently, this
file,
> which I hear is not a reason to follow suit) - to document the expected
argument
> types.

Generally, you shouldn't use Object unless you're absolutely sure that is what
you mean. In this case it is not correct. Only String is a valid argument for
the second argument.

> I'm aware that using "Object" as opposed to "var" (or nothing) will not make a
> difference in checked mode, but it will document that this code doesn't expect
> any other properties of the arguments.

I would agree if it was correct that you only expected Object's properties.

> I should add a "void" return too.

That would be incorrect.

[ahe, 2012-06-25 13:06:33]

LGTM (except for ssa/*)

[Lasse Reichstein Nielsen, 2012-06-25 15:16:07]

Addressed Peter's comments.
Florian, please take a look, and perhaps check the CL out and try running the
test. It seems to be giving more wrong property accesses like issue 3857.
I'll try to reduce them tomorrow, and ensure that they aren't introduced by this
code.

https://chromiumcodereview.appspot.com/10540048/diff/10001/corelib/src/except...
File corelib/src/exceptions.dart (right):

https://chromiumcodereview.appspot.com/10540048/diff/10001/corelib/src/except...
corelib/src/exceptions.dart:143: class CastException implements Exception {
Ah, to heck with consistency. I'll move them up here.

https://chromiumcodereview.appspot.com/10540048/diff/10001/corelib/src/except...
corelib/src/exceptions.dart:148: return "$exceptionName : Casting $actualValue
to $expectedType";
I also removed the special casing for when the type wasn't provided. It should
never be necessary to do an explicit cast without an available type.

https://chromiumcodereview.appspot.com/10540048/diff/10001/lib/compiler/imple...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10540048/diff/10001/lib/compiler/imple...
lib/compiler/implementation/lib/js_helper.dart:1073: propertyTypeCast(Object
value, Object property) {
Ack, I agree.
There is no good type to use here.

https://chromiumcodereview.appspot.com/10540048/diff/10001/lib/compiler/imple...
lib/compiler/implementation/lib/js_helper.dart:1168: if (JS('bool', '#[#]()',
value, property)) return value;
Actually, the "callTypeCast" does more tests than what we do here, so I'll just
change the ones that can be propertyTypeCast.

[floitsch, 2012-06-25 18:21:09]

LGTM.
couldn't reproduce your errors. Maybe they are already fixed.

http://codereview.chromium.org/10540048/diff/17001/corelib/src/exceptions.dart
File corelib/src/exceptions.dart (right):

http://codereview.chromium.org/10540048/diff/17001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:42: this._existingArgumentNames =
existingArgumentNames;
not your code, but 80chars.

http://codereview.chromium.org/10540048/diff/17001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/nodes.dart (right):

http://codereview.chromium.org/10540048/diff/17001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:1502: * the expression. If the switc
had no default case, this is the
switch

http://codereview.chromium.org/10540048/diff/17001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:2288: => other is HTypeConversion
use { } instead of "=>".

http://codereview.chromium.org/10540048/diff/17001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:2289: && ((other.kind ==
CAST_TYPE_CHECK) == (kind == CAST_TYPE_CHECK));
Explain why only CAST_TYPE_CHECKs are the same.

[Lasse Reichstein Nielsen, 2012-06-26 08:26:03]

http://codereview.chromium.org/10540048/diff/17001/corelib/src/exceptions.dart
File corelib/src/exceptions.dart (right):

http://codereview.chromium.org/10540048/diff/17001/corelib/src/exceptions.dar...
corelib/src/exceptions.dart:42: this._existingArgumentNames =
existingArgumentNames;
On 2012/06/25 18:21:09, floitsch wrote:
> not your code, but 80chars.

Done.

http://codereview.chromium.org/10540048/diff/17001/lib/compiler/implementatio...
File lib/compiler/implementation/ssa/nodes.dart (right):

http://codereview.chromium.org/10540048/diff/17001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:1502: * the expression. If the switc
had no default case, this is the
Fixed.

http://codereview.chromium.org/10540048/diff/17001/lib/compiler/implementatio...
lib/compiler/implementation/ssa/nodes.dart:2289: && ((other.kind ==
CAST_TYPE_CHECK) == (kind == CAST_TYPE_CHECK));
Non-casts would also be the same. I don't think I need this anyway, so I'll just
revert this change.

[ngeoffray, 2012-08-27 14:24:00]

https://chromiumcodereview.appspot.com/10540048/diff/14003/lib/compiler/imple...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://chromiumcodereview.appspot.com/10540048/diff/14003/lib/compiler/imple...
lib/compiler/implementation/lib/js_helper.dart:995: stringTypeCast(value) {
These helpers share sooo much with their checked mode equivalent. Any reason why
you did not use a boolean flag to the existing helpers that tells whether it's
checked mode or cast mode?