Merge pull request #22782 from hvitved/unified/static-name-binding-shadowing

Unified: Improve static name binding involving shadowing

Author: hvitved

unified/extractor/ast_types.yml

@@ -81,17 +81,20 @@ supertypes:
       body?: block
   # A member is anything that can appear in the body of a class-like declaration
   member:
-    - constructor_declaration
-    - destructor_declaration
-    - function_declaration
-    - variable_declaration
-    - accessor_declaration
-    - initializer_declaration
-    - class_like_declaration
-    - type_alias_declaration
-    - associated_type_declaration
-    - unsupported_node
-    - unhandled_node
+    subtypes:
+      - constructor_declaration
+      - destructor_declaration
+      - function_declaration
+      - variable_declaration
+      - accessor_declaration
+      - initializer_declaration
+      - class_like_declaration
+      - type_alias_declaration
+      - associated_type_declaration
+      - unsupported_node
+      - unhandled_node
+    fields:
+      name_node: identifier
   type_constraint:
     - equality_type_constraint
     - bound_type_constraint

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

@@ -113,7 +113,7 @@ module Unified {
     final F::Modifier getAModifier() { result = this.getModifier(_) }
 
     /** Gets the node corresponding to the field `name_node`. */
-    final F::Identifier getNameNode() { unified_accessor_declaration_def(this, _, result) }
+    final override F::Identifier getNameNode() { unified_accessor_declaration_def(this, _, result) }
 
     /** Gets the node corresponding to the field `parameter`. */
     final override F::Parameter getParameter(int i) {
@@ -200,7 +200,9 @@ module Unified {
     final F::Modifier getAModifier() { result = this.getModifier(_) }
 
     /** Gets the node corresponding to the field `name_node`. */
-    final F::Identifier getNameNode() { unified_associated_type_declaration_def(this, result) }
+    final override F::Identifier getNameNode() {
+      unified_associated_type_declaration_def(this, result)
+    }
 
     /** Gets a field or child node of this node. */
     final override F::AstNode getAFieldOrChild() {
@@ -427,7 +429,9 @@ module Unified {
     final F::Modifier getAModifier() { result = this.getModifier(_) }
 
     /** Gets the node corresponding to the field `name_node`. */
-    final F::Identifier getNameNode() { unified_class_like_declaration_name_node(this, result) }
+    final override F::Identifier getNameNode() {
+      unified_class_like_declaration_name_node(this, result)
+    }
 
     /** Gets the node corresponding to the field `type_constraint`. */
     final F::TypeConstraint getTypeConstraint(int i) {
@@ -501,7 +505,9 @@ module Unified {
     final F::Modifier getAModifier() { result = this.getModifier(_) }
 
     /** Gets the node corresponding to the field `name_node`. */
-    final F::Identifier getNameNode() { unified_constructor_declaration_name_node(this, result) }
+    final override F::Identifier getNameNode() {
+      unified_constructor_declaration_name_node(this, result)
+    }
 
     /** Gets the node corresponding to the field `parameter`. */
     final override F::Parameter getParameter(int i) {
@@ -704,7 +710,7 @@ module Unified {
     final F::Modifier getAModifier() { result = this.getModifier(_) }
 
     /** Gets the node corresponding to the field `name_node`. */
-    final F::Identifier getNameNode() { unified_function_declaration_def(this, result) }
+    final override F::Identifier getNameNode() { unified_function_declaration_def(this, result) }
 
     /** Gets the node corresponding to the field `parameter`. */
     final override F::Parameter getParameter(int i) {
@@ -1007,7 +1013,10 @@ module Unified {
     final override F::AstNode getAFieldOrChild() { unified_map_literal_element(this, _, result) }
   }
 
-  class Member extends @unified_member, F::AstNode { }
+  class Member extends @unified_member, F::AstNode {
+    /** Gets the node corresponding to the field `name_node`. */
+    F::Identifier getNameNode() { none() }
+  }
 
   /** A class representing `member_access_expr` nodes. */
   class MemberAccessExpr extends @unified_member_access_expr, F::Expr {
@@ -1369,7 +1378,9 @@ module Unified {
     final F::Modifier getAModifier() { result = this.getModifier(_) }
 
     /** Gets the node corresponding to the field `name_node`. */
-    final F::Identifier getNameNode() { unified_type_alias_declaration_def(this, result, _) }
+    final override F::Identifier getNameNode() {
+      unified_type_alias_declaration_def(this, result, _)
+    }
 
     /** Gets the node corresponding to the field `type`. */
     final F::Expr getType() { unified_type_alias_declaration_def(this, _, result) }

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

@@ -193,7 +193,7 @@ module Unified {
 
   class VariableDeclaration extends G::VariableDeclaration {
     /** Gets the name node of this variable declaration, if any. */
-    Identifier getNameNode() { result = this.getPattern() }
+    override Identifier getNameNode() { result = this.getPattern() }
 
     /** Gets the name of the variable being declared, if any. */
     string getName() { result = this.getNameNode().getValue() }

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

@@ -1,5 +1,7 @@
 private import unified
 private import codeql.util.Unit
+private import codeql.util.Option
+private import StaticNameBinding
 
 private module Plugins {
   private import codeql.unified.internal.NameBindingPluginSwift
@@ -43,6 +45,23 @@ class NameBindingPlugin extends Unit {
   bindingset[cls, member]
   predicate isInheritableMember(ClassLikeDeclaration cls, Member member) { none() }
 
+  /**
+   * Holds if `member` is considered invalid within the namespace `n`.
+   */
+  bindingset[n, member]
+  predicate isInvalidMember(NamespaceNode n, Member member) { none() }
+
+  /**
+   * Gets the key used to determine if `m` is shadowed by another declaration
+   * with the same name and key.
+   *
+   * Nodes without shadowing keys will not shadow inherited declarations.
+   *
+   * This means that shadowing can be completely disabled by not implementing this
+   * predicate, and completely enabled by assigning the same key to all members.
+   */
+  string getShadowingKey(Member m) { none() }
+
   /** Gets the name of the implicit receiver parameter in `callable`, if it has one. */
   string getImplicitReceiverParameterName(Callable callable) { none() }
 
@@ -89,6 +108,28 @@ predicate isInheritableMember(Member member) {
   )
 }
 
+bindingset[n, member]
+predicate isInvalidMember(NamespaceNode n, Member member) {
+  any(NameBindingPlugin p).isInvalidMember(n, member)
+}
+
+private string getShadowingKey0(NameBindingNode n) {
+  result = any(NameBindingPlugin p).getShadowingKey(any(Member m | n.isMember(m)))
+}
+
+private class ShadowingKey extends string {
+  ShadowingKey() { this = getShadowingKey0(_) }
+}
+
+class ShadowingKeyOpt = Option<ShadowingKey>::Option;
+
+ShadowingKeyOpt getShadowingKey(NameBindingNode n) {
+  result.asSome() = getShadowingKey0(n)
+  or
+  not exists(getShadowingKey0(n)) and
+  result.isNone()
+}
+
 /**
  * A representative for a module scope.
  *

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

@@ -3,8 +3,21 @@
  */
 
 private import unified
+private import codeql.unified.internal.StaticNameBinding
 private import codeql.unified.internal.NameBindingPlugin
 
+private class GeneratedConstructor extends ConstructorDeclaration {
+  GeneratedConstructor() { this.hasModifier("generated") }
+}
+
+private class ConvenienceConstructor extends ConstructorDeclaration {
+  ConvenienceConstructor() { this.hasModifier("convenience") }
+}
+
+private class DesignatedConstructor extends ConstructorDeclaration {
+  DesignatedConstructor() { not this instanceof ConvenienceConstructor }
+}
+
 class NameBindingPluginSwift extends NameBindingPlugin {
   bindingset[e]
   override predicate isNonPattern(Expr e) { isUnboundPattern(e.(Identifier)) }
@@ -36,7 +49,48 @@ class NameBindingPluginSwift extends NameBindingPlugin {
   bindingset[cls, member]
   override predicate isInheritableMember(ClassLikeDeclaration cls, Member member) {
     exists(cls) and
-    not member.hasModifier("private")
+    not member.hasModifier("private") and
+    not (cls.hasModifier("protocol") and member instanceof ConstructorDeclaration)
+  }
+
+  bindingset[n, member]
+  override predicate isInvalidMember(NamespaceNode n, Member member) {
+    exists(NamespaceNode parent |
+      parent = n.getAnInheritanceParent() and
+      not parent
+          .isInstanceOrStaticMemberNamespace(any(ClassLikeDeclaration p | p.hasModifier("protocol")))
+    |
+      // Remove generated constructors when there are inherited constructors available
+      // Note: If the base class only has private constructors, this class must have an
+      // explicit constructor, in which case there is no generated constructor to begin
+      // with
+      member instanceof GeneratedConstructor and
+      n.getOwnMember(_).isMember(member) and
+      parent.getMemberFull(_, _).isMember(any(ConstructorDeclaration inherited))
+      or
+      // Remove inherited designated constructors (generated or not) when there are
+      // explicit designated constructors available
+      // Note: We always inherit convenience constructors, even though it may not actually
+      // be the case in Swift; this should be OK, since there can then not exist any calls
+      // that target those constructors
+      parent.getMemberFull(_, _).isMember(member.(DesignatedConstructor)) and
+      exists(DesignatedConstructor designated |
+        n.getOwnMember(_).isMember(designated) and
+        not designated instanceof GeneratedConstructor
+      )
+    )
+  }
+
+  override string getShadowingKey(Member m) {
+    not m instanceof Callable and result = ""
+    or
+    not m instanceof GeneratedConstructor and
+    result =
+      concat(int i, Parameter p |
+        p = m.(Callable).getParameter(i)
+      |
+        p.getExternalNameNode().getValue(), "," order by i
+      )
   }
 
   override string getImplicitReceiverParameterName(Callable callable) {