GH472: substitute and then erasure if unchecked conversion was necessary

[testforstephen]

Signed-off-by: Jinbo Wang jinbwan@microsoft.com
Change-Id: I524ac3308c9a63aa66aab2c8bcaa5fce6f6b5ad6

What it does

Fixes #472

import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.function.Function;

public class GenericTypeTest {
    public static void main(String[] args) {
    	GenericTypeTest test = new GenericTypeTest();
        test.raw.put("first-entry", new HashMap<>());

        System.out.println(test.getFirstEntry());
    }

    private Map<String, Object> raw = new HashMap<>();

    public Map<Object, Object> getFirstEntry() {
        // JDT compiler reports the error "Type mismatch: cannot convert from Object to Map<Object,Object>" on the next line
        return getAndMap("first-entry", Map.class, Collections::unmodifiableMap);
    }

    <I, T> T getAndMap(String entry, Class<I> type, Function<I, T> f) {
        I value = get(entry, type);
        return value == null ? null : f.apply(value);
    }

    <T> T get(String entry, Class<T> type) {
        Object value = raw.get(entry);
        return value == null ? null : type.cast(value);
    }
}

When the code uses some raw types, the compiler will use unchecked conversion to infer their type. The problem with bug 472 is that the return type of the original method getAndMap is a type variable T, erasing a type variable provides nothing. The fix is to substitue the type variable with the relevant parameterized type before doing the erasure.

How to test

Author checklist

• I have thoroughly tested my changes
• The change is following the coding conventions
• I have signed the Eclipse Contributor Agreement (ECA)

[testforstephen]

@stephan-herrmann WDYT?

[stephan-herrmann]

Welcome to the wonderful world of type inference! :) :)

We should be looking at this sentence from JLS 18.5.2.2:

If unchecked conversion was necessary for the method to be applicable during constraint set reduction in §18.5.1, then [...] the return type [...] of the invocation type of m [is] given by the erasure of the return type [...] of m's type.

Here m is the generic method T getAndMap(String, Class<I>, Function<I,T>), so the return type of m is T and the erasure of it is Object. I don't think there's a lot of room for interpretation, or am I missing anything?

For definition of "return type of m's type" see JLS 8.2 which seems to justify our use of originalMethod.returnType (i.e., just take "the return type of m").

Please see how the variant with substitution is specified just above (my emphasis):

[...] the parameter types of the invocation type of m are obtained by applying θ' to the parameter types of m's type

You probably saw, that the flag you are modifying is commented "// propagate simulation of Bug JDK_8026527", so any substitution we already apply happens in deliberate violation of JLS, just to conform to buggy javac, where a fix was proposed in 2014/2015 but then the developers were afraid of unpleasant effects even when compiling the JDK.

I would not like to take responsibility for piling more ad-hoc changes onto this bug-compatibility implementation.

As I can see that you have dug quite deep into type inference, I could offer to assist you in working on a few more bugs in this area, which eventually might enable you to take responsibility yourself, if you like.

Meanwhile you could also try to interact with Oracle, suggesting that the specification be changed to cover your proposed change (and thus also legalize existing behavior of javac).

[testforstephen]

@stephan-herrmann, thank you for providing more context on this issue.

I use git blame to find the original issue https://bugs.eclipse.org/bugs/show_bug.cgi?id=473657#c19 that introduced the comment "// propagate simulation of Bug JDK_8026527". Thank you for writing in such detail about your investigation on that issue, it really helps me to understand more about this area.

From your comment, it links to another javac bug https://bugs.openjdk.org/browse/JDK-8135087, that's exactly the same case as my sample, where javac performs substitution on the return type, and then erasing to raw Class.

The below test exercises the clause in JLS 18.5.2 stating that, where unchecked conversion was necessary for the arguments, the return type of the invocation must be erased.

T m(Class arg1, Class arg2) { return null; }

void test(Class c1, Class<Class> c2) throws Exception {
m(c1, c2).newInstance(); // expected: error; actual: ok
m(c1, c2).newInstance().length(); // expected: error; actual: error
}

The erasure of the return type, T, is Object. However, javac seems to be producing raw Class instead. It appears that javac is inferring T=Class, performing substitution on the return type, and then erasing to raw Class. This is inconsistent with the spec, going back to JLS 3 (15.12.2.6), which makes clear that substitution of inference results only occurs in the case in which unchecked conversion was not necessary.

Dan confirmed this is a javac bug that's inconsistent with spec. Sadly, they marked this as a P4 low priority issue since it's only noticeable in rare use cases and practical impact is low. It has been inactive for 6 years and I don't expect them to take further action in the near future.

Given that it only occurs in rare cases, patching this is tolerable to me. And this can help provide a better out-of-the-box experience for those developers coming from javac-based tools such as maven, gradle, and Intellij IDEA.

WDYT?

As I can see that you have dug quite deep into type inference, I could offer to assist you in working on a few more bugs in this area, which eventually might enable you to take responsibility yourself, if you like.

Yes, I'm interested in getting involved in maintaining this area. Recently we're testing the whole out-of-the-box experience of JDT developer tools by using popular Java projects with top stars on GitHub. And type inference is one of the top issues we found, that's why I start to look into this area. I haven't went through all cases yet, more bugs may be created later. You are very expertise in this area and having your guidance to work through these bugs is definitely valuable to me😀. Thank you for stepping up to offer help.

[stephan-herrmann]

... javac bug https://bugs.openjdk.org/browse/JDK-8135087, that's exactly the same case as my sample, where javac performs substitution on the return type, and then erasing to raw Class.

Thanks for connecting the dots! This increases the confidence that your proposed change is indeed equal to the bug in javac, and thus will not cause bad surprises in other situations.

Given that it only occurs in rare cases, patching this is tolerable to me. And this can help provide a better out-of-the-box experience for those developers coming from javac-based tools such as maven, gradle, and Intellij IDEA.
WDYT?

Perhaps as last steps before merging you could (a) check if JDK-8135087 and friends suggest more extra constitutional substitutions than what you already proposed, and (b) ensure that those code tweaks properly link to the relevant JDK bugs.

[testforstephen]

Perhaps as last steps before merging you could (a) check if JDK-8135087 and friends suggest more extra constitutional substitutions than what you already proposed, and (b) ensure that those code tweaks properly link to the relevant JDK bugs.

Done. Had a look at the relevant issues with JDK-8135087, they all refer to the case of substituting and erasing the return/thrown types for unchecked invocation. I have updated the unit tests to cover the samples mentioned in these JDK issues.

[testforstephen]

The failing test cases on GTT.test1436, GTT.test1437, GTT.test1438, GTT.test1439, GTT.test1445 are same as the newly added test case GRT.testBugGH472_d (unchecked conversion for thrown type). Based on the new rule, we should not report error for them.

[stephan-herrmann]

I dropped some final nitpick comments. The signature change I'd rank as mandatory, other comments have less urgency. Once addressed that change is ready to go.

[testforstephen]

The comments are addressed. let me know if it's acceptable.

[stephan-herrmann]

The comments are addressed. let me know if it's acceptable.

All looks good now.

For future PRs I'd prefer if changes are done as individual commits, so I can easily see the changes. (The "Compare" link also showed unrelated stuff probably due to a rebase on HEAD on your side?).

Things will then be cleaned up by "Squash and merge".

[testforstephen]

For future PRs I'd prefer if changes are done as individual commits, so I can easily see the changes.

Sure, agree that keeping individual commits is more friendly to code review.