Fix ConstructorConstructor creating mismatching Collection and Map instances

[Marcono1234]

Fixes #419
Fixes #1708
Fixes #2730 (the proper exception that interfaces cannot be instantiated will be thrown instead)

Gson's ProtoTypeAdapter (in the non-published proto module) indirectly relied on ConstructorConstructor's broken behavior:
During deserialization of 'repeated' fields it peeks at the type of the internal Java field. The problem is that this field does not have the type List<E> is all cases but for some types has specialized Protobuf subinterfaces. For example for List<Long> the internal field has the type com.google.protobuf.Internal.LongList.
So previously Gson erroneously created ArrayList instances even though Protobuf's LongList is not related to ArrayList. This worked because LongList is only an implementation detail, the Protobuf builder API seems to expect just List<E> so it did not cause any problems that Gson used an ArrayList. (Still the behavior that Gson created ArrayList when asked for a LongList is arguably wrong.)

[eamonnmcmanus]

The current behaviour seems hard to understand, but I am concerned if we change it to be more logical we may break projects that are implicitly depending on the way it is now. I ran this PR against Google's internal tests and encountered four failures in four unrelated projects. All appeared to be related to protobufs, some directly via the ProtoTypeAdapter in this project and others via what appear to be modified clones of that class. My inclination is to leave this alone.

[Marcono1234]

Hmm, ok that is unfortunate. I have split EnumMap support into a separate pull request (#2071). If you want you can close this pull request here then, if you don't think there is any chance this will be integrated.

[Marcono1234]

@eamonnmcmanus, can / should I revive this pull request? As seen with #2730, the current behavior is broken and leads to quite confusing exceptions, for example in cases where you deserialize a custom List subtype but Gson creates an ArrayList instead.
Only in rare cases this actually works, where the user code is not completely type-safe.

In case this still applies to the Protobuf handling, then if necessary we could hardcode support for that, checking the class name and using reflection.

[eamonnmcmanus]

OK, if you want to bring this up to date I will retry the experiment of running it against Google's tests and we can possibly come up with an adjustment that makes those tests pass.

[Marcono1234]

Have updated this pull request now, and also tried to solve the Protobuf issue in a (hopefully) reasonable way, see changes in ProtoTypeAdapter.

However, it will require that users update their local copy of ProtoTypeAdapter.
Other ways of solving this would probably require special hardcoded Protobuf fallback behavior in ConstructorConstructor, which is possible, but probably not a very clean approach.

[Marcono1234]

It seems all other fromJson methods delegate to this one, so I have added the type check (which was previously Primitives.wrap(classOfT).cast(object) in the other fromJson methods) here instead.
However, this method did not actually have this check before.

[eamonnmcmanus]

I ran this change against all of Google's internal tests and found a lot of failures with this exception, or with an exception about not being able to instantiate the abstract class com.google.common.collect.ImmutableList. In both cases I think the code in question should be fixed, because it is relying on accidental behaviour. A pattern I saw quite often was this:

@AutoValue
public abstract Foo {
  public abstract ImmutableList<Bar> bars();

  public static TypeAdapter<Foo> typeAdapter(Gson gson) {
    return new AutoValue_Foo.GsonTypeAdapter(gson);
  }

  @AutoValue.Builder
  public interface Builder {
    public Builder setBars(List<Bar> bars);
    public Foo build();
  }
}

This is using auto-value-gson to serialize and deserialize AutoValue classes. It works even if no TypeAdapterFactory has been registered for ImmutableList, because Gson deserializes a plain List. Auto-value-gson constructs the Foo instance using its builder, and the deserialized List works with the setBars method. That method calls ImmutableList.copyOf(List) so we do get the right ImmutableList type. But the additional checks in this PR don't let us get that far.

As I say, code like that only works accidentally. My plan is to add a GuavaCollectionsTypeAdapterFactory to extras and update the internal code to use that. I already found several custom ImmutableListTypeAdapterFactory and ImmutableMapTypeAdapterFactory classes which could be switched to use this.

Some of the other failures were just plain using the wrong types, which again happened to work through erasure and the like.

So in short, I think this change is OK, but it may require some client code to be updated. Most of the updates should be straightforward and will lead to client code that is more correct.

I think it will be a while before we can complete this because of all the adjustments that will need to be made.

[Marcono1234]

Not completely sure if this custom ClassCastException is really worth it, maybe a Class#cast call would suffice, as done before.

This check will only help when directly using fromJson for an incorrect adapter, but if the adapter is used indirectly (for example for a list value) it would still cause other less descriptive exceptions later.

[Marcono1234]

In #419 (comment) the concern was mentioned that the fallback will be Unsafe which might leave the Collection or Map implementation in a broken state because fields have not been initialized. And the built-in Collection and Map adapters might then encounter confusing exceptions when trying to add elements.

Not sure if we should throw an exception then in case of Collection or Map? Or we could maybe add an allowUnsafe parameter here (which is considered in addition to GsonBuilder#disableJdkUnsafe), which the built-in Collection and Map adapters can set to false because calling add and put will likely fail afterwards.

[Marcono1234]

There is some overlap between the newly added tests here and in ConstructorConstructorTest, should we keep both nonetheless?

[eamonnmcmanus]

I ran this against all of Google's internal tests and there were several failures. I plan to investigate in more detail next week.