Rust: Account for trait visibility when resolving paths and methods

Author: paldepind

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

@@ -26,13 +26,6 @@ module Impl {
    */
   class AssocItem extends Generated::AssocItem {
     /** Holds if this item implements trait item `other`. */
-    pragma[nomagic]
-    predicate implements(AssocItem other) {
-      exists(TraitItemNode t, ImplItemNode i, string name |
-        other = t.getAssocItem(pragma[only_bind_into](name)) and
-        t = i.resolveTraitTy() and
-        this = i.getAssocItem(pragma[only_bind_into](name))
-      )
-    }
+    predicate implements(AssocItem other) { this.(AssocItemNode).implements(_, other) }
   }
 }

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

@@ -216,7 +216,7 @@ abstract class ItemNode extends Locatable {
     // items made available through `use` are available to nodes that contain the `use`
     exists(UseItemNode use |
       use = this.getASuccessor(_, _) and
-      result = use.(ItemNode).getASuccessor(name, kind)
+      result = use.getASuccessor(name, kind)
     )
     or
     exists(ExternCrateItemNode ec | result = ec.(ItemNode).getASuccessor(name, kind) |
@@ -476,10 +476,20 @@ class ExternCrateItemNode extends ItemNode instanceof ExternCrate {
 }
 
 /** An item that can occur in a trait or an `impl` block. */
-abstract private class AssocItemNode extends ItemNode instanceof AssocItem {
+abstract class AssocItemNode extends ItemNode instanceof AssocItem {
   /** Holds if this associated item has an implementation. */
   abstract predicate hasImplementation();
 
+  /** Holds if this item implements item `other` of `trait`. */
+  pragma[nomagic]
+  predicate implements(TraitItemNode trait, AssocItem other) {
+    exists(ImplItemNode impl, string name |
+      other = trait.getAssocItem(pragma[only_bind_into](name)) and
+      trait = impl.resolveTraitTy() and
+      this = impl.getAssocItem(pragma[only_bind_into](name))
+    )
+  }
+
   override predicate hasCanonicalPath(Crate c) { this.hasCanonicalPathPrefix(c) }
 
   bindingset[c]
@@ -1215,6 +1225,7 @@ private predicate declares(ItemNode item, Namespace ns, string name) {
 class RelevantPath extends Path {
   RelevantPath() { not this = any(VariableAccess va).(PathExpr).getPath() }
 
+  /** Holds if this is an unqualified path with the textual value `name`. */
   pragma[nomagic]
   predicate isUnqualified(string name) {
     not exists(this.getQualifier()) and
@@ -1325,6 +1336,12 @@ private ItemNode unqualifiedPathLookup(RelevantPath p, Namespace ns, SuccessorKi
 pragma[nomagic]
 private predicate isUnqualifiedSelfPath(RelevantPath path) { path.isUnqualified("Self") }
 
+/** Holds if the trait `trait` is visible at the element `element`. */
+bindingset[element, trait]
+predicate traitIsVisible(Element element, TraitItemNode trait) {
+  exists(ItemNode encl | encl.getADescendant*() = element and trait = encl.getASuccessor(_, _))
+}
+
 pragma[nomagic]
 private ItemNode resolvePath0(RelevantPath path, Namespace ns, SuccessorKind kind) {
   exists(ItemNode res |
@@ -1341,6 +1358,14 @@ private ItemNode resolvePath0(RelevantPath path, Namespace ns, SuccessorKind kin
     q = resolvePathQualifier(path, name) and
     result = getASuccessor(q, name, ns, kind) and
     kind.isExternalOrBoth()
+  |
+    // When the result is an associated item of a trait implementation the
+    // implemented trait must be visible.
+    exists(TraitItemNode trait |
+      result.(AssocItemNode).implements(trait, _) and traitIsVisible(path, trait)
+    )
+    or
+    not exists(ImplItemNode impl | impl.getAnAssocItem() = result and impl.(Impl).hasTrait())
   )
   or
   result = resolveUseTreeListItem(_, _, path, kind) and
@@ -1477,8 +1502,16 @@ private predicate useImportEdge(Use use, string name, ItemNode item, SuccessorKi
         not tree.hasRename() and
         name = item.getName()
         or
-        name = tree.getRename().getName().getText() and
-        name != "_"
+        exists(Rename rename | rename = tree.getRename() |
+          name = rename.getName().getText()
+          or
+          // When the rename doesn't have a name it's an underscore import. This
+          // makes the imported item visible but unnameable. We represent this
+          // by using the name `_` which can never occur in a path.  See also:
+          // https://doc.rust-lang.org/reference/items/use-declarations.html#r-items.use.as-underscore
+          not rename.hasName() and
+          name = "_"
+        )
       )
     )
   )
@@ -1550,7 +1583,7 @@ private module Debug {
     exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |
       result.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) and
       filepath.matches("%/main.rs") and
-      startline = 52
+      startline = [463 .. 511]
     )
   }
 
@@ -1572,7 +1605,7 @@ private module Debug {
     useImportEdge(use, name, item, kind)
   }
 
-  ItemNode debuggetASuccessor(ItemNode i, string name, SuccessorKind kind) {
+  ItemNode debugGetASuccessor(ItemNode i, string name, SuccessorKind kind) {
     i = getRelevantLocatable() and
     result = i.getASuccessor(name, kind)
   }

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

@@ -1891,7 +1891,7 @@ private predicate methodCandidate(Type type, string name, int arity, Impl impl)
  */
 pragma[nomagic]
 private predicate methodCandidateTrait(Type type, Trait trait, string name, int arity, Impl impl) {
-  trait = resolvePath(impl.(ImplItemNode).getTraitPath()) and
+  trait = impl.(ImplItemNode).resolveTraitTy() and
   methodCandidate(type, name, arity, impl)
 }
 
@@ -1912,7 +1912,12 @@ private module IsInstantiationOfInput implements IsInstantiationOfInputSig<Metho
       methodCandidateTrait(rootType, mc.getTrait(), name, arity, impl)
       or
       not exists(mc.getTrait()) and
-      methodCandidate(rootType, name, arity, impl)
+      methodCandidate(rootType, name, arity, impl) and
+      (
+        traitIsVisible(mc, impl.(ImplItemNode).resolveTraitTy())
+        or
+        not exists(impl.(ImplItemNode).resolveTraitTy())
+      )
     )
   }
 

rust/ql/test/library-tests/path-resolution/CONSISTENCY/PathResolutionConsistency.expected

@@ -1,8 +1,5 @@
 multipleCallTargets
 | main.rs:118:9:118:11 | f(...) |
-| main.rs:494:13:494:27 | ...::a_method(...) |
-| main.rs:498:13:498:27 | ...::a_method(...) |
-| main.rs:502:13:502:27 | ...::a_method(...) |
 | proc_macro.rs:9:5:9:10 | ...::new(...) |
 multiplePathResolutions
 | main.rs:685:3:685:12 | proc_macro |

rust/ql/test/library-tests/path-resolution/main.rs

@@ -491,15 +491,15 @@ mod trait_visibility {
         let x = X; // $ item=X
         {
             use m::Foo; // $ item=Foo
-            X::a_method(&x); // $ item=X_Foo::a_method SPURIOUS: item=X_Bar::a_method
+            X::a_method(&x); // $ item=X_Foo::a_method
         }
         {
             use m::Bar; // $ item=Bar
-            X::a_method(&x); // $ item=X_Bar::a_method SPURIOUS: item=X_Foo::a_method
+            X::a_method(&x); // $ item=X_Bar::a_method
         }
         {
             use m::Bar as _; // $ item=Bar
-            X::a_method(&x); // $ item=X_Bar::a_method SPURIOUS: item=X_Foo::a_method
+            X::a_method(&x); // $ item=X_Bar::a_method
         }
     } // trait_visibility::f
 }

rust/ql/test/library-tests/path-resolution/path-resolution.expected

@@ -236,16 +236,13 @@ resolvePath
 | main.rs:493:17:493:22 | ...::Foo | main.rs:465:9:467:9 | trait Foo |
 | main.rs:494:13:494:13 | X | main.rs:473:9:473:21 | struct X |
 | main.rs:494:13:494:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
-| main.rs:494:13:494:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
 | main.rs:497:17:497:17 | m | main.rs:464:5:486:5 | mod m |
 | main.rs:497:17:497:22 | ...::Bar | main.rs:469:9:471:9 | trait Bar |
 | main.rs:498:13:498:13 | X | main.rs:473:9:473:21 | struct X |
-| main.rs:498:13:498:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
 | main.rs:498:13:498:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
 | main.rs:501:17:501:17 | m | main.rs:464:5:486:5 | mod m |
 | main.rs:501:17:501:22 | ...::Bar | main.rs:469:9:471:9 | trait Bar |
 | main.rs:502:13:502:13 | X | main.rs:473:9:473:21 | struct X |
-| main.rs:502:13:502:23 | ...::a_method | main.rs:475:26:478:13 | fn a_method |
 | main.rs:502:13:502:23 | ...::a_method | main.rs:481:26:484:13 | fn a_method |
 | main.rs:515:10:515:16 | MyTrait | main.rs:508:5:510:5 | trait MyTrait |
 | main.rs:516:9:516:9 | S | main.rs:512:5:512:13 | struct S |

rust/ql/test/library-tests/type-inference/CONSISTENCY/PathResolutionConsistency.expected

@@ -1,8 +1,5 @@
 multipleCallTargets
 | dereference.rs:61:15:61:24 | e1.deref() |
-| main.rs:153:13:153:24 | x.a_method() |
-| main.rs:157:13:157:24 | x.a_method() |
-| main.rs:161:13:161:24 | x.a_method() |
 | main.rs:2357:13:2357:31 | ...::from(...) |
 | main.rs:2358:13:2358:31 | ...::from(...) |
 | main.rs:2359:13:2359:31 | ...::from(...) |

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

@@ -150,15 +150,15 @@ mod trait_visibility {
         let x = X;
         {
             use m::Foo;
-            x.a_method(); // $ target=Foo::a_method SPURIOUS: target=Bar::a_method
+            x.a_method(); // $ target=Foo::a_method
         }
         {
             use m::Bar;
-            x.a_method(); // $ target=Bar::a_method SPURIOUS: target=Foo::a_method
+            x.a_method(); // $ target=Bar::a_method
         }
         {
             use m::Bar as _;
-            x.a_method(); // $ target=Bar::a_method SPURIOUS: target=Foo::a_method
+            x.a_method(); // $ target=Bar::a_method
         }
         {
             use m::Bar;