Further fixes for https://github.com/google/guice/issues/904 -- java8 generates
default methods for subclasses when they override generic methods with the more
specific type.
We use a two-tiered approach to fixing: (1) try to use MethodHandles + unreflectSpecial, which lets us call default method implementations directly, and if that doesn't work then (2) try to map default methods to compatible method signatures that could be the overrides of the method. (1) may not always work because we're using a private API [new Lookup(clazz, int)], but we need to do that in order to non-public classes. (2) may not always work because it's possible to have more than one compatible method signature.
In the unlikely case that both (1) & (2) fail, we give an error message.
Also: we must validate the default method's return type for visibility vs the factory type's visibility too.
This ends up with two possible differences caused by java8:
a) If the Lookup cxtor can't be used (different JDK, version skew, etc..) and there's more than one compatible method signature: we fail.
b) If the default method's return type isn't public but the factory is public: we fail.
For reference, javac8 generates a default method in the following scenario:
interface Parent<I extends CharSequence, O extends Number> {
O create(I input);
}
interface Child<String, Integer> {
Integer create(String input);
}
Child has a generated default method of:
Number create(CharSequence input);
... so, for example, failure could be newly triggered if 'Number' was package-private but Child was public, or if the reflection APIs didn't work and Child also had a 'Integer create(StringBuilder input);' method.
-------------
Created by MOE: http://code.google.com/p/moe-java
MOE_MIGRATED_REVID=87097207
diff --git a/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java b/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java
index 15b4220..b40e19f 100644
--- a/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java
+++ b/extensions/assistedinject/src/com/google/inject/assistedinject/FactoryProvider2.java
@@ -20,11 +20,13 @@
import static com.google.common.collect.Iterables.getOnlyElement;
import com.google.common.base.Objects;
+import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
+import com.google.common.collect.Multimap;
import com.google.common.collect.Sets;
import com.google.inject.AbstractModule;
import com.google.inject.Binder;
@@ -57,6 +59,7 @@
import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationHandler;
+import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.lang.reflect.Proxy;
@@ -183,6 +186,9 @@
/** Mapping from method to the data about how the method will be assisted. */
private final ImmutableMap<Method, AssistData> assistDataByMethod;
+ /** Mapping from method to method handle, for generated default methods. */
+ private final ImmutableMap<Method, MethodHandleWrapper> methodHandleByMethod;
+
/** the hosting injector, or null if we haven't been initialized yet */
private Injector injector;
@@ -214,15 +220,22 @@
if(!factoryRawType.isInterface()) {
throw errors.addMessage("%s must be an interface.", factoryRawType).toException();
}
-
+
+ Multimap<String, Method> defaultMethods = HashMultimap.create();
+ Multimap<String, Method> otherMethods = HashMultimap.create();
ImmutableMap.Builder<Method, AssistData> assistDataBuilder = ImmutableMap.builder();
// TODO: also grab methods from superinterfaces
for (Method method : factoryRawType.getMethods()) {
- // Skip synthetic methods that java8 may have created.
- if (method.isBridge() || method.isSynthetic()) {
+ // Skip default methods that java8 may have created.
+ if (isDefault(method) && (method.isBridge() || method.isSynthetic())) {
+ // Even synthetic default methods need the return type validation...
+ // unavoidable consequence of javac8. :-(
+ validateFactoryReturnType(errors, method.getReturnType(), factoryRawType);
+ defaultMethods.put(method.getName(), method);
continue;
}
-
+ otherMethods.put(method.getName(), method);
+
TypeLiteral<?> returnTypeLiteral = factoryType.getReturnType(method);
Key<?> returnType;
try {
@@ -294,9 +307,51 @@
providers = providerListBuilder.build();
optimized = true;
}
- assistDataBuilder.put(method,
- new AssistData(constructor, returnType, immutableParamList, implementation,
- method, removeAssistedDeps(deps), optimized, providers));
+
+ AssistData data = new AssistData(constructor,
+ returnType,
+ immutableParamList,
+ implementation,
+ method,
+ removeAssistedDeps(deps),
+ optimized,
+ providers);
+ assistDataBuilder.put(method, data);
+ }
+
+ factory = factoryRawType.cast(Proxy.newProxyInstance(
+ BytecodeGen.getClassLoader(factoryRawType), new Class<?>[] {factoryRawType}, this));
+
+ // Now go back through default methods. Try to use MethodHandles to make things
+ // work. If that doesn't work, fallback to trying to find compatible method
+ // signatures.
+ Map<Method, AssistData> dataSoFar = assistDataBuilder.build();
+ ImmutableMap.Builder<Method, MethodHandleWrapper> methodHandleBuilder = ImmutableMap.builder();
+ for (Map.Entry<String, Method> entry : defaultMethods.entries()) {
+ Method defaultMethod = entry.getValue();
+ MethodHandleWrapper handle = MethodHandleWrapper.create(defaultMethod, factory);
+ if (handle != null) {
+ methodHandleBuilder.put(defaultMethod, handle);
+ } else {
+ boolean foundMatch = false;
+ for (Method otherMethod : otherMethods.get(defaultMethod.getName())) {
+ if (dataSoFar.containsKey(otherMethod) && isCompatible(defaultMethod, otherMethod)) {
+ if (foundMatch) {
+ errors.addMessage("Generated default method %s with parameters %s is"
+ + " signature-compatible with more than one non-default method."
+ + " Unable to create factory. As a workaround, remove the override"
+ + " so javac stops generating a default method.",
+ defaultMethod, Arrays.asList(defaultMethod.getParameterTypes()));
+ } else {
+ assistDataBuilder.put(defaultMethod, dataSoFar.get(otherMethod));
+ foundMatch = true;
+ }
+ }
+ }
+ if (!foundMatch) {
+ throw new IllegalStateException("Can't find method compatible with: " + defaultMethod);
+ }
+ }
}
// If we generated any errors (from finding matching constructors, for instance), throw an exception.
@@ -305,12 +360,35 @@
}
assistDataByMethod = assistDataBuilder.build();
+ methodHandleByMethod = methodHandleBuilder.build();
} catch (ErrorsException e) {
throw new ConfigurationException(e.getErrors().getMessages());
}
+ }
- factory = factoryRawType.cast(Proxy.newProxyInstance(BytecodeGen.getClassLoader(factoryRawType),
- new Class[] { factoryRawType }, this));
+ static boolean isDefault(Method method) {
+ // Per the javadoc, default methods are non-abstract, public, non-static.
+ // They're also in interfaces, but we can guarantee that already since we only act
+ // on interfaces.
+ return (method.getModifiers() & (Modifier.ABSTRACT | Modifier.PUBLIC | Modifier.STATIC))
+ == Modifier.PUBLIC;
+ }
+
+ private boolean isCompatible(Method src, Method dst) {
+ if (!src.getReturnType().isAssignableFrom(dst.getReturnType())) {
+ return false;
+ }
+ Class<?>[] srcParams = src.getParameterTypes();
+ Class<?>[] dstParams = dst.getParameterTypes();
+ if (srcParams.length != dstParams.length) {
+ return false;
+ }
+ for (int i = 0; i < srcParams.length; i++) {
+ if (!srcParams[i].isAssignableFrom(dstParams[i])) {
+ return false;
+ }
+ }
+ return true;
}
public F get() {
@@ -659,6 +737,13 @@
* use that to get an instance of the return type.
*/
public Object invoke(Object proxy, final Method method, final Object[] args) throws Throwable {
+ // If we setup a method handle earlier for this method, call it.
+ // This is necessary for default methods that java8 creates, so we
+ // can call the default method implementation (and not our proxied version of it).
+ if (methodHandleByMethod.containsKey(method)) {
+ return methodHandleByMethod.get(method).invokeWithArguments(args);
+ }
+
if (method.getDeclaringClass().equals(Object.class)) {
if ("equals".equals(method.getName())) {
return proxy == args[0];
@@ -670,6 +755,7 @@
}
AssistData data = assistDataByMethod.get(method);
+ checkState(data != null, "No data for method: %s", method);
Provider<?> provider;
if(data.cachedBinding != null) { // Try to get optimized form...
provider = data.cachedBinding.getProvider();
@@ -740,4 +826,84 @@
+ " (This should never happen. If it does, please report it.)");
}
}
+
+ /** Wrapper around MethodHandles/MethodHandle, so we can compile+run on java6. */
+ private static class MethodHandleWrapper {
+ static final int ALL_MODES = Modifier.PRIVATE
+ | Modifier.STATIC /* package */
+ | Modifier.PUBLIC
+ | Modifier.PROTECTED;
+
+ static final Method unreflectSpecial;
+ static final Method bindTo;
+ static final Method invokeWithArguments;
+ static final Constructor<?> lookupCxtor;
+ static final boolean valid;
+
+ static {
+ Method unreflectSpecialTmp = null;
+ Method bindToTmp = null;
+ Method invokeWithArgumentsTmp = null;
+ boolean validTmp = false;
+ Constructor<?> lookupCxtorTmp = null;
+ try {
+ Class<?> lookupClass = Class.forName("java.lang.invoke.MethodHandles$Lookup");
+ unreflectSpecialTmp = lookupClass.getMethod("unreflectSpecial", Method.class, Class.class);
+ Class<?> methodHandleClass = Class.forName("java.lang.invoke.MethodHandle");
+ bindToTmp = methodHandleClass.getMethod("bindTo", Object.class);
+ invokeWithArgumentsTmp = methodHandleClass.getMethod("invokeWithArguments", Object[].class);
+ lookupCxtorTmp = lookupClass.getDeclaredConstructor(Class.class, int.class);
+ lookupCxtorTmp.setAccessible(true);
+ validTmp = true;
+ } catch (Exception invalid) {
+ // Ignore the exception, store the values & exit early in create(..) if invalid.
+ }
+
+ // Store refs to later.
+ valid = validTmp;
+ unreflectSpecial = unreflectSpecialTmp;
+ bindTo = bindToTmp;
+ invokeWithArguments = invokeWithArgumentsTmp;
+ lookupCxtor = lookupCxtorTmp;
+ }
+
+ static MethodHandleWrapper create(Method method, Object proxy) {
+ if (!valid) {
+ return null;
+ }
+ try {
+ Class<?> declaringClass = method.getDeclaringClass();
+ // Note: this isn't a public API, but we need to use it in order to call default methods.
+ Object lookup = lookupCxtor.newInstance(declaringClass, ALL_MODES);
+ method.setAccessible(true);
+ // These are part of the public API, but we use reflection since we run on java6
+ // and they were introduced in java7.
+ lookup = unreflectSpecial.invoke(lookup, method, declaringClass);
+ Object handle = bindTo.invoke(lookup, proxy);
+ return new MethodHandleWrapper(handle);
+ } catch (InvocationTargetException ite) {
+ return null;
+ } catch (IllegalAccessException iae) {
+ return null;
+ } catch (InstantiationException ie) {
+ return null;
+ }
+ }
+
+ final Object handle;
+
+ MethodHandleWrapper(Object handle) {
+ this.handle = handle;
+ }
+
+ Object invokeWithArguments(Object[] args) throws Exception {
+ // We must cast the args to an object so the Object[] is the first param,
+ // as opposed to each individual varargs param.
+ return invokeWithArguments.invoke(handle, (Object) args);
+ }
+
+ @Override public String toString() {
+ return handle.toString();
+ }
+ }
}
diff --git a/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java b/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java
index f58352b..c0e9bbd 100644
--- a/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java
+++ b/extensions/assistedinject/test/com/google/inject/assistedinject/FactoryProvider2Test.java
@@ -1131,40 +1131,90 @@
this.delegate = null;
}
}
-
- static abstract class AbstractAssisted {
- interface Factory<T extends AbstractAssisted> {
- T create(String string);
+
+ public static abstract class AbstractAssisted {
+ interface Factory<O extends AbstractAssisted, I extends CharSequence> {
+ O create(I string);
}
}
-
- static class ConcreteAssisted extends AbstractAssisted {
+
+ static class ConcreteAssisted extends AbstractAssisted {
@Inject ConcreteAssisted(@SuppressWarnings("unused") @Assisted String string) {}
}
-
+
static class ConcreteAssistedWithOverride extends AbstractAssisted {
- @Inject ConcreteAssistedWithOverride(@SuppressWarnings("unused") @Assisted String string) {}
-
- interface Factory extends AbstractAssisted.Factory<ConcreteAssistedWithOverride> {
+ @AssistedInject
+ ConcreteAssistedWithOverride(@SuppressWarnings("unused") @Assisted String string) {}
+
+ @AssistedInject
+ ConcreteAssistedWithOverride(@SuppressWarnings("unused") @Assisted StringBuilder sb) {}
+
+ interface Factory extends AbstractAssisted.Factory<ConcreteAssistedWithOverride, String> {
@Override ConcreteAssistedWithOverride create(String string);
}
+
+ interface Factory2 extends AbstractAssisted.Factory<ConcreteAssistedWithOverride, String> {
+ @Override ConcreteAssistedWithOverride create(String string);
+ ConcreteAssistedWithOverride create(StringBuilder sb);
+ }
}
-
+
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> {}
}
-
+
+ public static class Public extends AbstractAssisted {
+ @AssistedInject Public(@SuppressWarnings("unused") @Assisted String string) {}
+ @AssistedInject Public(@SuppressWarnings("unused") @Assisted StringBuilder sb) {}
+
+ public interface Factory extends AbstractAssisted.Factory<Public, String> {
+ @Override Public create(String string);
+ Public create(StringBuilder sb);
+ }
+ }
+
// See https://github.com/google/guice/issues/904
- public void testIgnoresSyntheticFactoryMethods() {
- // Validate the injector can be successfully created.
- Guice.createInjector(new AbstractModule() {
+ public void testGeneratedDefaultMethodsForwardCorrectly() {
+ final Key<AbstractAssisted.Factory<ConcreteAssisted, String>> concreteKey =
+ new Key<AbstractAssisted.Factory<ConcreteAssisted, String>>() {};
+ Injector injector = Guice.createInjector(new AbstractModule() {
@Override protected void configure() {
install(new FactoryModuleBuilder().build(ConcreteAssistedWithOverride.Factory.class));
+ install(new FactoryModuleBuilder().build(ConcreteAssistedWithOverride.Factory2.class));
install(new FactoryModuleBuilder().build(ConcreteAssistedWithoutOverride.Factory.class));
- install(new FactoryModuleBuilder().build(
- new TypeLiteral<AbstractAssisted.Factory<ConcreteAssisted>>() {}));
+ install(new FactoryModuleBuilder().build(Public.Factory.class));
+ install(new FactoryModuleBuilder().build(concreteKey));
}
});
+
+ ConcreteAssistedWithOverride.Factory factory1 =
+ injector.getInstance(ConcreteAssistedWithOverride.Factory.class);
+ factory1.create("foo");
+ AbstractAssisted.Factory<ConcreteAssistedWithOverride, String> factory1Abstract = factory1;
+ factory1Abstract.create("foo");
+
+ ConcreteAssistedWithOverride.Factory2 factory2 =
+ injector.getInstance(ConcreteAssistedWithOverride.Factory2.class);
+ factory2.create("foo");
+ factory2.create(new StringBuilder("foo"));
+ AbstractAssisted.Factory<ConcreteAssistedWithOverride, String> factory2Abstract = factory2;
+ factory2Abstract.create("foo");
+
+ ConcreteAssistedWithoutOverride.Factory factory3 =
+ injector.getInstance(ConcreteAssistedWithoutOverride.Factory.class);
+ factory3.create("foo");
+ AbstractAssisted.Factory<ConcreteAssistedWithoutOverride, String> factory3Abstract = factory3;
+ factory3Abstract.create("foo");
+
+ Public.Factory factory4 = injector.getInstance(Public.Factory.class);
+ factory4.create("foo");
+ factory4.create(new StringBuilder("foo"));
+ AbstractAssisted.Factory<Public, String> factory4Abstract = factory4;
+ factory4Abstract.create("foo");
+
+ AbstractAssisted.Factory<ConcreteAssisted, String> factory5 =
+ injector.getInstance(concreteKey);
+ factory5.create("foo");
}
}
