Improper visibility restrictions when applying `MemberSubstitution`

[tylerbenson]

In your suggestion in #483:

You can then register a member substitution to replace the call to blaBla(thiz, object) in the advice class with a call to super.blaBla(arg).

This doesn't seem to work if the super call isn't accessible to the advice class. https://github.com/raphw/byte-buddy/blob/master/byte-buddy-dep/src/main/java/net/bytebuddy/asm/MemberSubstitution.java#L1121-L1123

For example, say I have some advice:

public class AbstractExecutorAdvice {
    @Advice.OnMethodEnter
    public static void execute(
        @Advice.This AbstractExecutorService executor,
        @Advice.Argument(value = 0, readOnly = false) Runnable task) {
      task = superNewTaskFor(executor, task, null);
    }

    private static <T> RunnableFuture<T> superNewTaskFor(
        AbstractExecutorService executor, Runnable runnable, T value) {
      return null;
    }
}

This results in the following exception with MemberSubstitution applied:

Caused by: java.lang.IllegalStateException: class AbstractExecutorAdvice cannot access protected java.util.concurrent.RunnableFuture java.util.concurrent.AbstractExecutorService.newTaskFor(java.lang.Runnable,java.lang.Object)
        at net.bytebuddy.asm.MemberSubstitution$Substitution$ForMethodInvocation.resolve(MemberSubstitution.java:1122)
        at net.bytebuddy.asm.MemberSubstitution$Replacement$Binding$Resolved.make(MemberSubstitution.java:1755)
        at net.bytebuddy.asm.MemberSubstitution$SubstitutingMethodVisitor.visitMethodInsn(MemberSubstitution.java:2371)

MemberSubstitution.strict()
  .method(named("superNewTaskFor"))
  .replaceWith(AbstractExecutorService.class.getDeclaredMethod("newTaskFor", Runnable.class, Object.class))
  .on(any())

It would also be nice if #484 could handle this case without the need for MemberSubstitution.

[raphw]

Seems like the target does not get rewritten properly, I will have a look!

[tylerbenson]

@raphw any update? I tried implementing this with a custom build Plugin but I'm getting verify errors. Doesn't seem like I can get the super call bytecode generated properly.

[raphw]

Sorry for the slow response, new baby makes me little productive these weeks. I'll have a look soon.

[raphw]

I think we are considering two different things here. I'd suggest to apply the substitution on top of the advice, I think you are instrumenting the advice itself before applying it?

Consider the following advice:

class FooAdvice {
  @OnMethodEnter
  static void onMethodEnter() { pseudo(); }
  static void pseudo() { }
}

You can then apply it as:

builder
  .visit(Advice.to(FooAdvice.class))
  .visit(<MemberSubstitution of pseudo to the method in question>)

Doing so, Byte Buddy can validate that the actual target class can read the substituted method and generate valid byte code.

Does this make sense?

[tylerbenson]

ok, that makes sense... I misunderstood your previous suggestion. Meanwhile, any plans for implementing #484?

[raphw]

Yes, it's still on the agenda. I have looked into it briefly but I want to plug a member substitution eventually which did not fit together well yet. But I certainly want to add it!