Merge pull request #21168 from hvitved/rust/type-inference-remove-blanket-constraint-restriction

Rust: Remove restriction that blanket(-like) impls must have a constraint

Author: hvitved

rust/ql/lib/codeql/rust/frameworks/stdlib/Stdlib.qll

@@ -5,7 +5,10 @@
 private import rust
 private import codeql.rust.Concepts
 private import codeql.rust.dataflow.DataFlow
+private import codeql.rust.dataflow.FlowSummary
 private import codeql.rust.internal.PathResolution
+private import codeql.rust.internal.typeinference.Type
+private import codeql.rust.internal.typeinference.TypeMention
 
 /**
  * A call to the `starts_with` method on a `Path`.
@@ -19,6 +22,34 @@ private class StartswithCall extends Path::SafeAccessCheck::Range, MethodCall {
   }
 }
 
+/**
+ * A flow summary for the [reflexive implementation of the `From` trait][1].
+ *
+ * Blanket implementations currently don't have a canonical path, so we cannot
+ * use models-as-data for this model.
+ *
+ * [1]: https://doc.rust-lang.org/std/convert/trait.From.html#impl-From%3CT%3E-for-T
+ */
+private class ReflexiveFrom extends SummarizedCallable::Range {
+  ReflexiveFrom() {
+    exists(ImplItemNode impl |
+      impl.resolveTraitTy().(Trait).getCanonicalPath() = "core::convert::From" and
+      this = impl.getAssocItem("from") and
+      resolvePath(this.getParam(0).getTypeRepr().(PathTypeRepr).getPath()) =
+        impl.getBlanketImplementationTypeParam()
+    )
+  }
+
+  override predicate propagatesFlow(
+    string input, string output, boolean preservesValue, string model
+  ) {
+    input = "Argument[0]" and
+    output = "ReturnValue" and
+    preservesValue = true and
+    model = "ReflexiveFrom"
+  }
+}
+
 /**
  * The [`Option` enum][1].
  *

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

@@ -126,7 +126,7 @@ module SatisfiesBlanketConstraint<
 
   /**
    * Holds if the argument type `at` satisfies the first non-trivial blanket
-   * constraint of `impl`.
+   * constraint of `impl`, or if there are no non-trivial constraints of `impl`.
    */
   pragma[nomagic]
   predicate satisfiesBlanketConstraint(ArgumentType at, ImplItemNode impl) {
@@ -135,6 +135,11 @@ module SatisfiesBlanketConstraint<
       SatisfiesBlanketConstraintInput::relevantConstraint(ato, impl, traitBound) and
       SatisfiesBlanketConstraint::satisfiesConstraintType(ato, TTrait(traitBound), _, _)
     )
+    or
+    exists(TypeParam blanketTypeParam |
+      hasBlanketCandidate(at, impl, _, blanketTypeParam) and
+      not hasFirstNonTrivialTraitBound(blanketTypeParam, _)
+    )
   }
 
   /**

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

@@ -35,6 +35,12 @@ private predicate implSiblingCandidate(
   rootType = selfTy.resolveType()
 }
 
+pragma[nomagic]
+private predicate blanketImplSiblingCandidate(ImplItemNode impl, Trait trait) {
+  impl.isBlanketImplementation() and
+  trait = impl.resolveTraitTy()
+}
+
 /**
  * Holds if `impl1` and `impl2` are a sibling implementations of `trait`. We
  * consider implementations to be siblings if they implement the same trait for
@@ -44,17 +50,22 @@ private predicate implSiblingCandidate(
  */
 pragma[inline]
 private predicate implSiblings(TraitItemNode trait, Impl impl1, Impl impl2) {
-  exists(Type rootType, TypeMention selfTy1, TypeMention selfTy2 |
-    impl1 != impl2 and
-    implSiblingCandidate(impl1, trait, rootType, selfTy1) and
-    implSiblingCandidate(impl2, trait, rootType, selfTy2) and
-    // In principle the second conjunct below should be superflous, 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)
+  impl1 != impl2 and
+  (
+    exists(Type rootType, TypeMention selfTy1, TypeMention selfTy2 |
+      implSiblingCandidate(impl1, trait, rootType, selfTy1) and
+      implSiblingCandidate(impl2, trait, rootType, selfTy2) and
+      // In principle the second conjunct below should be superflous, 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)
   )
 }
 
@@ -63,7 +74,7 @@ private predicate implSiblings(TraitItemNode trait, Impl impl1, Impl impl2) {
  * exists for the same type.
  */
 pragma[nomagic]
-private predicate implHasSibling(Impl impl, Trait trait) { implSiblings(trait, impl, _) }
+private predicate implHasSibling(ImplItemNode impl, Trait trait) { implSiblings(trait, impl, _) }
 
 /**
  * Holds if type parameter `tp` of `trait` occurs in the function `f` with the name

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

@@ -356,7 +356,7 @@ module ArgsAreInstantiationsOf<ArgsAreInstantiationsOfInputSig Input> {
     string toString() { result = call.toString() + " [arg " + pos + "]" }
   }
 
-  private module ArgIsInstantiationOfInput implements
+  private module ArgIsInstantiationOfToIndexInput implements
     IsInstantiationOfInputSig<CallAndPos, AssocFunctionType>
   {
     pragma[nomagic]
@@ -389,7 +389,7 @@ module ArgsAreInstantiationsOf<ArgsAreInstantiationsOfInputSig Input> {
   }
 
   private module ArgIsInstantiationOfToIndex =
-    ArgIsInstantiationOf<CallAndPos, ArgIsInstantiationOfInput>;
+    ArgIsInstantiationOf<CallAndPos, ArgIsInstantiationOfToIndexInput>;
 
   pragma[nomagic]
   private predicate argsAreInstantiationsOfToIndex(
@@ -413,4 +413,24 @@ module ArgsAreInstantiationsOf<ArgsAreInstantiationsOfInputSig Input> {
       rnk = max(int r | toCheckRanked(i, f, _, r))
     )
   }
+
+  pragma[nomagic]
+  private predicate argsAreNotInstantiationsOf0(
+    Input::Call call, FunctionPosition pos, ImplOrTraitItemNode i
+  ) {
+    ArgIsInstantiationOfToIndex::argIsNotInstantiationOf(MkCallAndPos(call, pos), i, _, _)
+  }
+
+  /**
+   * Holds if _some_ argument of `call` has a type that is not an instantiation of the
+   * type of the corresponding parameter of `f` inside `i`.
+   */
+  pragma[nomagic]
+  predicate argsAreNotInstantiationsOf(Input::Call call, ImplOrTraitItemNode i, Function f) {
+    exists(FunctionPosition pos |
+      argsAreNotInstantiationsOf0(call, pos, i) and
+      call.hasTargetCand(i, f) and
+      Input::toCheck(i, f, pos, _)
+    )
+  }
 }

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

@@ -1289,6 +1289,13 @@ private class BorrowKind extends TBorrowKind {
   }
 }
 
+// for now, we do not handle ambiguous targets when one of the types is itself
+// a constrained type parameter; we should be checking the constraints in this case
+private predicate typeCanBeUsedForDisambiguation(Type t) {
+  not t instanceof TypeParameter or
+  t.(TypeParamTypeParameter).getTypeParam() = any(TypeParam tp | not tp.hasTypeBound())
+}
+
 /**
  * Provides logic for resolving calls to methods.
  *
@@ -2234,7 +2241,8 @@ private module MethodResolution {
         methodCallBlanketLikeCandidate(mc, _, impl, _, blanketPath, blanketTypeParam) and
         // Only apply blanket implementations when no other implementations are possible;
         // this is to account for codebases that use the (unstable) specialization feature
-        // (https://rust-lang.github.io/rfcs/1210-impl-specialization.html)
+        // (https://rust-lang.github.io/rfcs/1210-impl-specialization.html), as well as
+        // cases where our blanket implementation filtering is not precise enough.
         (mcc.hasNoCompatibleNonBlanketTarget() or not impl.isBlanketImplementation())
       |
         borrow.isNoBorrow()
@@ -2384,10 +2392,7 @@ private module MethodResolution {
       exists(TypePath path, Type t0 |
         FunctionOverloading::functionResolutionDependsOnArgument(i, f, pos, path, t0) and
         t.appliesTo(f, i, pos) and
-        // for now, we do not handle ambiguous targets when one of the types it iself
-        // a type parameter; we should be checking the constraints on that type parameter
-        // in this case
-        not t0 instanceof TypeParameter
+        typeCanBeUsedForDisambiguation(t0)
       )
     }
 
@@ -2746,7 +2751,7 @@ private module NonMethodResolution {
      * Gets the blanket function that this call may resolve to, if any.
      */
     pragma[nomagic]
-    private NonMethodFunction resolveCallTargetBlanketCand(ImplItemNode impl) {
+    NonMethodFunction resolveCallTargetBlanketCand(ImplItemNode impl) {
       exists(string name |
         this.hasNameAndArity(pragma[only_bind_into](name), _) and
         ArgIsInstantiationOfBlanketParam::argIsInstantiationOf(MkCallAndBlanketPos(this, _), impl, _) and
@@ -2761,12 +2766,11 @@ private module NonMethodResolution {
     predicate hasTrait() { exists(this.getTrait()) }
 
     pragma[nomagic]
-    NonMethodFunction resolveAssocCallTargetCand(ImplItemNode i) {
+    NonMethodFunction resolveCallTargetNonBlanketCand(ImplItemNode i) {
       not this.hasTrait() and
       result = this.getPathResolutionResolved() and
-      result = i.getASuccessor(_)
-      or
-      result = this.resolveCallTargetBlanketCand(i)
+      result = i.getASuccessor(_) and
+      FunctionOverloading::functionResolutionDependsOnArgument(_, result, _, _, _)
     }
 
     AstNode getNodeAt(FunctionPosition pos) {
@@ -2798,6 +2802,21 @@ private module NonMethodResolution {
       trait = this.getTrait()
     }
 
+    /**
+     * Holds if this call has no compatible non-blanket target, and it has some
+     * candidate blanket target.
+     */
+    pragma[nomagic]
+    predicate hasNoCompatibleNonBlanketTarget() {
+      this.resolveCallTargetBlanketLikeCandidate(_, _, _, _) and
+      not exists(this.resolveCallTargetViaPathResolution()) and
+      forall(ImplOrTraitItemNode i, Function f |
+        this.(NonMethodArgsAreInstantiationsOfNonBlanketInput::Call).hasTargetCand(i, f)
+      |
+        NonMethodArgsAreInstantiationsOfNonBlanket::argsAreNotInstantiationsOf(this, i, f)
+      )
+    }
+
     /**
      * Gets the target of this call, which can be resolved using only path resolution.
      */
@@ -2816,7 +2835,9 @@ private module NonMethodResolution {
       result = this.resolveCallTargetBlanketCand(i) and
       not FunctionOverloading::functionResolutionDependsOnArgument(_, result, _, _, _)
       or
-      NonMethodArgsAreInstantiationsOf::argsAreInstantiationsOf(this, i, result)
+      NonMethodArgsAreInstantiationsOfBlanket::argsAreInstantiationsOf(this, i, result)
+      or
+      NonMethodArgsAreInstantiationsOfNonBlanket::argsAreInstantiationsOf(this, i, result)
     }
 
     pragma[nomagic]
@@ -2855,7 +2876,12 @@ private module NonMethodResolution {
     ) {
       exists(NonMethodCall fc, FunctionPosition pos |
         fcp = MkCallAndBlanketPos(fc, pos) and
-        fc.resolveCallTargetBlanketLikeCandidate(impl, pos, blanketPath, blanketTypeParam)
+        fc.resolveCallTargetBlanketLikeCandidate(impl, pos, blanketPath, blanketTypeParam) and
+        // Only apply blanket implementations when no other implementations are possible;
+        // this is to account for codebases that use the (unstable) specialization feature
+        // (https://rust-lang.github.io/rfcs/1210-impl-specialization.html), as well as
+        // cases where our blanket implementation filtering is not precise enough.
+        (fc.hasNoCompatibleNonBlanketTarget() or not impl.isBlanketImplementation())
       )
     }
   }
@@ -2890,37 +2916,24 @@ private module NonMethodResolution {
   private module ArgIsInstantiationOfBlanketParam =
     ArgIsInstantiationOf<CallAndBlanketPos, ArgIsInstantiationOfBlanketParamInput>;
 
-  private module NonMethodArgsAreInstantiationsOfInput implements ArgsAreInstantiationsOfInputSig {
+  private module NonMethodArgsAreInstantiationsOfBlanketInput implements
+    ArgsAreInstantiationsOfInputSig
+  {
     predicate toCheck(ImplOrTraitItemNode i, Function f, FunctionPosition pos, AssocFunctionType t) {
       t.appliesTo(f, i, pos) and
-      (
-        exists(Type t0 |
-          // for now, we do not handle ambiguous targets when one of the types it iself
-          // a type parameter; we should be checking the constraints on that type parameter
-          // in this case
-          not t0 instanceof TypeParameter
-        |
-          FunctionOverloading::functionResolutionDependsOnArgument(i, f, pos, _, t0)
-          or
-          traitFunctionDependsOnPos(_, _, pos, t0, i, f)
-        )
+      exists(Type t0 | typeCanBeUsedForDisambiguation(t0) |
+        FunctionOverloading::functionResolutionDependsOnArgument(i, f, pos, _, t0)
         or
-        // match against the trait function itself
-        exists(Trait trait |
-          FunctionOverloading::traitTypeParameterOccurrence(trait, f, _, pos, _,
-            TSelfTypeParameter(trait))
-        )
+        traitFunctionDependsOnPos(_, _, pos, t0, i, f)
       )
     }
 
-    class Call extends NonMethodCall {
+    final class Call extends NonMethodCall {
       Type getArgType(FunctionPosition pos, TypePath path) {
         result = inferType(this.getNodeAt(pos), path)
       }
 
-      predicate hasTargetCand(ImplOrTraitItemNode i, Function f) {
-        f = this.resolveAssocCallTargetCand(i)
-        or
+      predicate hasTraitResolvedCand(ImplOrTraitItemNode i, Function f) {
         exists(TraitItemNode trait, NonMethodFunction resolved, ImplItemNode i1, Function f1 |
           this.hasTraitResolved(trait, resolved) and
           traitFunctionDependsOnPos(trait, resolved, _, _, i1, f1)
@@ -2932,11 +2945,45 @@ private module NonMethodResolution {
           i = trait
         )
       }
+
+      predicate hasTargetCand(ImplOrTraitItemNode i, Function f) {
+        f = this.resolveCallTargetBlanketCand(i)
+        or
+        this.hasTraitResolvedCand(i, f) and
+        BlanketImplementation::isBlanketLike(i, _, _)
+      }
+    }
+  }
+
+  private module NonMethodArgsAreInstantiationsOfBlanket =
+    ArgsAreInstantiationsOf<NonMethodArgsAreInstantiationsOfBlanketInput>;
+
+  private module NonMethodArgsAreInstantiationsOfNonBlanketInput implements
+    ArgsAreInstantiationsOfInputSig
+  {
+    predicate toCheck(ImplOrTraitItemNode i, Function f, FunctionPosition pos, AssocFunctionType t) {
+      NonMethodArgsAreInstantiationsOfBlanketInput::toCheck(i, f, pos, t)
+      or
+      // match against the trait function itself
+      t.appliesTo(f, i, pos) and
+      exists(Trait trait |
+        FunctionOverloading::traitTypeParameterOccurrence(trait, f, _, pos, _,
+          TSelfTypeParameter(trait))
+      )
+    }
+
+    class Call extends NonMethodArgsAreInstantiationsOfBlanketInput::Call {
+      predicate hasTargetCand(ImplOrTraitItemNode i, Function f) {
+        f = this.resolveCallTargetNonBlanketCand(i)
+        or
+        this.hasTraitResolvedCand(i, f) and
+        not BlanketImplementation::isBlanketLike(i, _, _)
+      }
     }
   }
 
-  private module NonMethodArgsAreInstantiationsOf =
-    ArgsAreInstantiationsOf<NonMethodArgsAreInstantiationsOfInput>;
+  private module NonMethodArgsAreInstantiationsOfNonBlanket =
+    ArgsAreInstantiationsOf<NonMethodArgsAreInstantiationsOfNonBlanketInput>;
 }
 
 abstract private class TupleLikeConstructor extends Addressable {