Don't create interface forwarders for other interfaces, and target super accesses correctly

Intermediate interfaces don't need interface forwarders, since the Kotlin compiler won't try to make them non-abstract by synthesising methods.

Super references should always target an immediate superclass, not the ancestor containing the intended implementation.
This commit is contained in:
Chris Smowton
2022-10-19 13:02:22 +01:00
parent 50f99d8e82
commit 14b8892ced
7 changed files with 74 additions and 96 deletions

View File

@@ -839,7 +839,7 @@ open class KotlinFileExtractor(
// (NB. kotlinc's actual implementation strategy is different -- it makes an inner class called InterfaceWithDefault$DefaultImpls and stores the default methods
// there to allow default method usage in Java < 8, but this is hopefully niche.
!pluginContext.languageVersionSettings.getFlag(JvmAnalysisFlags.jvmDefaultMode).forAllMethodsWithBody &&
f.parentClassOrNull?.origin != IrDeclarationOrigin.IR_EXTERNAL_JAVA_DECLARATION_STUB &&
f.parentClassOrNull.let { it != null && it.origin != IrDeclarationOrigin.IR_EXTERNAL_JAVA_DECLARATION_STUB && it.modality != Modality.ABSTRACT } &&
f.realOverrideTarget.let { it != f && (it as? IrSimpleFunction)?.modality != Modality.ABSTRACT && isKotlinDefinedInterface(it.parentClassOrNull) }
private fun makeInterfaceForwarder(f: IrFunction, parentId: Label<out DbReftype>, extractBody: Boolean, extractMethodAndParameterTypeAccesses: Boolean, typeSubstitution: TypeSubstitution?, classTypeArgsIncludingOuterClasses: List<IrTypeArgument>?) =
@@ -848,7 +848,15 @@ open class KotlinFileExtractor(
if (extractBody) {
val realFunctionLocId = tw.getLocation(f)
val inheritedDefaultFunction = f.realOverrideTarget
val defaultDefiningInterfaceType = (inheritedDefaultFunction.parentClassOrNull ?: return functionId).typeWith()
val directlyInheritedSymbol =
when(f) {
is IrSimpleFunction ->
f.overriddenSymbols.find { it.owner === inheritedDefaultFunction }
?: f.overriddenSymbols.find { it.owner.realOverrideTarget === inheritedDefaultFunction }
?: inheritedDefaultFunction.symbol
else -> inheritedDefaultFunction.symbol // This is strictly invalid, since we shouldn't use A.super.f(...) where A may not be a direct supertype, but this path should also be unreachable.
}
val defaultDefiningInterfaceType = (directlyInheritedSymbol.owner.parentClassOrNull ?: return functionId).typeWith()
extractExpressionBody(functionId, realFunctionLocId).also { returnId ->
extractRawMethodAccess(

View File

@@ -5,7 +5,8 @@ public class User {
// Real is compiled with synthetic interface method forwarders, so it appears from a Java perspective to override the interface Test.
// RealNoForwards is compiled with -Xjvm-default=all, meaning real Java 8 default interface methods are used, no synthetic forwarders
// are created, and call resolution should go straight to the default.
public static void test(Real r1, RealNoForwards r2) {
// RealIndirect is similar to Real, except it inherits its methods indirectly via MiddleInterface.
public static void test(Real r1, RealNoForwards r2, RealIndirect r3) {
sink(r1.f());
sink(r1.g(2));
@@ -15,6 +16,10 @@ public class User {
sink(r2.g(5));
sink(r2.getX());
sink(r3.f());
sink(r3.g(8));
sink(r3.getX());
}
}

View File

@@ -1,6 +1,36 @@
| User.java:11:15:11:15 | 2 | User.java:11:10:11:16 | g(...) |
| User.java:15:15:15:15 | 5 | User.java:15:10:15:16 | g(...) |
| noforwards.kt:3:13:3:13 | 4 | User.java:14:10:14:15 | f(...) |
| noforwards.kt:8:13:8:13 | 6 | User.java:16:10:16:18 | getX(...) |
| test.kt:3:13:3:13 | 1 | User.java:10:10:10:15 | f(...) |
| test.kt:8:13:8:13 | 3 | User.java:12:10:12:18 | getX(...) |
callables
| User.java:1:14:1:17 | User | User.java:1:14:1:17 | User | from source |
| User.java:3:22:3:25 | sink | User.java:1:14:1:17 | User | from source |
| User.java:9:22:9:25 | test | User.java:1:14:1:17 | User | from source |
| noforwards.kt:3:3:3:13 | f | noforwards.kt:1:1:10:1 | NoForwards | from source |
| noforwards.kt:5:3:5:19 | g | noforwards.kt:1:1:10:1 | NoForwards | from source |
| noforwards.kt:8:5:8:13 | getX | noforwards.kt:1:1:10:1 | NoForwards | from source |
| noforwards.kt:12:1:12:37 | RealNoForwards | noforwards.kt:12:1:12:37 | RealNoForwards | from source |
| test.kt:3:3:3:13 | f | test.kt:1:1:10:1 | Test | from source |
| test.kt:5:3:5:19 | g | test.kt:1:1:10:1 | Test | from source |
| test.kt:8:5:8:13 | getX | test.kt:1:1:10:1 | Test | from source |
| test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | Real | from source |
| test.kt:12:1:12:21 | f | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
| test.kt:12:1:12:21 | g | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
| test.kt:12:1:12:21 | getX | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
| test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | RealIndirect | from source |
| test.kt:16:1:16:40 | f | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
| test.kt:16:1:16:40 | g | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
| test.kt:16:1:16:40 | getX | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
superAccesses
| test.kt:12:1:12:21 | Test.super | test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | f | test.kt:12:1:12:21 | Test |
| test.kt:12:1:12:21 | Test.super | test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | g | test.kt:12:1:12:21 | Test |
| test.kt:12:1:12:21 | Test.super | test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | getX | test.kt:12:1:12:21 | Test |
| test.kt:16:1:16:40 | MiddleInterface.super | test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | f | test.kt:16:1:16:40 | MiddleInterface |
| test.kt:16:1:16:40 | MiddleInterface.super | test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | g | test.kt:16:1:16:40 | MiddleInterface |
| test.kt:16:1:16:40 | MiddleInterface.super | test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | getX | test.kt:16:1:16:40 | MiddleInterface |
#select
| User.java:12:15:12:15 | 2 | User.java:12:10:12:16 | g(...) |
| User.java:16:15:16:15 | 5 | User.java:16:10:16:16 | g(...) |
| User.java:20:15:20:15 | 8 | User.java:20:10:20:16 | g(...) |
| noforwards.kt:3:13:3:13 | 4 | User.java:15:10:15:15 | f(...) |
| noforwards.kt:8:13:8:13 | 6 | User.java:17:10:17:18 | getX(...) |
| test.kt:3:13:3:13 | 1 | User.java:11:10:11:15 | f(...) |
| test.kt:3:13:3:13 | 1 | User.java:19:10:19:15 | f(...) |
| test.kt:8:13:8:13 | 3 | User.java:13:10:13:18 | getX(...) |
| test.kt:8:13:8:13 | 3 | User.java:21:10:21:18 | getX(...) |

View File

@@ -10,3 +10,7 @@ interface Test {
}
class Real : Test { }
interface MiddleInterface : Test { }
class RealIndirect : MiddleInterface { }

View File

@@ -1,6 +1,24 @@
import java
import semmle.code.java.dataflow.DataFlow
query predicate callables(Callable c, RefType declType, string kind) {
c.fromSource() and
declType = c.getDeclaringType() and
(
kind = c.compilerGeneratedReason()
or
not exists(c.compilerGeneratedReason()) and kind = "from source"
)
}
query predicate superAccesses(
SuperAccess sa, RefType enclosingType, Callable enclosingCallable, Expr qualifier
) {
sa.getQualifier() = qualifier and
enclosingCallable = sa.getEnclosingCallable() and
enclosingType = enclosingCallable.getDeclaringType()
}
class Config extends DataFlow::Configuration {
Config() { this = "testconfig" }

View File

@@ -4,38 +4,31 @@ methodWithDuplicate
| AbstractCollection | addAll | Collection<? extends E> |
| AbstractCollection | contains | Object |
| AbstractCollection | containsAll | Collection<?> |
| AbstractCollection | forEach | Consumer<? super E> |
| AbstractCollection | remove | Object |
| AbstractCollection | removeAll | Collection<?> |
| AbstractCollection | retainAll | Collection<?> |
| AbstractCollection | toArray | IntFunction<T[]> |
| AbstractCollection | toArray | T[] |
| AbstractCollection<E> | add | E |
| AbstractCollection<E> | addAll | Collection<? extends E> |
| AbstractCollection<E> | contains | Object |
| AbstractCollection<E> | containsAll | Collection<?> |
| AbstractCollection<E> | forEach | Consumer<? super E> |
| AbstractCollection<E> | remove | Object |
| AbstractCollection<E> | removeAll | Collection<?> |
| AbstractCollection<E> | retainAll | Collection<?> |
| AbstractCollection<E> | toArray | IntFunction<T[]> |
| AbstractCollection<E> | toArray | T[] |
| AbstractCollection<String> | add | String |
| AbstractCollection<String> | addAll | Collection<? extends String> |
| AbstractCollection<String> | contains | Object |
| AbstractCollection<String> | containsAll | Collection<?> |
| AbstractCollection<String> | forEach | Consumer<? super String> |
| AbstractCollection<String> | remove | Object |
| AbstractCollection<String> | removeAll | Collection<?> |
| AbstractCollection<String> | retainAll | Collection<?> |
| AbstractCollection<String> | toArray | IntFunction<T[]> |
| AbstractCollection<String> | toArray | T[] |
| AbstractList | add | E |
| AbstractList | add | int |
| AbstractList | addAll | Collection<? extends E> |
| AbstractList | addAll | int |
| AbstractList | equals | Object |
| AbstractList | forEach | Consumer<? super E> |
| AbstractList | get | int |
| AbstractList | indexOf | Object |
| AbstractList | lastIndexOf | Object |
@@ -46,7 +39,6 @@ methodWithDuplicate
| AbstractList | set | int |
| AbstractList | subList | int |
| AbstractList | subListRangeCheck | int |
| AbstractList | toArray | IntFunction<T[]> |
| AbstractList<E> | add | E |
| AbstractList<E> | add | int |
| AbstractList<E> | addAll | Collection<? extends E> |
@@ -66,10 +58,7 @@ methodWithDuplicate
| AbstractMap | containsKey | Object |
| AbstractMap | containsValue | Object |
| AbstractMap | equals | Object |
| AbstractMap | forEach | BiConsumer<? super K,? super V> |
| AbstractMap | get | Object |
| AbstractMap | getOrDefault | Object |
| AbstractMap | getOrDefault | V |
| AbstractMap | put | K |
| AbstractMap | put | V |
| AbstractMap | putAll | Map<? extends K,? extends V> |
@@ -94,53 +83,23 @@ methodWithDuplicate
| AbstractMap<String,String> | containsKey | Object |
| AbstractMap<String,String> | containsValue | Object |
| AbstractMap<String,String> | equals | Object |
| AbstractMap<String,String> | forEach | BiConsumer<? super String,? super String> |
| AbstractMap<String,String> | get | Object |
| AbstractMap<String,String> | getOrDefault | Object |
| AbstractMap<String,String> | getOrDefault | String |
| AbstractMap<String,String> | put | String |
| AbstractMap<String,String> | putAll | Map<? extends String,? extends String> |
| AbstractMap<String,String> | remove | Object |
| AbstractMutableCollection | add | E |
| AbstractMutableCollection | forEach | Consumer<? super E> |
| AbstractMutableCollection | removeIf | Predicate<? super E> |
| AbstractMutableCollection | toArray | IntFunction<T[]> |
| AbstractMutableList | add | E |
| AbstractMutableList | add | int |
| AbstractMutableList | forEach | Consumer<? super E> |
| AbstractMutableList | remove | int |
| AbstractMutableList | removeIf | Predicate<? super E> |
| AbstractMutableList | replaceAll | UnaryOperator<E> |
| AbstractMutableList | set | E |
| AbstractMutableList | set | int |
| AbstractMutableList | sort | Comparator<? super E> |
| AbstractMutableList | toArray | IntFunction<T[]> |
| AbstractMutableMap | compute | BiFunction<? super K,? super V,? extends V> |
| AbstractMutableMap | compute | K |
| AbstractMutableMap | computeIfAbsent | Function<? super K,? extends V> |
| AbstractMutableMap | computeIfAbsent | K |
| AbstractMutableMap | computeIfPresent | BiFunction<? super K,? super V,? extends V> |
| AbstractMutableMap | computeIfPresent | K |
| AbstractMutableMap | forEach | BiConsumer<? super K,? super V> |
| AbstractMutableMap | getOrDefault | Object |
| AbstractMutableMap | getOrDefault | V |
| AbstractMutableMap | merge | BiFunction<? super V,? super V,? extends V> |
| AbstractMutableMap | merge | K |
| AbstractMutableMap | merge | V |
| AbstractMutableMap | put | K |
| AbstractMutableMap | put | V |
| AbstractMutableMap | putIfAbsent | K |
| AbstractMutableMap | putIfAbsent | V |
| AbstractMutableMap | remove | Object |
| AbstractMutableMap | replace | K |
| AbstractMutableMap | replace | V |
| AbstractMutableMap | replaceAll | BiFunction<? super K,? super V,? extends V> |
| Collection | add | E |
| Collection | addAll | Collection<? extends E> |
| Collection | contains | Object |
| Collection | containsAll | Collection<?> |
| Collection | equals | Object |
| Collection | forEach | Consumer<? super E> |
| Collection | remove | Object |
| Collection | removeAll | Collection<?> |
| Collection | removeIf | Predicate<? super E> |
@@ -206,7 +165,6 @@ methodWithDuplicate
| List | containsAll | Collection<?> |
| List | copyOf | Collection<? extends E> |
| List | equals | Object |
| List | forEach | Consumer<? super E> |
| List | get | int |
| List | indexOf | Object |
| List | lastIndexOf | Object |
@@ -222,7 +180,6 @@ methodWithDuplicate
| List | set | int |
| List | sort | Comparator<? super E> |
| List | subList | int |
| List | toArray | IntFunction<T[]> |
| List | toArray | T[] |
| List<E> | add | E |
| List<E> | add | int |
@@ -416,38 +373,30 @@ methodWithDuplicate
| Map<String,String> | replaceAll | BiFunction<? super String,? super String,? extends String> |
| MutableCollection | add | E |
| MutableCollection | addAll | Collection<? extends E> |
| MutableCollection | forEach | Consumer<? super E> |
| MutableCollection | remove | Object |
| MutableCollection | removeAll | Collection<?> |
| MutableCollection | removeIf | Predicate<? super E> |
| MutableCollection | retainAll | Collection<?> |
| MutableCollection | toArray | IntFunction<T[]> |
| MutableList | add | E |
| MutableList | add | int |
| MutableList | addAll | Collection<? extends E> |
| MutableList | addAll | int |
| MutableList | forEach | Consumer<? super E> |
| MutableList | listIterator | int |
| MutableList | remove | Object |
| MutableList | remove | int |
| MutableList | removeAll | Collection<?> |
| MutableList | removeIf | Predicate<? super E> |
| MutableList | replaceAll | UnaryOperator<E> |
| MutableList | retainAll | Collection<?> |
| MutableList | set | E |
| MutableList | set | int |
| MutableList | sort | Comparator<? super E> |
| MutableList | subList | int |
| MutableList | toArray | IntFunction<T[]> |
| MutableMap | compute | BiFunction<? super K,? super V,? extends V> |
| MutableMap | compute | K |
| MutableMap | computeIfAbsent | Function<? super K,? extends V> |
| MutableMap | computeIfAbsent | K |
| MutableMap | computeIfPresent | BiFunction<? super K,? super V,? extends V> |
| MutableMap | computeIfPresent | K |
| MutableMap | forEach | BiConsumer<? super K,? super V> |
| MutableMap | getOrDefault | Object |
| MutableMap | getOrDefault | V |
| MutableMap | merge | BiFunction<? super V,? super V,? extends V> |
| MutableMap | merge | K |
| MutableMap | merge | V |

View File

@@ -263,15 +263,12 @@ modifiers
| reflection.kt:162:25:162:45 | ...::... | reflection.kt:162:25:162:45 | invoke | override |
| reflection.kt:162:25:162:45 | ...::... | reflection.kt:162:25:162:45 | invoke | public |
compGenerated
| file://<external>/CharIterator.class:0:0:0:0 | forEachRemaining | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/CharProgression.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/CharProgression.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/CharRange.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/CharRange.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/Class2.class:0:0:0:0 | getValue | Default property accessor |
| file://<external>/Class2.class:0:0:0:0 | getValue | Default property accessor |
| file://<external>/Collection.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/IntIterator.class:0:0:0:0 | forEachRemaining | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/IntProgression.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/IntProgression.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/IntRange.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
@@ -280,43 +277,10 @@ compGenerated
| file://<external>/KTypeProjection.class:0:0:0:0 | copy$default | Forwarder for Kotlin calls that need default arguments filling in |
| file://<external>/KTypeProjection.class:0:0:0:0 | covariant | Proxy static method for a @JvmStatic-annotated function or property |
| file://<external>/KTypeProjection.class:0:0:0:0 | invariant | Proxy static method for a @JvmStatic-annotated function or property |
| file://<external>/List.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/List.class:0:0:0:0 | parallelStream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/List.class:0:0:0:0 | stream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/List.class:0:0:0:0 | toArray | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/ListIterator.class:0:0:0:0 | forEachRemaining | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/LongIterator.class:0:0:0:0 | forEachRemaining | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/LongProgression.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/LongProgression.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/LongRange.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/LongRange.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableCollection.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableCollection.class:0:0:0:0 | parallelStream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableCollection.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableCollection.class:0:0:0:0 | stream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableCollection.class:0:0:0:0 | toArray | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableIterable.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableIterable.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableIterator.class:0:0:0:0 | forEachRemaining | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableList.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableList.class:0:0:0:0 | parallelStream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableList.class:0:0:0:0 | removeIf | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableList.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableList.class:0:0:0:0 | stream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableList.class:0:0:0:0 | toArray | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableListIterator.class:0:0:0:0 | forEachRemaining | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableMap.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableMap.class:0:0:0:0 | getOrDefault | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableSet.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableSet.class:0:0:0:0 | parallelStream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableSet.class:0:0:0:0 | removeIf | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableSet.class:0:0:0:0 | spliterator | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableSet.class:0:0:0:0 | stream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/MutableSet.class:0:0:0:0 | toArray | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/Set.class:0:0:0:0 | forEach | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/Set.class:0:0:0:0 | parallelStream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/Set.class:0:0:0:0 | stream | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/Set.class:0:0:0:0 | toArray | Forwarder for a Kotlin class inheriting an interface default method |
| file://<external>/String.class:0:0:0:0 | isEmpty | Forwarder for a Kotlin class inheriting an interface default method |
| reflection.kt:33:9:33:23 | getP0 | Default property accessor |
| reflection.kt:34:9:34:23 | getP1 | Default property accessor |