Unchecked conversion warning in `JsonMapper` wrt `TreeCodec.readTree()` override

[ilgrosso]

Describe your Issue

When defining a class like as

public class SyncopeJsonMapper extends JsonMapper {
...
}

(since I need to override some methods), I am getting the build warning as follows:

[WARNING] readTree(tools.jackson.core.JsonParser) in tools.jackson.databind.ObjectMapper implements <T>readTree(tools.jackson.core.JsonParser) in tools.jackson.core.TreeCodec
  return type requires unchecked conversion from tools.jackson.databind.JsonNode to T

This happens with 3.0.3.

Adding

@SuppressWarnings("unchecked")

does not seem to help.

[JooHyukKim]

Did you find a workaround? @ilgrosso

[ilgrosso]

@JooHyukKim unfortunately, no
I hope someone can take a look and fix it

[cowtowncoder]

I don't see such warning, either on Eclipse or via Maven compile. So a reproduction of some kind would be needed.

Not sure what to do, even if reproducible, but at least should be able to see the warning first.

[ilgrosso]

@cowtowncoder here you go: https://github.com/ilgrosso/Jackson3UncheckedConversionReproducer

Just build via plain mvn clean install with JDK 21 and recent Maven.

You can check the GH action output showing the issue at https://github.com/ilgrosso/Jackson3UncheckedConversionReproducer/actions/runs/20453989782/job/58772276038#step:6:1967

Thank you.

[ilgrosso]

Moreover, when building https://github.com/FasterXML/jackson-databind/tree/3.0 from sources, the Maven output shows:

[INFO] /home/ilgrosso/work/jackson-databind/src/main/java/tools/jackson/databind/ObjectMapper.java: Some input files use unchecked or unsafe operations.
[INFO] /home/ilgrosso/work/jackson-databind/src/main/java/tools/jackson/databind/ObjectMapper.java: Recompile with -Xlint:unchecked for details.

[ilgrosso]

From my local tests, just turning TreeCodec's

public abstract <T extends TreeNode> T readTree(JsonParser p) throws JacksonException;

into

public abstract TreeNode readTree(JsonParser p) throws JacksonException;

without touching ObjectMapper nor ObjectReader seems to fix the issue - but of course I have very limited visibility on other side effects.

[pjfanning]

So ObjectMapper.java has:

    @SuppressWarnings("unchecked")
    @Override
    public JsonNode readTree(JsonParser p) throws JacksonException

So we suppress the type warning.

The types are erased at the end of the day. Is it just not feasible for everyone to suppress the warning? Extending ObjectMapper/JsonMapper is not something that many people do.

It is true that Jackson could change TreeCodec as described in #5485 (comment) but that is a breaking change. It may be safe to put in 3.1 but it might not.
The main question is if we want to keep the TreeCodec support for supporting JsonNode subclasses.

[ilgrosso]

So we suppress the type warning.

This does seem to be effective, however - see #5485 (comment)

[pjfanning]

Nothing is broken, right now.

[pjfanning]

I don't really understand https://github.com/FasterXML/jackson-core/blob/3.x/src/main/java/tools/jackson/core/TreeCodec.java

I think the <T extends JsonNode> should appear like this

public interface TreeCodec<T extends JsonNode> {
  public abstract T readTree(JsonParser p) throws JacksonException;
}

Then ObjectMapper can implement TreeCodec<JsonNode>.

But for me, a bigger question is why only readTree has the T return type while everything else in TreeCodec is hardcoded as JsonNode return type.

[ilgrosso]

Nothing is broken, right now.

True. Just an annoying warning.

The main question is if we want to keep the TreeCodec support for supporting JsonNode subclasses.

I don't know if there are actually explicit calls to TreeCode#readTree or rather direct calls to ObjectMapper#readTree / ObjectReader#readTree, since the latter will just stay as they are, e.g.

@Override
public JsonNode readTree(JsonParser p) throws JacksonException {
 ...
}

which is perfectly legal and should allow to avoid breaking changes.

I am anyway unaware of other TreeCodec implementations out there.

[yawkat]

It doesn't matter whether there is only one implementation. Changing the signature would still be a breaking change, so we can't do it.