Guice: Default methods aren't skipped

[nathanmerrill]

Interfaces can have default methods since Java 8. There was an attempted code fix to ignore them in the FactoryProvider: https://github.com/google/guice/commit/85f14e03ee00b20e20fd0f018a00ef52fcf909b1

However, the fix doesn't work because the condition needs to be || instead of &&. (Default methods aren't synthetic or bridge)

This makes it so that if you call any default method in an interface, you end up with the Guice-provided method instead of the default one.

public interface FooFactory {
    Foo create();

    default Foo createWithData(Data data){ 
        Foo foo = create();  // This will never be called, even if you call `fooFactory.createWithData(data)`
        foo.addData(data);
        return foo;
    }
}

[dimo414]

Thanks for the bug report Nathan, any chance you'd be willing to send a pull request (along with a test case)?

[nathanmerrill]

I'm not confident that I know what the fix is. I _think_ that the fix is to use an ||, but with the commit, they added two lines:

validateFactoryReturnType(errors, method.getReturnType(), factoryRawType);
defaultMethods.put(method.getName(), method);

which I'm not sure what it does, and whether it applies to all three types.

As far as testing, where would that test go? I imagine somewhere in this package, would I simply add another class to that package?

[sameb]

Note that https://github.com/google/guice/commit/85f14e03ee00b20e20fd0f018a00ef52fcf909b1 wasn't intended to fix "default" methods per say -- it was intended to fix synthetic/bridge methods that java8 created as default methods. See the issue (https://github.com/google/guice/issues/904) for more details.

That said, skipping non-bridge/synthetic default methods may be desirable, but also may be backwards incompatible. We reviewed a similar change internally (for reference: cl/148494483), but the review stalled because we couldn't come up with a good answer for how to make it work without silently changing behavior for folks that might have relied on this.

FWIW, the validateFactoryReturnType line is about ensuring the return type visibility matches the factory type visibility -- otherwise the java.lang.reflect.Proxy freaks out. The message in that method gives a good explanation.

The defaultMethods map is used to keep track of our default methods so that our Proxy implementation knows how to call it. Making this work is made more complex by the fact that the JDK doesn't give any good way to call a default method, so we have to hack around it. The CL description gives some good explanation of what we're doing here.

[nathanmerrill]

You mention skipping non-bridge/synthetic default methods may be backwards incompatible: I'm struggling to come up with a use-case of somebody writing a default method, but wanting it to be overridden by Guice.

That said, assuming that there is a use-case, I think it would it be possible to use some annotation to indicate to "skip this method"? I'm not a huge fan of introducing an entirely new annotation for a small edge case as this, but I think it would work.

[badbob]

Is there are any progress on this issue?