Address review comments

Author: hvitved

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

@@ -22,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].
  *
@@ -300,30 +328,3 @@ class Vec extends Struct {
   /** Gets the type parameter representing the element type. */
   TypeParam getElementTypeParam() { result = this.getGenericParamList().getTypeParam(0) }
 }
-
-// Blanket implementations currently don't have a canonical path, so we cannot
-// use models-as-data for this model.
-private class ReflexiveFrom extends SummarizedCallable::Range {
-  ReflexiveFrom() {
-    exists(ImplItemNode impl |
-      impl.resolveTraitTy().(Trait).getCanonicalPath() = "core::convert::From" and
-      this = impl.getAnAssocItem() and
-      impl.isBlanketImplementation() and
-      this.getParam(0)
-          .getTypeRepr()
-          .(TypeMention)
-          .resolveType()
-          .(TypeParamTypeParameter)
-          .getTypeParam() = impl.getTypeParam(0)
-    )
-  }
-
-  override predicate propagatesFlow(
-    string input, string output, boolean preservesValue, string model
-  ) {
-    input = "Argument[0]" and
-    output = "ReturnValue" and
-    preservesValue = true and
-    model = "ReflexiveFrom"
-  }
-}

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,40 +50,31 @@ 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)
   )
 }
 
-pragma[nomagic]
-private predicate isBlanketImpl(ImplItemNode impl, Trait trait) {
-  impl.isBlanketImplementation() and
-  trait = impl.resolveTraitTy()
-}
-
 /**
  * Holds if `impl` is an implementation of `trait` and if another implementation
  * exists for the same type.
  */
 pragma[nomagic]
-private predicate implHasSibling(ImplItemNode impl, Trait trait) {
-  implSiblings(trait, impl, _)
-  or
-  exists(ImplItemNode other |
-    isBlanketImpl(impl, trait) and
-    isBlanketImpl(other, trait) and
-    impl != other
-  )
-}
+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/TypeInference.qll

@@ -1293,7 +1293,7 @@ private class BorrowKind extends TBorrowKind {
 // 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 exists(tp.getATypeBound()))
+  t.(TypeParamTypeParameter).getTypeParam() = any(TypeParam tp | not tp.hasTypeBound())
 }
 
 /**
@@ -2241,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()
@@ -2878,7 +2879,8 @@ private module NonMethodResolution {
         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)
+        // (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())
       )
     }