Skip to content

Commit eacf034

Browse files
committed
Rust: Disambiguate some method calls based on argument types
1 parent d1aee7f commit eacf034

File tree

4 files changed

+118
-20
lines changed

4 files changed

+118
-20
lines changed

rust/ql/lib/codeql/rust/internal/TypeInference.qll

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1231,11 +1231,126 @@ private Function getTypeParameterMethod(TypeParameter tp, string name) {
12311231
result = getMethodSuccessor(tp.(ImplTraitTypeTypeParameter).getImplTraitTypeRepr(), name)
12321232
}
12331233

1234+
bindingset[t1, t2]
1235+
private predicate typeMentionEqual(TypeMention t1, TypeMention t2) {
1236+
forex(TypePath path, Type type | t1.resolveTypeAt(path) = type | t2.resolveTypeAt(path) = type)
1237+
}
1238+
1239+
pragma[nomagic]
1240+
private predicate implSiblingCandidate(
1241+
Impl impl, TraitItemNode trait, Type rootType, TypeMention selfTy
1242+
) {
1243+
trait = impl.(ImplItemNode).resolveTraitTy() and
1244+
not exists(impl.getAttributeMacroExpansion()) and
1245+
// We use this for resolving methods, so exclude traits that do not have methods.
1246+
exists(Function f | f = trait.getASuccessor(_) and f.getParamList().hasSelfParam()) and
1247+
selfTy = impl.getSelfTy() and
1248+
rootType = selfTy.resolveType()
1249+
}
1250+
1251+
/**
1252+
* Holds if `impl1` and `impl2` are a sibling implementations of `trait`. We
1253+
* consider implementations to be siblings if they implement the same trait for
1254+
* the same type. In that case `Self` is the same type in both implementations,
1255+
* and method calls to the implementations cannot be resolved unambiguously
1256+
* based only on the receiver type.
1257+
*/
1258+
pragma[inline]
1259+
private predicate implSiblings(TraitItemNode trait, Impl impl1, Impl impl2) {
1260+
exists(Type rootType, TypeMention selfTy1, TypeMention selfTy2 |
1261+
impl1 != impl2 and
1262+
implSiblingCandidate(impl1, trait, rootType, selfTy1) and
1263+
implSiblingCandidate(impl2, trait, rootType, selfTy2) and
1264+
// In principle the second conjunct below should be superflous, but we still
1265+
// have ill-formed type mentions for types that we don't understand. For
1266+
// those checking both directions restricts further. Note also that we check
1267+
// syntactic equality, whereas equality up to renaming would be more
1268+
// correct.
1269+
typeMentionEqual(selfTy1, selfTy2) and
1270+
typeMentionEqual(selfTy2, selfTy1)
1271+
)
1272+
}
1273+
1274+
/**
1275+
* Holds if `impl` is an implementation of `trait` and if another implementation
1276+
* exists for the same type.
1277+
*/
1278+
pragma[nomagic]
1279+
private predicate implHasSibling(Impl impl, Trait trait) { implSiblings(trait, impl, _) }
1280+
1281+
/**
1282+
* Holds if a type parameter of `trait` occurs in the method with the name
1283+
* `methodName` at the `pos`th parameter at `path`.
1284+
*/
1285+
bindingset[trait]
1286+
pragma[inline_late]
1287+
private predicate traitTypeParameterOccurrence(
1288+
TraitItemNode trait, string methodName, int pos, TypePath path
1289+
) {
1290+
exists(Function f | f = trait.getASuccessor(methodName) |
1291+
f.getParam(pos).getTypeRepr().(TypeMention).resolveTypeAt(path) =
1292+
trait.(TraitTypeAbstraction).getATypeParameter()
1293+
)
1294+
}
1295+
1296+
bindingset[f, pos, path]
1297+
pragma[inline_late]
1298+
private predicate methodTypeAtPath(Function f, int pos, TypePath path, Type type) {
1299+
f.getParam(pos).getTypeRepr().(TypeMention).resolveTypeAt(path) = type
1300+
}
1301+
1302+
/**
1303+
* Holds if resolving the method in `impl` with the name `methodName` requires
1304+
* inspecting the types of applied _arguments_ in order to determine whether it
1305+
* is the correct resolution.
1306+
*/
1307+
private predicate methodResolutionDependsOnArgument(
1308+
Impl impl, string methodName, int pos, TypePath path, Type type
1309+
) {
1310+
/*
1311+
* As seen in the example below, when an implementation has a sibling for a
1312+
* trait we find occurrences of a type parameter of the trait in a method
1313+
* signature in the trait. We then find the type given in the implementation
1314+
* at the same position, which is a position that might disambiguate the
1315+
* method from its siblings.
1316+
*
1317+
* ```rust
1318+
* trait MyTrait<T> {
1319+
* fn method(&self, value: Foo<T>) -> Self;
1320+
* // ^^^^^^^^^^^^^ `pos` = 0
1321+
* // ^ `path` = "T"
1322+
* }
1323+
* impl MyAdd<i64> for i64 {
1324+
* fn method(&self, value: i64) -> Self { ... }
1325+
* // ^^^ `type` = i64
1326+
* }
1327+
* ```
1328+
*
1329+
* Note that we only check the root type symbol at the position. If the type
1330+
* at that position is a type constructor (for instance `Vec<..>`) then
1331+
* inspecting the entire type tree could be necessary to disambiguate the
1332+
* method. In that case we will still resolve several methods.
1333+
*/
1334+
1335+
exists(TraitItemNode trait |
1336+
implHasSibling(impl, trait) and
1337+
traitTypeParameterOccurrence(trait, methodName, pos, path) and
1338+
methodTypeAtPath(getMethodSuccessor(impl, methodName), pos, path, type)
1339+
)
1340+
}
1341+
12341342
/** Gets a method from an `impl` block that matches the method call `mc`. */
12351343
private Function getMethodFromImpl(MethodCall mc) {
12361344
exists(Impl impl |
12371345
IsInstantiationOf<MethodCall, IsInstantiationOfInput>::isInstantiationOf(mc, impl, _) and
12381346
result = getMethodSuccessor(impl, mc.getMethodName())
1347+
|
1348+
not methodResolutionDependsOnArgument(impl, _, _, _, _)
1349+
or
1350+
exists(int pos, TypePath path, Type type |
1351+
methodResolutionDependsOnArgument(impl, mc.getMethodName(), pos, path, type) and
1352+
inferType(mc.getArgument(pos), path) = type
1353+
)
12391354
)
12401355
}
12411356

rust/ql/test/library-tests/type-inference/main.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1847,9 +1847,9 @@ mod method_determined_by_argument_type {
18471847

18481848
pub fn f() {
18491849
let x: i64 = 73;
1850-
x.my_add(5i64); // $ method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<bool>::my_add SPURIOUS: method=MyAdd<&i64>::my_add
1851-
x.my_add(&5i64); // $ method=MyAdd<&i64>::my_add SPURIOUS: method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<bool>::my_add
1852-
x.my_add(true); // $ method=MyAdd<bool>::my_add SPURIOUS: method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<&i64>::my_add
1850+
x.my_add(5i64); // $ method=MyAdd<i64>::my_add
1851+
x.my_add(&5i64); // $ method=MyAdd<&i64>::my_add
1852+
x.my_add(true); // $ method=MyAdd<bool>::my_add
18531853
}
18541854
}
18551855

rust/ql/test/library-tests/type-inference/type-inference.expected

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2645,25 +2645,17 @@ inferType
26452645
| main.rs:1850:9:1850:9 | x | | {EXTERNAL LOCATION} | i32 |
26462646
| main.rs:1850:9:1850:9 | x | | {EXTERNAL LOCATION} | i64 |
26472647
| main.rs:1850:9:1850:22 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
2648-
| main.rs:1850:18:1850:21 | 5i64 | | {EXTERNAL LOCATION} | bool |
26492648
| main.rs:1850:18:1850:21 | 5i64 | | {EXTERNAL LOCATION} | i64 |
2650-
| main.rs:1850:18:1850:21 | 5i64 | | file://:0:0:0:0 | & |
2651-
| main.rs:1850:18:1850:21 | 5i64 | &T | {EXTERNAL LOCATION} | i64 |
26522649
| main.rs:1851:9:1851:9 | x | | {EXTERNAL LOCATION} | i32 |
26532650
| main.rs:1851:9:1851:9 | x | | {EXTERNAL LOCATION} | i64 |
26542651
| main.rs:1851:9:1851:23 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
2655-
| main.rs:1851:18:1851:22 | &5i64 | | {EXTERNAL LOCATION} | bool |
2656-
| main.rs:1851:18:1851:22 | &5i64 | | {EXTERNAL LOCATION} | i64 |
26572652
| main.rs:1851:18:1851:22 | &5i64 | | file://:0:0:0:0 | & |
26582653
| main.rs:1851:18:1851:22 | &5i64 | &T | {EXTERNAL LOCATION} | i64 |
26592654
| main.rs:1851:19:1851:22 | 5i64 | | {EXTERNAL LOCATION} | i64 |
26602655
| main.rs:1852:9:1852:9 | x | | {EXTERNAL LOCATION} | i32 |
26612656
| main.rs:1852:9:1852:9 | x | | {EXTERNAL LOCATION} | i64 |
26622657
| main.rs:1852:9:1852:22 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
26632658
| main.rs:1852:18:1852:21 | true | | {EXTERNAL LOCATION} | bool |
2664-
| main.rs:1852:18:1852:21 | true | | {EXTERNAL LOCATION} | i64 |
2665-
| main.rs:1852:18:1852:21 | true | | file://:0:0:0:0 | & |
2666-
| main.rs:1852:18:1852:21 | true | &T | {EXTERNAL LOCATION} | i64 |
26672659
| main.rs:1858:5:1858:20 | ...::f(...) | | main.rs:67:5:67:21 | Foo |
26682660
| main.rs:1859:5:1859:60 | ...::g(...) | | main.rs:67:5:67:21 | Foo |
26692661
| main.rs:1859:20:1859:38 | ...::Foo {...} | | main.rs:67:5:67:21 | Foo |

rust/ql/test/library-tests/variables/CONSISTENCY/PathResolutionConsistency.expected

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,3 @@
1-
multipleMethodCallTargets
2-
| main.rs:374:5:374:27 | ... .add_assign(...) | file://:0:0:0:0 | fn add_assign |
3-
| main.rs:374:5:374:27 | ... .add_assign(...) | file://:0:0:0:0 | fn add_assign |
4-
| main.rs:374:5:374:27 | ... .add_assign(...) | file://:0:0:0:0 | fn add_assign |
5-
| main.rs:374:5:374:27 | ... .add_assign(...) | file://:0:0:0:0 | fn add_assign |
6-
| main.rs:459:9:459:23 | z.add_assign(...) | file://:0:0:0:0 | fn add_assign |
7-
| main.rs:459:9:459:23 | z.add_assign(...) | file://:0:0:0:0 | fn add_assign |
8-
| main.rs:459:9:459:23 | z.add_assign(...) | file://:0:0:0:0 | fn add_assign |
9-
| main.rs:459:9:459:23 | z.add_assign(...) | file://:0:0:0:0 | fn add_assign |
101
multiplePathResolutions
112
| main.rs:85:19:85:30 | ...::from | file://:0:0:0:0 | fn from |
123
| main.rs:85:19:85:30 | ...::from | file://:0:0:0:0 | fn from |

0 commit comments

Comments
 (0)