parceler icon indicating copy to clipboard operation
parceler copied to clipboard

Generic collection isn't being cast, causes compile error

Open DSteve595 opened this issue 7 years ago • 14 comments

With this simple class:

@Parcel
public class MyModel<T> {
    Set<Long> someLongs;
}

I'm getting the same error reported in #293. someLongs is indeed a Set<Long>, but the generated class can only see it as a Set<Object>. In 1.1.6, the generated code manually cast it to Set<Long>, which worked. In 1.1.7 through 1.1.11, the cast is gone, so compilation fails.

DSteve595 avatar Aug 07 '18 19:08 DSteve595

This is because you're asking Parceler to generate a Parcelable wrapper for an undefined generic type which resolves to Object. How are you using MyModel? Do you have a concrete implementation that extends it?

johncarl81 avatar Aug 07 '18 20:08 johncarl81

Appreciate the quick response! I'm not using MyModel anywhere, I created it to try to boil down a model in a large project to just the part that was causing the issue. I still tested it in that project, but it isn't referenced or extended anywhere. Could the project's setup/dependencies possibly be an issue?

DSteve595 avatar Aug 07 '18 20:08 DSteve595

Ah, just reread the issue. Adding the generic <T> seem to cause the problem... something is happening here, ill have to dig in further.

johncarl81 avatar Aug 07 '18 20:08 johncarl81

This is interesting... Java issues a compiler error for defined generics in undefined genericised classes:

public class Tester {

    public static class Pass {
        Set<String> stuff;
    }

    public static class Fail<T> {
        Set<String> stuff;
    }
    
    public static class PassAgain extends Fail<Integer> {}

    public void run() {
        for (String stuff : new Pass().stuff) {}  // Good
        for (String stuff : new Fail().stuff) {}  // Compiler error
        for (String stuff : new Fail<>().stuff) {}  // Good
        for (String stuff : new Fail<Object>().stuff) {}  // Good
        for (String stuff : new PassAgain().stuff) {}  // Good
    }
}

... I guess because of type erasure?

johncarl81 avatar Aug 07 '18 21:08 johncarl81

That's really weird. I wish I could help more, no idea why that would be happening. I'm sure there's some arcane historic JVM-related reason. Adding a cast should be a decent fix though, right? At worst, it'll be unnecessary (and probably compiled out).

DSteve595 avatar Aug 07 '18 21:08 DSteve595

We actually removed the cast a while ago because it was typically unnecessary. Here's another fun case:

public class Tester {

    public static class Fail<T> {
        Set<String> stuff;
    }
    
    public static class Referencing {
        Fail<Object> fail;
    }

    public void run() {
        for(String stuff : new Referencing().fail.stuff) {} // Good
    }

}

I wonder what a fix for this would look like. Honestly you shouldn't annotate a class with undefined generic parameters. Maybe we should issue an error in this case instead of attempting to generate code with errors?

johncarl81 avatar Aug 07 '18 21:08 johncarl81

If it matters, in our codebase it's closer to public class MyModel<T extends MyInterface>.

DSteve595 avatar Aug 07 '18 21:08 DSteve595

In your codebase how is it used? Are there concrete implementations or is it referenced in a non-generic way as a field type (or something similar)?

johncarl81 avatar Aug 07 '18 22:08 johncarl81

There are no subclasses, not totally sure if I'm following your examples, but essentially the generic type is an interface with an ID property, and we want those IDs in the parcel.

DSteve595 avatar Aug 07 '18 23:08 DSteve595

Sounds like it's the following case then:

@Parcel
public class SomeClass {
    MyModel<String> model;
}

johncarl81 avatar Aug 08 '18 19:08 johncarl81

That might work, but I'm still not sure why the code generated for the original case is bad.

DSteve595 avatar Aug 08 '18 22:08 DSteve595

Sorry, the above case would not work, as long as MyModel<T> is annotated with @Parcel.

johncarl81 avatar Aug 08 '18 23:08 johncarl81

No chance of the cast being reinstated?

DSteve595 avatar Aug 09 '18 00:08 DSteve595

I'm not eager to reinstate it. There is a deeper problem here that needs a more direct solution. I think the answer lies in understanding why generics behave in this way.

johncarl81 avatar Aug 09 '18 01:08 johncarl81