Substitution for type variables in mirrors on instantiated generics.

Substitution for type variables in mirrors on instantiated generics.

BUG=http://dartbug.com/12282
BUG=http://dartbug.com/13808
R=gbracha@google.com, regis@google.com

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

[rmacnak, 2013-10-09 01:15:00]

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

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:489: ? _supertype(_reflectedType)
Unfortunately, this is not the same as reflecting on the instantiated supertype
and getting its original declaration.

[gbracha, 2013-10-09 01:19:11]

lgtm

[regis, 2013-10-09 15:46:51]

LGTM with some questions and comments

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

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:875: return super_type.raw();
You can spare a handle: return cls.super_type();

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:884: ASSERT(super_type.IsType() ||
super_type.IsMixinAppType());
That is not correct. This could also be a BoundedType, but you cannot encounter
a MixinAppType after class finalization.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:886: // Why should mixin app types always be
instantiated?
MixinAppType live at compile time only.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:887: ASSERT(super_type.IsType());
|| super_type.IsBoundedType()

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:891: result ^= super_type.InstantiateFrom(type_args,
&error);
Instantiating is correct, but using a prefix of type's vector would work too.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:892: // Is result canonical?
No, result is finalized, but not canonical.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:893: ASSERT(error.IsNull());
You could get a bound error if the instance was created in production mode and
you are running here in checked mode. I do not know if this is possible. I guess
not.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1120: ASSERT(error.IsNull());
Why are you certain you will not get a bound error? Btw, you should rename error
to bound_error to make it clear, here and above too.

In the ClassMirror_supertype function above, you cannot get an error (unless you
switch from production mode to checked mode on the fly), because you instantiate
the super_type with the vector of the instance of its own subtype. But is this
the case here? Is the instantiator guaranteed to be valid for this type
parameter? If it is not, it will assert fault. Some comments about how this
function is supposed to be used would be helpful.

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

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:483: ClassMirror get defaultFactory => null;
Would it be a good time for cleanup? Default factories have been gone for quite
some time. Not in this cl, if this is more involved than just removing these
lines.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:872: static Type
_TypeVariableMirror_instantiate_from(reflectee, instantator)
instantiator

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:877: while (instantator is MethodMirror)
instantator = instantator.owner;
ditto

[regis, 2013-10-09 15:50:07]

On 2013/10/09 15:46:51, regis wrote:
> LGTM with some questions and comments
> 
> https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc
> File runtime/lib/mirrors.cc (right):
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:875: return super_type.raw();
> You can spare a handle: return cls.super_type();
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:884: ASSERT(super_type.IsType() ||
> super_type.IsMixinAppType());
> That is not correct. This could also be a BoundedType, but you cannot
encounter
> a MixinAppType after class finalization.
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:886: // Why should mixin app types always be
> instantiated?
> MixinAppType live at compile time only.
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:887: ASSERT(super_type.IsType());
> || super_type.IsBoundedType()
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:891: result ^= super_type.InstantiateFrom(type_args,
> &error);
> Instantiating is correct, but using a prefix of type's vector would work too.
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:892: // Is result canonical?
> No, result is finalized, but not canonical.
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:893: ASSERT(error.IsNull());
> You could get a bound error if the instance was created in production mode and
> you are running here in checked mode. I do not know if this is possible. I
guess
> not.
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
> runtime/lib/mirrors.cc:1120: ASSERT(error.IsNull());
> Why are you certain you will not get a bound error? Btw, you should rename
error
> to bound_error to make it clear, here and above too.
> 
> In the ClassMirror_supertype function above, you cannot get an error (unless
you
> switch from production mode to checked mode on the fly), because you
instantiate
> the super_type with the vector of the instance of its own subtype. But is this
> the case here? Is the instantiator guaranteed to be valid for this type
> parameter? If it is not, it will assert fault. Some comments about how this
> function is supposed to be used would be helpful.
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> File runtime/lib/mirrors_impl.dart (right):
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> runtime/lib/mirrors_impl.dart:483: ClassMirror get defaultFactory => null;
> Would it be a good time for cleanup? Default factories have been gone for
quite
> some time. Not in this cl, if this is more involved than just removing these
> lines.
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> runtime/lib/mirrors_impl.dart:872: static Type
> _TypeVariableMirror_instantiate_from(reflectee, instantator)
> instantiator
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> runtime/lib/mirrors_impl.dart:877: while (instantator is MethodMirror)
> instantator = instantator.owner;
> ditto

I forgot to type a question:
I thought that you were hiding the fact that mixin typedefs extend the aliased
mixin application, but I do not see code doing so in ClassMirror_supertype, for
example.

[ahe, 2013-10-10 16:53:38]

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

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:489: ? _supertype(_reflectedType)
On 2013/10/09 01:15:00, Ryan Macnak wrote:
> Unfortunately, this is not the same as reflecting on the instantiated
supertype
> and getting its original declaration.

Why?

[rmacnak, 2013-10-11 01:04:22]

PTAL. Now passing the bounds tests modulo an equality issue.

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

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:875: return super_type.raw();
On 2013/10/09 15:46:51, regis wrote:
> You can spare a handle: return cls.super_type();

Done.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:884: ASSERT(super_type.IsType() ||
super_type.IsMixinAppType());
On 2013/10/09 15:46:51, regis wrote:
> That is not correct. This could also be a BoundedType, but you cannot
encounter
> a MixinAppType after class finalization.

We never see a BoundType here even with the addition of the bounded type tests.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:886: // Why should mixin app types always be
instantiated?
On 2013/10/09 15:46:51, regis wrote:
> MixinAppType live at compile time only.

ASSERT now excludes MixinAppType

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:893: ASSERT(error.IsNull());
On 2013/10/09 15:46:51, regis wrote:
> You could get a bound error if the instance was created in production mode and
> you are running here in checked mode. I do not know if this is possible. I
guess
> not.

Added ThrowInvokeError to be cautious.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:1120: ASSERT(error.IsNull());
On 2013/10/09 15:46:51, regis wrote:
> Why are you certain you will not get a bound error? Btw, you should rename
error
> to bound_error to make it clear, here and above too.
> 
> In the ClassMirror_supertype function above, you cannot get an error (unless
you
> switch from production mode to checked mode on the fly), because you
instantiate
> the super_type with the vector of the instance of its own subtype. But is this
> the case here? Is the instantiator guaranteed to be valid for this type
> parameter? If it is not, it will assert fault. Some comments about how this
> function is supposed to be used would be helpful.

The instantiator should always be the type of the class that defined the
parameter. This is basically like what happens inside t() below.

class G<T> {
  t() => T;
}

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

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:483: ClassMirror get defaultFactory => null;
On 2013/10/09 15:46:51, regis wrote:
> Would it be a good time for cleanup? Default factories have been gone for
quite
> some time. Not in this cl, if this is more involved than just removing these
> lines.

This is already in another CL.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:489: ? _supertype(_reflectedType)
On 2013/10/10 16:53:38, ahe wrote:
> On 2013/10/09 01:15:00, Ryan Macnak wrote:
> > Unfortunately, this is not the same as reflecting on the instantiated
> supertype
> > and getting its original declaration.
> 
> Why?

Consider
class S<T>
class C<R> extends S<R>

The superclass of the declaration of C is S<R>, not the declaration of S.

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:872: static Type
_TypeVariableMirror_instantiate_from(reflectee, instantator)
On 2013/10/09 15:46:51, regis wrote:
> instantiator

Done.

https://codereview.chromium.org/26344006/diff/27001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/26344006/diff/27001/tests/lib/lib.status#newc...
tests/lib/lib.status:167: mirrors/generic_f_bounded_test: RuntimeError
Fails because we somehow got two distinct Types for the class Real.

[rmacnak, 2013-10-11 01:06:37]

On 2013/10/09 15:50:07, regis wrote:
> I thought that you were hiding the fact that mixin typedefs extend the aliased
> mixin application, but I do not see code doing so in ClassMirror_supertype,
for
> example.

This is done in Dart code. We need the true superclass to find the mixin and its
members.

[regis, 2013-10-11 01:28:59]

LGTM

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

https://codereview.chromium.org/26344006/diff/27001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:889: ASSERT(super_type.IsFinalized());
You could write
ASSERT(!super_type.IsType() || super_type.IsFinalized());

But in what cases can you encounter a non-finalized super type?
The only case you need to keep in mind is that the super type of Object is null.
Therefore, this should work:
ASSERT(super_type.IsNull() || super_type.IsFinalized());

https://codereview.chromium.org/26344006/diff/27001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:908: result ^= result.Canonicalize();
I would canonicalize after checking for a bound error.

https://codereview.chromium.org/26344006/diff/27001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/26344006/diff/27001/tests/lib/lib.status#newc...
tests/lib/lib.status:167: mirrors/generic_f_bounded_test: RuntimeError
On 2013/10/11 01:04:23, Ryan Macnak wrote:
> Fails because we somehow got two distinct Types for the class Real.

I guess this is due to canonicalization of f-bounded types? You should file a
bug and reference it here.

[rmacnak, 2013-10-11 01:48:01]

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

https://codereview.chromium.org/26344006/diff/27001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:889: ASSERT(super_type.IsFinalized());
On 2013/10/11 01:28:59, regis wrote:
> You could write
> ASSERT(!super_type.IsType() || super_type.IsFinalized());
> 
> But in what cases can you encounter a non-finalized super type?
> The only case you need to keep in mind is that the super type of Object is
null.
> Therefore, this should work:
> ASSERT(super_type.IsNull() || super_type.IsFinalized());

Yep, that holds. Also explains why I had trouble printing what the non-Type
thing was...

https://codereview.chromium.org/26344006/diff/27001/runtime/lib/mirrors.cc#ne...
runtime/lib/mirrors.cc:908: result ^= result.Canonicalize();
On 2013/10/11 01:28:59, regis wrote:
> I would canonicalize after checking for a bound error.

Done.

https://codereview.chromium.org/26344006/diff/27001/tests/lib/lib.status
File tests/lib/lib.status (right):

https://codereview.chromium.org/26344006/diff/27001/tests/lib/lib.status#newc...
tests/lib/lib.status:167: mirrors/generic_f_bounded_test: RuntimeError
On 2013/10/11 01:28:59, regis wrote:
> On 2013/10/11 01:04:23, Ryan Macnak wrote:
> > Fails because we somehow got two distinct Types for the class Real.
> 
> I guess this is due to canonicalization of f-bounded types? You should file a
> bug and reference it here.

Done.

[rmacnak, 2013-10-11 01:51:18]

Committed patchset #6 manually as r28495 (presubmit successful).

[ahe, 2013-10-11 12:43:33]

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

https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
runtime/lib/mirrors_impl.dart:489: ? _supertype(_reflectedType)
On 2013/10/11 01:04:22, Ryan Macnak wrote:
> On 2013/10/10 16:53:38, ahe wrote:
> > On 2013/10/09 01:15:00, Ryan Macnak wrote:
> > > Unfortunately, this is not the same as reflecting on the instantiated
> > supertype
> > > and getting its original declaration.
> > 
> > Why?
> 
> Consider
> class S<T>
> class C<R> extends S<R>
> 
> The superclass of the declaration of C is S<R>, not the declaration of S.

I'm not sure I understand this. What is the difference between _supertype and
_supertypeInstantiated?

[rmacnak, 2013-10-11 22:12:44]

On 2013/10/11 12:43:33, ahe wrote:
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> File runtime/lib/mirrors_impl.dart (right):
> 
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> runtime/lib/mirrors_impl.dart:489: ? _supertype(_reflectedType)
> On 2013/10/11 01:04:22, Ryan Macnak wrote:
> > On 2013/10/10 16:53:38, ahe wrote:
> > > On 2013/10/09 01:15:00, Ryan Macnak wrote:
> > > > Unfortunately, this is not the same as reflecting on the instantiated
> > > supertype
> > > > and getting its original declaration.
> > > 
> > > Why?
> > 
> > Consider
> > class S<T>
> > class C<R> extends S<R>
> > 
> > The superclass of the declaration of C is S<R>, not the declaration of S.
> 
> I'm not sure I understand this. What is the difference between _supertype and
> _supertypeInstantiated?

supertype(C<int>) = S<R>
supertypeInstanitated(C<int>) = S<int>

[ahe, 2013-10-14 12:21:04]

On 2013/10/11 22:12:44, Ryan Macnak wrote:
> On 2013/10/11 12:43:33, ahe wrote:
> >
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> > File runtime/lib/mirrors_impl.dart (right):
> > 
> >
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> > runtime/lib/mirrors_impl.dart:489: ? _supertype(_reflectedType)
> > On 2013/10/11 01:04:22, Ryan Macnak wrote:
> > > On 2013/10/10 16:53:38, ahe wrote:
> > > > On 2013/10/09 01:15:00, Ryan Macnak wrote:
> > > > > Unfortunately, this is not the same as reflecting on the instantiated
> > > > supertype
> > > > > and getting its original declaration.
> > > > 
> > > > Why?
> > > 
> > > Consider
> > > class S<T>
> > > class C<R> extends S<R>
> > > 
> > > The superclass of the declaration of C is S<R>, not the declaration of S.
> > 
> > I'm not sure I understand this. What is the difference between _supertype
and
> > _supertypeInstantiated?
> 
> supertype(C<int>) = S<R>
> supertypeInstanitated(C<int>) = S<int>

So the problem is that _reflectedType is C<dynamic> when isOriginalDeclaration
is true, which leads to S<dynamic> if you don't have this special case?

[rmacnak, 2013-10-14 16:50:14]

On 2013/10/14 12:21:04, ahe wrote:
> On 2013/10/11 22:12:44, Ryan Macnak wrote:
> > On 2013/10/11 12:43:33, ahe wrote:
> > >
> >
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> > > File runtime/lib/mirrors_impl.dart (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/26344006/diff/11001/runtime/lib/mirrors_impl....
> > > runtime/lib/mirrors_impl.dart:489: ? _supertype(_reflectedType)
> > > On 2013/10/11 01:04:22, Ryan Macnak wrote:
> > > > On 2013/10/10 16:53:38, ahe wrote:
> > > > > On 2013/10/09 01:15:00, Ryan Macnak wrote:
> > > > > > Unfortunately, this is not the same as reflecting on the
instantiated
> > > > > supertype
> > > > > > and getting its original declaration.
> > > > > 
> > > > > Why?
> > > > 
> > > > Consider
> > > > class S<T>
> > > > class C<R> extends S<R>
> > > > 
> > > > The superclass of the declaration of C is S<R>, not the declaration of
S.
> > > 
> > > I'm not sure I understand this. What is the difference between _supertype
> and
> > > _supertypeInstantiated?
> > 
> > supertype(C<int>) = S<R>
> > supertypeInstanitated(C<int>) = S<int>
> 
> So the problem is that _reflectedType is C<dynamic> when isOriginalDeclaration
> is true, which leads to S<dynamic> if you don't have this special case?

Yes.