Fix runtimeType for strings, integers and double: return their interface type.

Fix runtimeType for strings, integers and double: return their interface type.

R=asiva@google.com

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

[siva, 2013-10-04 16:35:58]

LGTM

I was wondering why not do the same for other standard types too, would be more
efficient than calling into the VM and canonicalizing the type etc.

[rmacnak, 2013-10-04 16:49:56]

https://codereview.chromium.org/25674017/diff/8001/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/25674017/diff/8001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:345: static _computeType(reflectee) =>
reflectee.runtimeType;
Please don't change this. The mirrors should retrieve the object's true type,
even if a user overrides runtimeType. We're missing test coverage for this, so
I'll add it.

[siva, 2013-10-04 16:56:34]

https://codereview.chromium.org/25674017/diff/8001/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/25674017/diff/8001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:345: static _computeType(reflectee) =>
reflectee.runtimeType;
I thought we don't allow end users to override methods in core
classes like Integer, String, Bool etc.

On 2013/10/04 16:49:56, Ryan Macnak wrote:
> Please don't change this. The mirrors should retrieve the object's true type,
> even if a user overrides runtimeType. We're missing test coverage for this, so
> I'll add it.

[rmacnak, 2013-10-04 17:11:49]

https://codereview.chromium.org/25674017/diff/8001/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/25674017/diff/8001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:345: static _computeType(reflectee) =>
reflectee.runtimeType;
We don't, and the changes to the built-in types are fine. What I'm saying is the
following should pass:

import 'dart:mirrors';
import 'package:expect/expect.dart';

class NotAString {
  Type get runtimeType => String;
}
main(){
  Expect.equals(String,
                new NotAString().runtimeType);
  Expect.equals(reflectClass(NotAString),
                reflect(new NotAString()).type);
}

So _computeType should go to the native, rather than calling runtimeType on the
reflectee.

Whether reflectClass(String) == reflect('str').type is an implementation detail.
(Which will happen to be false on the VM and true in dart2js.)

On 2013/10/04 16:56:34, siva wrote:
> I thought we don't allow end users to override methods in core
> classes like Integer, String, Bool etc.
> 
> On 2013/10/04 16:49:56, Ryan Macnak wrote:
> > Please don't change this. The mirrors should retrieve the object's true
type,
> > even if a user overrides runtimeType. We're missing test coverage for this,
so
> > I'll add it.
>

[Ivan Posva, 2013-10-04 17:49:55]

https://codereview.chromium.org/25674017/diff/8001/runtime/lib/mirrors_impl.dart
File runtime/lib/mirrors_impl.dart (right):

https://codereview.chromium.org/25674017/diff/8001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:345: static _computeType(reflectee) =>
reflectee.runtimeType;
On 2013/10/04 17:11:50, Ryan Macnak wrote:
> We don't, and the changes to the built-in types are fine. What I'm saying is
the
> following should pass:
> 
> import 'dart:mirrors';
> import 'package:expect/expect.dart';
> 
> class NotAString {
>   Type get runtimeType => String;
> }
> main(){
>   Expect.equals(String,
>                 new NotAString().runtimeType);
>   Expect.equals(reflectClass(NotAString),
>                 reflect(new NotAString()).type);
> }
> 
> So _computeType should go to the native, rather than calling runtimeType on
the
> reflectee.
> 
> Whether reflectClass(String) == reflect('str').type is an implementation
detail.
> (Which will happen to be false on the VM and true in dart2js.)
> 
> On 2013/10/04 16:56:34, siva wrote:
> > I thought we don't allow end users to override methods in core
> > classes like Integer, String, Bool etc.
> > 
> > On 2013/10/04 16:49:56, Ryan Macnak wrote:
> > > Please don't change this. The mirrors should retrieve the object's true
> type,
> > > even if a user overrides runtimeType. We're missing test coverage for
this,
> so
> > > I'll add it.
> > 
> 

Ryan,

In that case you should be calling your own native method and not be relying on
the method of Object. Also note that you were calling a instance native from a
static method, which is another problem.

-Ivan

[srdjan, 2013-10-04 18:44:55]

Ryan, PTAL

[rmacnak, 2013-10-04 21:21:21]

https://codereview.chromium.org/25674017/diff/17001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (right):

https://codereview.chromium.org/25674017/diff/17001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1153: if (instance.IsString()) {
We are not required to hide these implementation types for the mirrors, and I
think we should answer the real type. Completing the illusion that these types
are implemented directly also requires special cases in deciding the members of
a library, the owners of methods, and probably other places that don't come
immediately to mind.

[srdjan, 2013-10-04 22:09:02]

Committed patchset #5 manually as r28286.