Skip to content

Comments

Reduce b2 when reducing ConstraintExpressionFormula#4315

Merged
stephan-herrmann merged 2 commits intoeclipse-jdt:masterfrom
coehlrich:fix-inference
Oct 25, 2025
Merged

Reduce b2 when reducing ConstraintExpressionFormula#4315
stephan-herrmann merged 2 commits intoeclipse-jdt:masterfrom
coehlrich:fix-inference

Conversation

@coehlrich
Copy link
Contributor

What it does

Fixes #4314 by solving the b2 bound set prior to using it for the b3 bound set when the class of the method isn't ParameterizedGenericMethodBinding

The removed error from the existing test seems to be originally caused by the b3 bound set (which succeeded without solving the b2 bound set)

How to test

Attempt to compile the test case

Author checklist

@stephan-herrmann
Copy link
Contributor

@coehlrich I found this by chance only since I chose not to be informed about every activity in github. In the future feel free to ping (mention) me in issues / PRs that relate to generics / type inference and all that. Sometimes the good folks in India pick up and redirect such issues, but they may miss a few.

That said, now we're late in the cycle of 4.37. Do you mind postponing this issue to right when 4.38 re-opens, or is anybody currently blocked by this bug?

@stephan-herrmann stephan-herrmann self-requested a review August 19, 2025 19:06
@coehlrich
Copy link
Contributor Author

I'm fine with waiting until 4.38.

@stephan-herrmann stephan-herrmann added this to the MilestoneNxt milestone Aug 19, 2025
@stephan-herrmann
Copy link
Contributor

@coehlrich thanks for your sharp analysis and fix. Still let me play the pedantic guardian of JLS :)

We are coming from § 18.2.1

If the expression is a class instance creation expression or a method invocation expression, the constraint reduces to the bound set B3 which would be used to determine the expression's compatibility with target type T, as defined in §18.5.2.1.

So let's have a look also at §18.5.2.1:

Let B2 be the bound set produced by reduction in order to demonstrate that m is applicable in §18.5.1.
(While it was necessary in §18.5.1 to demonstrate that the inference variables in B2 could be resolved, in order to establish applicability, the instantiations produced by this resolution step are not considered part of B2.)

I read this like so:

  • when performing full 18.5.1 we go all the way through resolve
  • the "invocation" from 18.5.2.1, however, only performs the reduction part.

So rather than doing too much and then reverting part of that, I tried to add only this:

	if (!inferenceContext.reduce())
		return FALSE;

which also passes tests and seems more in line with JLS.

Incorporation can happen at any point later, it's only the reduction from constraint to bound which was "forgotten".

What do you think?

@stephan-herrmann
Copy link
Contributor

The removed error from the existing test seems to be originally caused by the b3 bound set (which succeeded without solving the b2 bound set)

Also javac does not report an error on line 15 of that test, so that change looks good to me.

@coehlrich
Copy link
Contributor Author

the solve method also includes entering the capturing context so I tried this:

    public static void main(String[] args) {
        m(
                i -> new A<>((C<B<?>>) null),
                b -> b.intValue());
    }
    private static <T, R> void m(Function<B<Integer>, A<T>> f1, Function<T, R> f2) {}
    private static class A<T> {
        public A(C<? extends C<T>> t) {}
    }
    private record B<T extends Integer>(T t) implements C<T> {
    	
    }
    private interface C<T> {
    	T t();
    }

which does compile when using solve and not when using reduce and for some reason does not compile with javac.

@stephan-herrmann
Copy link
Contributor

... which does compile when using solve and not when using reduce and for some reason does not compile with javac.

Given that both compilers agree, should we add that as a negative test?

Other than that the PR looks good to go.

@stephan-herrmann
Copy link
Contributor

... which does compile when using solve and not when using reduce and for some reason does not compile with javac.

Given that both compilers agree, should we add that as a negative test?

Other than that the PR looks good to go.

At a second look I see the conflict: the variant with CapturingContext.enter() looks more consistent, but accepts that additional test thus disagreeing with javac. Sigh.

@stephan-herrmann
Copy link
Contributor

Something suspicious: I modified the example to be otherwise warning free and to use unique names for type variables:

import java.util.function.Function;
public class Test {
    public static void main(String[] args) {
		C<B<?>> c = null;
        m(
                _ -> new A<>(c),
                b -> b.intValue());
    }
    static <T, R> void m(Function<B<Number>, A<T>> f1, Function<T, R> f2) {}
    static class A<U> {
        public A(C<? extends C<U>> t) {}
    }
    record B<V extends Number>(V t) implements C<V> {

    }
    interface C<W> {
        W t();
    }
}

Now with the pending fix inference gives:

    T#0    :    capture#1-of ?
    R#1    :    java.lang.Integer
    U#3    :    capture#1-of ? 

Wait a minute: two different inference variables are instantiated to the same capture? Is this legal?

2 Experiments:

  1. Let CapturingContext increment end after each actual capture() call, i.e., ensure that we don't return the same capture more than once => several failures throughout our test suite
  2. Tweak the change of this PR to invoke CapturingContext.enter() with a modified end (e.g., invocation.sourceEnd()+1), i.e., ensure that during reduce() captures are not shared with the outside. => the above program is rejected like javac, all else is unchanged.

I asked for clarification: https://mail.openjdk.org/pipermail/compiler-dev/2025-October/031848.html

@coehlrich
Copy link
Contributor Author

    public static void main(String[] args) {
	C<B<?>> c = null;
        m(
                a -> new A<>(c),
                b -> b.intValue());
    }
    static <T, R> void m(Function<B<Number>, A<T>> f1, Function<T, R> f2) {}
    static class A<U> {
        public A(C<? extends C<? extends U>> t) {}
    }
    record B<V extends Number>(V t) implements C<V> {

    }
    interface C<W> {
        W t();
    }

Changing U to ? extends U in the constructor parameters allows it to compile with javac and the current PR but doesn't without CapturingContext or when changing the end argument to invocation.sourceEnd() + 1.

I think javac is trying to avoid having U potentially representing different types while keeping the fact that the wildcard from C<B<?>> can only be a subtype of Number since if the outer C was replaced with List and C had a method to change the return value of t() the List could contain B<Integer> and B<Double> allowing the constructor of A with List<? extends C<U>> to copy the value from B<Double> to B<Integer> allowing B<Integer> to store a Double.

@stephan-herrmann
Copy link
Contributor

Changing U to ? extends U in the constructor parameters allows it to compile with javac and the current PR but doesn't without CapturingContext or when changing the end argument to invocation.sourceEnd() + 1.

Interesting.

With the initial answer in https://mail.openjdk.org/pipermail/compiler-dev/2025-October/031850.html and your latest variant of the test case I'm leaning towards the following conclusion:

  • The example from this comment should indeed be accepted and javac's failure to do so is a bug
    • In that case we should probably add a new constant JavacHasABug.JavacBug8016196 to document the difference
  • Entering CapturingContext with untainted source positions is probably the right thing to do.

I'll wait if Vicente has some additional news for us. Otherwise I'll just add the two mentioned variants of the test and merge the change.

@stephan-herrmann stephan-herrmann merged commit 0933895 into eclipse-jdt:master Oct 25, 2025
13 checks passed
@stephan-herrmann
Copy link
Contributor

I believe we have achieved the best feasible solution here.
Many thanks @coehlrich!

@stephan-herrmann stephan-herrmann changed the title Solve b2 when reducing ConstraintExpressionFormula Reduce b2 when reducing ConstraintExpressionFormula Oct 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Javac compiles but not ecj with inference

2 participants