RFR: 8026369: javac potentially ambiguous overload warning needs an improved scheme [v4]

[Vicente Romero]

On Fri, 24 Feb 2023 18:09:16 GMT, Archie L. Cobbs <duke at openjdk.org> wrote:

>> This bug relates to the "potentially ambiguous overload" warning which is enabled by `-Xlint:overloads`.
>> 
>> The warning detects certain ambiguities that can cause problems for lambdas. For example, consider the interface `Spliterator.OfInt`, which declares these two methods:
>> 
>> void forEachRemaining(Consumer<? super Integer> action);
>> void forEachRemaining(IntConsumer action);
>> 
>> Both methods have the same name, same number of parameters, and take a lambda with the same "shape" in the same argument position. This causes an ambiguity in any code that wants to do this:
>> 
>>     spliterator.forEachRemaining(x -> { ... });
>> 
>> That code won't compile; instead, you'll get this error:
>> 
>> Ambiguity.java:4: error: reference to forEachRemaining is ambiguous
>>         spliterator.forEachRemaining(x -> { });
>>                    ^
>>   both method forEachRemaining(IntConsumer) in OfInt and method forEachRemaining(Consumer<? super Integer>) in OfInt match
>> 
>> 
>> The problem reported by the bug is that the warning fails to detect ambiguities which are created purely by inheritance, for example:
>> 
>> interface ConsumerOfInteger {
>>     void foo(Consumer<Integer> c);
>> }
>> 
>> interface IntegerConsumer {
>>     void foo(IntConsumer c);
>> }
>> 
>> // We should get a warning here...
>> interface Test extends ConsumerOfInteger, IntegerConsumer {
>> }
>> 
>> 
>> The cause of the bug is that ambiguities are detected on a per-method basis, by checking whether a method is part of an ambiguity pair when we visit that method. So if the methods in an ambiguity pair are inherited from two distinct supertypes, we'll miss the ambiguity.
>> 
>> To fix the problem, we need to look for ambiguities on a per-class level, checking all pairs of methods. However, it's not that simple - we only want to "blame" a class when that class itself, and not some supertype, is responsible for creating the ambiguity. For example, any interface extending `Spliterator.OfInt` will automatically inherit the two ambiguities mentioned above, but these are not the interface's fault so to speak so no warning should be generated. Making things more complicated is the fact that methods can be overridden and declared in generic classes so they only conflict in some subtypes, etc.
>> 
>> So we generate the warning when there are two methods m1 and m2 in a class C such that:
>> 
>> * m1 and m2 consitiute a "potentially ambiguous overload" (using the same definition as before)
>> * There is no direct supertype T of C such that m1 and m2, or some methods they override, both exist in T and constitute a "potentially ambiguous overload" as members of T
>> * We haven't already generated a warning for either m1 or m2 in class C
>> 
>> If either method is declared in C, we locate the warning there, but when both methods are inherited, there's no method declaration to point at so the warning is instead located at the class declaration.
>> 
>> I noticed a couple of other minor bugs; these are also being fixed here:
>> 
>> (1) For inherited methods, the method signatures were being reported as they are declared, rather than in the context of the class being visited. As a result, when a methods is inherited from a generic supertype, the ambiguity is less clear. Here's an example:
>> 
>> interface Upper<T> {
>>     void foo(T c);
>> }
>> 
>> interface Lower extends Upper<IntConsumer> {
>>     void foo(Consumer<Integer> c);
>> }
>> 
>> Currently, the error is reported as:
>> 
>> warning: [overloads] foo(Consumer<Integer>) in Lower is potentially ambiguous with foo(T) in Upper
>> 
>> Reporting the method signatures in the context of the class being visited makes the ambiguity clearer:
>> 
>> warning: [overloads] foo(Consumer<Integer>) in Lower is potentially ambiguous with foo(IntConsumer) in Upper
>> 
>> 
>> (2) When a method is identified as part of an ambiguous pair, we were setting a `POTENTIALLY_AMBIGUOUS` flag on it. This caused it to be forever excluded from future warnings. For methods that are declared in the class we're visiting, this makes sense, but it doesn't make sense for inherited methods, because it disqualifies them from participating in the analysis of any other class that also inherits them.
>> 
>> As a result, for a class like the one below, the compiler was only generating one warning instead of three:
>> 
>> public interface SuperIface {
>>     void foo(Consumer<Integer> c);
>> }
>> 
>> public interface I1 extends SuperIface { 
>>     void foo(IntConsumer c);        // warning was generated here
>> }   
>> 
>> public interface I2 extends SuperIface { 
>>     void foo(IntConsumer c);        // no warning was generated here
>> }   
>> 
>> public interface I3 extends SuperIface { 
>>     void foo(IntConsumer c);        // no warning was generated here
>> }   
>> 
>> 
>> With this patch the `POTENTIALLY_AMBIGUOUS` flag is no longer needed. I wasn't sure whether to renumber all the subsequent flags, or just leave an empty placeholder, so I chose the latter.
>> 
>> Finally, this fix uncovers new warnings in `java.base` and `java.desktop`, so these are now suppressed in the patch.
>
> Archie L. Cobbs has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix typo in one comment and clarify another.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 2736:

> 2734:           .sorted(Comparator.comparing(e -> e.getKey().toString()))
> 2735:           .map(Map.Entry::getValue)
> 2736:           .peek(Collections::reverse)               // seems to help warning ordering

not sure about reversing the order here, it seems to me that if the order is reversed then the warning is shown in the first occurrence of a method instead of in the second which is the offending one. So for example for this code:


    interface I1 {
        void foo(Consumer<Integer> c);
        void foo(IntConsumer c);
    }

the warning is shown for the first method when I think it should be shown for the second which is the one introducing the ambiguity

-------------

PR: https://git.openjdk.org/jdk/pull/12645