byte-buddy icon indicating copy to clipboard operation
byte-buddy copied to clipboard

ByteBuddy doesn't handle diamond problem properly

Open novoj opened this issue 4 years ago • 4 comments

I have default method defined in SomeSharedInterface and have two additional interfaces Trait1 and Trait2 - both extending SomeSharedInterface - i.e. inheriting default implementation of default method in SomeSharedInterface.

I declare new non-abstract class that implements both Trait1 and Trait2 interfaces and call the method, that has default implementation in SomeSharedInterface. Plain java executes method correctly, ByteBuddy proxy receives NULL in @DefaultCall, probably because it thinks that tracking this call is impossible due to ambiguity. But it's resolvable.

I've tested the problem on the latest version available 1.12.5 - because I noticed somewhere on Twitter that the default method handling should have been corrected in latest versions, but it seems that there are still blind spots.

Thanks for your answer, I appreciate the work invested in ByteBuddy!

The problem is demonstrated by following test:

public class DiamondProblemTest {

	@Test
	public void demonstrateDiamondProblem() throws InstantiationException, IllegalAccessException {
		// this works properly - plain java
		assertEquals("whatever", new ExampleJavaClass().getCode());

		// this does not
		final Class<?> proxy = new ByteBuddy()
			.subclass(Object.class)
			.implement(Trait1.class, Trait2.class)
			// AND TRAP ALL METHODS EXCEPT CONSTRUCTORS AND FINALIZER
			.method(
				ElementMatchers.noneOf(
					ElementMatchers.isFinalizer(), ElementMatchers.isConstructor()
				)
			)
			// AND DELEGATE CALL TO OUR INVOCATION HANDLER STORED IN PRIVATE FIELD OF THE CLASS
			.intercept(MethodDelegation.to(ByteBuddyDispatcherInvocationHandler.class))
			// NOW CREATE THE BYTE-CODE
			.make()
			// AND LOAD IT IN CURRENT CLASSLOADER
			.load(DiamondProblemTest.class.getClassLoader())
			// RETURN
			.getLoaded();

		// that's still ok
		final Object theInstance = proxy.newInstance();
		assertNotNull(theInstance);
		// here I receive NullPointerException
		assertEquals("whatever", ((SomeSharedInterface)theInstance).getCode());
	}

	public interface Trait1 extends SomeSharedInterface {

	}

	public interface Trait2 extends SomeSharedInterface {

	}

	public interface SomeSharedInterface {

		default String getCode() {
			return "whatever";
		}

	}

	public static class ExampleJavaClass implements Trait1, Trait2 {

	}

	public static class ByteBuddyDispatcherInvocationHandler {

		@RuntimeType
		public static Object interceptMethodCall(
			@This Object proxy,
			@Origin Method method,
			@SuperCall(nullIfImpossible = true, serializableProxy = true, fallbackToDefault = false) Callable<Object> superMethod,
			@DefaultCall(nullIfImpossible = true, serializableProxy = true) Callable<Object> defaultMethod,
			@AllArguments Object[] args
		) throws Throwable {
			// default method should always retrieve default method reference, should it not?!
			if (method.isDefault()) {
				return defaultMethod.call();
			} else {
				return superMethod.call();
			}
		}

	}

}

novoj avatar Dec 20 '21 13:12 novoj

Well, it's more complicated then that. For example, you could not override getCode from ExampleJavaClass without specifying the path downwards:

public static class ExampleJavaClass implements Trait1, Trait2 {
    @Override
    public String getCode() {
        return super.getCode();
    }
}

This is the situation Byte Buddy is put. Instead, it needs to specify if it wants to invoke the super method via Trait1.super.getCode() or Trait2.super.getCode(). Byte Buddy normally chooses this for you if there is only one choice, but here you need to specify the interface via targetType = Trait1.class (or the other).

raphw avatar Dec 20 '21 20:12 raphw

Ok, I understand. For the time being I'm forced to use something like:

public class DiamondProblemTest {

	@Test
	public void demonstrateDiamondProblem() throws InstantiationException, IllegalAccessException {
		// this works properly - plain java
		assertEquals("whatever", new ExampleJavaClass().getCode());

		// this does not
		final Class<?> proxy = new ByteBuddy()
			.subclass(Object.class)
			.implement(Trait1.class, Trait2.class)
			// AND TRAP ALL METHODS EXCEPT CONSTRUCTORS AND FINALIZER
			.method(
				ElementMatchers.noneOf(
					ElementMatchers.isFinalizer(), ElementMatchers.isConstructor()
				)
			)
			// AND DELEGATE CALL TO OUR INVOCATION HANDLER STORED IN PRIVATE FIELD OF THE CLASS
			.intercept(MethodDelegation.to(ByteBuddyDispatcherInvocationHandler.class))
			// NOW CREATE THE BYTE-CODE
			.make()
			// AND LOAD IT IN CURRENT CLASSLOADER
			.load(DiamondProblemTest.class.getClassLoader())
			// RETURN
			.getLoaded();

		// that's still ok
		final Object theInstance = proxy.newInstance();
		assertNotNull(theInstance);
		// here I receive NullPointerException
		assertEquals("whatever", ((SomeSharedInterface)theInstance).getCode());
	}

	public interface Trait1 extends SomeSharedInterface {

	}

	public interface Trait2 extends SomeSharedInterface {

	}

	public interface SomeSharedInterface {

		default String getCode() {
			return "whatever";
		}

	}

	public static class ExampleJavaClass implements Trait1, Trait2 {

	}

	public static class ByteBuddyDispatcherInvocationHandler {

		@RuntimeType
		public static Object interceptMethodCall(
			@This Object proxy,
			@Origin Method method,
			@SuperCall(nullIfImpossible = true, serializableProxy = true, fallbackToDefault = false) Callable<Object> superMethod,
			@DefaultCall(nullIfImpossible = true, serializableProxy = true) Callable<Object> defaultMethod,
			@AllArguments Object[] args
		) throws Throwable {
			// default method should always retrieve default method reference, should it not?!
			if (method.isDefault()) {
				if (defaultMethod == null) {
					return findMethodHandle(method).bindTo(proxy).invokeWithArguments(args);
				} else {
					return defaultMethod.call();
				}
			} else {
				return superMethod.call();
			}
		}

	}

	/**
	 * Finds method handle for the default method.
	 * @param method
	 * @return
	 */
	static MethodHandle findMethodHandle(Method method) {
		try {

			final Constructor<Lookup> constructor = Lookup.class.getDeclaredConstructor(Class.class, int.class);
			constructor.setAccessible(true);

			final Class<?> declaringClass = method.getDeclaringClass();
			return constructor
				.newInstance(declaringClass, Lookup.PRIVATE)
				.unreflectSpecial(method, declaringClass);

		} catch (Exception ex) {
			throw new IllegalArgumentException("Can't find handle to method " + method.toGenericString() + "!", ex);
		}

	}

}

Of course MethodHandle is good to be cached somewhere - and it's still reflection code that is probably much slower than if it would be supported by ByteBuddy directly.

(note for other people that might run at the problem)

novoj avatar Dec 21 '21 07:12 novoj

You can use Origin on a method handle parameter. The handle should have the correct privilege for invocation etc.

raphw avatar Dec 21 '21 10:12 raphw

I tried it, and when calling:

methodHandle.bindTo(proxy).invokeWithArguments(args)

On the handle I got 'java.lang.StackOverflowError'.

When using:

methodHandle.invokeWithArguments(args)

I got java.lang.invoke.WrongMethodTypeException: cannot convert MethodHandle(SomeSharedInterface)String to ()Object.

But my original workaround works - so no need to worry.

novoj avatar Dec 21 '21 11:12 novoj

Just to clarify the reason for closing. The issue was closed because "the question was answered" but no changes in the codebase were made or it was closed because you've somehow addressed the diamond problem?

novoj avatar Jan 05 '23 07:01 novoj

Sorry, my bad. I was clearing out old issues, but I will try to look into this.

raphw avatar Jan 05 '23 10:01 raphw

I added a new value for @Origin to accept MethodHandles.Lookup to make this always possible but this is how it would work (since Java 9):

  public static class ByteBuddyDispatcherInvocationHandler {

      @RuntimeType
      public static Object interceptMethodCall(
              @This Object proxy,
              @Origin Method method,
              @Origin MethodHandles.Lookup lookup,
              @SuperCall(nullIfImpossible = true, serializableProxy = true, fallbackToDefault = false) Callable<Object> superMethod,
              @DefaultCall(nullIfImpossible = true, serializableProxy = true) Callable<Object> defaultMethod,
              @AllArguments Object[] args
      ) throws Throwable {
          // default method should always retrieve default method reference, should it not?!
          if (method.isDefault()) {
              if (defaultMethod == null) {
                  return MethodHandles.privateLookupIn(SomeSharedInterface.class, lookup)
                          .findSpecial(SomeSharedInterface.class, "getCode", MethodType.methodType(String.class), SomeSharedInterface.class)
                          .bindTo(proxy)
                          .invokeWithArguments(args);
              } else {
                  return defaultMethod.call();
              }
          } else {
              return superMethod.call();
          }
      }
  }

Alternatively, you could create your own binding to supply your own proxy. Of course, the resulting method handle should be cached for reuse.

The reason this cannot be resolved is that both Trait1 and Trait2 could later introduce an override of the method which requires to make this explicit. This is in principal not a determinate invocation.

raphw avatar Jan 09 '23 13:01 raphw

Thank you! What version this change will be part of? I have an unit test for this situation, so I can test the new approach there.

novoj avatar Jan 09 '23 14:01 novoj

Next release.

I tested it on master but always happy if you validate the solution on your end.

raphw avatar Jan 09 '23 14:01 raphw

I can confirm, it works with locally built 1.12.23-SNAPSHOT version of ByteBuddy.

The generic code, that derives the called method from the context is:

final Callable<Object> superCallable;
if (method.isDefault()) {
	if (defaultMethod == null) {
		superCallable = () -> {
			try {
				return MethodHandles.privateLookupIn(method.getDeclaringClass(), lookup)
					.findSpecial(
						method.getDeclaringClass(),
						method.getName(),
						MethodType.methodType(String.class),
						method.getDeclaringClass()
					)
					.bindTo(proxy)
					.invokeWithArguments(args);
			} catch (Throwable e) {
				throw new InvocationTargetException(e);
			}
		};
	} else {
		superCallable = defaultMethod;
	}
} else {
	superCallable = superMethod;
}

The test-case with diamond problem passes in my code. Thank you for your support!

novoj avatar Jan 17 '23 21:01 novoj