Generated 'from' method has trouble with parameterized interfaces that declare properties

[stevenschlansker]

Hi, we use Immutables and love it, but recently ran into what feels like a bug.
We declare some properties on mixin interfaces so they can be shared among related types, and use from(otherImpl) liberally to copy all relevant properties between the different sub-types. In some cases, we parameterize the types Mixin<Self extends Mixin<Self>> in order to give proper return types to Self withProperty() methods.

However, this seems to break the from() method in some cases. Consider:

interface LastModified<Self extends LastModified<Self>> {
    Instant lastModified();
    Self withLastModified(Instant lastModified);
}

interface Property {
    @Nullable
    String property();
}

@Value.Immutable
interface ImplA extends Property, LastModified<ImplA> {}

@Value.Immutable
interface ImplB extends Property, LastModified<ImplB> {}

public class TestImmutableFrom {
    @Test
    public void testImmutableFrom() {
        final ImmutableImplA a = ImmutableImplA.builder()
                .lastModified(Instant.EPOCH)
                .build();
        ImmutableImplB.builder()
                .from(a)
                .build();
    }
}

Expected:
Since both ImplA and ImplB extend LastModified, and while the parameterization of the type is different, it does not affect the property in any way, I would expect the from method to copy the property.

Actual:
The generated ImmutableImplBBuilder.from method unfortunately seems to not be correct.

private void from(short _unused, Object object) {
  @Var long bits = 0;
  if (object instanceof Property) {
    Property instance = (Property) object;
    if ((bits & 0x1L) == 0) {
      @Nullable String propertyValue = instance.property();
      if (propertyValue != null) {
        property(propertyValue);
      }
      bits |= 0x1L;
    }
  }
  if (object instanceof ImplB) {
    ImplB instance = (ImplB) object;
    if ((bits & 0x1L) == 0) {
      @Nullable String propertyValue = instance.property();
      if (propertyValue != null) {
        property(propertyValue);
      }
      bits |= 0x1L;
    }
    lastModified(instance.lastModified());
  }
}

Since ImplB already implements Property, the two instanceof clauses are redundant. The second check should probably check for object instanceof LastModified.

[elucash]

It may be a little bit weird, so what it does, it consider each supertype and then the same type and uses runtime bits to avoid pulling things already set. It doesn't do good analysis of what properties subtypes are covered and which are not (again - runtime check). And I don't remember for sure, but we're probably skipping supertypes with Generic parameters to avoid unsoundness/heap pollution. In the given example, it seems clear that the check like if (object instanceof LastModified<?>) and pulling lastModified() should be fine (as no type parameters used on this accessor). So maybe this can be fixed, given that generic will be properly handled (i.e. allow only wildcard like from(LastModified<?> ) and only using attributes with no type parameter used in return type). I guess this seemed a bit not trivial, so were originally omitted. I'll revisit this part to see how hard it will be to improve the handling, but it will take time

[stevenschlansker]

Thanks so much for looking at this. In the meantime, we refactored our code to avoid this specific problem, so we are not in a rush to get it fixed. But it feels like a pitfall other users might run into as well, even if it is a bit of an edge case.