TypeReference hack to ensure non-raw usage doesn't actually work.

[rzwitserloot]

The source of TypeReference.java refers in its javadoc to specifically this comment from Neal Gafter's blogpost about Super TypeTokens:

Brian Oxley said... Rather than check for a type parameter in the constructor, you can make it a syntax error to leave out:

public abstract TypeReference<R> implements Comparable<TypeReference<R>> {
  // ...
  
  public int compareTo(TypeReference<R> o) {
    // Need a real implementation.
    // Only saying "return 0" for illustration.
    return 0;
  }
}

Now this is legal:

new TypeReference<Object>() { };

But this is a syntax error for not implementing Comparable:

new TypeReference() { };

February 20, 2007 6:12 AM

The problem is, that comment is utter hogwash and it does absolutely nothing. And yet, jackson's TypeReference code copies this idea.

To reproduce, make a blank dir and save this to Test.java:

import java.lang.reflect.*;

abstract class TypeReference<T> implements Comparable<TypeReference<T>> {
  protected final Type _type;
  protected TypeReference() {
    Type superClass = getClass().getGenericSuperclass();
    if (superClass instanceof Class<?>) { // sanity check, should never happen
      throw new IllegalArgumentException("Internal error: TypeReference constructed without actual type information");
    }
    _type = ((ParameterizedType) superClass).getActualTypeArguments()[0];
  }

  @Override public int compareTo(TypeReference<T> o) { return 0; }

  public static void main(String[] args) {
    TypeReference raw = new TypeReference() {};
  }
}

(That is the current TypeRef code, with a main that tests the theory that the Comparable 'trick' does anything useful).

Then:

javac Test.java

and it just compiles. It shouldn't be compiling. (tried it on my system, with AdoptOpenJDK 14.0.1, and with OpenJDK8 as well. Perhaps this really did work in 6 or 7, but keeping this weirdness around for the sake of versions EOLed so long ago is perhaps not worthwhile).

SUGGESTED FIX:

• Adjust the javadoc by removing the commentary what Comparator is for and the reference to the comment, keeping the link to the STT blogpost, though.
• Remove implements Comparable<TypeReference<T>>.
• Remove the compareTo method implementation.
• Change the text of the thrown exception; remove the "Internal error: " part. Probably mention that a raw TypeReference isn't how you're supposed to use it, so that the programmer who runs into this gets a hint about what they are doing wrong.
• Remove the // sanity check comment.

Alternatively, first investigate ways to make this code fail at compile time (which is the original intent of this non-working 'hack'), but I don't think there's any way to make it happen without preprocessors annotation processor hackery.

NB: I can provide a PR for this trivial change; the principal time effort is presumably to investigate if removing Comparable is going to cause backwards compatibility issues and if the Jackson dev team is willing to give up on the dream of compile-time-checking for an actual type vs. spending some time trying to find another inventive hack. I thought it best to kickstart discussion first.

IMPACT ANALYSIS OF BUG: Light, it just confuses folks. If you try this stunt of making a 'raw' TypeReference, then you get no compiler error (even though one is intended). However, resolving the expression new TypeReference() {} still throws that 'Internal error' exception. Someone who does that by accident is presumably confused and would be helped out a little more with better exception text, but that's all.

IMPACT ANALYSIS OF FIX: Well, anybody who decided to use a TypeReference as a Comparator for some bizarre reason is going to find that this is technically backwards incompatible, but presumably you'd be doing them a favour by breaking code that makes no sense at all. This 'fix' also means the notion that raw TypeReference STTs are turned into compiler errors no longer works (but then it never did, presumably).

[rzwitserloot]

How I think the hack is supposed to work, is that due to erasure, your STT class is supposed to have an impl for signature public int compareTo(Object other), given that it's a raw type, but, the TypeReference class (the superclass of your STT) only has an implementation for public int compareTo(TypeReference), and therefore, is a compiler error.

A nice thought. But that's not how javac appears to work. If anyone is thinking of routes on how to achieve a compile-time error, this may be worth investigating a little further.

[cowtowncoder]

To be honest, aside from cleanup of Javadoc, I am not sure how spending time on changing code here (I agree that alleged benefit of Comparable impl does not exist) would actually improve user experience. I fully agree that much of premise of work-arounds from original code is bogus.

My main concern on usage -- based on user reports -- is just that users tend to assume Type Variables worked (as opposed to Type Erasure just eating them), so something like

   public static <T> TypeReference<T> makeReference() {
     return new TypeReference<T>() {} ;
   }
```

is attempted and users are surprised to find their code just binds to `LinkedHashMap`.

Other than that, I don't want to spend much time in this area. PRs for clean up might be ok, but you are right that my primary concern is backwards compatibility -- changes here are unfortunately very difficult to test since it all comes to down various existing usage in code outside of Jackson project.
We do not have much visibility into testing outside our projects, which is unfortunate. So we rely a lot on voluntary testing by users during Release Candidate phase: it is useful but in large context very limited.

[rzwitserloot]

If that's your concern, may I offer a solution?

You can do some fairly deep introspection, (unfortunately, short of extreme steps involving lombok-level annotation processors or preprocessors, only at runtime), of the typeref in the <>.

Presumably, the confusion is mostly avoided if you restrict the X in new TypeReference<X>() {} to consist of this recurisvely applied ruleset:

X may be:

• A non-parameterized, reified type, such as String, LocalDate, MyCustomThingie, etcetera.
• A parameterized, reified type, where each type param is an X. e.g. a List<String> is fine.
• [optional] Given that you sometimes have to sprinkle ? extends around to enforce covariance, I think it's less friction all around if any type param is also allowed to be ? extends X instead, which is then treated as just X by jackson both ways (when serializing or when deserializing). But, it has to be wildcards (?) and it has to be extends. No ? super X allowed, no ? extends T allowed.

Specifically, then, X may not be:

• Raw
• Type variable
• Open for debate, but presumably either ? or ? extends Object is sufficiently useless as to warrant disallowing? Tricky one.
• Contravariance (? super T)
• Combinations, such as ? extends Serializable & RandomAccess

Thus, all of these would cause an exception be thrown at runtime when the expression is resolved:

new TypeReference() {};
new TypeReference<T>() {};
new TypeReference<? super Integer>() {};
new TypeReference<Map<String, T>>() {};
new TypeReference<Map>() {};
new TypeReference<List<? extends Serializable & RandomAccess>() {};

I could try my hand at a single PR that addresses both points:

• Take the steps I laid out above to remove the non-working hack.
• Add the above described intelligence in order to throw an exception in the TypeReference constructor in cases such as the one @cowtowncoder just pasted (new TypeReference<T>() {};).

[cowtowncoder]

Yes, that sounds good in general.

One practical consideration: I suspect most of handling needs to be in jackson-databind, specifically in TypeFactory. This type is only declared here in jackson-core due to needs of API interaction b/w streaming and databinding, but all type resolution occurs in or through databind level TypeFactory.

[rzwitserloot]

@cowtowncoder two feasible routes then:

• Keep the simplest possible logic in jackson-core, suitable to generate a exception with an informative error message upon detecting an STT that isn't going to work out, but nothing more. This does mean there is a modicum of code duplication between jackson-databind and jackson-core, -or-
• Move the code that extracts the actual intended type from an STT to jackson-core, and then have jackson-core's TypeReference use this code to throw the exception, and refactor jackson-databind to use the utility method in core.

I'm not particularly familiar with the jackson codebase. The first option feels like I can probably contribute a useful PR. The second is a bit more tricky due to the need of refactoring things and figuring out what, precisely, jackson-databind actually needs (I know precisely what is needed to produce a useful error message. I merely have an inkling of what jackson-databind needs to (un)marshall data into and out of the POJOs, but when programming, that's quite a big gap, of course).

So, naturally, I'd strongly prefer #1, but it's your code base, and #1 definitely does introduce both some DRY violations and a chance of discrepancy (e.g. where jackson-core lets an STT pass without an exception, but jackson-databind throws something when POJOifying due to the STT not being up to its needs).

[cowtowncoder]

Maybe we can experiment with #1? A little bit of duplication is fine, although I guess it is easier to comment when seeing more concrete case. Changes to TypeFactory might be tricky, too; in some ways it would probably be more natural place as code already handles type variables, bounds and all that stuff, but at the same time there would now be new state to carry around to differentiate between TypeReference that does not allow type variables (and a few other constructs) at all, vs. other sources of java.lang.reflect.Type that do allow these. And making that change through processing chain sounds like a plausible source of hard-to-find regressions for change that for most users adds no value (if you already use TypeReferences the way they are intended).

[cowtowncoder]

Not sure this is worth pursuing; will close.