Rust: Rework visibility logic in path resolution and also apply to method call resolution

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]
@@ -264,10 +286,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() }
 }
 
@@ -328,17 +356,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 {
@@ -365,6 +421,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) }
@@ -381,7 +441,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 {
@@ -513,6 +577,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() }
 }
 
@@ -533,6 +601,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() }
@@ -548,6 +620,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) }
@@ -568,6 +644,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) }
@@ -590,6 +670,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) }
@@ -600,6 +684,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() }
@@ -610,6 +698,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() }
@@ -673,6 +765,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() }
@@ -1057,7 +1153,7 @@ private predicate pathUsesNamespace(Path p, Namespace n) {
 }
 
 pragma[nomagic]
-private ItemNode resolvePath1(RelevantPath path) {
+private ItemNode resolvePathCand(AstNode path) {
   exists(Namespace ns | result = resolvePath0(path, ns) |
     pathUsesNamespace(path, ns)
     or
@@ -1066,54 +1162,70 @@ private ItemNode resolvePath1(RelevantPath path) {
   )
 }
 
-pragma[nomagic]
-private ItemNode resolvePathPrivate(
-  RelevantPath path, ModuleLikeNode itemParent, ModuleLikeNode pathParent
-) {
-  not path.requiresExtractorWorkaround() and
-  result = resolvePath1(path) and
-  itemParent = result.getImmediateParentModule() and
-  not result.isPublic() and
-  (
-    pathParent.getADescendant() = path
-    or
-    pathParent = any(ItemNode mid | path = mid.getADescendant()).getImmediateParentModule()
-  )
-}
+/** Gets an item that `n` may resolve to, not taking visibility into account. */
+signature ItemNode resolveCandidateSig(AstNode n);
 
-pragma[nomagic]
-private predicate isItemParent(ModuleLikeNode itemParent) {
-  exists(resolvePathPrivate(_, itemParent, _))
-}
+/** Provides the predicate `resolve` for resolving items while taking visibility into account. */
+module ResolveWithVisibility<resolveCandidateSig/1 resolvePrivateCandidate> {
+  pragma[nomagic]
+  private ItemNode resolvePathPrivate(
+    AstNode n, ModuleLikeNode itemParent, ModuleLikeNode pathParent
+  ) {
+    not n.(RelevantPath).requiresExtractorWorkaround() and
+    result = resolvePrivateCandidate(n) and
+    result.isPrivate() and
+    (
+      pathParent.getADescendant() = n
+      or
+      pathParent = any(ItemNode mid | n = mid.getADescendant()).getImmediateParentModule()
+    ) and
+    (
+      itemParent = result.getVisibilityParent().getImmediateParentModule()
+      or
+      not result.hasVisibilityParent() and
+      itemParent = result.getImmediateParentModule()
+    )
+  }
 
-/**
- * Gets a module that has access to private items defined inside `itemParent`.
- *
- * According to [The Rust Reference][1] this is either `itemParent` itself or any
- * descendant of `itemParent`.
- *
- * [1]: https://doc.rust-lang.org/reference/visibility-and-privacy.html#r-vis.access
- */
-pragma[nomagic]
-private ModuleLikeNode getAPrivateVisibleModule(ModuleLikeNode itemParent) {
-  isItemParent(itemParent) and
-  result.getImmediateParentModule*() = itemParent
+  pragma[nomagic]
+  private predicate isItemParent(ModuleLikeNode itemParent) {
+    exists(resolvePathPrivate(_, itemParent, _))
+  }
+
+  /**
+   * Gets a module that has access to private items defined inside `itemParent`.
+   *
+   * According to [The Rust Reference][1] this is either `itemParent` itself or any
+   * descendant of `itemParent`.
+   *
+   * [1]: https://doc.rust-lang.org/reference/visibility-and-privacy.html#r-vis.access
+   */
+  pragma[nomagic]
+  private ModuleLikeNode getAPrivateVisibleModule(ModuleLikeNode itemParent) {
+    isItemParent(itemParent) and
+    result.getImmediateParentModule*() = itemParent
+  }
+
+  /** Gets an item that `n` may resolve to, taking visibility into account. */
+  ItemNode resolve(AstNode n) {
+    result = resolvePrivateCandidate(n) and
+    (
+      result.isPublic()
+      or
+      n.(RelevantPath).requiresExtractorWorkaround()
+    )
+    or
+    exists(ModuleLikeNode itemParent, ModuleLikeNode pathParent |
+      result = resolvePathPrivate(n, itemParent, pathParent) and
+      pathParent = getAPrivateVisibleModule(itemParent)
+    )
+  }
 }
 
 /** Gets the item that `path` resolves to, if any. */
 cached
 ItemNode resolvePath(RelevantPath path) {
-  result = resolvePath1(path) and
-  (
-    result.isPublic()
-    or
-    path.requiresExtractorWorkaround()
-  )
-  or
-  exists(ModuleLikeNode itemParent, ModuleLikeNode pathParent |
-    result = resolvePathPrivate(path, itemParent, pathParent) and
-    pathParent = getAPrivateVisibleModule(itemParent)
-  )
+  result = ResolveWithVisibility<resolvePathCand/1>::resolve(path)
 }
 
 pragma[nomagic]
@@ -1222,19 +1334,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/lib/codeql/rust/internal/TypeInference.qll

@@ -913,12 +913,16 @@ private module Cached {
     name = mce.getIdentifier().getText()
   }
 
+  private ItemNode resolveMethodCallExprCand(AstNode mce) {
+    exists(string name | result = getMethodCallExprLookupType(mce, name).getMethod(name))
+  }
+
   /**
    * Gets a method that the method call `mce` resolves to, if any.
    */
   cached
   Function resolveMethodCallExpr(MethodCallExpr mce) {
-    exists(string name | result = getMethodCallExprLookupType(mce, name).getMethod(name))
+    result = ResolveWithVisibility<resolveMethodCallExprCand/1>::resolve(mce)
   }
 
   pragma[nomagic]

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

@@ -0,0 +1,2 @@
+extractionWarning
+| illegal/main.rs:1:1:1:1 | semantic analyzer unavailable (not included as a module) |