MemberSubstitution.failIfNoMatch seems to be ignored

[manoelcampos]

Hello, I searched the issues and questions here and in the Google Group, but I couldn't find any results.
I'm writing a tiny lib that implements a Byte Buddy plugin to replace accesses to public fields by the respective accessor (getter/setter) method call. After the directions you gave at #1514, everything is working fine.

However, if there is a missing accessor, the build fails with the message below:

Failed to transform class io.github.manoelcampos.accessors.sample.Main: [java.lang.IllegalStateException: Not exactly one method that matches SetterMatcher{field=available}: []].

Looking at the MemberSubstitution class, I realized there is a failIfNoMatch attribute.
The default value for any instance of the class (created by different constructors and factory methods) is false.

I downloaded Byte Buddy's source code and checked that this attribute is being set but it's only checked inside the MemberSubstitution.visitMaxs() method. Inside the resolve() method from MemberSubstitution.Matching, it always expects exactly 1 method match (otherwise, the exception above is thrown).

I'm wondering if the failIfNoMatch attribute should be checked when calling the MemberSubstitution.Matching.resolve() method.
Otherwise, how can I say that the field access must not be replaced if the accessor method is missing?

For instance, the matcher that looks for a setter related to a given field is this simple class here.

[raphw]

failIfNoMatch is meant to fail an application if a method does not contain any substitution that is specified. That means, you mean to replace a callback method, but the method has changed and does no longer contain an invocation to a callback.

The error you are getting is different. It says that you try to replace a method with another, by using a matcher, but the matcher does not yield exactly one replacement. If you want to only replace a method or field if a method exists, you should already exclude that element in the method or field matcher that identifies the element that is to be substituted.

[manoelcampos]

I get it, thanks again.

[manoelcampos]

Hey @raphw. I see how I can implement that, but I would like to discuss/suggest something, if I may.
I know that some changes in a complex and widely adopted project like this one are difficult, mainly when they affect current behaviour and existing users.

But see my scenario. I have a field matcher that is used to find direct accesses to public instance fields.
Then I have a method matcher to lookup for the corresponding accessor to be called in replacement of the field access.
If the field matcher is required to first check whether the accessor method exists, I start to have coupling between the field and method matcher (this one needs to lookup for the accessor too), making the field matcher more complex and less intuitive.

I'm wondering if it wouldn't be great for Byte Buddy users to indicate that a transformation must not be applied if the method to be called in replacement of some instruction is missing. The current behaviour of throwing an exception if the method is not found can stay. A new property such as the existing failIfNoMatch could be introduced to enable the new behavior of just ignoring the transformation.

I won't pretend I fully understand what is going on under Byte Buddy's hood.
But I've just made a fork of the project to play around. I performed some small changes and it kinda works, except that instead of ignoring the replacement, it removes the field access from the code.

Consider the following main method. There is no setAvailable and getName methods inside the Product class.

public static void main(final String[] args) {
    final var product = new Product();
    product.id = 1L;
    product.available = true;
    product.name = "Product Name";
    System.out.printf("id: %d name: %s%n", product.id, product.name);
}

The transformed main method should be:

public static void main(String[] args) {
    Product product = new Product();
    product.setId(1L);
    product.available = true;
    product.setName("Product Name");
    System.out.printf("id: %d name: %s%n", product.getId(), product.name);
}

but it is:

public static void main(String[] args) {
    Product product = new Product();
    product.setId(1L);
    true; // removed product.available =
    System.out.printf("id: %d name: %s%n", product.getId(), product); // removed .name after the last product reference
}

If you think other users could benefit from this proposed change, I appreciate if you could take a look at these small changes here. It's a start and I'd love to contribute somehow.

[raphw]

I cannot change this API and Optional is a Java 8 type that I cannot introduce.

I see your point of course, but wouldn't this be solved easier with a custom adapter? You can implement a custom Substitution where you can check if a Target.ForMember represents a given element and return a method write if you wanted this. This gives you the same result without API changes.

[manoelcampos]

but wouldn't this be solved easier with a custom adapter? You can implement a custom Substitution where you can check if a Target.ForMember represents a given element and return a method write if you wanted this. This gives you the same result without API changes.

Firstly, thanks for your always prompt and thoughtful reply. From the user perspective, honestly I don't think so. Byte Buddy provides a nice and fluent API that usually makes it easier for new users to just press a dot from a class/interface, browse through existing methods and fluidly start making chained calls until the desired end. My instrumentation plugin class below is a proof of that.

// Some code ommited
public class EntityAccessorInstrumentationPlugin implements Plugin {
    @Override
    public DynamicType.Builder<?> apply(/* params here */) {
        final var visitorForMethodWithFieldRead = 
                  MemberSubstitution.relaxed()
                                    .failIfNoMatch(false)
                                    .field(new InstanceFieldMatcher())
                                    .onRead()
                                    .replaceWithMethod(new GetterMatcher())
                                    .on(ElementMatchers.isMethod());

        // code for visitorForMethodWithFieldWrite goes here

        return builder.visit(visitorForMethodWithFieldRead).visit(visitorForMethodWithFieldWrite);
    }
}

Implementing custom classes is not that obvious and requires more knowledge about the lib.
For instance, I haven't thought about implementing a new Substitution since the lib was providing most of the implementations I needed, and that would be my last resort.

If I decide to go this way, implementing some interfaces such as Substitution is not intuitive. That is explained since the resolve method needs to return a StackManipulation object, that starts to touch the internals of Byte Buddy. I think you can agree that not everyone knows how such an object is expected to work (me included), or even what are the concrete classes I can instantiate.

Looking at the changes I've made in my fork, you can see that I struggled with this StackManipulation object, since it didn't work nicely as I showed in the transformed code previously.

Therefore, my point is: the lib already provides what I need (and probably my scenario is not a particular one), it just has to avoid throwing an exception that I don't care about.

I cannot change this API and Optional is a Java 8 type that I cannot introduce

I'm confused, because IntelliJ allowed me to use Java 8 features, so I thought this version was allowed.
Anyway, I just removed the Optional and returned null instead, ensuring to perform a null check inside the only place I found the method being called (check link above).

This way, no changes in the public API were performed. We just need to introduce a new boolean attribute to enable this new behavior, so that existing users won't be affected.

Looking forward to know what you think about it.

[raphw]

I wonder: could you not expand your matcher to check if a field has a getter/setter and can be substituted?

You have access to the declaring type from the matcher, so this would certainly work.

[manoelcampos]

Yes, for sure. Considering the current state of the lib, that is the easiest way.
But then, we go back to the beginning of the discussion: it creates coupling between the Field Matcher and the Method Matcher (this one will have to lookup for the accessor again).

If I have a flag to avoid throwing the "method not found" exception, I don't even need to write any additional code.
Please don't get me wrong. I could implement as you suggested, but I'm just trying to provide a user point of view and contribute somehow. And I understand that you need to be extra careful before adding anything to a lib like that.

Thanks.

[manoelcampos]

Maybe this will make it easier to see that the implementation burden on the user side is not that light.
I followed your suggestion and made my InstanceFieldMatcher to firstly lookup for the corresponding getter/setter and it works perfectly.

But just check this commit manoelcampos/auto-class-accessors-java@0154801
It's too much code complexity and coupling for something that was expected to be straightforward.