Skip to content

Do not let an abstract interface method outrank a real implementation #830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
markro49 wants to merge 4 commits into
base: master
Choose a base branch
from
+382 −60
Open

Do not let an abstract interface method outrank a real implementation #830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
46993be
Do not let an abstract interface method outrank a real implementation
markro49 Sep 4, 2026
2a9206e
Respond to CodeRabbit comments
markro49 Sep 10, 2026
9beec99
Fix Checker Framework lock errors
markro49 Sep 11, 2026
ee9c80e
Respond to CodeRabbit comments
markro49 Sep 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 82 additions & 30 deletions java/daikon/dcomp/DCInstrument.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,8 +463,8 @@ public class DCInstrument extends InstructionListUtils {
/** If true, enable JUnit analysis debugging. */
protected static final boolean debugJunitAnalysis = false;

/** If true, enable {@link #getDefiningInterface} debugging. */
protected static final boolean debugGetDefiningInterface = false;
/** If true, enable {@link #getDeclaringInterface} debugging. */
protected static final boolean debugGetDeclaringInterface = false;

/** If true, enable {@link #handleInvoke} debugging. */
protected static final boolean debugHandleInvoke = false;
Expand Down Expand Up @@ -2275,22 +2275,52 @@ void add_exit(MethodGen mgen, MethodInfo mi, int method_info_index) {
}

/**
* Returns the interface class containing the implementation of the given method. The interfaces
* of {@code startClass} are recursively searched.
* Returns the first argument if it is non-null, otherwise the second.
*
* @param first the preferred value
* @param second the fallback value
* @return the first non-null argument, or null if both are null
*/
private static @Nullable @ClassGetName String firstNonNull(
@Nullable @ClassGetName String first, @Nullable @ClassGetName String second) {
return first != null ? first : second;
}

/**
* Returns the name of the interface that declares the given method. The interfaces of {@code
* startClass} are recursively searched.
*
* <p>Note that this finds a <em>declaration</em>, which is usually not an implementation: an
* interface method is implicitly abstract unless it is {@code default}, {@code static}, or
* private. Pass true for {@code implementationsOnly} to match only a {@code default} method,
* which is the one case where the interface really does hold the code that will run.
*
* <p>Limitation: when several interfaces match, this returns the first one reached rather than
* the maximally specific one that JVMS 5.4.3.3 selects. A class that implements both an interface
* and a subinterface that reabstracts the same method gets the first of the two in declaration
* order, which may be the supertype. The consequence is confined to precision: the caller uses
* the answer only to decide whether the target is instrumented, and a wrong answer there loses
* comparability through the call rather than breaking it, because the uninstrumented overload it
* then invokes always exists.
*
* @param startClass the class whose interfaces are to be searched
* @param methodName the target method to search for
* @param paramTypes the target method's parameter types
* @return the name of the interface class containing target method, or null if not found
* @param implementationsOnly if true, match only a {@code default} method; if false, match any
* declaration, abstract ones included
* @return the name of the interface that declares the target method, or null if not found
*/
private @Nullable @ClassGetName String getDefiningInterface(
JavaClass startClass, @Identifier String methodName, Type[] paramTypes) {
private @Nullable @ClassGetName String getDeclaringInterface(
JavaClass startClass,
@Identifier String methodName,
Type[] paramTypes,
boolean implementationsOnly) {

if (debugGetDefiningInterface) {
if (debugGetDeclaringInterface) {
System.out.println("searching interfaces of: " + startClass.getClassName());
}
for (@ClassGetName String interfaceName : startClass.getInterfaceNames()) {
if (debugGetDefiningInterface) {
if (debugGetDeclaringInterface) {
System.out.println("interface: " + interfaceName);
}
JavaClass ji;
Expand All @@ -2302,17 +2332,32 @@ void add_exit(MethodGen mgen, MethodInfo mi, int method_info_index) {
if (ji == null) {
throw new Error("Unable to find class: " + interfaceName);
}
boolean reabstracted = false;
for (Method jm : ji.getMethods()) {
if (debugGetDefiningInterface) {
if (debugGetDeclaringInterface) {
System.out.println(" " + jm.getName() + Arrays.toString(jm.getArgumentTypes()));
}
if (jm.getName().equals(methodName) && Arrays.equals(jm.getArgumentTypes(), paramTypes)) {
// We have a match.
// We have a match. Neither a static nor a private interface method is ever the
// target of an INVOKEVIRTUAL: a private one is not even inherited.
if (jm.isStatic() || jm.isPrivate()) {
continue;
}
if (implementationsOnly && jm.isAbstract()) {
// This interface declares the method abstract. An interface may reabstract a default
// it inherits, and an implementor must then define the method, so any default above
// this point is hidden: do not search this branch further.
reabstracted = true;
break;
}
return interfaceName;
}
}
if (reabstracted) {
continue;
}
// no match found; does this interface extend other interfaces?
@ClassGetName String foundAbove = getDefiningInterface(ji, methodName, paramTypes);
@ClassGetName String foundAbove = getDeclaringInterface(ji, methodName, paramTypes, implementationsOnly);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
if (foundAbove != null) {
// We have a match.
return foundAbove;
Expand Down Expand Up @@ -2564,6 +2609,10 @@ private boolean isTargetInstrumented(
}

@ClassGetName String targetClassname = classname;
// Interfaces are not consulted in the loop below: JVMS 5.4.3.3 resolves a method
// against the class's own declaration, then the superclass chain, and only then the
// superinterfaces, so the whole chain is searched first and the interfaces of the
// original target class afterwards.
// Search this class for the target method. If not found, set targetClassname to
// its superclass and try again.
mainloop:
Expand Down Expand Up @@ -2605,37 +2654,40 @@ private boolean isTargetInstrumented(
}
}

{
// no methods match - search this class's interfaces
// Method not found; perhaps inherited from superclass.
// Cannot use "targetClass = targetClass.getSuperClass()" because the superclass might
// not have been loaded into BCEL yet.
if (targetClass.getSuperclassNameIndex() == 0) {
// No class in the chain declares the method, so it comes from an interface. Prefer
// a default method, which is an implementation; an abstract declaration only says
// where the method is declared, but that is the best available answer.
@ClassGetName String found;
try {
found = getDefiningInterface(targetClass, methodName, paramTypes);
JavaClass origin = getJavaClass(classname);
found =
origin == null
? null
: firstNonNull(
getDeclaringInterface(origin, methodName, paramTypes, true),
getDeclaringInterface(origin, methodName, paramTypes, false));
Comment thread
coderabbitai[bot] marked this conversation as resolved.
} catch (Throwable e) {
// We cannot locate or read the .class file, better assume it is not instrumented.
targetInstrumented = false;
break;
}
if (found != null) {
// We have a match.
if (found == null) {
if (debugHandleInvoke) {
System.out.printf("we have a match%n%n");
System.out.printf("Unable to locate method: %s%n%n", methodName);
}
targetInstrumented = false;
} else {
if (debugHandleInvoke) {
System.out.printf("declared by interface %s%n%n", found);
}
if (BcelUtil.inJdk(found)) {
targetInstrumented = false;
}
break;
}
}

// Method not found; perhaps inherited from superclass.
// Cannot use "targetClass = targetClass.getSuperClass()" because the superclass might
// not have been loaded into BCEL yet.
if (targetClass.getSuperclassNameIndex() == 0) {
// The target class is Object; the search completed without finding a matching method.
if (debugHandleInvoke) {
System.out.printf("Unable to locate method: %s%n%n", methodName);
}
targetInstrumented = false;
break;
}
// Recurse looking in the superclass.
Expand Down
110 changes: 80 additions & 30 deletions java/daikon/dcomp/DCInstrument24.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,8 +538,8 @@ public record myLocalVariable(int slot, String name, ClassDesc descriptor) {}
/** If true, enable JUnit analysis debugging. */
protected static final boolean debugJunitAnalysis = false;

/** If true, enable {@link #getDefiningInterface} debugging. */
protected static final boolean debugGetDefiningInterface = false;
/** If true, enable {@link #getDeclaringInterface} debugging. */
protected static final boolean debugGetDeclaringInterface = false;

/** If true, enable {@link #handleInvoke} debugging. */
protected static final boolean debugHandleInvoke = false;
Expand Down Expand Up @@ -2887,23 +2887,53 @@ private void add_exit(
}

/**
* Returns the interface class name containing the implementation of the given method. The
* interfaces of {@code startClass} are recursively searched.
* Returns the first argument if it is non-null, otherwise the second.
*
* @param first the preferred value
* @param second the fallback value
* @return the first non-null argument, or null if both are null
*/
private static @Nullable @BinaryName String firstNonNull(
@Nullable @BinaryName String first, @Nullable @BinaryName String second) {
return first != null ? first : second;
}

/**
* Returns the name of the interface that declares the given method. The interfaces of {@code
* startClass} are recursively searched.
*
* <p>Note that this finds a <em>declaration</em>, which is usually not an implementation: an
* interface method is implicitly abstract unless it is {@code default}, {@code static}, or
* private. Pass true for {@code implementationsOnly} to match only a {@code default} method,
* which is the one case where the interface really does hold the code that will run.
*
* <p>Limitation: when several interfaces match, this returns the first one reached rather than
* the maximally specific one that JVMS 5.4.3.3 selects. A class that implements both an interface
* and a subinterface that reabstracts the same method gets the first of the two in declaration
* order, which may be the supertype. The consequence is confined to precision: the caller uses
* the answer only to decide whether the target is instrumented, and a wrong answer there loses
* comparability through the call rather than breaking it, because the uninstrumented overload it
* then invokes always exists.
*
* @param startClass the class whose interfaces are to be searched
* @param methodName the target method to search for
* @param paramTypes the target method's parameter types
* @return the name of the interface class containing target method, or null if not found
* @param implementationsOnly if true, match only a {@code default} method; if false, match any
* declaration, abstract ones included
* @return the name of the interface that declares the target method, or null if not found
*/
private @Nullable @BinaryName String getDefiningInterface(
ClassModel startClass, @Identifier String methodName, ClassDesc[] paramTypes) {
private @Nullable @BinaryName String getDeclaringInterface(
ClassModel startClass,
@Identifier String methodName,
ClassDesc[] paramTypes,
boolean implementationsOnly) {

if (debugGetDefiningInterface) {
if (debugGetDeclaringInterface) {
System.out.println("searching interfaces of: " + ClassGen24.getClassName(startClass));
}
for (ClassEntry classEntry : startClass.interfaces()) {
@BinaryName String interfaceName = Signatures.internalFormToBinaryName(classEntry.asInternalName());
if (debugGetDefiningInterface) {
if (debugGetDeclaringInterface) {
System.out.println("interface: " + interfaceName);
}
ClassModel cm;
Expand All @@ -2914,19 +2944,35 @@ private void add_exit(
} catch (Throwable t) {
throw new DynCompError(String.format("Unable to load class: %s", interfaceName), t);
}
boolean reabstracted = false;
for (MethodModel jm : cm.methods()) {
String jmName = jm.methodName().stringValue();
MethodTypeDesc mtd = jm.methodTypeSymbol();
if (debugGetDefiningInterface) {
if (debugGetDeclaringInterface) {
System.out.println(" " + jmName + Arrays.toString(mtd.parameterArray()));
}
if (jmName.equals(methodName) && Arrays.equals(mtd.parameterArray(), paramTypes)) {
// We have a match.
// We have a match. Neither a static nor a private interface method is ever the
// target of an INVOKEVIRTUAL: a private one is not even inherited.
AccessFlags jmFlags = jm.flags();
if (jmFlags.has(AccessFlag.STATIC) || jmFlags.has(AccessFlag.PRIVATE)) {
continue;
}
if (implementationsOnly && jmFlags.has(AccessFlag.ABSTRACT)) {
// This interface declares the method abstract. An interface may reabstract a default
// it inherits, and an implementor must then define the method, so any default above
// this point is hidden: do not search this branch further.
reabstracted = true;
break;
}
return interfaceName;
}
}
if (reabstracted) {
continue;
}
// no match found; does this interface extend other interfaces?
@BinaryName String foundAbove = getDefiningInterface(cm, methodName, paramTypes);
@BinaryName String foundAbove = getDeclaringInterface(cm, methodName, paramTypes, implementationsOnly);
if (foundAbove != null) {
// We have a match.
return foundAbove;
Expand Down Expand Up @@ -3216,7 +3262,10 @@ private boolean isTargetInstrumented(

@BinaryName String targetClassname = classname;
// Search this class for the target method. If not found, set targetClassname to
// its superclass and try again.
// its superclass and try again. Interfaces are not consulted here: JVMS 5.4.3.3
// resolves a method against the class's own declaration, then the superclass chain,
// and only then the superinterfaces, so the whole chain is searched first and the
// interfaces of the original target class afterwards.
mainloop:
while (true) {
// Check that the class exists
Expand Down Expand Up @@ -3256,36 +3305,37 @@ private boolean isTargetInstrumented(
}
}

{
// no methods match - search this class's interfaces
// No class in the chain declares the method, so it comes from an interface. Prefer
// a default method, which is an implementation; an abstract declaration only says
// where the method is declared, but that is the best available answer.
if (targetClassname.equals("java.lang.Object")) {
@BinaryName String found;
try {
found = getDefiningInterface(targetClass, methodName, paramTypes);
ClassModel origin = getClassModel(classname);
found =
origin == null
? null
: firstNonNull(
getDeclaringInterface(origin, methodName, paramTypes, true),
getDeclaringInterface(origin, methodName, paramTypes, false));
} catch (Throwable e) {
// We cannot locate or read the .class file, better assume it is not instrumented.
targetInstrumented = false;
break;
}
if (found != null) {
// We have a match.
if (found == null) {
if (debugHandleInvoke) {
System.out.printf("we have a match%n%n");
System.out.printf("Unable to locate method: %s%n%n", methodName);
}
targetInstrumented = false;
} else {
if (debugHandleInvoke) {
System.out.printf("declared by interface %s%n%n", found);
}
if (BcelUtil.inJdk(found)) {
targetInstrumented = false;
}
break;
}
}

// Method not found; perhaps inherited from superclass.
if (targetClassname.equals("java.lang.Object")) {
// The target class was Object; the search completed without finding a matching
// method.
if (debugHandleInvoke) {
System.out.printf("Unable to locate method: %s%n%n", methodName);
}
targetInstrumented = false;
break;
}

Expand Down
Loading
Loading