Unified: Fix two bugs in parameter name scoping

shared/namebinding/codeql/namebinding/LocalNameBinding.qll

@@ -449,6 +449,12 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
 
     /** Gets a textual representation of this local entity. */
     string toString() { result = this.getName() }
+
+    /** Holds if this variable has the given name and scope. */
+    pragma[nomagic]
+    predicate hasNameAndScope(string name, AstNode scope) {
+      this = TExplicitLocal(_, name, scope) or this = TImplicitLocal(name, scope)
+    }
   }
 
   final class Local = LocalImpl;
@@ -484,10 +490,6 @@ module LocalNameBinding<LocationSig Location, LocalNameBindingInputSig<Location>
     override string getName() { result = name }
 
     override Location getLocation() { result = scope.getLocation() }
-
-    /** Holds if this variable has the given name and scope. */
-    pragma[nomagic]
-    predicate hasNameAndScope(string name_, AstNode scope_) { name = name_ and scope = scope_ }
   }
 
   /** A local access. */

unified/ql/lib/codeql/unified/internal/LocalNameBinding.qll

@@ -76,28 +76,21 @@
     or
     result = n.(LogicalAndRoot).getNthLeaf(index)
     or
-    index = 0 and
-    relocatedClassMember(n, result)
-  }
-
-  /**
-   * Holds if `member` is moved onto a child of `className` instead of the class itself,
-   * so the member name is not in scope in the base types and type parameter constraints.
-   */
-  private predicate relocatedClassMember(Identifier className, Member member) {
+    // Move members under the name node of its class, so the member name is not in scope
+    // in the base types and type parameter constraints.
     exists(ClassLikeDeclaration cls |
-      className = cls.getNameNode() and
-      member = cls.getAMember()
+      n = cls.getNameNode() and
+      result = cls.getAMember() and
+      index = 0
     )
   }
 
   AstNode getChild(AstNode n, int index) {
     result = getChild1(n, index)
     or
-    not exists(getChild1(n, _)) and
+    not result = getChild1(_, _) and
     not n instanceof LogicalAndExpr and // also ignore intermediate nodes within a 'logical and' tree
     not n instanceof GuardIfStmt and
-    not relocatedClassMember(_, result) and
     index = result.getParentIndex() and
     result = n.getAFieldOrChild()
   }
@@ -203,8 +196,17 @@
         declaration = func
       )
       or
-      exists(Parameter param |
-        scope = param.getParent() and // TODO: add SourceCallable and use .getParameter() instead
+      exists(Parameter param, Callable callable |
+        callable = param.getDeclaringCallable() and
+        (
+          // Scope to the body so parameter defaults cannot see the parameter list
+          // TODO: Use a plugin for languages where parameter defaults can reference earlier parameters
+          scope = callable.getBody()
+          or
+          // For bodyless functions, choose an arbitrary scope
+          not exists(callable.getBody()) and
+          scope = param
+        ) and
         pattern = param.getPattern() and
         declaration = param
       )
@@ -330,7 +332,9 @@
 
   predicate implicitDeclInScope(string name, AstNode scope) { implicitDeclInScope(name, scope, _) }
 
-  predicate accessCand(AstNode n, string name) { n.(PotentialLocalNameAccess).getName() = name }
+  predicate accessCand(AstNode n, string name) {
+    n.(PotentialLocalNameAccess).getName() = name and not n instanceof NameBinding
+  }
 
   predicate uncertainScope(AstNode scope) {
     // Classes can have uncertain members due to unqualified access to inherited members.
@@ -347,6 +351,17 @@
 
 module LocalNameBindingOutput = LocalNameBinding<Location, LocalNameBindingInput>;
 
+private LocalNameBindingOutput::Local resolveId(AstNode n) {
+  // For names in binding position, always resolve to the scope set by the bindingContext predicate
+  exists(AstNode scope |
+    bindingContext(n, scope, _) and
+    result.hasNameAndScope(n.(Identifier).getValue(), scope)
+  )
+  or
+  not bindingContext(n, _, _) and
+  result = n.(LocalNameBindingOutput::LocalAccess).getLocal()
+}
+
 module Public {
   /**
    * A representative for a lexically scoped entity, such as a local variable, type name, or module name.
@@ -379,7 +394,7 @@
     string getName() { result = this.getValue() }
 
     /** Gets the representative for the local name introduced by this declaration. */
-    LocalName getLocalName() { result = this.(LocalNameBindingOutput::LocalAccess).getLocal() }
+    LocalName getLocalName() { result = resolveId(this) }
   }
 
   final class LocalVariable = LocalVariableImpl;
@@ -497,7 +512,7 @@
  */
 abstract private class PotentialLocalNameAccessImpl extends AstNode {
   /** Gets the representative for the local name being accessed. */
-  LocalName getLocalName() { result = this.(LocalNameBindingOutput::LocalAccess).getLocal() }
+  LocalName getLocalName() { result = resolveId(this) }
 
   /** Gets the name being accessed. */
   abstract string getName();