Rust: Refine implSiblings

Consider two implementations of the same trait to be siblings when the
type being implemented by one is an instantiation of the type being
implemented by the other.

Author: hvitved

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,54 +41,91 @@ 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)
+  pragma[nomagic]
+  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
   }
 
   /**
    * Holds if `impl` is an implementation of `trait` and if another implementation
    * exists for the same type.
    */
-  pragma[nomagic]
   predicate implHasSibling(ImplItemNode impl, Trait trait) { implSiblings(trait, impl, _) }
 
+  pragma[nomagic]
+  predicate implSiblings(TraitItemNode trait, ImplItemNode impl1, Tm traitMention1, Tm traitMention2) {
+    exists(ImplItemNode impl2 |
+      implSiblings(trait, impl1, impl2) and
+      traitMention1 = impl1.getTraitPath() and
+      traitMention2 = impl2.getTraitPath()
+    )
+  }
+
+  bindingset[t1, t2]
+  pragma[inline_late]
+  private predicate differentTypes(Type t1, Type t2) {
+    t1 != t2 and
+    (not t1 instanceof TypeParameter or not t2 instanceof TypeParameter)
+  }
+
   pragma[nomagic]
   predicate implHasAmbiguousSiblingAt(ImplItemNode impl, Trait trait, TypePath path) {
-    exists(ImplItemNode impl2, Type t1, Type t2 |
-      implSiblings(trait, impl, impl2) and
-      t1 = resolveTypeMentionAt(impl.getTraitPath(), path) and
-      t2 = resolveTypeMentionAt(impl2.getTraitPath(), path) and
-      t1 != t2
-    |
-      not t1 instanceof TypeParameter or
-      not t2 instanceof TypeParameter
+    exists(Tm traitMention, Tm traitMention2, Type t1, Type t2 |
+      implSiblings(trait, impl, traitMention, traitMention2) and
+      t1 = traitMention.getTypeAt(path) and
+      t2 = traitMention2.getTypeAt(path) and
+      differentTypes(t1, t2)
     )
     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 +180,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
+        )
       }
     }