Problem with abstract class and assisted inject after migration to Java 8

[davido]

With migration change [1] to Java 8, Gerrit Code Review doesn't start any more: [2]. This diff fixed this: [3]. All is fine with the code on Java 7. Is this expected?

[1] https://gerrit-review.googlesource.com/57852
[2] http://paste.openstack.org/show/176620
[3] https://gerrit-review.googlesource.com/64820

[sameb]

Java8 does change a few things, but the error & diff you pasted don't seem to have anything to do with java8. Can you point to com.google.gerrit.server.config.GerritGlobalModule & com.google.gerrit.server.change.Module? I'd like to see how the factory is being installed.

[davido]

• com.google.gerrit.server.config.GerritGlobalModule: [1]
• com.google.gerrit.server.change.Module: [2]

[1] https://gerrit.googlesource.com/gerrit/+/master/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
[2] https://gerrit.googlesource.com/gerrit/+/master/gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java

[sameb]

This indeed looks like a problem with the way assistedinject scans for factory methods & the way java8 introduces new synthetic/bridge methods. I have an idea of what's wrong and will write up a testcase + fix. Thanks for reporting it!

[victorr]

Hi,

I have been banging my head against this problem this week. Hopefully the following will aid you in your testing.

Any updates on a fix will be greatly appreciated, as my shop can't wait to upgrade to Java 8.

Kind regards.

package java8;

import com.google.inject.AbstractModule;
import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.Module;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import com.google.inject.assistedinject.FactoryModuleBuilder;

public class InyeccionAsistida {
    public interface AbstractClass {}

    public interface AbstractConstructionParam {}

    public interface AbstractFactory <T extends AbstractConstructionParam> {
        AbstractClass create( T param );
    }

    public static class ConcreteClass implements AbstractClass {
        private ConcreteConstructionParam mValue;

        @AssistedInject
        ConcreteClass( @Assisted ConcreteConstructionParam value ) {
            mValue = value;
        }

        @Override
        public String toString() {
            return mValue.toString();
        }
    }

    public static class ConcreteConstructionParam implements AbstractConstructionParam {}

    public static interface ConcreteFactory extends AbstractFactory<ConcreteConstructionParam> {
        ConcreteClass create( ConcreteConstructionParam param );
    }

    public static void main( String[] args ) {
        Module module = new AbstractModule() {
            @Override
            protected void configure() {
                Module factoryModule = new FactoryModuleBuilder()
                    .implement( AbstractClass.class, ConcreteClass.class )
                    .build( ConcreteFactory.class );

                install( factoryModule );
            }
        };

        Injector injector = Guice.createInjector( module );

        ConcreteFactory concreteFactory = injector.getInstance( ConcreteFactory.class );
        System.out.println( concreteFactory.create( new ConcreteConstructionParam() ) );

        AbstractFactory<AbstractConstructionParam> abstractFactory = cast( concreteFactory );
        System.out.println( abstractFactory.create( new ConcreteConstructionParam() ) );
    }

    @SuppressWarnings("unchecked")
    private static <T extends AbstractConstructionParam> AbstractFactory<T> cast( ConcreteFactory concreteFactory ) {
        return (AbstractFactory<T>) concreteFactory;
    }
}

[sameb]

Fix will be merged soon, in the latest pull request. Sorry for the troubles.

[victorr]

Thanks for the pointers to your commit.

Unfortunately the fix is not quite there yet. I've attempted to modify your unit test to demonstrate the problem:

diff --git a/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java b/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java
index f58352b..26e6f21 100644
--- a/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java
+++ b/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java
@@ -29,6 +29,7 @@ import com.google.inject.Key;
 import com.google.inject.Provider;
 import com.google.inject.Stage;
 import com.google.inject.TypeLiteral;
+import com.google.inject.assistedinject.FactoryProvider2Test.ConcreteAssistedWithOverride.Factory;
 import com.google.inject.assistedinject.FactoryProvider2Test.Equals.ComparisonMethod;
 import com.google.inject.assistedinject.FactoryProvider2Test.Equals.Impl;
 import com.google.inject.matcher.Matchers;
@@ -1133,8 +1134,8 @@ public class FactoryProvider2Test extends TestCase {
   }

   static abstract class AbstractAssisted {
-    interface Factory<T extends AbstractAssisted> {
-      T create(String string);
+    interface Factory<T extends AbstractAssisted,P extends CharSequence> {
+      T create(P string);
     }
   }

@@ -1145,26 +1146,33 @@ public class FactoryProvider2Test extends TestCase {
   static class ConcreteAssistedWithOverride extends AbstractAssisted {
     @Inject ConcreteAssistedWithOverride(@SuppressWarnings("unused") @Assisted String string) {}

-    interface Factory extends AbstractAssisted.Factory<ConcreteAssistedWithOverride> {
+    interface Factory extends AbstractAssisted.Factory<ConcreteAssistedWithOverride,String> {
       @Override ConcreteAssistedWithOverride create(String string);
     }
   }

   static class ConcreteAssistedWithoutOverride extends AbstractAssisted {
     @Inject ConcreteAssistedWithoutOverride(@SuppressWarnings("unused") @Assisted String string) {}
-    interface Factory extends AbstractAssisted.Factory<ConcreteAssistedWithoutOverride> {}
+    interface Factory extends AbstractAssisted.Factory<ConcreteAssistedWithoutOverride,String> {}
   }

   // See https://github.com/google/guice/issues/904
   public void testIgnoresSyntheticFactoryMethods() {
     // Validate the injector can be successfully created.
-    Guice.createInjector(new AbstractModule() {
+    Injector injector = Guice.createInjector(new AbstractModule() {
       @Override protected void configure() {
         install(new FactoryModuleBuilder().build(ConcreteAssistedWithOverride.Factory.class));
         install(new FactoryModuleBuilder().build(ConcreteAssistedWithoutOverride.Factory.class));
         install(new FactoryModuleBuilder().build(
-            new TypeLiteral<AbstractAssisted.Factory<ConcreteAssisted>>() {}));
+            new TypeLiteral<AbstractAssisted.Factory<ConcreteAssisted,String>>() {}));
       }
     });
+
+    Factory concreteFactory = injector.getInstance( ConcreteAssistedWithOverride.Factory.class );
+    concreteFactory.create( "" );
+    
+    AbstractAssisted.Factory abstractFactory = concreteFactory;
+    
+    abstractFactory.create( "NPE" );
   }
 }

If you run the modified unit test, you get this result:

$ java -cp "build:lib/*" junit.textui.TestRunner  com.google.inject.assistedinject.FactoryProvider2Test
.E
Time: 0,262
There was 1 error:
1) testIgnoresSyntheticFactoryMethods(com.google.inject.assistedinject.FactoryProvider2Test)java.lang.NullPointerException
    at com.google.inject.assistedinject.FactoryProvider2.invoke(FactoryProvider2.java:674)
    at com.google.inject.assistedinject.$Proxy6.create(Unknown Source)
    at com.google.inject.assistedinject.FactoryProvider2Test.testIgnoresSyntheticFactoryMethods(FactoryProvider2Test.java:1174)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)

FAILURES!!!
Tests run: 1,  Failures: 0,  Errors: 1

As you probably already know, this is not a problem with the Eclipse compiler, since it doesn't insert the additional method to the interface.

BTW, if you run the code I posted before, you get the same error:

java8.InyeccionAsistida$ConcreteConstructionParam@1defd3f
Exception in thread "main" java.lang.NullPointerException
    at com.google.inject.assistedinject.FactoryProvider2.invoke(FactoryProvider2.java:674)
    at com.sun.proxy.$Proxy5.create(Unknown Source)
    at java8.InyeccionAsistida.main(InyeccionAsistida.java:68)

I hope you find this useful, and thanks for looking into this.

[sameb]

Thanks for the expanded test, I'll give this another shot.

[victorr]

I hacked a crude fix for FactoryProvider2.invoke() to look up the AssistData for the synthetic method using only its name. I'm not really suggesting that as a proper fix, but for what it is worth, with that fix in place nothing else broke in our code base.

In other words, we are eagerly awaiting the proper fix.

Thanks again.

[sameb]

I have a hack in the works. The JDK unfortunately gives no good way to do this. The problem is we need to identify and call default methods with the default (not proxied) implementation. The MethodHandles class is supposed to let you, but it doesn't let you override visibility restrictions. Supposedly it's possible to use reflection to hack up MethodHandles to let you do it, but I couldn't get it working reliably, and it's also not safe to not use the public API. So regardless, we need a backup way of doing it... I've got a backup, but it isn't foolproof, so I'm going to take another stab at MethodHandles.

[sameb]

FYI, latest push to #905 includes a fix for the NPE too. Could you give it a shot and let me know if it works for you?

[victorr]

I tested my code and didn't run into any more issues.

Thanks for the outstanding work!

[davido]

Thanks for the fix. Any ETA for new release that contains it?