diff --git a/rust/ql/lib/codeql/rust/internal/typeinference/FunctionOverloading.qll b/rust/ql/lib/codeql/rust/internal/typeinference/FunctionOverloading.qll index ec0152c7ecae..991fa0c0da8f 100644 --- a/rust/ql/lib/codeql/rust/internal/typeinference/FunctionOverloading.qll +++ b/rust/ql/lib/codeql/rust/internal/typeinference/FunctionOverloading.qll @@ -22,17 +22,8 @@ private signature Type resolveTypeMentionAtSig(AstNode tm, TypePath path); * how to resolve type mentions (`PreTypeMention` vs. `TypeMention`). */ private module MkSiblingImpls { - pragma[nomagic] - private Type resolveNonTypeParameterTypeAt(AstNode tm, TypePath path) { - result = resolveTypeMentionAt(tm, path) and - not result instanceof TypeParameter - } - - bindingset[t1, t2] - private predicate typeMentionEqual(AstNode t1, AstNode t2) { - forex(TypePath path, Type type | resolveNonTypeParameterTypeAt(t1, path) = type | - resolveNonTypeParameterTypeAt(t2, path) = type - ) + private class Tm extends AstNode { + Type getTypeAt(TypePath path) { result = resolveTypeMentionAt(this, path) } } pragma[nomagic] @@ -50,32 +41,55 @@ private module MkSiblingImpls { trait = impl.resolveTraitTy() } + private module ImplIsInstantiationOfSiblingInput implements IsInstantiationOfInputSig { + predicate potentialInstantiationOf(Tm cond, TypeAbstraction abs, Tm constraint) { + exists(TraitItemNode trait, Type rootType | + implSiblingCandidate(_, trait, rootType, cond) and + implSiblingCandidate(abs, trait, rootType, constraint) and + cond != constraint + ) + } + } + + private module ImplIsInstantiationOfSibling = + IsInstantiationOf; + /** * Holds if `impl1` and `impl2` are sibling implementations of `trait`. We - * consider implementations to be siblings if they implement the same trait for - * the same type. In that case `Self` is the same type in both implementations, - * and method calls to the implementations cannot be resolved unambiguously - * based only on the receiver type. + * consider implementations to be siblings if they implement the same trait and + * the type being implemented by one of the implementations is an instantiation + * of the type being implemented by the other. + * + * For example, in + * + * ```rust + * trait MyTrait { ... } + * impl MyTrait for i64 { ... } // I1 + * impl MyTrait for i64 { ... } // I2 + * + * impl MyTrait for S { ... } // I3 + * impl MyTrait for S { ... } // I4 + * impl MyTrait for S { ... } // I5 + * ``` + * + * the pairs `(I1, I2)`, `(I3, I5)`, and `(I4, I5)` are siblings, but not `(I3, I4)`. + * + * Whenever an implementation has a sibling, calls to the implementations cannot be + * resolved unambiguously based only on the `Self` type alone. */ pragma[inline] - predicate implSiblings(TraitItemNode trait, Impl impl1, Impl impl2) { - impl1 != impl2 and - ( - exists(Type rootType, AstNode selfTy1, AstNode selfTy2 | - implSiblingCandidate(impl1, trait, rootType, selfTy1) and - implSiblingCandidate(impl2, trait, rootType, selfTy2) and - // In principle the second conjunct below should be superfluous, but we still - // have ill-formed type mentions for types that we don't understand. For - // those checking both directions restricts further. Note also that we check - // syntactic equality, whereas equality up to renaming would be more - // correct. - typeMentionEqual(selfTy1, selfTy2) and - typeMentionEqual(selfTy2, selfTy1) - ) - or - blanketImplSiblingCandidate(impl1, trait) and - blanketImplSiblingCandidate(impl2, trait) + predicate implSiblings(TraitItemNode trait, ImplItemNode impl1, ImplItemNode impl2) { + exists(Type rootType, AstNode selfTy1, AstNode selfTy2 | + implSiblingCandidate(impl1, trait, rootType, selfTy1) and + implSiblingCandidate(impl2, trait, rootType, selfTy2) + | + ImplIsInstantiationOfSibling::isInstantiationOf(selfTy1, impl2, selfTy2) or + ImplIsInstantiationOfSibling::isInstantiationOf(selfTy2, impl1, selfTy1) ) + or + blanketImplSiblingCandidate(impl1, trait) and + blanketImplSiblingCandidate(impl2, trait) and + impl1 != impl2 } /** @@ -97,7 +111,9 @@ private module MkSiblingImpls { not t2 instanceof TypeParameter ) or - // todo: handle blanket/non-blanket siblings in `implSiblings` + // Since we cannot resolve the `Output` types of certain built-in `Index` trait + // implementations, we need to ensure that the type-specialized versions that we + // ship do not apply unless there is an exact type match trait = any(IndexTrait it | implSiblingCandidate(impl, it, _, _) and @@ -152,7 +168,7 @@ private predicate functionResolutionDependsOnArgumentCand( * ```rust * trait MyTrait { * fn method(&self, value: Foo) -> Self; - * // ^^^^^^^^^^^^^ `pos` = 0 + * // ^^^^^^^^^^^^^ `pos` = 1 * // ^ `path` = "T" * } * impl MyAdd for i64 { @@ -160,11 +176,6 @@ private predicate functionResolutionDependsOnArgumentCand( * // ^^^ `type` = i64 * } * ``` - * - * Note that we only check the root type symbol at the position. If the type - * at that position is a type constructor (for instance `Vec<..>`) then - * inspecting the entire type tree could be necessary to disambiguate the - * method. In that case we will still resolve several methods. */ exists(TraitItemNode trait | diff --git a/rust/ql/lib/codeql/rust/internal/typeinference/TypeInference.qll b/rust/ql/lib/codeql/rust/internal/typeinference/TypeInference.qll index 6c0034c2b9c7..423ad21ae4ac 100644 --- a/rust/ql/lib/codeql/rust/internal/typeinference/TypeInference.qll +++ b/rust/ql/lib/codeql/rust/internal/typeinference/TypeInference.qll @@ -37,6 +37,11 @@ private module Input1 implements InputSig1 { class Type = T::Type; + predicate isPseudoType(Type t) { + t instanceof UnknownType or + t instanceof NeverType + } + class TypeParameter = T::TypeParameter; class TypeAbstraction = TA::TypeAbstraction; diff --git a/rust/ql/test/library-tests/type-inference/regressions.rs b/rust/ql/test/library-tests/type-inference/regressions.rs index 5c830bb3db2f..465475475bfb 100644 --- a/rust/ql/test/library-tests/type-inference/regressions.rs +++ b/rust/ql/test/library-tests/type-inference/regressions.rs @@ -149,7 +149,7 @@ mod regression5 { fn foo() -> S2 { let x = S1.into(); // $ target=into - x // $ SPURIOUS: type=x:T2.TRef.S1 -- happens because we currently do not consider the two `impl` blocks to be siblings + x // $ type=x:T2.S1 } } diff --git a/rust/ql/test/library-tests/type-inference/type-inference.expected b/rust/ql/test/library-tests/type-inference/type-inference.expected index 5e870ae6ca5d..3344fc45f74f 100644 --- a/rust/ql/test/library-tests/type-inference/type-inference.expected +++ b/rust/ql/test/library-tests/type-inference/type-inference.expected @@ -15818,18 +15818,12 @@ inferType | regressions.rs:150:24:153:5 | { ... } | | regressions.rs:136:5:136:22 | S2 | | regressions.rs:150:24:153:5 | { ... } | T2 | regressions.rs:135:5:135:14 | S1 | | regressions.rs:151:13:151:13 | x | | regressions.rs:136:5:136:22 | S2 | -| regressions.rs:151:13:151:13 | x | T2 | {EXTERNAL LOCATION} | & | | regressions.rs:151:13:151:13 | x | T2 | regressions.rs:135:5:135:14 | S1 | -| regressions.rs:151:13:151:13 | x | T2.TRef | regressions.rs:135:5:135:14 | S1 | | regressions.rs:151:17:151:18 | S1 | | regressions.rs:135:5:135:14 | S1 | | regressions.rs:151:17:151:25 | S1.into() | | regressions.rs:136:5:136:22 | S2 | -| regressions.rs:151:17:151:25 | S1.into() | T2 | {EXTERNAL LOCATION} | & | | regressions.rs:151:17:151:25 | S1.into() | T2 | regressions.rs:135:5:135:14 | S1 | -| regressions.rs:151:17:151:25 | S1.into() | T2.TRef | regressions.rs:135:5:135:14 | S1 | | regressions.rs:152:9:152:9 | x | | regressions.rs:136:5:136:22 | S2 | -| regressions.rs:152:9:152:9 | x | T2 | {EXTERNAL LOCATION} | & | | regressions.rs:152:9:152:9 | x | T2 | regressions.rs:135:5:135:14 | S1 | -| regressions.rs:152:9:152:9 | x | T2.TRef | regressions.rs:135:5:135:14 | S1 | | regressions.rs:164:16:164:19 | SelfParam | | regressions.rs:158:5:158:19 | S | | regressions.rs:164:16:164:19 | SelfParam | T | regressions.rs:160:10:160:10 | T | | regressions.rs:164:22:164:25 | _rhs | | regressions.rs:158:5:158:19 | S | diff --git a/shared/typeinference/codeql/typeinference/internal/TypeInference.qll b/shared/typeinference/codeql/typeinference/internal/TypeInference.qll index 9f4be49d878a..1f4400d8f2d7 100644 --- a/shared/typeinference/codeql/typeinference/internal/TypeInference.qll +++ b/shared/typeinference/codeql/typeinference/internal/TypeInference.qll @@ -145,6 +145,12 @@ signature module InputSig1 { Location getLocation(); } + /** + * Holds if `t` is a pseudo type. Pseudo types are skipped when checking for + * non-instantiations in `isNotInstantiationOf`. + */ + predicate isPseudoType(Type t); + /** A type parameter. */ class TypeParameter extends Type; @@ -624,6 +630,26 @@ module Make1 Input1> { ) } + pragma[nomagic] + private predicate hasTypeParameterAt( + App app, TypeAbstraction abs, Constraint constraint, TypePath path, TypeParameter tp + ) { + tp = getTypeAt(app, abs, constraint, path) and + tp = abs.getATypeParameter() + } + + private Type getNonPseudoTypeAt(App app, TypePath path) { + result = app.getTypeAt(path) and not isPseudoType(result) + } + + pragma[nomagic] + private Type getNonPseudoTypeAtTypeParameter( + App app, TypeAbstraction abs, Constraint constraint, TypeParameter tp, TypePath path + ) { + hasTypeParameterAt(app, abs, constraint, path, tp) and + result = getNonPseudoTypeAt(app, path) + } + /** * Holds if `app` is _not_ a possible instantiation of `constraint`, because `app` * and `constraint` differ on concrete types at `path`. @@ -643,13 +669,22 @@ module Make1 Input1> { predicate isNotInstantiationOf( App app, TypeAbstraction abs, Constraint constraint, TypePath path ) { - // `app` and `constraint` differ on a concrete type + // `app` and `constraint` differ on a non-pseudo concrete type exists(Type t, Type t2 | t = getTypeAt(app, abs, constraint, path) and not t = abs.getATypeParameter() and - app.getTypeAt(path) = t2 and + t2 = getNonPseudoTypeAt(app, path) and t2 != t ) + or + // `app` has different instantiations of a type parameter mentioned at two + // different paths + exists(TypeParameter tp, TypePath path2, Type t, Type t2 | + t = getNonPseudoTypeAtTypeParameter(app, abs, constraint, tp, path) and + t2 = getNonPseudoTypeAtTypeParameter(app, abs, constraint, tp, path2) and + t != t2 and + path != path2 + ) } }