Fix capture conversion for method references

[coehlrich]

What it does

Fixes #2302 by applying capture conversion at the parameter level.

If capture conversion were to apply only to type arguments then

    private static interface C<T> {
        void run(A<T> arg);
    }

would be invalid with void method(A<B<?>> a); due to capture conversion making the wildcard a capture.

    private static interface C<T> {
        void run(A<?> a, A<?> b);
    }

would be valid with <T> void method(A<T> a, A<T> b) which should be invalid due to the interface not requiring the type arguments of a and b to be the same.

and about "fanned out"

    private static interface C<T> {
        void run(T a, T b);
    }

would be valid with <T> void method(A<T> a, A<T> b) and C<A<?>> c = this::method which should be invalid due to c not requiring the type arguments of a and b to be the same.

How to test

Attempt to compile:

public class TestRecord {
    public void test() {
        A<B<?>> a = null;
        call(a, this::error);
    }

    public void error(A<B<?>> a) {
    }

    public <T> void call(A<T> a, C<T> c) {}

    private static interface C<T> {
        void run(A<T> arg);
    }
    private static class A<T> {}
    private static class B<T> {}
}

Fails with The type TestRecord does not define error(TestRecord.A<TestRecord.B<capture#1-of ?>>) that is applicable here without this change but successfully compiles with this change.

Author checklist

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

Additional information

While working on this I also found that createCapturedWildcard reuses the same capture binding for each wildcard of a method reference due to the contextType being the containing class along with start and end being the start and end of the method reference so the same capture binding will be reused for each wildcard in the parameters of the method reference.

[iloveeclipse]

@coehlrich : thanks for the patch. I see multiple commits there - are they needed, or just represent work in progress?If later, please squash all changes to one commit, rebase it on latest master state & force push to github.

@srikanth-sankaran : I believe parts of changed code were written long time ago by you. Is this something you could review?

[srikanth-sankaran]

@coehlrich : thanks for the patch. I see multiple commits there - are they needed, or just represent work in progress?If later, please squash all changes to one commit, rebase it on latest master state & force push to github.

@srikanth-sankaran : I believe parts of changed code were written long time ago by you. Is this something you could review?

I am woefully rusty on that code. @stephan-herrmann would be the best person to review this. Stephan, thanks for taking a look.

[srikanth-sankaran]

Also @mpalat or @jarthana should approve this after some stress tests.

[stephan-herrmann]

@stephan-herrmann would be the best person to review this. Stephan, thanks for taking a look.

If I'm to review this I will have to ask some nasty questions about how the change is backed by JLS.

Should I?

[srikanth-sankaran]

@stephan-herrmann would be the best person to review this. Stephan, thanks for taking a look.

If I'm to review this I will have to ask some nasty questions about how the change is backed by JLS.

Should I?

Asking questions about how the change is backed by JLS is very valid line of enquiry and is not nasty is my opinion :)

[coehlrich]

In JLS §15.13.1:

Otherwise, given a targeted function type with parameter types P₁, ..., Pₙ and a set of potentially applicable methods, the compile-time declaration is selected as follows:
...
the method reference is treated as if it were an invocation with argument expressions of types P₁, ..., Pₙ. Type arguments, if any, are given by the method reference expression

This can be interpreted as with:

    public void reference(Object o) {}

    interface A {
        void run(String s);
    }

treat the reference in this:

    A a = this::reference;

as if it were the invocation in this for the purpose of looking up which method to use:

    String s = "";
    this.reference(s);

which makes sense since later on a.run("") could be called effectively being the execution part of this.reference("") so the compiler needs to make sure that whatever parameters can be used for the functional interface method can also be used for the method being referenced.

With capture conversion if it applied only to the type arguments of the functional interface:

    public void test() {
    	B<?> b1 = new B<>();
    	B<?> b2 = new B<>();
        A<B<?>> a = this::error; // This makes both B<?> parameters use the same capture variable
        a.run(b1, b2); // which can be run as if it were `run(B<?> a, B<?> b)`
        this.error(b1, b2); // This makes both B<?> parameters use seperate capture variables which because of the type variable T makes it not compile
    }

    public <T> void error(B<T> a, B<T> b) {}

    interface A<T> {
        void run(T t1, T t2);
    }

    class B<T> {}

[stephan-herrmann]

Let's forget about "nasty" for now 😄, as I'm gradually trying to understand the different ingredients to this issue.

Do we agree that capturing A<B<?>> does not change the type? This is because capturing is never applied recursively, so we check if B<?> is a wildcard, and since it is not, no capturing occurs.

So, with this original example:

        A<B<?>> a = null;
        call(a, this::error);

inference instantiates <T> from call() to B<?>, i.e., call resolves to call(<A<B<?>>, C<B<?>>).

In the debugger I can see that both occurrences of B<?> are actually the same ParameterizedTypeBinding. If that is correct then semantics would indeed be different from a situation where void call(<A<B<?>>, C<B<?>>) would be written in source code, which would define two independent fresh type variables, rather than one and the same.

The interesting part happens after inferring call() while propagating the inference result into the argument this::error. Here descriptorParametersAsArgumentExpressions() computes the argument type to A<B<capture#1-of ?>>. This looks bogus indeed, as now we managed to capture at a nesting level which should not be touched by capturing.

With only the change in ReferenceExpression descriptorParametersAsArgumentExpressions() computes the argument type to A<B<?>> and the B<?> is indeed the same type as we found in the resolved signature of call().

I read this as saying that inference succeeds to unify the two arguments to call() such that both are referring to the same nested B<?>.

So far all looks good, but some questions remain:

• the change basically reverts the fix for bug 432759, so
  • what other change made the fix for bug 432759 unnecessary?
  • is there any reasoning from bug 432759 which we should follow up after 'revert'?
• I couldn't see any need for the changes in ConstraintExpressionFormula nor ParameterizedTypeBinding, so what's the motivation for those changes?
• While playing with the example I found a few more situations where ecj and javac differ, I think I saw at least one such difference even with the proposed changes applied.

[stephan-herrmann]

With capture conversion if it applied only to the type arguments of the functional interface:

    public void test() {
    	B<?> b1 = new B<>();
    	B<?> b2 = new B<>();
        A<B<?>> a = this::error; // This makes both B<?> parameters use the same capture variable
        a.run(b1, b2); // which can be run as if it were `run(B<?> a, B<?> b)`
        this.error(b1, b2); // This makes both B<?> parameters use seperate capture variables which because of the type variable T makes it not compile
    }

    public <T> void error(B<T> a, B<T> b) {}

    interface A<T> {
        void run(T t1, T t2);
    }

    class B<T> {}

Regarding this.error(b1, b2) all compilers agree to reject the code: javac and ecj latest release and ecj with proposed changes.

Regarding the assignment A<B<?>> a = this::error ecj accepts (with and without proposed changes), whereas javac rejects, which sent me down another rabbit hole: #2419.

[stephan-herrmann]

Another point in favor of accepting: ecj already accepts the same program when rewritten using lambda instead of method reference:

                    public void test() {
                        A<B<?>> a = null;
                        call(a, a1 -> this.error(a1));
                    }

                    public void error(A<B<?>> a) {
                    }

                    public <T> void call(A<T> a, C<T> c) {}

                    private static interface C<T> {
                        void run(A<T> arg);
                    }
                    private static class A<T> {}
                    private static class B<T> {}

This underlines that indeed treatment for ReferenceExpression is to blame specifically, not our general approach to wildcard captures.

[stephan-herrmann]

the change basically reverts the fix for bug 432759, so

• what other change made the fix for bug 432759 unnecessary?
• is there any reasoning from bug 432759 which we should follow up after 'revert'?

Let's resolve those questions via #2419: a quick-and-dirty experiment for that issue indicates that accepting might actually be wrong, and a tiny fix could make ecj report the same errors that javac reports.

[coehlrich]

The "interning" was why I created #2351.

For question 2
In JLS §18.2.1:

A constraint formula of the form ‹MethodReference → T›.
...
Otherwise, a search for a compile-time declaration is performed, as specified in §15.13.1.

is the reason for the change in ConstraintExpressionFormula and the change in ParameterizedTypeBinding was because it was only used for in the places in ConstraintExpressionFormula and ReferenceExpression.

[stephan-herrmann]

The "interning" was why I created #2351.

Thanks, I hadn't seen that one. Let's see if #2419 can be folded into that one.

For question 2 In JLS §18.2.1:

A constraint formula of the form ‹MethodReference → T›.
...
Otherwise, a search for a compile-time declaration is performed, as specified in §15.13.1.
is the reason for the change in ConstraintExpressionFormula

I still don't understand, how does the reference to §15.13.1 relate to the way how we apply capture?

OTOH, I can see that bug 492939 followed in the foot steps of bug 432759, and if that one is due for revisiting any way, all these individual fixes might now be converging to a cleaner solution.

[coehlrich]

The code modified in ConstraintExpressionFormula is for the quoted part of jls and is seemingly intended to mimic the capture conversion used for method references.

[stephan-herrmann]

The code modified in ConstraintExpressionFormula is for the quoted part of jls and is seemingly intended to mimic the capture conversion used for method references.

Please do help me understand your intention:

• Does JLS specify how, where or when capture should happen during §15.13.1?
• What exactly is the change improving?

I can see that you thoroughly studied the issue and I don't want to block the merge longer than necessary. I just want to verify each change either against a test that would otherwise fail (not the case here) or against some other goal which I can understand 😄

[coehlrich]

JLS section §15.13.1 includes "In the first search, the method reference is treated as if it were an invocation with argument expressions of types P1, ..., Pn;" and is the same section as the one that is relevant for the change in ReferenceExpression.

I haven't managed to find any test cases for ConstraintExpressionFormula since inference seems to be fine with nested capture bindings.

[coehlrich]

Seems like the argumentTypes variable was actually related to the clause a couple lines further down than the reference to §15.13.1 in JLS. Although the capture conversion in ReferenceExpression and ConstraintExpressionFormula still seem like they should be doing the same method of capturing

The argumentTypes variable in ConstraintExpressionFormula is being used in a very similar way to the result of descriptorParametersAsArgumentExpressions in ReferenceExpression in that it is being used for relating a method being referenced from a method reference to a method in a functional interface which is represented in JLS §18.2.1

If R does not mention one of the type parameters of the function type, then the constraint reduces to the bound set B3 which would be used to determine the method reference's compatibility when targeting the return type of the function type, as defined in §18.5.2.1. B3 may contain new inference variables, as well as dependencies between these new variables and the inference variables in T.

In bug 492939 (which is the bug report where the change to ConstraintExpressionFormula which added the capturing was suggested) it seems that capturing the type parameters was made to also avoid duplicate capturing like in bug 432759.

I made an experiment to insert capturing into reduceReferenceExpressionCompatibility(). This succeeds to fix the current bug.
But then I was alerted by a comment in RE#descriptorParametersAsArgumentExpressions() (on behalf of Bug 432759), mentioning that we need to avoid duplicate capturing. Not sure whether/how this applies here, too. The situation is similar in that we use parameters to construct argument types. If needed, perhaps the RE can store the captured argument types to avoid duplicate capturing?

[coehlrich]

I undid the changes to ConstraintExpressionFormula and ParameterizedTypeBinding.

[stephan-herrmann]

Thanks @coehlrich with part of the proposed change reverted I think we are on firm grounds. The change in ReferenceExpression looks good to me.

Still I appreciate that you looked also at the related code location in ConstraintExpressionFormula. Due to the connection between bug 492939 and bug 432759 it is still possible your originally proposed change is good, too. Just it's harder to tell without a confirming test case. As type inference is good for unexpected butterfly effects, I prefer to avoid changes with unclear benefit. Perhaps we can conclude this part via #2351.

Also @mpalat or @jarthana should approve this after some stress tests.

Do we have any results from stress tests?

[srikanth-sankaran]

Thanks @coehlrich with part of the proposed change reverted I think we are on firm grounds. The change in ReferenceExpression looks good to me.

Still I appreciate that you looked also at the related code location in ConstraintExpressionFormula. Due to the connection between bug 492939 and bug 432759 it is still possible your originally proposed change is good, too. Just it's harder to tell without a confirming test case. As type inference is good for unexpected butterfly effects, I prefer to avoid changes with unclear benefit. Perhaps we can conclude this part via #2351.

Also @mpalat or @jarthana should approve this after some stress tests.

Do we have any results from stress tests?

They may have waited for the dust to settle on this. @mpalat and @jarthana could you approve this after some testing please ?? TIA

[srikanth-sankaran]

@mpalat/@jarthana is one of you looking into the request for stress testing this fix ?? Thanks in advance.

[iloveeclipse]

@mpalat/@jarthana is one of you looking into the request for stress testing this fix ?? Thanks in advance.

@mpalat, @jarthana : this Friday we have M3, and it would be nice to have this PR reviewed / merged before that.

[jarthana]

@mpalat/@jarthana is one of you looking into the request for stress testing this fix ?? Thanks in advance.

@mpalat, @jarthana : this Friday we have M3, and it would be nice to have this PR reviewed / merged before that.

I am on it. I will update in an hour or so.