No more working with `reflectable` version 0.3.4

[dam0vm3nt]

Since reflectable version 0.3.4 annotating properties of type List with @property leads to the following exception :

 Uncaught Unhandled exception:
Unsupported operation: Cannot provide `reflectedType` of instance of generic type 'List'.
#0      InstantiatedGenericClassMirrorImpl.reflectedType (package:reflectable/src/reflectable_transformer_based.dart:713:5)
#1      _getPropertyInfoForType (package:polymer/src/common/polymer_descriptor.dart:210:46)
#2      _buildPropertiesObject.<anonymous closure> (package:polymer/src/common/polymer_descriptor.dart:58:24)
#3      _HashVMBase&MapMixin&&_LinkedHashMapMixin.forEach (dart:collection-patch/compact_hash.dart:340)
#4      _buildPropertiesObject (package:polymer/src/common/polymer_descriptor.dart:56:16)
#5      createPolymerDescriptor (package:polymer/src/common/polymer_descriptor.dart:26:19)
#6      PolymerRegister.initialize (package:polymer/src/common/polymer_register.dart:19:36)
#7      loadInitializers.<anonymous closure>.<anonymous closure> (package:initialize/src/static_loader.dart:46:32)
#8      _runInitQueue (package:initialize/initialize.dart:35:24)
#9      _runInitQueue.<anonymous closure> (package:initialize/initialize.dart:38:26)
#10     _RootZone.runUnary (dart:async/zone.dart:1149)
#11     _Future._propagateToListeners.handleValueCallback (dart:async/future_impl.dart:502)
#12     _Future._propagateToListeners (dart:async/future_impl.dart:585)
#13     _Future._completeWithValue (dart:async/future_impl.dart:376)
#14     _Future._asyncComplete.<anonymous closure> (dart:async/future_impl.dart:430)
#15     _microtaskLoop (dart:async/schedule_microtask.dart:43)
#16     _microtaskLoopEntry (dart:async/schedule_microtask.dart:52)
#17     _ScheduleImmediateHelper._handleMutation (dart:html:49298)
#18     MutationObserver._create.<anonymous closure> (dart:html:27545)

For example with :

<dom-module id="example-list">
 <template>
   <h1>hello</h1>
 </template>
</dom-module>

and dart :

@PolymerRegister("example-list")
class ExampleList extends PolymerElement {
  @property List<String> values = [
    "one",
    "two",
    "three"
    ];
  ExampleList.created() : super.created();
}

while forcing to version 0.3.3 make it work again.

[zoechi]

Same here. See also http://stackoverflow.com/questions/33900888/reflectable-0-3-4-new-version-throwing-exception

[eernstg]

Reflectable 0.3.4 supports many things associated with genericity (whereas 0.3.3 just did a few things to avoid crashing when encountering a generic type), so the general situation should be much better.

However, one thing that 0.3.3 couldn't, 0.3.4 can't, and no upcoming version of reflectable is expected to be able to do for a while is to return the actual type arguments of an instantiation of a generic class, also known as a parameterized type (e.g., List is an instantiation of List): The actual type arguments are not not statically known in general, and there is no primitive at runtime (except for built-in reflection) providing access to actual type arguments. E.g., if we have the Type value t corresponding to List<int>, there is no way we can get our hands on the corresponding Type value for int from that. We can't even test dynamically whether t is a subtype of List<S> for any given S, not even if we know that S == dynamic. Because of this, typeArguments on an instantiation of a generic class will fail with an UnimplementedError.

In order to implement typeArguments and originalDeclaration etc. correctly (e.g., typeArguments on an originalDeclaration must simply return the empty list), we had to distinguish explicitly between generic classes and instantiations of generic classes (so we distinguish between the generic class List which is ready to take a type argument, and instantiations thereof such as List<int>; originalDeclaration delivers a mirror of the former given a mirror of the latter).

Generic classes _do_nothave a reflected type, which is by the way also the response you'll get from 'dart:mirrors', so hasReflectedType is false and anyone trying to get reflectedType from such an entity must get an exception.

Unfortunately, these two issues sometimes conspire to make mirrors of instantiations of generic classes (e.g., a mirror of List<int>) unable to provide a reflectedType as well. If you evaluate myReflector.reflect(o) where o is an instance of List<int> then we can harvest the Type value for List<int> and store it in the resulting class mirror for List<int>. On the other hand, if you get a class mirror for List<int> from any other source than an instance of List<int> then we don't have access to the Type value that reflectedType should return.

Say, your program might have a method with an argument of type List<int>, and you might obtain a mirror for that by getting myMethodMirror.parameters[0].type. Your program might use objects of type List<dynamic> or the value null every single time this method is called, or maybe it is never called. So maybe your program heap never contains any instances of List<int>, so there is no hope that we can just go and find the right Type object for List<int> anywhere, it doesn't exist. Hence, we cannot return the Type object for List<int> in this situation (and that's on top of the initial problem, that we cannot find these type objects even if they exist).

This could very well be the situation that you have encountered: The program is asking for the reflectedType of an instantiation of a generic class C, say C<T>, and the mirror for C<T> was not obtained from an instance of C<T>.

The first step in fixing this problem could be to ask hasReflectedType before calling reflectedType and thus avoid the exception, but that of course just leaves us with the problem: what to do then?

If you wish to check things like "is the object o actually an instance of the type annotation of that parameter?" then you may be in trouble. In particular, if your argument is of type List<int> and the argument is declared to have type List<E> inside an instance of a generic class with header class C<E> .. then we would need to extract the actual type argument 's' of the receiver object (because that's the actual value of E in this situation), and then we'll need to build the Type object for List<s>, and then you'd need to evaluate o is List<s>. None of these things are supported by the runtime today.

In 0.3.3 you could get away with it: The generic class class List<E>.. and its instantiation List<int> would both consider a plain List to be their reflectedType, so all class mirrors would have a reflectedtype. That's plain wrong, though, if you get into scenarios like o is List<s> mentioned above, so we can't go back to that.

But it might be helpful to add an is method to class mirrors (we'd need to find a reasonable name, because is is a keyword), which could at least offer a test for o is C<T> in the cases where T is a type expression that does not contain type variables.

We might also add a method dynamicReflectedType method on class mirrors which would return List for an instantiation List<T> for all T. It won't be easy to use such a thing safely, but it is at least well-defined.

Another possibility is that Polymer only needs to cover a fixed set of classes, in which case the hasReflectedType == false case can be handled "manually", by special casing code that directly tests for List etc.

[zoechi]

So, not using generic type arguments would be a workaround?

[eernstg]

It may not be enough. If it just means using C as a type annotation rather than C<Something> then it won't help, because C in that context is just a shorthand for C<dynamic>.

[dam0vm3nt]

(EDITED) This test case is failing even though the annotated field is of plain List type with no type parameters.

I think there are use cases (like polymer-dart) where something like ’dynamicReflectedType’ will be necessary, or more generally an approach similar to 0.3.3.

Also using generics is a good practice and that means that reflectable could become not very useful in a context where they are widely used.

[dam0vm3nt]

Also I don't fully agree on the opinion that returning List as relfectedType for a List<X> should be entirely wrong.

After all I can always call method like say length regardless of what is X. Even tough List<X> identifies a family of classes rather than a specific type they all share a common anchestor interface i.e. the one obtained when type parameters are erased i.e List<dynamic>.

[jakemac53]

@eernstg The behavior is also now inconsistent, the mirrors implementation will still return List, and the transformer based version will throw.

[jakemac53]

I have also confirmed that in regular dart:mirrors both of the following have a reflectedType:

import 'dart:mirrors';

main() {
  List<String> foo = [];
  print(reflect(foo).type.reflectedType); // Prints `List`
  print(reflectClass(Foo).declarations[#bar].type.reflectedType); // Prints `List<String>`
}

class Foo {
  List<String> bar;
}

[eernstg]

I've experimented with dynamicReflectedType, which would be available for class mirrors (isOriginalDeclaration as well as "isnt"), parameter mirrors, variable mirrors, and for the return type of method-mirrors. That method would consistently use the fully dynamic instantiation of all generic types, and programmers would then be able to make the trade-off.

[jakemac53]

Added a version constraint and released a new version to alleviate the problem for now. This can be removed once we figure out a workaround.

[dam0vm3nt]

@jakemac53 :+1:

[eernstg]

Good! Btw. dynamicReflectedType is upcoming, and that's probably enough to fix it.

[jakemac53]

Yes, I think that will fix the issue as well

[eernstg]

There is a dynamicReflectedType in CL https://codereview.chromium.org/1473073009/.

[eernstg]

https://github.com/dart-lang/reflectable commit 10da7e398ce439314dacfb276e2a58ecbabce5b4 contains dynamicReflectedType.

[eernstg]

Reflectable 0.4.0 has been published, containing the above-mentioned CL as well as another one that changes the implementation to share multiple occurrences of the same type expression (which reduces the size of the source code).

It also adds bestEffortReflectedType which will try reflectedType and then dynamicReflectedType. This is very close to the 0.3.3 semantics of reflectedType, but now it's explicit that this is on shaky ground: reflectedType and dynamicReflectedType are semantically different, so you have to be careful when reasoning about the meaning of bestEffortReflectedType.

I suspect that you may be able to simply change reflectedType / hasReflectedType to bestEffortReflectedType / hasBestEffortReflectedType, but you may also want to scrutinize each invocation and then make an explicit choice among reflectedType and dynamicReflectedType, in order to avoid the more complex semantics of ..estEffort...

The tricky point is that dynamicReflectedType cannot be supported in pre-transform code in some cases, because we have no primitives for applying a given generic class to a given list of type arguments; and reflectedType cannot be supported in post-transform code when it requires an application of a generic class to a type argument (e.g., we cannot create a Type object for a type like List<E> when it is used inside the class List, not even in the case where we have an instance of, say, List<int> and we have navigated to List<E> via an instance mirror, then class mirror, then method mirror, then parameter mirror, then .type on that, because we do not have the primitive to look up that the type parameter is int, and on top of that we do not have the primitive to apply List to that type argument). So, unfortunately, you can't simply assume that the scope of dynamicReflectedType (i.e., the set of cases where hasDynamicReflectedType returns true) is larger than the scope of reflectedType, nor vice versa.

If you focus on transformed code, though, the scope of dynamicReflectedType is larger than the scope of reflectedType.

We gave this version a new major number because reflectedType has a more strict semantics than it used to have (so hasReflectedType will return false more often). The breaking changes in this area happened already in 0.3.4, so you could argue that we should nuke that version. We haven't done that because there are strong recommendations against nuking a published version.

[jakemac53]

Great, I can work on updating Polymer soon.

In terms of the accidental breaking change in 0.3.4, usually the process would be to just release a new version (0.3.5, or 0.3.4+1) which reverts back to 0.3.3.

[eernstg]

It would be most useful if we were to create a new version 0.3.5-or-so which includes all the fixes in 0.3.4 and just makes reflectedType emulate 0.3.3 (CHANGELOG.md for 0.3.4 is rather long). I'll think about it. ;-)

[jakemac53]

Yes, that would be the ideal. It is probably only worth doing if you are getting non-polymer users who ran into this issue though. Mostly I just wanted to highlight that as the normal procedure for this sort of thing :)

[eernstg]

Right, I think I'll leave it as it is. There is an inconsistency lurking in the dark, but we did after all consider the changes from 0.3.3 to 0.3.4 to be bug fixes and implementations of missing features, including the behavior of reflectedType. ;-)

[jakemac53]

Sure, but a even a bug fix is a breaking change if it changes behavior :). I just don't think that it is worth fixing it unless its causing an issue. Polymer has already released a new version which works with 0.4.0, so if nobody else is filing issues you are probably ok.

[eernstg]

Done deal. ;)

[donny-dont]

@eernstg I think I found another case where 0.3.4 or later breaks stuff that was working in 0.3.3. If you have a behavior that's generic then it will fail to compile it.

abstract class FooBehavior<T> {
  ...
}

class FooElement extends PolymerElement with FooBehavior<MyModel> {
  ...
}

[jakemac53]

This could very well be an issue with Polymer itself as well, I need to add some tests around this type of thing.

[donny-dont]

It was working with 1.0.0-rc.6 and reflectable 0.3.3. This sounded similarish. More than happy to open a new issue.

[dam0vm3nt]

@jakemac53 I had a similar problem (see conversation on slack) and I don't think the problem is in polymer. Infact if you look at what is generated by the reflectable transformer you will find that the generated code is messed up and it doesn't compile.

For example there are things like : o is! FooElement with FooMixin.

[donny-dont]

I was specifically getting whenever using a generic mixin there.

web\main.dart:29:7:
A final variable must be initialized.
Try adding an initializer or removing the 'final' modifier.

[dam0vm3nt]

@donny-dont look at the whole output you should see the messed up generated code. The error you are seeing (final variable not initialized) is not the real problem but a consequence of the fact that the initilizing code is messed up.

[eernstg]

Clearly reflectable should not generate o is! FooElement with FooMixin, so there's a bug. There is only one place where this expression could have been generated; I have a fix for that problem upcoming. There may be more, of course..

[dam0vm3nt]

@eernstg wonderful, sorry I didn't saw your reply before posting the new issue #653 ... but there you can find more info that maybe can help you to find the problem.

[eernstg]

You can do git clone 'https://github.com/dart-lang/reflectable.git' now and put something like

  dependencies:
    reflectable:
      path: ../the/path/to/reflectable

into your 'pubspec.yaml', and try it out with the version of reflectable that contains a fix for the code generation bug (0f06199f91c49601ba1133a0a8f42cd0faa0d4b9).

[dam0vm3nt]

roger.

[dam0vm3nt]

ok now it works. tnx.

[dam0vm3nt]

and BTW : seams also a lot faster ... maybe a suggestion.

[eernstg]

Cool! I'm not sure why it might be faster, though.