Skip to content

Commit 3a6f5ca

Browse files
committed
Rust: Disambiguate some method calls based on argument types
1 parent 7efad77 commit 3a6f5ca

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
@@ -1206,11 +1206,126 @@ private Function getTypeParameterMethod(TypeParameter tp, string name) {
12061206
result = getMethodSuccessor(tp.(ImplTraitTypeTypeParameter).getImplTraitTypeRepr(), name)
12071207
}
12081208

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

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

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

18371837
pub fn f() {
18381838
let x: i64 = 73;
1839-
x.my_add(5i64); // $ method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<bool>::my_add SPURIOUS: method=MyAdd<&i64>::my_add
1840-
x.my_add(&5i64); // $ method=MyAdd<&i64>::my_add SPURIOUS: method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<bool>::my_add
1841-
x.my_add(true); // $ method=MyAdd<bool>::my_add SPURIOUS: method=MyAdd<i64>::my_add SPURIOUS: method=MyAdd<&i64>::my_add
1839+
x.my_add(5i64); // $ method=MyAdd<i64>::my_add
1840+
x.my_add(&5i64); // $ method=MyAdd<&i64>::my_add
1841+
x.my_add(true); // $ method=MyAdd<bool>::my_add
18421842
}
18431843
}
18441844

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

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2677,25 +2677,17 @@ inferType
26772677
| main.rs:1839:9:1839:9 | x | | {EXTERNAL LOCATION} | i32 |
26782678
| main.rs:1839:9:1839:9 | x | | {EXTERNAL LOCATION} | i64 |
26792679
| main.rs:1839:9:1839:22 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
2680-
| main.rs:1839:18:1839:21 | 5i64 | | {EXTERNAL LOCATION} | bool |
26812680
| main.rs:1839:18:1839:21 | 5i64 | | {EXTERNAL LOCATION} | i64 |
2682-
| main.rs:1839:18:1839:21 | 5i64 | | file://:0:0:0:0 | & |
2683-
| main.rs:1839:18:1839:21 | 5i64 | &T | {EXTERNAL LOCATION} | i64 |
26842681
| main.rs:1840:9:1840:9 | x | | {EXTERNAL LOCATION} | i32 |
26852682
| main.rs:1840:9:1840:9 | x | | {EXTERNAL LOCATION} | i64 |
26862683
| main.rs:1840:9:1840:23 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
2687-
| main.rs:1840:18:1840:22 | &5i64 | | {EXTERNAL LOCATION} | bool |
2688-
| main.rs:1840:18:1840:22 | &5i64 | | {EXTERNAL LOCATION} | i64 |
26892684
| main.rs:1840:18:1840:22 | &5i64 | | file://:0:0:0:0 | & |
26902685
| main.rs:1840:18:1840:22 | &5i64 | &T | {EXTERNAL LOCATION} | i64 |
26912686
| main.rs:1840:19:1840:22 | 5i64 | | {EXTERNAL LOCATION} | i64 |
26922687
| main.rs:1841:9:1841:9 | x | | {EXTERNAL LOCATION} | i32 |
26932688
| main.rs:1841:9:1841:9 | x | | {EXTERNAL LOCATION} | i64 |
26942689
| main.rs:1841:9:1841:22 | x.my_add(...) | | {EXTERNAL LOCATION} | i64 |
26952690
| main.rs:1841:18:1841:21 | true | | {EXTERNAL LOCATION} | bool |
2696-
| main.rs:1841:18:1841:21 | true | | {EXTERNAL LOCATION} | i64 |
2697-
| main.rs:1841:18:1841:21 | true | | file://:0:0:0:0 | & |
2698-
| main.rs:1841:18:1841:21 | true | &T | {EXTERNAL LOCATION} | i64 |
26992691
| main.rs:1847:5:1847:20 | ...::f(...) | | main.rs:67:5:67:21 | Foo |
27002692
| main.rs:1848:5:1848:60 | ...::g(...) | | main.rs:67:5:67:21 | Foo |
27012693
| main.rs:1848:20:1848: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)