Skip to content

Commit 570be12

Browse files
committed
Rust: Disambiguate some method calls based on argument types
1 parent 474294e commit 570be12

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
@@ -1227,11 +1227,126 @@ private Function getTypeParameterMethod(TypeParameter tp, string name) {
12271227
result = getMethodSuccessor(tp.(ImplTraitTypeTypeParameter).getImplTraitTypeRepr(), name)
12281228
}
12291229

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

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

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

18421842
pub fn f() {
18431843
let x: i64 = 73;
1844-
x.my_add(5i64); // $ method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<bool>::my_add SPURIOUS: method=MyAdd<&i64>::my_add
1845-
x.my_add(&5i64); // $ method=MyAdd<&i64>::my_add SPURIOUS: method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<bool>::my_add
1846-
x.my_add(true); // $ method=MyAdd<bool>::my_add SPURIOUS: method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<&i64>::my_add
1844+
x.my_add(5i64); // $ method=MyAdd<i64>::my_add
1845+
x.my_add(&5i64); // $ method=MyAdd<&i64>::my_add
1846+
x.my_add(true); // $ method=MyAdd<bool>::my_add
18471847
}
18481848
}
18491849

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2685,25 +2685,17 @@ inferType
26852685
| main.rs:1844:9:1844:9 | x | | {EXTERNAL LOCATION} | i32 |
26862686
| main.rs:1844:9:1844:9 | x | | {EXTERNAL LOCATION} | i64 |
26872687
| main.rs:1844:9:1844:22 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
2688-
| main.rs:1844:18:1844:21 | 5i64 | | {EXTERNAL LOCATION} | bool |
26892688
| main.rs:1844:18:1844:21 | 5i64 | | {EXTERNAL LOCATION} | i64 |
2690-
| main.rs:1844:18:1844:21 | 5i64 | | file://:0:0:0:0 | & |
2691-
| main.rs:1844:18:1844:21 | 5i64 | &T | {EXTERNAL LOCATION} | i64 |
26922689
| main.rs:1845:9:1845:9 | x | | {EXTERNAL LOCATION} | i32 |
26932690
| main.rs:1845:9:1845:9 | x | | {EXTERNAL LOCATION} | i64 |
26942691
| main.rs:1845:9:1845:23 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
2695-
| main.rs:1845:18:1845:22 | &5i64 | | {EXTERNAL LOCATION} | bool |
2696-
| main.rs:1845:18:1845:22 | &5i64 | | {EXTERNAL LOCATION} | i64 |
26972692
| main.rs:1845:18:1845:22 | &5i64 | | file://:0:0:0:0 | & |
26982693
| main.rs:1845:18:1845:22 | &5i64 | &T | {EXTERNAL LOCATION} | i64 |
26992694
| main.rs:1845:19:1845:22 | 5i64 | | {EXTERNAL LOCATION} | i64 |
27002695
| main.rs:1846:9:1846:9 | x | | {EXTERNAL LOCATION} | i32 |
27012696
| main.rs:1846:9:1846:9 | x | | {EXTERNAL LOCATION} | i64 |
27022697
| main.rs:1846:9:1846:22 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
27032698
| main.rs:1846:18:1846:21 | true | | {EXTERNAL LOCATION} | bool |
2704-
| main.rs:1846:18:1846:21 | true | | {EXTERNAL LOCATION} | i64 |
2705-
| main.rs:1846:18:1846:21 | true | | file://:0:0:0:0 | & |
2706-
| main.rs:1846:18:1846:21 | true | &T | {EXTERNAL LOCATION} | i64 |
27072699
| main.rs:1852:5:1852:20 | ...::f(...) | | main.rs:67:5:67:21 | Foo |
27082700
| main.rs:1853:5:1853:60 | ...::g(...) | | main.rs:67:5:67:21 | Foo |
27092701
| main.rs:1853:20:1853: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)