Rust: Refine implHasAmbiguousSiblingAt

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<resolveTypeMentionAtSig/2 resolveTypeMentionAt> {
-  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<resolveTypeMentionAtSig/2 resolveTypeMentionAt> {
     trait = impl.resolveTraitTy()
   }
 
+  private module ImplIsInstantiationOfSiblingInput implements IsInstantiationOfInputSig<Tm, Tm> {
+    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<Tm, Tm, ImplIsInstantiationOfSiblingInput>;
+
   /**
    * 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<T> { ... }
+   * impl MyTrait<i64> for i64 { ... }    // I1
+   * impl MyTrait<u64> for i64 { ... }    // I2
+   *
+   * impl MyTrait<i64> for S<i64> { ... } // I3
+   * impl MyTrait<u64> for S<u64> { ... } // I4
+   * impl MyTrait<bool> for S<T> { ... }  // 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<resolveTypeMentionAtSig/2 resolveTypeMentionAt> {
       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,19 +168,14 @@ private predicate functionResolutionDependsOnArgumentCand(
    * ```rust
    * trait MyTrait<T> {
    *     fn method(&self, value: Foo<T>) -> Self;
-   * //                   ^^^^^^^^^^^^^ `pos` = 0
+   * //                   ^^^^^^^^^^^^^ `pos` = 1
    * //                              ^ `path` = "T"
    * }
    * impl MyAdd<i64> for i64 {
    *     fn method(&self, value: Foo<i64>) -> Self { ... }
    * //                              ^^^ `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 |

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

@@ -37,6 +37,11 @@ private module Input1 implements InputSig1<Location> {
 
   class Type = T::Type;
 
+  predicate isPseudoType(Type t) {
+    t instanceof UnknownType or
+    t instanceof NeverType
+  }
+
   class TypeParameter = T::TypeParameter;
 
   class TypeAbstraction = TA::TypeAbstraction;

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

@@ -149,7 +149,7 @@ mod regression5 {
 
     fn foo() -> S2<S1> {
         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
     }
 }
 

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 |

shared/typeinference/codeql/typeinference/internal/TypeInference.qll

@@ -145,6 +145,12 @@ signature module InputSig1<LocationSig Location> {
     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<LocationSig Location, InputSig1<Location> 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<LocationSig Location, InputSig1<Location> 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
+        )
       }
     }