Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement TypeMirror#typeArguments in transformer #79

Closed
lassedamgaard opened this issue Mar 2, 2016 · 4 comments
Closed

Implement TypeMirror#typeArguments in transformer #79

lassedamgaard opened this issue Mar 2, 2016 · 4 comments
Labels

Comments

@lassedamgaard
Copy link

Porting over some code that needs access to generic type variables because MirrorsUsed is a strange beast when doing that, I find out at the very end of my endeavour (#77 and #78) that typeArguments is not implemented in the reflectable transformer. Oh the irony...

Anyway, this would be super neat to have!

On a more general note, consider clearly documenting which features are not supported in the transformer and/or having an option on the capability specification (my Reflectable subclass) to throw when running in the Dart VM if you try to use an unimplemented feature.

@eernstg
Copy link
Collaborator

eernstg commented Mar 2, 2016

On unsupported features: At the bottom of the page https://github.com/dart-lang/reflectable/tree/master/reflectable there is a section about 'Known Limitations'. It does mention that type arguments (that is, actual arguments to a generic entity) are not supported except in trivial cases; the problem is that we don't have a primitive which will allow us to get access to that data without using anything from 'dart:mirrors' (in short, given a Type for a generic class C<X> and a type for D, we cannot dynamically create a type for C<D>; and given a type for C<D> we cannot dynamically obtain a type for D).

On the type variables (that is, formal type parameters declared on a generic entity): They should be supported (with a TypeCapability).

@eernstg
Copy link
Collaborator

eernstg commented Mar 2, 2016

You wouldn't have the opportunity to use a workaround, would you?

class TypeArgumentReifier1<X> {
  Type get typeArguments => <Type>[X];
}

class TypeArgumentReifier2<X1, X2> {
  Type get typeArguments => <Type>[X1, X2];
}

...

class MyClass<E, W> implements TypeArgumentsReifier2<E, W> {
  ...
}

You would then be able to use if (x is TypeArgumentsReifier2) { ... x.typeArguments ... }, which is of course out of style along with otherwise reflective code, but it might work OK in practice.

@lassedamgaard lassedamgaard changed the title Implement TypeMirror#typeVariables in transformer Implement TypeMirror#typeArguments in transformer Mar 2, 2016
@lassedamgaard
Copy link
Author

Sorry, I meant typeArguments, not typeVariables (which is implemented). I have updated the title and description.

The workaround looks legit, but in my case access to the type arguments is not important enough to justify such a workaround polluting the whole codebase. It would just be the most elegant and generic (pun intended) solution but I can find a different route for now.

I did notice the mention in the list of known limitations but it wasn't (isn't) really clear to me what constitutes a 'trivial' case. Further, the known limitations talk of 'parts of the library' that have not been implemented so I expected it to fail before running the transformed code. My main here objection here is that it's not clear that this will fail until you try to run the transformed code but not e.g. in unittests. Having a workflow were you need to constantly test the transformed code to avoid such pitfalls is not optimal.

@eernstg eernstg added the wontfix label Mar 9, 2016
@eernstg
Copy link
Collaborator

eernstg commented Mar 9, 2016

The trivial cases are the ones that we implemented from the beginning (e.g., dynamic, void, and non-generic classes can all trivially respond to typeArguments by returning the empty list). At that point we expected to get the primitives needed in order to dynamically construct and destruct generic class instantiations (like the C<D> mentioned above). However, we didn't get those primitives, mainly because of the performance implications, and we won't get them anytime soon. This means that the existing typeArguments implementation is useless: It can return the empty list in a few trivial cases, and there isn't much hope that we can go beyond that within a reasonable amount of time. This is the reason why I suggested using a workaround.

You may be able to use yet another workaround in the case of type annotations: We have implemented support for obtaining Type values for type annotations representing their reflectedType. This is supported iff hasReflectedType is true (if it isn't then reflectedType will throw an UnsupportedError, not a NoSuchCapabilityError, because it is simply wrong to call reflectedType when hasReflectedType is false). A similar situation arises for reflectedReturnType and its brethren.

The cases that we can support are cases where a given type annotation receives only compile time constant types (that is, no type variables occur anywhere in the type annotation), and you only get the Type value (nothing with a structure that you can explore). But you can still do things like

  assert(myMethodMirror.hasReflectedReturnType);
  assert(myMethodMirror.hasDynamicReflectedReturnType);
  assert(myMethodMirror.dynamicReflectedReturnType == C);
  if (myMethodMirror.reflectedReturnType == const TypeValue<C<D>>().type) {
     // May rely on `myMethodMirror.returnType.typeArguments == <Type>[D]`.
    ...
  }

in order to detect that you have a case where the type argument is D. You have to use something like TypeValue because a plain C<D> is a syntax error here, but it's the same thing.

Obviously this is a very weak workaround, because you have to check that the base type is C, and then check the type argument list via the equality check on Type instances. You have to be really lucky and/or desperate in order to get much help from that. But who knows, it might happen. ;-)

I'll mark this issue as 'wontfix' and close it, in order to make this situation visible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants