From 25d5064e7f8cea5babb248b709348077a4f376bd Mon Sep 17 00:00:00 2001 From: kmb Date: Thu, 15 Feb 2018 10:43:41 -0800 Subject: Resolve the owner of interface.super calls to inherited default methods for android desugaring RELNOTES: None. PiperOrigin-RevId: 185863194 GitOrigin-RevId: c8e8749adc7b98c272b2421569dc97a88d487771 Change-Id: I063c2caa4b38fff2f9111f9fc09c317a5b097834 --- .../android/desugar/DefaultMethodClassFixer.java | 1 + .../devtools/build/android/desugar/Desugar.java | 2 + .../build/android/desugar/InterfaceDesugaring.java | 42 ++++++++++++++++++++- .../desugar/DesugarJava8FunctionalTest.java | 8 ++++ .../java8/InterfaceWithInheritedMethods.java | 44 ++++++++++++++++++++++ .../testdata_desugared_java8_jar_toc_golden.txt | 3 ++ 6 files changed, 98 insertions(+), 2 deletions(-) create mode 100644 test/java/com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithInheritedMethods.java diff --git a/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java b/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java index 2eda141..52964b7 100644 --- a/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java +++ b/java/com/google/devtools/build/android/desugar/DefaultMethodClassFixer.java @@ -502,6 +502,7 @@ public class DefaultMethodClassFixer extends ClassVisitor { DefaultMethodClassFixer.this.visitMethod(access, name, desc, (String) null, exceptions), stubbedInterfaceName, bootclasspath, + targetLoader, depsCollector, internalName); } else { diff --git a/java/com/google/devtools/build/android/desugar/Desugar.java b/java/com/google/devtools/build/android/desugar/Desugar.java index 053e55e..58b9b88 100644 --- a/java/com/google/devtools/build/android/desugar/Desugar.java +++ b/java/com/google/devtools/build/android/desugar/Desugar.java @@ -697,6 +697,7 @@ class Desugar { interfaceCache, depsCollector, bootclasspathReader, + loader, store, options.legacyJacocoFix); } @@ -776,6 +777,7 @@ class Desugar { interfaceCache, depsCollector, bootclasspathReader, + loader, store, options.legacyJacocoFix); } diff --git a/java/com/google/devtools/build/android/desugar/InterfaceDesugaring.java b/java/com/google/devtools/build/android/desugar/InterfaceDesugaring.java index 3524fae..f17f114 100644 --- a/java/com/google/devtools/build/android/desugar/InterfaceDesugaring.java +++ b/java/com/google/devtools/build/android/desugar/InterfaceDesugaring.java @@ -17,6 +17,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Preconditions.checkState; +import java.lang.reflect.Method; import javax.annotation.Nullable; import org.objectweb.asm.AnnotationVisitor; import org.objectweb.asm.ClassVisitor; @@ -47,6 +48,7 @@ class InterfaceDesugaring extends ClassVisitor { private final ClassVsInterface interfaceCache; private final DependencyCollector depsCollector; private final ClassReaderFactory bootclasspath; + private final ClassLoader targetLoader; private final GeneratedClassStore store; private final boolean legacyJaCoCo; @@ -62,12 +64,14 @@ class InterfaceDesugaring extends ClassVisitor { ClassVsInterface interfaceCache, DependencyCollector depsCollector, ClassReaderFactory bootclasspath, + ClassLoader targetLoader, GeneratedClassStore store, boolean legacyJaCoCo) { super(Opcodes.ASM6, dest); this.interfaceCache = interfaceCache; this.depsCollector = depsCollector; this.bootclasspath = bootclasspath; + this.targetLoader = targetLoader; this.store = store; this.legacyJaCoCo = legacyJaCoCo; } @@ -234,7 +238,12 @@ class InterfaceDesugaring extends ClassVisitor { } return result != null ? new InterfaceInvocationRewriter( - result, isInterface() ? internalName : null, bootclasspath, depsCollector, codeOwner) + result, + isInterface() ? internalName : null, + bootclasspath, + targetLoader, + depsCollector, + codeOwner) : null; } @@ -354,6 +363,7 @@ class InterfaceDesugaring extends ClassVisitor { @Nullable private final String interfaceName; private final ClassReaderFactory bootclasspath; + private final ClassLoader targetLoader; private final DependencyCollector depsCollector; /** Internal name that'll be used to record any dependencies on interface methods. */ private final String declaringClass; @@ -362,11 +372,13 @@ class InterfaceDesugaring extends ClassVisitor { MethodVisitor dest, @Nullable String knownInterfaceName, ClassReaderFactory bootclasspath, + ClassLoader targetLoader, DependencyCollector depsCollector, String declaringClass) { super(Opcodes.ASM6, dest); this.interfaceName = knownInterfaceName; this.bootclasspath = bootclasspath; + this.targetLoader = targetLoader; this.depsCollector = depsCollector; this.declaringClass = declaringClass; } @@ -409,7 +421,16 @@ class InterfaceDesugaring extends ClassVisitor { checkArgument(!owner.endsWith(DependencyCollector.INTERFACE_COMPANION_SUFFIX), "shouldn't consider %s an interface", owner); if (opcode == Opcodes.INVOKESPECIAL) { - // Turn Interface.super.m() into Interface$$CC.m(receiver) + // Turn Interface.super.m() into DefiningInterface$$CC.m(receiver). Note that owner + // always refers to the current type's immediate super-interface, but the default method + // may be inherited by that interface, so we have to figure out where the method is + // defined and invoke it in the corresponding companion class (b/73355452). Note that + // we're always dealing with interfaces here, and all interface methods are public, + // so using Class.getMethods should suffice to find inherited methods. Also note this + // can only be a default method invocation, no abstract method invocation. + owner = + findDefaultMethod(owner, name, desc) + .getDeclaringClass().getName().replace('.', '/'); opcode = Opcodes.INVOKESTATIC; desc = companionDefaultMethodDescriptor(owner, desc); } @@ -421,6 +442,23 @@ class InterfaceDesugaring extends ClassVisitor { } super.visitMethodInsn(opcode, owner, name, desc, itf); } + + private Method findDefaultMethod(String owner, String name, String desc) { + try { + Class clazz = targetLoader.loadClass(owner.replace('/', '.')); + // otherwise getting public methods with getMethods() below isn't enough + checkArgument(clazz.isInterface(), "Not an interface: %s", owner); + for (Method m : clazz.getMethods()) { + if (m.getName().equals(name) && Type.getMethodDescriptor(m).equals(desc)) { + checkState(m.isDefault(), "Found non-default method: %s", m); + return m; + } + } + } catch (ClassNotFoundException e) { + throw new IllegalStateException("Couldn't load " + owner, e); + } + throw new IllegalArgumentException("Method not found: " + owner + "." + name + desc); + } } /** diff --git a/test/java/com/google/devtools/build/android/desugar/DesugarJava8FunctionalTest.java b/test/java/com/google/devtools/build/android/desugar/DesugarJava8FunctionalTest.java index 20e6028..75d4f43 100644 --- a/test/java/com/google/devtools/build/android/desugar/DesugarJava8FunctionalTest.java +++ b/test/java/com/google/devtools/build/android/desugar/DesugarJava8FunctionalTest.java @@ -32,6 +32,7 @@ import com.google.devtools.build.android.desugar.testdata.java8.GenericDefaultIn import com.google.devtools.build.android.desugar.testdata.java8.InterfaceMethod; import com.google.devtools.build.android.desugar.testdata.java8.InterfaceWithDefaultMethod; import com.google.devtools.build.android.desugar.testdata.java8.InterfaceWithDuplicateMethods.ClassWithDuplicateMethods; +import com.google.devtools.build.android.desugar.testdata.java8.InterfaceWithInheritedMethods; import com.google.devtools.build.android.desugar.testdata.java8.Java7InterfaceWithBridges; import com.google.devtools.build.android.desugar.testdata.java8.Named; import com.google.devtools.build.android.desugar.testdata.java8.TwoInheritedDefaultMethods; @@ -415,4 +416,11 @@ public class DesugarJava8FunctionalTest extends DesugarFunctionalTest { assertThat(new DefaultMethodTransitivelyFromSeparateJava8Target().dflt()).isEqualTo("dflt"); assertThat(new DefaultMethodFromSeparateJava8TargetOverridden().dflt()).isEqualTo("override"); } + + /** Regression test for b/73355452 */ + @Test + public void testSuperCallToInheritedDefaultMethod() { + assertThat(new InterfaceWithInheritedMethods.Impl().name()).isEqualTo("Base"); + assertThat(new InterfaceWithInheritedMethods.Impl().suffix()).isEqualTo("!"); + } } diff --git a/test/java/com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithInheritedMethods.java b/test/java/com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithInheritedMethods.java new file mode 100644 index 0000000..8656e26 --- /dev/null +++ b/test/java/com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithInheritedMethods.java @@ -0,0 +1,44 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.android.desugar.testdata.java8; + +/** Regression test data for b/73355452 that also includes calling static methods. */ +public interface InterfaceWithInheritedMethods { + default String name() { + return "Base"; + } + + static String staticSuffix() { + return "!"; + } + + static interface Passthrough extends InterfaceWithInheritedMethods { + // inherits name(). Note that desugar doesn't produce a companion class for this interface + // since it doesn't define any default or static interface methods itself. + } + + static class Impl implements Passthrough { + @Override + public String name() { + // Even though Passthrough itself doesn't define name(), bytecode refers to Passthrough.name. + return Passthrough.super.name(); + } + + public String suffix() { + // Note that Passthrough.defaultSuffix doesn't compile and bytecode refers to + // InterfaceWithInheritedMethods.staticSuffix, so this shouldn't cause issues like b/73355452 + return staticSuffix(); + } + } +} diff --git a/test/java/com/google/devtools/build/android/desugar/testdata_desugared_java8_jar_toc_golden.txt b/test/java/com/google/devtools/build/android/desugar/testdata_desugared_java8_jar_toc_golden.txt index 67a46c3..16439ae 100644 --- a/test/java/com/google/devtools/build/android/desugar/testdata_desugared_java8_jar_toc_golden.txt +++ b/test/java/com/google/devtools/build/android/desugar/testdata_desugared_java8_jar_toc_golden.txt @@ -82,6 +82,9 @@ com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithDefaultMet com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithDefaultMethod.class com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithDuplicateMethods$ClassWithDuplicateMethods.class com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithDuplicateMethods.class +com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithInheritedMethods$Impl.class +com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithInheritedMethods$Passthrough.class +com/google/devtools/build/android/desugar/testdata/java8/InterfaceWithInheritedMethods.class com/google/devtools/build/android/desugar/testdata/java8/Java7InterfaceWithBridges$AbstractClassOne.class com/google/devtools/build/android/desugar/testdata/java8/Java7InterfaceWithBridges$ClassAddOne.class com/google/devtools/build/android/desugar/testdata/java8/Java7InterfaceWithBridges$ClassAddTwo.class -- cgit v1.2.3