Rust: Do not dispatch to all implementations when trait target is accurate

Author: paldepind

rust/ql/lib/codeql/rust/elements/internal/CallImpl.qll

@@ -66,7 +66,7 @@ module Impl {
     }
 
     /** Gets the resolved target of this call, if any. */
-    Function getStaticTarget() { result = TypeInference::resolveCallTarget(this) }
+    Function getStaticTarget() { result = TypeInference::resolveCallTarget(this, _) }
 
     /** Gets the name of the function called, if any. */
     string getTargetName() { result = this.getStaticTarget().getName().getText() }
@@ -79,9 +79,9 @@ module Impl {
     Function getARuntimeTarget() {
       result.hasImplementation() and
       (
-        result = this.getStaticTarget()
+        result = TypeInference::resolveCallTarget(this, _)
         or
-        result.implements(this.getStaticTarget())
+        result.implements(TypeInference::resolveCallTarget(this, true))
       )
     }
   }

rust/ql/lib/codeql/rust/elements/internal/InvocationExprImpl.qll

@@ -94,6 +94,6 @@ module Impl {
     }
 
     /** Gets the resolved target (function or tuple struct/variant), if any. */
-    Addressable getResolvedTarget() { result = TypeInference::resolveCallTarget(this) }
+    Addressable getResolvedTarget() { result = TypeInference::resolveCallTarget(this, _) }
   }
 }

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

@@ -3522,12 +3522,27 @@ private module Cached {
     any(MethodResolution::MethodCall mc).argumentHasImplicitBorrow(n)
   }
 
-  /** Gets an item (function or tuple struct/variant) that `call` resolves to, if any. */
+  /**
+   * Gets an item (function or tuple struct/variant) that `call` resolves to, if
+   * any.
+   *
+   * The parameter `dispatch` is `true` if and only if the resolved target is a
+   * trait item because the a precise target could not be determined from the
+   * types (for instance in the presence of generics or `dyn` types)
+   */
   cached
-  Addressable resolveCallTarget(Expr call) {
-    result = call.(MethodResolution::MethodCall).resolveCallTarget(_, _, _)
+  Addressable resolveCallTarget(InvocationExpr call, boolean dispatch) {
+    dispatch = false and
+    result = call.(NonMethodResolution::NonMethodCall).resolveCallTargetViaPathResolution()
     or
-    result = call.(NonMethodResolution::NonMethodCall).resolveCallTarget()
+    exists(ImplOrTraitItemNode i |
+      i instanceof TraitItemNode and dispatch = true
+      or
+      i instanceof ImplItemNode and dispatch = false
+    |
+      result = call.(MethodResolution::MethodCall).resolveCallTarget(i, _, _) or
+      result = call.(NonMethodResolution::NonMethodCall).resolveCallTargetViaTypeInference(i)
+    )
   }
 
   /**
@@ -3668,9 +3683,9 @@ private module Debug {
     result = inferType(n, path)
   }
 
-  Addressable debugResolveCallTarget(InvocationExpr c) {
+  Addressable debugResolveCallTarget(InvocationExpr c, boolean dispatch) {
     c = getRelevantLocatable() and
-    result = resolveCallTarget(c)
+    result = resolveCallTarget(c, dispatch)
   }
 
   predicate debugConditionSatisfiesConstraint(

rust/ql/test/library-tests/dataflow/global/inline-flow.expected

@@ -194,11 +194,8 @@ edges
 | main.rs:351:13:351:21 | source(...) | main.rs:350:33:352:9 | { ... } | provenance |  |
 | main.rs:358:37:360:9 | { ... } | main.rs:347:13:347:29 | self.get_number() | provenance |  |
 | main.rs:359:13:359:21 | source(...) | main.rs:358:37:360:9 | { ... } | provenance |  |
-| main.rs:370:44:372:9 | { ... } | main.rs:383:18:383:38 | t.get_double_number() | provenance |  |
-| main.rs:370:44:372:9 | { ... } | main.rs:387:18:387:50 | ...::get_double_number(...) | provenance |  |
 | main.rs:370:44:372:9 | { ... } | main.rs:395:18:395:38 | i.get_double_number() | provenance |  |
 | main.rs:371:13:371:22 | source(...) | main.rs:370:44:372:9 | { ... } | provenance |  |
-| main.rs:374:33:376:9 | { ... } | main.rs:391:18:391:37 | ...::get_default(...) | provenance |  |
 | main.rs:374:33:376:9 | { ... } | main.rs:398:18:398:41 | ...::get_default(...) | provenance |  |
 | main.rs:375:13:375:21 | source(...) | main.rs:374:33:376:9 | { ... } | provenance |  |
 | main.rs:383:13:383:14 | n1 | main.rs:384:14:384:15 | n1 | provenance |  |
@@ -492,10 +489,7 @@ testFailures
 | main.rs:327:14:327:14 | c | main.rs:326:17:326:25 | source(...) | main.rs:327:14:327:14 | c | $@ | main.rs:326:17:326:25 | source(...) | source(...) |
 | main.rs:335:10:335:10 | a | main.rs:316:13:316:21 | source(...) | main.rs:335:10:335:10 | a | $@ | main.rs:316:13:316:21 | source(...) | source(...) |
 | main.rs:384:14:384:15 | n1 | main.rs:359:13:359:21 | source(...) | main.rs:384:14:384:15 | n1 | $@ | main.rs:359:13:359:21 | source(...) | source(...) |
-| main.rs:384:14:384:15 | n1 | main.rs:371:13:371:22 | source(...) | main.rs:384:14:384:15 | n1 | $@ | main.rs:371:13:371:22 | source(...) | source(...) |
 | main.rs:388:14:388:15 | n2 | main.rs:359:13:359:21 | source(...) | main.rs:388:14:388:15 | n2 | $@ | main.rs:359:13:359:21 | source(...) | source(...) |
-| main.rs:388:14:388:15 | n2 | main.rs:371:13:371:22 | source(...) | main.rs:388:14:388:15 | n2 | $@ | main.rs:371:13:371:22 | source(...) | source(...) |
 | main.rs:392:14:392:15 | n3 | main.rs:351:13:351:21 | source(...) | main.rs:392:14:392:15 | n3 | $@ | main.rs:351:13:351:21 | source(...) | source(...) |
-| main.rs:392:14:392:15 | n3 | main.rs:375:13:375:21 | source(...) | main.rs:392:14:392:15 | n3 | $@ | main.rs:375:13:375:21 | source(...) | source(...) |
 | main.rs:396:14:396:15 | n4 | main.rs:371:13:371:22 | source(...) | main.rs:396:14:396:15 | n4 | $@ | main.rs:371:13:371:22 | source(...) | source(...) |
 | main.rs:399:14:399:15 | n5 | main.rs:375:13:375:21 | source(...) | main.rs:399:14:399:15 | n5 | $@ | main.rs:375:13:375:21 | source(...) | source(...) |

rust/ql/test/library-tests/dataflow/global/main.rs

@@ -381,15 +381,15 @@ mod not_trait_dispatch {
 
         // This call is to the default method implementation.
         let n1 = t.get_double_number();
-        sink(n1); // $ hasTaintFlow=3 SPURIOUS: hasValueFlow=44
+        sink(n1); // $ hasTaintFlow=3
 
         // This call is to the default method implementation.
         let n2 = HasNumbers::get_double_number(&t);
-        sink(n2); // $ hasTaintFlow=3 SPURIOUS: hasValueFlow=44
+        sink(n2); // $ hasTaintFlow=3
 
         // This call is to the default function implementation.
         let n3 = Three::get_default();
-        sink(n3); // $ hasValueFlow=0 SPURIOUS: hasValueFlow=1
+        sink(n3); // $ hasValueFlow=0
 
         let i = TwentyTwo;
         let n4 = i.get_double_number();

rust/ql/test/library-tests/dataflow/global/viableCallable.expected

@@ -112,13 +112,10 @@
 | main.rs:371:13:371:22 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:375:13:375:21 | source(...) | main.rs:1:1:3:1 | fn source |
 | main.rs:383:18:383:38 | t.get_double_number() | main.rs:346:9:348:9 | fn get_double_number |
-| main.rs:383:18:383:38 | t.get_double_number() | main.rs:370:9:372:9 | fn get_double_number |
 | main.rs:384:9:384:16 | sink(...) | main.rs:5:1:7:1 | fn sink |
 | main.rs:387:18:387:50 | ...::get_double_number(...) | main.rs:346:9:348:9 | fn get_double_number |
-| main.rs:387:18:387:50 | ...::get_double_number(...) | main.rs:370:9:372:9 | fn get_double_number |
 | main.rs:388:9:388:16 | sink(...) | main.rs:5:1:7:1 | fn sink |
 | main.rs:391:18:391:37 | ...::get_default(...) | main.rs:350:9:352:9 | fn get_default |
-| main.rs:391:18:391:37 | ...::get_default(...) | main.rs:374:9:376:9 | fn get_default |
 | main.rs:392:9:392:16 | sink(...) | main.rs:5:1:7:1 | fn sink |
 | main.rs:395:18:395:38 | i.get_double_number() | main.rs:370:9:372:9 | fn get_double_number |
 | main.rs:396:9:396:16 | sink(...) | main.rs:5:1:7:1 | fn sink |