Rust: Refine visibility check in path resolution

Author: hvitved

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

@@ -87,10 +87,32 @@ abstract class ItemNode extends Locatable {
   /** Gets the `i`th type parameter of this item, if any. */
   abstract TypeParam getTypeParam(int i);
 
+  /** Gets the parent item from which this node inherits visibility, if any. */
+  abstract ItemNode getVisibilityParent();
+
+  /** Holds if this item has a visibility parent. */
+  // Cannot be defined as `exists(this.getVisibilityParent())` because of non-monotonicity
+  abstract predicate hasVisibilityParent();
+
   /** Holds if this item is declared as `pub`. */
-  bindingset[this]
-  pragma[inline_late]
-  predicate isPublic() { exists(this.getVisibility()) }
+  pragma[nomagic]
+  predicate isPublic() {
+    exists(this.getVisibility())
+    or
+    this.getVisibilityParent().isPublic()
+  }
+
+  /** Holds if this item is not declared as `pub`. */
+  pragma[nomagic]
+  predicate isPrivate() {
+    // Cannot be defined as `not this.isPublic()` because of non-monotonicity
+    not exists(this.getVisibility()) and
+    (
+      not this.hasVisibilityParent()
+      or
+      this.getVisibilityParent().isPrivate()
+    )
+  }
 
   /** Gets an element that has this item as immediately enclosing item. */
   pragma[nomagic]
@@ -243,10 +265,16 @@ private class SourceFileItemNode extends ModuleLikeNode, SourceFile {
     result.isType() // can be referenced with `super`
   }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { none() }
 
   override predicate isPublic() { any() }
 
+  override predicate isPrivate() { none() }
+
   override TypeParam getTypeParam(int i) { none() }
 }
 
@@ -307,17 +335,45 @@ class CrateItemNode extends ItemNode instanceof Crate {
     result.isType() // can be referenced with `crate`
   }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { none() }
 
   override predicate isPublic() { any() }
 
+  override predicate isPrivate() { none() }
+
   override TypeParam getTypeParam(int i) { none() }
 }
 
 /** An item that can occur in a trait or an `impl` block. */
 abstract private class AssocItemNode extends ItemNode, AssocItem {
   /** Holds if this associated item has an implementation. */
   abstract predicate hasImplementation();
+
+  override ItemNode getVisibilityParent() {
+    exists(ImplItemNode i | this = i.getAnAssocItem() |
+      // trait implementations inherit visibility from the trait
+      result = i.resolveTraitTy()
+      or
+      // for  inherent implementations that are not explicitly marked as
+      // `pub`, the `impl` block itself must be visible
+      not i.(Impl).hasTrait() and
+      not exists(this.getVisibility()) and
+      result = i
+    )
+  }
+
+  override predicate hasVisibilityParent() {
+    exists(ImplItemNode i | this = i.getAnAssocItem() |
+      i.(Impl).hasTrait()
+      or
+      not i.(Impl).hasTrait() and
+      not exists(this.getVisibility())
+    )
+  }
 }
 
 private class ConstItemNode extends AssocItemNode instanceof Const {
@@ -346,6 +402,10 @@ private class EnumItemNode extends ItemNode instanceof Enum {
 
   override Namespace getNamespace() { result.isType() }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { result = Enum.super.getVisibility() }
 
   override TypeParam getTypeParam(int i) { result = super.getGenericParamList().getTypeParam(i) }
@@ -362,7 +422,11 @@ private class VariantItemNode extends ItemNode instanceof Variant {
     result = super.getEnum().getGenericParamList().getTypeParam(i)
   }
 
-  override Visibility getVisibility() { result = super.getEnum().getVisibility() }
+  override predicate hasVisibilityParent() { any() }
+
+  override ItemNode getVisibilityParent() { result = super.getEnum() }
+
+  override Visibility getVisibility() { none() }
 }
 
 class FunctionItemNode extends AssocItemNode instanceof Function {
@@ -496,6 +560,10 @@ class ImplItemNode extends ImplOrTraitItemNode instanceof Impl {
 
   override TypeParam getTypeParam(int i) { result = super.getGenericParamList().getTypeParam(i) }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { result = Impl.super.getVisibility() }
 }
 
@@ -516,6 +584,10 @@ private class ModuleItemNode extends ModuleLikeNode instanceof Module {
 
   override Namespace getNamespace() { result.isType() }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { result = Module.super.getVisibility() }
 
   override TypeParam getTypeParam(int i) { none() }
@@ -531,6 +603,10 @@ private class StructItemNode extends ItemNode instanceof Struct {
     result.isValue() // the constructor
   }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { result = Struct.super.getVisibility() }
 
   override TypeParam getTypeParam(int i) { result = super.getGenericParamList().getTypeParam(i) }
@@ -551,6 +627,10 @@ class TraitItemNode extends ImplOrTraitItemNode instanceof Trait {
 
   override Namespace getNamespace() { result.isType() }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { result = Trait.super.getVisibility() }
 
   override TypeParam getTypeParam(int i) { result = super.getGenericParamList().getTypeParam(i) }
@@ -573,6 +653,10 @@ private class UnionItemNode extends ItemNode instanceof Union {
 
   override Namespace getNamespace() { result.isType() }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { result = Union.super.getVisibility() }
 
   override TypeParam getTypeParam(int i) { result = super.getGenericParamList().getTypeParam(i) }
@@ -583,6 +667,10 @@ private class UseItemNode extends ItemNode instanceof Use {
 
   override Namespace getNamespace() { none() }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { result = Use.super.getVisibility() }
 
   override TypeParam getTypeParam(int i) { none() }
@@ -593,6 +681,10 @@ private class BlockExprItemNode extends ItemNode instanceof BlockExpr {
 
   override Namespace getNamespace() { none() }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { none() }
 
   override TypeParam getTypeParam(int i) { none() }
@@ -656,6 +748,10 @@ class TypeParamItemNode extends ItemNode instanceof TypeParam {
 
   override Namespace getNamespace() { result.isType() }
 
+  override ItemNode getVisibilityParent() { none() }
+
+  override predicate hasVisibilityParent() { none() }
+
   override Visibility getVisibility() { none() }
 
   override TypeParam getTypeParam(int i) { none() }
@@ -1055,12 +1151,17 @@ private ItemNode resolvePathPrivate(
 ) {
   not path.requiresExtractorWorkaround() and
   result = resolvePath1(path) and
-  itemParent = result.getImmediateParentModule() and
-  not result.isPublic() and
+  result.isPrivate() and
   (
     pathParent.getADescendant() = path
     or
     pathParent = any(ItemNode mid | path = mid.getADescendant()).getImmediateParentModule()
+  ) and
+  (
+    itemParent = result.getVisibilityParent().getImmediateParentModule()
+    or
+    not result.hasVisibilityParent() and
+    itemParent = result.getImmediateParentModule()
   )
 }
 
@@ -1205,19 +1306,11 @@ private module Debug {
   private Locatable getRelevantLocatable() {
     exists(string filepath, int startline, int startcolumn, int endline, int endcolumn |
       result.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn) and
-      filepath.matches("%/test_logging.rs") and
-      startline = 163
+      filepath.matches("%/illegal/main.rs") and
+      startline = 51
     )
   }
 
-  predicate debugUnqualifiedPathLookup(
-    RelevantPath p, string name, Namespace ns, ItemNode encl, string path
-  ) {
-    p = getRelevantLocatable() and
-    unqualifiedPathLookup(p, name, ns, encl) and
-    path = p.toStringDebug()
-  }
-
   ItemNode debugResolvePath(RelevantPath path) {
     path = getRelevantLocatable() and
     result = resolvePath(path)

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

@@ -58,7 +58,7 @@ mod m1 {
             );
             super::S(). // $ item=I1
                 f1(); // $ item=I6
-            super::S::f1( // $ MISSING: item=I6
+            super::S::f1( // $ item=I6
                 super::S() // $ item=I1
             );
             super::S(). // $ item=I1

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

@@ -77,6 +77,7 @@ resolvePath
 | illegal/main.rs:59:13:59:20 | ...::S | illegal/main.rs:2:5:2:15 | struct S |
 | illegal/main.rs:61:13:61:17 | super | illegal/main.rs:1:1:71:1 | mod m1 |
 | illegal/main.rs:61:13:61:20 | ...::S | illegal/main.rs:2:5:2:15 | struct S |
+| illegal/main.rs:61:13:61:24 | ...::f1 | illegal/main.rs:27:24:30:13 | fn f1 |
 | illegal/main.rs:62:17:62:21 | super | illegal/main.rs:1:1:71:1 | mod m1 |
 | illegal/main.rs:62:17:62:24 | ...::S | illegal/main.rs:2:5:2:15 | struct S |
 | illegal/main.rs:64:13:64:17 | super | illegal/main.rs:1:1:71:1 | mod m1 |