Skip to content

Commit 14b8892

Browse files
smowtonsmowton
committed
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.
1 parent 50f99d8 commit 14b8892

File tree

7 files changed

+74
-96
lines changed

7 files changed

+74
-96
lines changed

‎java/kotlin-extractor/src/main/kotlin/KotlinFileExtractor.kt

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -839,7 +839,7 @@ open class KotlinFileExtractor(
839839
// (NB. kotlinc's actual implementation strategy is different -- it makes an inner class called InterfaceWithDefault$DefaultImpls and stores the default methods
840840
// there to allow default method usage in Java < 8, but this is hopefully niche.
841841
!pluginContext.languageVersionSettings.getFlag(JvmAnalysisFlags.jvmDefaultMode).forAllMethodsWithBody &&
842-
f.parentClassOrNull?.origin != IrDeclarationOrigin.IR_EXTERNAL_JAVA_DECLARATION_STUB &&
842+
f.parentClassOrNull.let { it != null && it.origin != IrDeclarationOrigin.IR_EXTERNAL_JAVA_DECLARATION_STUB && it.modality != Modality.ABSTRACT } &&
843843
f.realOverrideTarget.let { it != f && (it as? IrSimpleFunction)?.modality != Modality.ABSTRACT && isKotlinDefinedInterface(it.parentClassOrNull) }
844844

845845
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(
848848
if (extractBody) {
849849
val realFunctionLocId = tw.getLocation(f)
850850
val inheritedDefaultFunction = f.realOverrideTarget
851-
val defaultDefiningInterfaceType = (inheritedDefaultFunction.parentClassOrNull ?: return functionId).typeWith()
851+
val directlyInheritedSymbol =
852+
when(f) {
853+
is IrSimpleFunction ->
854+
f.overriddenSymbols.find { it.owner === inheritedDefaultFunction }
855+
?: f.overriddenSymbols.find { it.owner.realOverrideTarget === inheritedDefaultFunction }
856+
?: inheritedDefaultFunction.symbol
857+
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.
858+
}
859+
val defaultDefiningInterfaceType = (directlyInheritedSymbol.owner.parentClassOrNull ?: return functionId).typeWith()
852860

853861
extractExpressionBody(functionId, realFunctionLocId).also { returnId ->
854862
extractRawMethodAccess(

‎java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/User.java

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,8 @@ public static void sink(int x) { }
55
// Real is compiled with synthetic interface method forwarders, so it appears from a Java perspective to override the interface Test.
66
// RealNoForwards is compiled with -Xjvm-default=all, meaning real Java 8 default interface methods are used, no synthetic forwarders
77
// are created, and call resolution should go straight to the default.
8-
public static void test(Real r1, RealNoForwards r2) {
8+
// RealIndirect is similar to Real, except it inherits its methods indirectly via MiddleInterface.
9+
public static void test(Real r1, RealNoForwards r2, RealIndirect r3) {
910

1011
sink(r1.f());
1112
sink(r1.g(2));
@@ -15,6 +16,10 @@ public static void test(Real r1, RealNoForwards r2) {
1516
sink(r2.g(5));
1617
sink(r2.getX());
1718

19+
sink(r3.f());
20+
sink(r3.g(8));
21+
sink(r3.getX());
22+
1823
}
1924

2025
}

‎java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/test.expected

Lines changed: 36 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,36 @@
1-
| User.java:11:15:11:15 | 2 | User.java:11:10:11:16 | g(...) |
2-
| User.java:15:15:15:15 | 5 | User.java:15:10:15:16 | g(...) |
3-
| noforwards.kt:3:13:3:13 | 4 | User.java:14:10:14:15 | f(...) |
4-
| noforwards.kt:8:13:8:13 | 6 | User.java:16:10:16:18 | getX(...) |
5-
| test.kt:3:13:3:13 | 1 | User.java:10:10:10:15 | f(...) |
6-
| test.kt:8:13:8:13 | 3 | User.java:12:10:12:18 | getX(...) |
1+
callables
2+
| User.java:1:14:1:17 | User | User.java:1:14:1:17 | User | from source |
3+
| User.java:3:22:3:25 | sink | User.java:1:14:1:17 | User | from source |
4+
| User.java:9:22:9:25 | test | User.java:1:14:1:17 | User | from source |
5+
| noforwards.kt:3:3:3:13 | f | noforwards.kt:1:1:10:1 | NoForwards | from source |
6+
| noforwards.kt:5:3:5:19 | g | noforwards.kt:1:1:10:1 | NoForwards | from source |
7+
| noforwards.kt:8:5:8:13 | getX | noforwards.kt:1:1:10:1 | NoForwards | from source |
8+
| noforwards.kt:12:1:12:37 | RealNoForwards | noforwards.kt:12:1:12:37 | RealNoForwards | from source |
9+
| test.kt:3:3:3:13 | f | test.kt:1:1:10:1 | Test | from source |
10+
| test.kt:5:3:5:19 | g | test.kt:1:1:10:1 | Test | from source |
11+
| test.kt:8:5:8:13 | getX | test.kt:1:1:10:1 | Test | from source |
12+
| test.kt:12:1:12:21 | Real | test.kt:12:1:12:21 | Real | from source |
13+
| test.kt:12:1:12:21 | f | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
14+
| test.kt:12:1:12:21 | g | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
15+
| test.kt:12:1:12:21 | getX | test.kt:12:1:12:21 | Real | Forwarder for a Kotlin class inheriting an interface default method |
16+
| test.kt:16:1:16:40 | RealIndirect | test.kt:16:1:16:40 | RealIndirect | from source |
17+
| test.kt:16:1:16:40 | f | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
18+
| test.kt:16:1:16:40 | g | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
19+
| test.kt:16:1:16:40 | getX | test.kt:16:1:16:40 | RealIndirect | Forwarder for a Kotlin class inheriting an interface default method |
20+
superAccesses
21+
| 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 |
22+
| 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 |
23+
| 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 |
24+
| 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 |
25+
| 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 |
26+
| 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 |
27+
#select
28+
| User.java:12:15:12:15 | 2 | User.java:12:10:12:16 | g(...) |
29+
| User.java:16:15:16:15 | 5 | User.java:16:10:16:16 | g(...) |
30+
| User.java:20:15:20:15 | 8 | User.java:20:10:20:16 | g(...) |
31+
| noforwards.kt:3:13:3:13 | 4 | User.java:15:10:15:15 | f(...) |
32+
| noforwards.kt:8:13:8:13 | 6 | User.java:17:10:17:18 | getX(...) |
33+
| test.kt:3:13:3:13 | 1 | User.java:11:10:11:15 | f(...) |
34+
| test.kt:3:13:3:13 | 1 | User.java:19:10:19:15 | f(...) |
35+
| test.kt:8:13:8:13 | 3 | User.java:13:10:13:18 | getX(...) |
36+
| test.kt:8:13:8:13 | 3 | User.java:21:10:21:18 | getX(...) |

‎java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/test.kt

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,3 +10,7 @@ interface Test {
1010
}
1111

1212
class Real : Test { }
13+
14+
interface MiddleInterface : Test { }
15+
16+
class RealIndirect : MiddleInterface { }

‎java/ql/integration-tests/posix-only/kotlin/kotlin-interface-inherited-default/test.ql

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,24 @@
11
import java
22
import semmle.code.java.dataflow.DataFlow
33

4+
query predicate callables(Callable c, RefType declType, string kind) {
5+
c.fromSource() and
6+
declType = c.getDeclaringType() and
7+
(
8+
kind = c.compilerGeneratedReason()
9+
or
10+
not exists(c.compilerGeneratedReason()) and kind = "from source"
11+
)
12+
}
13+
14+
query predicate superAccesses(
15+
SuperAccess sa, RefType enclosingType, Callable enclosingCallable, Expr qualifier
16+
) {
17+
sa.getQualifier() = qualifier and
18+
enclosingCallable = sa.getEnclosingCallable() and
19+
enclosingType = enclosingCallable.getDeclaringType()
20+
}
21+
422
class Config extends DataFlow::Configuration {
523
Config() { this = "testconfig" }
624

‎java/ql/test/kotlin/library-tests/java-kotlin-collection-type-generic-methods/test.expected

Lines changed: 0 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -4,38 +4,31 @@ methodWithDuplicate
44
| AbstractCollection | addAll | Collection<? extends E> |
55
| AbstractCollection | contains | Object |
66
| AbstractCollection | containsAll | Collection<?> |
7-
| AbstractCollection | forEach | Consumer<? super E> |
87
| AbstractCollection | remove | Object |
98
| AbstractCollection | removeAll | Collection<?> |
109
| AbstractCollection | retainAll | Collection<?> |
11-
| AbstractCollection | toArray | IntFunction<T[]> |
1210
| AbstractCollection | toArray | T[] |
1311
| AbstractCollection<E> | add | E |
1412
| AbstractCollection<E> | addAll | Collection<? extends E> |
1513
| AbstractCollection<E> | contains | Object |
1614
| AbstractCollection<E> | containsAll | Collection<?> |
17-
| AbstractCollection<E> | forEach | Consumer<? super E> |
1815
| AbstractCollection<E> | remove | Object |
1916
| AbstractCollection<E> | removeAll | Collection<?> |
2017
| AbstractCollection<E> | retainAll | Collection<?> |
21-
| AbstractCollection<E> | toArray | IntFunction<T[]> |
2218
| AbstractCollection<E> | toArray | T[] |
2319
| AbstractCollection<String> | add | String |
2420
| AbstractCollection<String> | addAll | Collection<? extends String> |
2521
| AbstractCollection<String> | contains | Object |
2622
| AbstractCollection<String> | containsAll | Collection<?> |
27-
| AbstractCollection<String> | forEach | Consumer<? super String> |
2823
| AbstractCollection<String> | remove | Object |
2924
| AbstractCollection<String> | removeAll | Collection<?> |
3025
| AbstractCollection<String> | retainAll | Collection<?> |
31-
| AbstractCollection<String> | toArray | IntFunction<T[]> |
3226
| AbstractCollection<String> | toArray | T[] |
3327
| AbstractList | add | E |
3428
| AbstractList | add | int |
3529
| AbstractList | addAll | Collection<? extends E> |
3630
| AbstractList | addAll | int |
3731
| AbstractList | equals | Object |
38-
| AbstractList | forEach | Consumer<? super E> |
3932
| AbstractList | get | int |
4033
| AbstractList | indexOf | Object |
4134
| AbstractList | lastIndexOf | Object |
@@ -46,7 +39,6 @@ methodWithDuplicate
4639
| AbstractList | set | int |
4740
| AbstractList | subList | int |
4841
| AbstractList | subListRangeCheck | int |
49-
| AbstractList | toArray | IntFunction<T[]> |
5042
| AbstractList<E> | add | E |
5143
| AbstractList<E> | add | int |
5244
| AbstractList<E> | addAll | Collection<? extends E> |
@@ -66,10 +58,7 @@ methodWithDuplicate
6658
| AbstractMap | containsKey | Object |
6759
| AbstractMap | containsValue | Object |
6860
| AbstractMap | equals | Object |
69-
| AbstractMap | forEach | BiConsumer<? super K,? super V> |
7061
| AbstractMap | get | Object |
71-
| AbstractMap | getOrDefault | Object |
72-
| AbstractMap | getOrDefault | V |
7362
| AbstractMap | put | K |
7463
| AbstractMap | put | V |
7564
| AbstractMap | putAll | Map<? extends K,? extends V> |
@@ -94,53 +83,23 @@ methodWithDuplicate
9483
| AbstractMap<String,String> | containsKey | Object |
9584
| AbstractMap<String,String> | containsValue | Object |
9685
| AbstractMap<String,String> | equals | Object |
97-
| AbstractMap<String,String> | forEach | BiConsumer<? super String,? super String> |
9886
| AbstractMap<String,String> | get | Object |
99-
| AbstractMap<String,String> | getOrDefault | Object |
100-
| AbstractMap<String,String> | getOrDefault | String |
10187
| AbstractMap<String,String> | put | String |
10288
| AbstractMap<String,String> | putAll | Map<? extends String,? extends String> |
10389
| AbstractMap<String,String> | remove | Object |
10490
| AbstractMutableCollection | add | E |
105-
| AbstractMutableCollection | forEach | Consumer<? super E> |
106-
| AbstractMutableCollection | removeIf | Predicate<? super E> |
107-
| AbstractMutableCollection | toArray | IntFunction<T[]> |
10891
| AbstractMutableList | add | E |
10992
| AbstractMutableList | add | int |
110-
| AbstractMutableList | forEach | Consumer<? super E> |
11193
| AbstractMutableList | remove | int |
112-
| AbstractMutableList | removeIf | Predicate<? super E> |
113-
| AbstractMutableList | replaceAll | UnaryOperator<E> |
11494
| AbstractMutableList | set | E |
11595
| AbstractMutableList | set | int |
116-
| AbstractMutableList | sort | Comparator<? super E> |
117-
| AbstractMutableList | toArray | IntFunction<T[]> |
118-
| AbstractMutableMap | compute | BiFunction<? super K,? super V,? extends V> |
119-
| AbstractMutableMap | compute | K |
120-
| AbstractMutableMap | computeIfAbsent | Function<? super K,? extends V> |
121-
| AbstractMutableMap | computeIfAbsent | K |
122-
| AbstractMutableMap | computeIfPresent | BiFunction<? super K,? super V,? extends V> |
123-
| AbstractMutableMap | computeIfPresent | K |
124-
| AbstractMutableMap | forEach | BiConsumer<? super K,? super V> |
125-
| AbstractMutableMap | getOrDefault | Object |
126-
| AbstractMutableMap | getOrDefault | V |
127-
| AbstractMutableMap | merge | BiFunction<? super V,? super V,? extends V> |
128-
| AbstractMutableMap | merge | K |
129-
| AbstractMutableMap | merge | V |
13096
| AbstractMutableMap | put | K |
13197
| AbstractMutableMap | put | V |
132-
| AbstractMutableMap | putIfAbsent | K |
133-
| AbstractMutableMap | putIfAbsent | V |
134-
| AbstractMutableMap | remove | Object |
135-
| AbstractMutableMap | replace | K |
136-
| AbstractMutableMap | replace | V |
137-
| AbstractMutableMap | replaceAll | BiFunction<? super K,? super V,? extends V> |
13898
| Collection | add | E |
13999
| Collection | addAll | Collection<? extends E> |
140100
| Collection | contains | Object |
141101
| Collection | containsAll | Collection<?> |
142102
| Collection | equals | Object |
143-
| Collection | forEach | Consumer<? super E> |
144103
| Collection | remove | Object |
145104
| Collection | removeAll | Collection<?> |
146105
| Collection | removeIf | Predicate<? super E> |
@@ -206,7 +165,6 @@ methodWithDuplicate
206165
| List | containsAll | Collection<?> |
207166
| List | copyOf | Collection<? extends E> |
208167
| List | equals | Object |
209-
| List | forEach | Consumer<? super E> |
210168
| List | get | int |
211169
| List | indexOf | Object |
212170
| List | lastIndexOf | Object |
@@ -222,7 +180,6 @@ methodWithDuplicate
222180
| List | set | int |
223181
| List | sort | Comparator<? super E> |
224182
| List | subList | int |
225-
| List | toArray | IntFunction<T[]> |
226183
| List | toArray | T[] |
227184
| List<E> | add | E |
228185
| List<E> | add | int |
@@ -416,38 +373,30 @@ methodWithDuplicate
416373
| Map<String,String> | replaceAll | BiFunction<? super String,? super String,? extends String> |
417374
| MutableCollection | add | E |
418375
| MutableCollection | addAll | Collection<? extends E> |
419-
| MutableCollection | forEach | Consumer<? super E> |
420376
| MutableCollection | remove | Object |
421377
| MutableCollection | removeAll | Collection<?> |
422378
| MutableCollection | removeIf | Predicate<? super E> |
423379
| MutableCollection | retainAll | Collection<?> |
424-
| MutableCollection | toArray | IntFunction<T[]> |
425380
| MutableList | add | E |
426381
| MutableList | add | int |
427382
| MutableList | addAll | Collection<? extends E> |
428383
| MutableList | addAll | int |
429-
| MutableList | forEach | Consumer<? super E> |
430384
| MutableList | listIterator | int |
431385
| MutableList | remove | Object |
432386
| MutableList | remove | int |
433387
| MutableList | removeAll | Collection<?> |
434-
| MutableList | removeIf | Predicate<? super E> |
435388
| MutableList | replaceAll | UnaryOperator<E> |
436389
| MutableList | retainAll | Collection<?> |
437390
| MutableList | set | E |
438391
| MutableList | set | int |
439392
| MutableList | sort | Comparator<? super E> |
440393
| MutableList | subList | int |
441-
| MutableList | toArray | IntFunction<T[]> |
442394
| MutableMap | compute | BiFunction<? super K,? super V,? extends V> |
443395
| MutableMap | compute | K |
444396
| MutableMap | computeIfAbsent | Function<? super K,? extends V> |
445397
| MutableMap | computeIfAbsent | K |
446398
| MutableMap | computeIfPresent | BiFunction<? super K,? super V,? extends V> |
447399
| MutableMap | computeIfPresent | K |
448-
| MutableMap | forEach | BiConsumer<? super K,? super V> |
449-
| MutableMap | getOrDefault | Object |
450-
| MutableMap | getOrDefault | V |
451400
| MutableMap | merge | BiFunction<? super V,? super V,? extends V> |
452401
| MutableMap | merge | K |
453402
| MutableMap | merge | V |

0 commit comments

Comments
 (0)