Convert MethodMirror.returnType to native calls (and more)

Since returnType is represented by a mirror, we make it lazy. We do not use a lazy mirror anymore, but instead create a type mirror upon first access.

CreateTypeMirror is already factored out of MethodMirror_return_type since it is needed by others mirrors (future CLs). Also introduces CreateMirror() which will be used in future CLs.

BUG=
R=asiva@google.com

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

[Michael Lippautz (Google), 2013-07-19 19:03:01]

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc
File runtime/lib/mirrors.cc (left):

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#old...
runtime/lib/mirrors.cc:525: Dart_Handle return_type =
Dart_FunctionReturnType(func);
We get rid of Dart_FunctionReturnType() in this place. There's still one caller
left, though.

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

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:26: DartLibraryCalls::ExceptionCreate(mirrors_lib,
ExceptionCreate should at some point (this CL?) be renamed to CreateObject,
because it's a general function to create objects (nothing specific to
exceptions).

https://codereview.chromium.org/19780002/diff/2001/tests/lib/mirrors/method_m...
File tests/lib/mirrors/method_mirror_returntype_test.dart (right):

https://codereview.chromium.org/19780002/diff/2001/tests/lib/mirrors/method_m...
tests/lib/mirrors/method_mirror_returntype_test.dart:46: // The spec for this is
ambigious and needs to be updated before it is clear
The spec is yet clear about what type of ClassMirror reflectClass() returns. It
should probably be about the declaration, hence return a TypeMirror for "E" (not
"dynamic"). A future CL will be more specific about this, enabling this test.

[rmacnak, 2013-07-19 20:37:22]

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

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:31: ASSERT(result.IsInstance());
Doesn't IsInstance() guarantee !IsError()?

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1036: // Until we get rid of lazy mirrors, we must have
owners.
Unnecessary after https://chromiumcodereview.appspot.com/19749008/ ?

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1931: // We handle constructors in dart code.
Dart

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

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:929: if (isConstructor) {
What if _returnType isn't null? This will fetch it again.

[Michael Lippautz (Google), 2013-07-19 20:53:01]

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

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:31: ASSERT(result.IsInstance());
On 2013/07/19 20:37:22, Ryan Macnak wrote:
> Doesn't IsInstance() guarantee !IsError()?

Leftover from debugging. Done.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1036: // Until we get rid of lazy mirrors, we must have
owners.
On 2013/07/19 20:37:22, Ryan Macnak wrote:
> Unnecessary after https://chromiumcodereview.appspot.com/19749008/ ?

As I think of it now, it was not even necessary in first place, because _owner
gets initialized lazily if it was unknown at creation time.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1931: // We handle constructors in dart code.
On 2013/07/19 20:37:22, Ryan Macnak wrote:
> Dart

Done.

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

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors_impl.d...
runtime/lib/mirrors_impl.dart:929: if (isConstructor) {
On 2013/07/19 20:37:22, Ryan Macnak wrote:
> What if _returnType isn't null? This will fetch it again.

Fixed.

[siva, 2013-07-19 21:01:39]

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

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:26: DartLibraryCalls::ExceptionCreate(mirrors_lib,
On 2013/07/19 19:03:02, Michael Lippautz wrote:
> ExceptionCreate should at some point (this CL?) be renamed to CreateObject,
> because it's a general function to create objects (nothing specific to
> exceptions).


Good point, I do not know why we called it ExceptionCreate, we should probably
call it InstanceCreate (creates an instance using the constructor).
Please do the rename in a new CL (the only place it is used is exceptions.cc).

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1108: String::New("_SpecialTypeMirrorImpl"));
Please add _SpecialTypeMirrorImpl to the symbols list and use
Symbols::_SpecialTypeMirrorImpl() here

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1109: const String& name =
String::Handle(String::New("void"));
this should be Symbols::Void()

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1115: String::New("_SpecialTypeMirrorImpl"));
Symbols::_SpecialTypeMirrorImpl() here.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1116: const String& name =
String::Handle(String::New("dynamic"));
Symbols::Dynamic() here.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1124: isolate, TypeParameter::Cast(type).raw());
why cast an reassign back to another newly created handle?

return CreateTypeVariableMirror(TypeParameter::Cast(type),
Object::null_instance());

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1908:
MirrorReference::GetFunctionReferent(arguments->NativeArgAt(0)));
I just realized for all native methods we normally throw an exception if an
illegal argument is passed in. For some we may also have to
check to make sure it is not null.

We should write these native methods as:

So this would be:

{
  GET_NON_NULL_NATIVE_ARGUMENT(MirrorReference, ref, arguments->NativeArgAt(0));
  const Function& func = Function::Handle(ref.GetFunctionReferent());
  return func.UserVisibleName();

}

Could you do this part in a separate CL.

https://codereview.chromium.org/19780002/diff/2001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/19780002/diff/2001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:14222: return
Function::Cast(Object::Handle(ref.referent())).raw();
We use CheckedHandle only in the native code methods when dealing with
parameters to native methods and also we don't normally
declare methods which take raw pointers to object as parameters.

I would change this code as follows:

RawFunction* MirrorReference::GetFunctionReferent() {
  ASSERT(Object::Handle(ref.referent()).IsFunction());
  return Function::Cast(Object::Handle(referent())).raw();
}

https://codereview.chromium.org/19780002/diff/2001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/19780002/diff/2001/runtime/vm/object.h#newcod...
runtime/vm/object.h:5879: static RawFunction* GetFunctionReferent(RawObject*
raw_ptr);
why not make this an instance method:

RawFunction* GetFunctionReferent();

You probably also need methods like:

RawClass* GetClassReferent();
RawLibrary* GetLibraryReferent();

[Michael Lippautz (Google), 2013-07-19 22:13:20]

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

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:26: DartLibraryCalls::ExceptionCreate(mirrors_lib,
On 2013/07/19 21:01:40, siva wrote:
> On 2013/07/19 19:03:02, Michael Lippautz wrote:
> > ExceptionCreate should at some point (this CL?) be renamed to CreateObject,
> > because it's a general function to create objects (nothing specific to
> > exceptions).
> 
> 
> Good point, I do not know why we called it ExceptionCreate, we should probably
> call it InstanceCreate (creates an instance using the constructor).
> Please do the rename in a new CL (the only place it is used is exceptions.cc).

Done.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1108: String::New("_SpecialTypeMirrorImpl"));
On 2013/07/19 21:01:40, siva wrote:
> Please add _SpecialTypeMirrorImpl to the symbols list and use
> Symbols::_SpecialTypeMirrorImpl() here

Done.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1109: const String& name =
String::Handle(String::New("void"));
On 2013/07/19 21:01:40, siva wrote:
> this should be Symbols::Void()

Done.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1115: String::New("_SpecialTypeMirrorImpl"));
On 2013/07/19 21:01:40, siva wrote:
> Symbols::_SpecialTypeMirrorImpl() here.

Done.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1116: const String& name =
String::Handle(String::New("dynamic"));
On 2013/07/19 21:01:40, siva wrote:
> Symbols::Dynamic() here.

Done.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1124: isolate, TypeParameter::Cast(type).raw());
On 2013/07/19 21:01:40, siva wrote:
> why cast an reassign back to another newly created handle?
> 
> return CreateTypeVariableMirror(TypeParameter::Cast(type),
> Object::null_instance());

Done.

https://codereview.chromium.org/19780002/diff/2001/runtime/lib/mirrors.cc#new...
runtime/lib/mirrors.cc:1908:
MirrorReference::GetFunctionReferent(arguments->NativeArgAt(0)));
On 2013/07/19 21:01:40, siva wrote:
> I just realized for all native methods we normally throw an exception if an
> illegal argument is passed in. For some we may also have to
> check to make sure it is not null.
> 
> We should write these native methods as:
> 
> So this would be:
> 
> {
>   GET_NON_NULL_NATIVE_ARGUMENT(MirrorReference, ref,
arguments->NativeArgAt(0));
>   const Function& func = Function::Handle(ref.GetFunctionReferent());
>   return func.UserVisibleName();
> 
> }
> 
> Could you do this part in a separate CL.

Ok, I will do the change for the two existing native calls in a separate CL.

https://codereview.chromium.org/19780002/diff/2001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/19780002/diff/2001/runtime/vm/object.cc#newco...
runtime/vm/object.cc:14222: return
Function::Cast(Object::Handle(ref.referent())).raw();
On 2013/07/19 21:01:40, siva wrote:
> We use CheckedHandle only in the native code methods when dealing with
> parameters to native methods and also we don't normally
> declare methods which take raw pointers to object as parameters.
> 
> I would change this code as follows:
> 
> RawFunction* MirrorReference::GetFunctionReferent() {
>   ASSERT(Object::Handle(ref.referent()).IsFunction());
>   return Function::Cast(Object::Handle(referent())).raw();
> }

Done.

https://codereview.chromium.org/19780002/diff/2001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/19780002/diff/2001/runtime/vm/object.h#newcod...
runtime/vm/object.h:5879: static RawFunction* GetFunctionReferent(RawObject*
raw_ptr);
On 2013/07/19 21:01:40, siva wrote:
> why not make this an instance method:
> 
> RawFunction* GetFunctionReferent();
> 
> You probably also need methods like:
> 
> RawClass* GetClassReferent();
> RawLibrary* GetLibraryReferent();

Changed to an instance method. The other ones will be added as they are needed.

[siva, 2013-07-19 22:59:00]

lgtm

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

https://codereview.chromium.org/19780002/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1094: return
CreateMirror(Symbols::_SpecialTypeMirrorImpl(), args);
This ends up creating a Mirror object everytime we go through this code. We
should create it in the VM isolate once and return that.

Maybe add a TODO for this CL and implement that in a new CL.

https://codereview.chromium.org/19780002/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1100: return CreateClassMirror(cls, Instance::Handle());
Object::null_instance() here too instead of Instance::Handle() saves creating a
handle.

[Michael Lippautz (Google), 2013-07-19 23:09:26]

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

https://codereview.chromium.org/19780002/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1094: return
CreateMirror(Symbols::_SpecialTypeMirrorImpl(), args);
On 2013/07/19 22:59:01, siva wrote:
> This ends up creating a Mirror object everytime we go through this code. We
> should create it in the VM isolate once and return that.
> 
> Maybe add a TODO for this CL and implement that in a new CL.

Added a TODO.

https://codereview.chromium.org/19780002/diff/15001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1100: return CreateClassMirror(cls, Instance::Handle());
On 2013/07/19 22:59:01, siva wrote:
> Object::null_instance() here too instead of Instance::Handle() saves creating
a
> handle.

Done.

[Michael Lippautz (Google), 2013-07-19 23:17:55]

Committed patchset #5 manually as r25252 (presubmit successful).

[ahe, 2013-07-20 16:27:40]

DBC

https://codereview.chromium.org/19780002/diff/21001/tests/lib/mirrors/method_...
File tests/lib/mirrors/method_mirror_returntype_test.dart (right):

https://codereview.chromium.org/19780002/diff/21001/tests/lib/mirrors/method_...
tests/lib/mirrors/method_mirror_returntype_test.dart:28: Expect.equals("int",
_n(mm.returnType.simpleName));
I would prefer if you used const Symbol('int') instead of calling
MirrorSystem.getName.

When we implement symbol literals, we can update code that uses const Symbol.