I am having a null analysis issue when compiling GriefPrevention in Eclipse. For example, line 304 in PlayerEventHandler.java invokes Player#getLocation(). The API specification for this class can be found here.
The problem is that this class inherits from both the Entity and OfflinePlayer interfaces, which have conflicting annotations.
For Entity, getLocation() is defined as NonNull:
@NotNull
Location getLocation();
But for OfflinePlayer, it is defined as Nullable:
/**
* Gets the player's current location.
*
* @return the player's location, {@code null} if player hasn't ever played
* before.
*/
@Nullable
public Location getLocation();
It's my opinion that, given this situation, the least permissive annotation (Nullable) should be given precedence. However, IntelliJ disagrees. This creates a problem, because I cannot compile this code with Eclipse without disabling null annotations, which is undesirable, and the developer who uses IntelliJ insists that this is not a problem (according to his IDE).
Is either IntelliJ or Eclipse's interpretation "more correct" or is the result ambiguous given the conflicting annotation in the API specification?
When testing this kind of ambiguous inheritance, IntelliJ allows @NotNull without any warning or error, but @Nullable throws a warning:
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
public class Main {
interface ITestNotNull {
@NotNull
Object ambiguous();
}
interface ITestNullable {
@Nullable
Object ambiguous();
}
static class ITest implements ITestNullable, ITestNotNull {
@Override
// Method annotated with @Nullable must not override @NotNull method
public @Nullable Object ambiguous() {
return new Object();
}
}
public static void main(String[] args) {
ITest a = new ITest();
System.out.println(a.ambiguous());
}
}
The opposite is true in Eclipse, however I receive a compiler error instead:
import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.Nullable;
public class Main {
interface ITestNotNull {
@NonNull
Object ambiguous();
}
interface ITestNullable {
@Nullable
Object ambiguous();
}
static class ITest implements ITestNullable, ITestNotNull {
@Override
// The return type is incompatible with '@NonNull Object' returned from
// Main.ITestNotNull.ambiguous() (mismatching null constraints)
public @Nullable Object ambiguous() {
return new Object();
}
}
public static void main(String[] args) {
ITest a = new ITest();
System.out.println(a.ambiguous());
}
}
Upon further consideration and analysis of the problem, the issue I have been experience seems to be a bug in Eclipse. IntelliJ's implementation of @NonNull seems to be correct. Regardless of the ambiguity, it satisfies both possible implementations.
https://github.com/eclipse-jdt/eclipse.jdt.core/issues/2308
My original report was under the false premise that the annotation behavior experienced when importing a jar was correct and that the native source-level implementation was wrong. The opposite is true. There is nothing wrong with the source-level implementation; this bug can only be demonstrated when importing a jar file (see the git bug report).
Here is a very simple example demonstrating the actual bug:
import java.util.Objects;
import org.bukkit.Bukkit;
import org.bukkit.OfflinePlayer;
import org.bukkit.entity.Player;
import org.eclipse.jdt.annotation.NonNull;
import org.eclipse.jdt.annotation.Nullable;
public class Main {
public static void main(String[] args) {
@Nullable Object nullableObject;
@NonNull Object nonNullObject;
Player player = Objects.requireNonNull(Bukkit.getPlayer(""));
/* Null annotation error *WRONGLY* derives the @Nullable implementation
when choosing between the two conflicting interfaces */
// Unexpected inverse @Nullable implementation is derived
// nonNullObject = player.getLocation();
// ^^^^^^^^^^^^^^^^^^^^ Error: Null type mismatch
// This is the expected @NonNull interface implementation
nonNullObject = ((Entity)player).getLocation();
// This would be a NPE if Player was actually implementing @Nullable!
nullableObject = ((OfflinePlayer)player).getLocation();
}
}
Spigot API jar library with source:
spigot-api-1.20.4-R0.1-20240407.022953-115.jar
spigot-api-1.20.4-R0.1-20240407.022953-115-sources.jar
Eclipse External Annotations (EEA) for Spigot API
spigot-api-eea.zip
In ecj, annotation based null analysis piggy-backs on compilation proper. With that the primary rule is to comply with JLS. In this example the compiler entered an area, where JLS 15.12.2.5 mandates that the method invocation selects a method from several equivalent methods by arbitrary choice. After that selection null analysis could either find a @Nullable or a @NonNull return type.
NB: Aside from the undesired compiler warning, the ambiguity is harmless for two reasons: From a pure Java p.o.v. null annotations are irrelevant for compilation as well as for execution. Secondly, any class implementing an interface like Player is forced (by the rules of null analysis) to respect both contracts, in which case the stronger contract, viz to return a non-null value, wins.
Luckily the leeway given by the specification ("chosen arbitrarily") could be used to improve null analysis in ecj. With that fix callers of Player.getLocation() will see the stronger of both contracts, and can thus leverage the promise that non-null will be returned.
Thanks to @Zhro and @howlger for reporting the issue and helping in its analysis.