Rust: Make path resolution robust against invalid code with conflicting declarations

Author: hvitved

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

@@ -107,7 +107,7 @@ class SuccessorKind extends TSuccessorKind {
 }
 
 pragma[nomagic]
-private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind kind) {
+private ItemNode getAChildSuccessor0(ItemNode item, string name, SuccessorKind kind) {
   item = result.getImmediateParent() and
   name = result.getName() and
   // Associated items in `impl` and `trait` blocks are handled elsewhere
@@ -116,7 +116,7 @@ private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind ki
   if result instanceof TypeParam
   then kind.isInternal()
   else
-    if result.isPublic()
+    if result.isPublic() or item instanceof SourceFile
     then kind.isBoth()
     else kind.isInternal()
   or
@@ -130,6 +130,41 @@ private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind ki
   result = item
 }
 
+pragma[nomagic]
+private NamedItemNode getANamedNonModuleChildSuccessor(
+  ItemNode item, string name, Namespace ns, int startline, int startcolumn, int endline,
+  int endcolumn
+) {
+  result.getLocation().hasLocationInfo(_, startline, startcolumn, endline, endcolumn) and
+  result = getAChildSuccessor0(item, name, _) and
+  ns = result.getNamespace() and
+  not result instanceof ModuleItemNode
+}
+
+pragma[nomagic]
+private ItemNode getAChildSuccessor(ItemNode item, string name, SuccessorKind kind) {
+  result = getAChildSuccessor0(item, name, kind) and
+  // In valid Rust code, there cannot be multiple children with the same name and namespace,
+  // but to safeguard against combinatorial explosions in invalid code, we always pick the
+  // last child, except for modules, where we take the union.
+  (
+    not result instanceof NamedItemNode
+    or
+    result instanceof ModuleItemNode
+    or
+    exists(Namespace ns |
+      result =
+        max(NamedItemNode i, int startline, int startcolumn, int endline, int endcolumn |
+          i =
+            getANamedNonModuleChildSuccessor(item, name, ns, startline, startcolumn, endline,
+              endcolumn)
+        |
+          i order by startline, startcolumn, endline, endcolumn
+        )
+    )
+  )
+}
+
 private module UseOption = Option<Use>;
 
 private class UseOption = UseOption::Option;
@@ -288,10 +323,6 @@ abstract class ItemNode extends Locatable {
   cached
   ItemNode getASuccessor(string name, SuccessorKind kind, UseOption useOpt) {
     Stages::PathResolutionStage::ref() and
-    sourceFileEdge(this, name, result) and
-    kind.isBoth() and
-    useOpt.isNone()
-    or
     result = getAChildSuccessor(this, name, kind) and
     useOpt.isNone()
     or
@@ -471,6 +502,8 @@ abstract class ItemNode extends Locatable {
   Location getLocation() { result = super.getLocation() }
 }
 
+abstract class NamedItemNode extends ItemNode { }
+
 abstract class TypeItemNode extends ItemNode { }
 
 /** A module or a source file. */
@@ -509,7 +542,7 @@ private class SourceFileItemNode extends ModuleLikeNode instanceof SourceFile {
   override string getCanonicalPath(Crate c) { none() }
 }
 
-class CrateItemNode extends ItemNode instanceof Crate {
+class CrateItemNode extends NamedItemNode instanceof Crate {
   /**
    * Gets the source file that defines this crate.
    */
@@ -565,15 +598,15 @@ class CrateItemNode extends ItemNode instanceof Crate {
   override string getCanonicalPath(Crate c) { c = this and result = Crate.super.getName() }
 }
 
-class ExternCrateItemNode extends ItemNode instanceof ExternCrate {
+class ExternCrateItemNode extends NamedItemNode instanceof ExternCrate {
   override string getName() {
     result = super.getRename().getName().getText()
     or
     not super.hasRename() and
     result = super.getIdentifier().getText()
   }
 
-  override Namespace getNamespace() { none() }
+  override Namespace getNamespace() { result.isType() }
 
   override Visibility getVisibility() { result = ExternCrate.super.getVisibility() }
 
@@ -587,7 +620,7 @@ 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 private class AssocItemNode extends NamedItemNode instanceof AssocItem {
   /** Holds if this associated item has an implementation. */
   abstract predicate hasImplementation();
 
@@ -626,7 +659,7 @@ private class ConstItemNode extends AssocItemNode instanceof Const {
   override TypeParam getTypeParam(int i) { none() }
 }
 
-private class TypeItemTypeItemNode extends TypeItemNode instanceof TypeItem {
+private class TypeItemTypeItemNode extends NamedItemNode, TypeItemNode instanceof TypeItem {
   override string getName() { result = TypeItem.super.getName().getText() }
 
   override Namespace getNamespace() { result.isType() }
@@ -659,7 +692,7 @@ private class TypeItemTypeItemNode extends TypeItemNode instanceof TypeItem {
 }
 
 /** An item that can be referenced with arguments. */
-abstract class ParameterizableItemNode extends ItemNode {
+abstract class ParameterizableItemNode extends NamedItemNode {
   /** Gets the arity this item. */
   abstract int getArity();
 }
@@ -911,7 +944,7 @@ private class ImplTraitTypeReprItemNodeImpl extends ImplTraitTypeReprItemNode {
   ItemNode resolveABoundCand() { result = resolvePathCand(this.getABoundPath()) }
 }
 
-private class ModuleItemNode extends ModuleLikeNode instanceof Module {
+private class ModuleItemNode extends NamedItemNode, ModuleLikeNode instanceof Module {
   override string getName() { result = Module.super.getName().getText() }
 
   override Namespace getNamespace() { result.isType() }
@@ -929,7 +962,7 @@ private class ModuleItemNode extends ModuleLikeNode instanceof Module {
     (
       exists(SourceFile f |
         fileImport(this, f) and
-        sourceFileEdge(f, _, child)
+        child = getAChildSuccessor(f, _, _)
       )
       or
       this = child.getImmediateParent()
@@ -1001,7 +1034,7 @@ private class StructItemNode extends TypeItemTypeItemNode, ParameterizableItemNo
   }
 }
 
-final class TraitItemNode extends ImplOrTraitItemNode, TypeItemNode instanceof Trait {
+final class TraitItemNode extends ImplOrTraitItemNode, NamedItemNode, TypeItemNode instanceof Trait {
   pragma[nomagic]
   Path getABoundPath() { result = super.getATypeBound().getTypeRepr().(PathTypeRepr).getPath() }
 
@@ -1126,7 +1159,7 @@ private class BlockExprItemNode extends ItemNode instanceof BlockExpr {
 pragma[nomagic]
 private Path getWherePredPath(WherePred wp) { result = wp.getTypeRepr().(PathTypeRepr).getPath() }
 
-final class TypeParamItemNode extends TypeItemNode instanceof TypeParam {
+final class TypeParamItemNode extends NamedItemNode, TypeItemNode instanceof TypeParam {
   /** Gets a where predicate for this type parameter, if any */
   pragma[nomagic]
   private WherePred getAWherePred() {
@@ -1214,7 +1247,7 @@ final private class TypeParamItemNodeImpl extends TypeParamItemNode instanceof T
   ItemNode resolveABoundCand() { result = resolvePathCand(this.getABoundPathCand()) }
 }
 
-abstract private class MacroItemNode extends ItemNode {
+abstract private class MacroItemNode extends NamedItemNode {
   override Namespace getNamespace() { result.isMacro() }
 
   override TypeParam getTypeParam(int i) { none() }
@@ -1256,12 +1289,6 @@ private class MacroDefItemNode extends MacroItemNode instanceof MacroDef {
   override Attr getAnAttr() { result = MacroDef.super.getAnAttr() }
 }
 
-/** Holds if `item` has the name `name` and is a top-level item inside `f`. */
-private predicate sourceFileEdge(SourceFile f, string name, ItemNode item) {
-  item = f.(ItemNode).getADescendant() and
-  name = item.getName()
-}
-
 /** Holds if `f` is available as `mod name;` inside `folder`. */
 pragma[nomagic]
 private predicate fileModule(SourceFile f, string name, Folder folder) {

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

@@ -0,0 +1,6 @@
+// The code in this file is not valid Rust code
+
+struct A; // A1
+struct A; // A2
+
+fn f(x: A) {} // $ item=A2 (the latter occurence takes precedence)

rust/ql/test/library-tests/path-resolution/invalid/options.yml

@@ -0,0 +1 @@
+qltest_cargo_check: false

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

@@ -51,6 +51,7 @@ mod
 | my/nested.rs:1:1:17:1 | mod nested1 |
 | my/nested.rs:2:5:11:5 | mod nested2 |
 resolvePath
+| invalid/main.rs:6:9:6:9 | A | invalid/main.rs:3:11:4:9 | struct A |
 | main.rs:4:8:4:9 | my | main.rs:1:1:1:7 | mod my |
 | main.rs:4:14:4:17 | self | main.rs:1:1:1:7 | mod my |
 | main.rs:6:5:6:6 | my | main.rs:1:1:1:7 | mod my |