On Fri, 24 Feb 2023 16:04:38 GMT, Archie L. Cobbs <[email protected]> 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:
>
> Remove placeholder flag UNUSED_1; just leave a comment instead.
src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Check.java line 2719:
> 2717: * does not override any other methods (in which case site is
> responsible).
> 2718: */
> 2719: void checkPotentiallyAmbiguousOverloads(JCClassDecl tree, Type
> site) {
general comments: overall looks correct but I think the code should be split a
bit using helper methods, that will help with readability, I think. Side: I'm a
bit worried that overuse of streams in this code could imply some performance
hit. Of course if the corresponding lint warning is not enabled we will skip it
but a lot of projects compile with `-Xlint:all` nowadays.
-------------
PR: https://git.openjdk.org/jdk/pull/12645
