Unified: Local scoping followups

unified/extractor/ast_types.yml

@@ -49,6 +49,7 @@ supertypes:
     - tuple_pattern
     - constructor_pattern
     - or_pattern
+    - conditional_pattern
     - ignore_pattern
     - expr_equality_pattern
     - bulk_importing_pattern
@@ -369,7 +370,6 @@ named:
   catch_clause:
     modifier*: modifier
     pattern?: pattern
-    guard?: expr
     body: block
 
   # `switch value { case pattern: body case ...: default: body }`
@@ -381,11 +381,9 @@ named:
   # A single `case ...:` (or `default:`) entry in a switch.
   # An entry with multiple `case p1, p2:` patterns uses an `or_pattern`.
   # A `default:` entry has no pattern.
-  # An optional `guard` corresponds to a `where`-clause on the case.
   switch_case:
     modifier*: modifier
     pattern?: pattern
-    guard?: expr
     body: block
 
   # Evaluate 'expr' and match its result against 'pattern', and return true if it matches.
@@ -446,6 +444,14 @@ named:
     modifier*: modifier
     pattern*: pattern
 
+  # A pattern that matches against a nested pattern, and subsequently checks a condition.
+  # The match is rejected if the condition does not hold.
+  # Variables bound in the nested pattern are in scope within the condition.
+  conditional_pattern:
+    modifier*: modifier
+    condition: expr
+    pattern: pattern
+
   # A pattern with an optional associated name.
   pattern_element:
     modifier*: modifier

unified/extractor/src/languages/swift/swift.rs

@@ -94,6 +94,19 @@ fn and_chain(
         .expect("control-flow statement must have at least one condition")
 }
 
+/// Return the only pattern unchanged when there is exactly one, otherwise
+/// wrap the list in an `or_pattern`.
+fn make_or_pattern(
+    ctx: &mut yeast::build::BuildCtx<'_, SwiftContext>,
+    items: Vec<yeast::Id>,
+) -> yeast::Id {
+    if items.len() == 1 {
+        items[0]
+    } else {
+        tree!((or_pattern pattern: {items}))
+    }
+}
+
 /// Translate a multi-part identifier (for example `Foo.Bar.Baz`) into a
 /// `member_access_expr` chain rooted at a `name_expr` over the first
 /// part. Panics on an empty input because the grammar's `_+` quantifier
@@ -746,22 +759,17 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
         rule!(
             (switchCase label: (switchCaseLabel caseItems: _* @items) statements: _* @body)
             =>
-            switch_case {
-                let pattern = if items.len() == 1 {
-                    items[0]
-                } else {
-                    tree!((or_pattern pattern: {items}))
-                };
-                tree!((switch_case pattern: {pattern} body: (block stmt: {body})))
-            }
+            (switch_case
+                pattern: {make_or_pattern(&mut ctx, items)}
+                body: (block stmt: {body}))
         ),
         rule!(
             (switchCase label: (switchDefaultLabel) statements: _* @body)
             =>
             (switch_case body: (block stmt: {body}))
         ),
-        // A single case item unwraps to its pattern (used as an `or_pattern`
-        // element).
+        // A single case item unwraps to its pattern, possibly boxed in conditional_pattern
+        rule!((switchCaseItem pattern: @p whereClause: (whereClause condition: @cond)) => (conditional_pattern pattern: { p } condition: {cond})),
         rule!((switchCaseItem pattern: @p) => pattern { p }),
         // A pattern-matching condition (`if case let x = e`, `if case .foo(let x)
         // = e`) becomes a `pattern_guard_expr`: the matched pattern and the
@@ -877,17 +885,24 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
                 body: {body}
                 catch_clause: {catches})
         ),
-        // Catch block with bound identifier; optional where-clause guard.
+        rule!(
+            (catchItem pattern: @pattern whereClause: (whereClause condition: @guard))
+            =>
+            (conditional_pattern pattern: {pattern} condition: {guard})
+        ),
+        rule!(
+            (catchItem pattern: @pattern)
+            =>
+            pattern {pattern}
+        ),
+        // Catch block with one or more patterns (which have been translated by the catchItem rules)
         rule!(
             (catchClause
-                catchItems: (catchItem
-                    pattern: @pattern
-                    whereClause: (whereClause condition: @guard)?)
+                catchItems: _+ @patterns
                 body: @body)
             =>
             (catch_clause
-                pattern: {pattern}
-                guard: {guard}
+                pattern: {make_or_pattern(&mut ctx, patterns)}
                 body: {body})
         ),
         // Catch block without error binding

unified/extractor/tests/corpus/swift/control-flow/switch-case-item-where-clauses.swift

@@ -0,0 +1,8 @@
+switch n {
+case let x where x > 0:
+  print("positive")
+case let y where y < 0, 0:
+  print("non-positive")
+default:
+  print("other")
+}

unified/extractor/tests/corpus/swift/optionals-and-errors/catch-where-clauses.swift

@@ -0,0 +1,7 @@
+do {
+  try foo()
+} catch let e where isNetworkError(e), let f where isTimeout(f) {
+  print("retry")
+} catch {
+  print("fallback")
+}

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

@@ -349,9 +349,6 @@
     /** Gets the node corresponding to the field `body`. */
     final Block getBody() { unified_catch_clause_def(this, result) }
 
-    /** Gets the node corresponding to the field `guard`. */
-    final Expr getGuard() { unified_catch_clause_guard(this, result) }
-
     /** Gets the node corresponding to the field `modifier`. */
     final Modifier getModifier(int i) { unified_catch_clause_modifier(this, i, result) }
 
@@ -361,7 +358,6 @@
     /** Gets a field or child node of this node. */
     final override AstNode getAFieldOrChild() {
       unified_catch_clause_def(this, result) or
-      unified_catch_clause_guard(this, result) or
       unified_catch_clause_modifier(this, _, result) or
       unified_catch_clause_pattern(this, result)
     }
@@ -427,6 +423,28 @@
     }
   }
 
+  /** A class representing `conditional_pattern` nodes. */
+  final class ConditionalPattern extends @unified_conditional_pattern, AstNodeImpl {
+    /** Gets the name of the primary QL class for this element. */
+    final override string getAPrimaryQlClass() { result = "ConditionalPattern" }
+
+    /** Gets the node corresponding to the field `condition`. */
+    final Expr getCondition() { unified_conditional_pattern_def(this, result, _) }
+
+    /** Gets the node corresponding to the field `modifier`. */
+    final Modifier getModifier(int i) { unified_conditional_pattern_modifier(this, i, result) }
+
+    /** Gets the node corresponding to the field `pattern`. */
+    final Pattern getPattern() { unified_conditional_pattern_def(this, _, result) }
+
+    /** Gets a field or child node of this node. */
+    final override AstNode getAFieldOrChild() {
+      unified_conditional_pattern_def(this, result, _) or
+      unified_conditional_pattern_modifier(this, _, result) or
+      unified_conditional_pattern_def(this, _, result)
+    }
+  }
+
   /** A class representing `constructor_declaration` nodes. */
   final class ConstructorDeclaration extends @unified_constructor_declaration, AstNodeImpl {
     /** Gets the name of the primary QL class for this element. */
@@ -1125,9 +1143,6 @@
     /** Gets the node corresponding to the field `body`. */
     final Block getBody() { unified_switch_case_def(this, result) }
 
-    /** Gets the node corresponding to the field `guard`. */
-    final Expr getGuard() { unified_switch_case_guard(this, result) }
-
     /** Gets the node corresponding to the field `modifier`. */
     final Modifier getModifier(int i) { unified_switch_case_modifier(this, i, result) }
 
@@ -1137,7 +1152,6 @@
     /** Gets a field or child node of this node. */
     final override AstNode getAFieldOrChild() {
       unified_switch_case_def(this, result) or
-      unified_switch_case_guard(this, result) or
       unified_switch_case_modifier(this, _, result) or
       unified_switch_case_pattern(this, result)
     }
@@ -1541,8 +1555,6 @@
       or
       result = node.(CatchClause).getBody() and i = -1 and name = "getBody"
       or
-      result = node.(CatchClause).getGuard() and i = -1 and name = "getGuard"
-      or
       result = node.(CatchClause).getModifier(i) and name = "getModifier"
       or
       result = node.(CatchClause).getPattern() and i = -1 and name = "getPattern"
@@ -1565,6 +1577,12 @@
       or
       result = node.(CompoundAssignExpr).getValue() and i = -1 and name = "getValue"
       or
+      result = node.(ConditionalPattern).getCondition() and i = -1 and name = "getCondition"
+      or
+      result = node.(ConditionalPattern).getModifier(i) and name = "getModifier"
+      or
+      result = node.(ConditionalPattern).getPattern() and i = -1 and name = "getPattern"
+      or
       result = node.(ConstructorDeclaration).getBody() and i = -1 and name = "getBody"
       or
       result = node.(ConstructorDeclaration).getModifier(i) and name = "getModifier"
@@ -1721,8 +1739,6 @@
       or
       result = node.(SwitchCase).getBody() and i = -1 and name = "getBody"
       or
-      result = node.(SwitchCase).getGuard() and i = -1 and name = "getGuard"
-      or
       result = node.(SwitchCase).getModifier(i) and name = "getModifier"
       or
       result = node.(SwitchCase).getPattern() and i = -1 and name = "getPattern"

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

@@ -178,12 +178,12 @@ private module LocalNameBindingInput implements LocalNameBindingInputSig<Locatio
     )
     or
     exists(CatchClause catch |
-      scope = catch and // ensure both 'body' and 'guard' clause are in scope
+      scope = catch and // ensure both body and pattern are in scope
       pattern = catch.getPattern()
     )
     or
     exists(SwitchCase case |
-      scope = case and // ensure both 'body' and 'guard' clause are in scope (TODO: merge CatchClause and SwitchCase?)
+      scope = case and // ensure both body and pattern are in scope
       pattern = case.getPattern()
     )
     or
@@ -207,6 +207,11 @@ private module LocalNameBindingInput implements LocalNameBindingInputSig<Locatio
       pattern = pat.getPattern(_)
     )
     or
+    exists(ConditionalPattern pat |
+      bindingContext(pat, scope) and
+      pattern = pat.getPattern()
+    )
+    or
     exists(PatternGuardExpr expr |
       pattern = expr.getPattern() and
       scope = expr
@@ -262,8 +267,6 @@ private module LocalNameBindingInput implements LocalNameBindingInputSig<Locatio
     n = any(LocalFunctionDeclaration f).getName() and
     n.(Identifier).getValue() = name
   }
-
-  predicate lookupStartsAt(AstNode n, AstNode scope) { none() }
 }
 
 module LocalNameBindingOutput = LocalNameBinding<Location, LocalNameBindingInput>;

unified/ql/lib/unified.dbscheme

@@ -292,11 +292,6 @@ unified_call_expr_def(
   int callee: @unified_expr_or_type ref
 );
 
-unified_catch_clause_guard(
-  unique int unified_catch_clause: @unified_catch_clause ref,
-  unique int guard: @unified_expr ref
-);
-
 #keyset[unified_catch_clause, index]
 unified_catch_clause_modifier(
   int unified_catch_clause: @unified_catch_clause ref,
@@ -365,6 +360,19 @@ unified_compound_assign_expr_def(
   int value: @unified_expr ref
 );
 
+#keyset[unified_conditional_pattern, index]
+unified_conditional_pattern_modifier(
+  int unified_conditional_pattern: @unified_conditional_pattern ref,
+  int index: int ref,
+  unique int modifier: @unified_token_modifier ref
+);
+
+unified_conditional_pattern_def(
+  unique int id: @unified_conditional_pattern,
+  int condition: @unified_expr ref,
+  int pattern: @unified_pattern ref
+);
+
 #keyset[unified_constructor_declaration, index]
 unified_constructor_declaration_modifier(
   int unified_constructor_declaration: @unified_constructor_declaration ref,
@@ -767,7 +775,7 @@ unified_parameter_def(
   unique int id: @unified_parameter
 );
 
-@unified_pattern = @unified_bulk_importing_pattern | @unified_constructor_pattern | @unified_expr_equality_pattern | @unified_name_pattern | @unified_or_pattern | @unified_token_ignore_pattern | @unified_token_unsupported_node | @unified_tuple_pattern
+@unified_pattern = @unified_bulk_importing_pattern | @unified_conditional_pattern | @unified_constructor_pattern | @unified_expr_equality_pattern | @unified_name_pattern | @unified_or_pattern | @unified_token_ignore_pattern | @unified_token_unsupported_node | @unified_tuple_pattern
 
 unified_pattern_element_key(
   unique int unified_pattern_element: @unified_pattern_element ref,
@@ -803,11 +811,6 @@ unified_return_expr_def(
 
 @unified_stmt = @unified_accessor_declaration | @unified_class_like_declaration | @unified_constructor_declaration | @unified_destructor_declaration | @unified_do_while_stmt | @unified_expr | @unified_for_each_stmt | @unified_function_declaration | @unified_guard_if_stmt | @unified_import_declaration | @unified_labeled_stmt | @unified_operator_syntax_declaration | @unified_type_alias_declaration | @unified_variable_declaration | @unified_while_stmt
 
-unified_switch_case_guard(
-  unique int unified_switch_case: @unified_switch_case ref,
-  unique int guard: @unified_expr ref
-);
-
 #keyset[unified_switch_case, index]
 unified_switch_case_modifier(
   int unified_switch_case: @unified_switch_case ref,
@@ -1085,7 +1088,7 @@ unified_trivia_tokeninfo(
   string value: string ref
 );
 
-@unified_ast_node = @unified_accessor_declaration | @unified_argument | @unified_array_literal | @unified_assign_expr | @unified_associated_type_declaration | @unified_base_type | @unified_binary_expr | @unified_block | @unified_bound_type_constraint | @unified_break_expr | @unified_bulk_importing_pattern | @unified_call_expr | @unified_catch_clause | @unified_class_like_declaration | @unified_compound_assign_expr | @unified_constructor_declaration | @unified_constructor_pattern | @unified_continue_expr | @unified_destructor_declaration | @unified_do_while_stmt | @unified_equality_type_constraint | @unified_expr_equality_pattern | @unified_for_each_stmt | @unified_function_declaration | @unified_function_expr | @unified_function_type_expr | @unified_generic_type_expr | @unified_guard_if_stmt | @unified_if_expr | @unified_import_declaration | @unified_initializer_declaration | @unified_key_value_pair | @unified_labeled_stmt | @unified_map_literal | @unified_member_access_expr | @unified_name_expr | @unified_name_pattern | @unified_named_type_expr | @unified_operator_syntax_declaration | @unified_or_pattern | @unified_parameter | @unified_pattern_element | @unified_pattern_guard_expr | @unified_return_expr | @unified_switch_case | @unified_switch_expr | @unified_throw_expr | @unified_token | @unified_top_level | @unified_trivia_token | @unified_try_expr | @unified_tuple_expr | @unified_tuple_pattern | @unified_tuple_type_element | @unified_tuple_type_expr | @unified_type_alias_declaration | @unified_type_cast_expr | @unified_type_parameter | @unified_type_test_expr | @unified_type_test_pattern | @unified_unary_expr | @unified_unresolved_operator_sequence | @unified_variable_declaration | @unified_while_stmt
+@unified_ast_node = @unified_accessor_declaration | @unified_argument | @unified_array_literal | @unified_assign_expr | @unified_associated_type_declaration | @unified_base_type | @unified_binary_expr | @unified_block | @unified_bound_type_constraint | @unified_break_expr | @unified_bulk_importing_pattern | @unified_call_expr | @unified_catch_clause | @unified_class_like_declaration | @unified_compound_assign_expr | @unified_conditional_pattern | @unified_constructor_declaration | @unified_constructor_pattern | @unified_continue_expr | @unified_destructor_declaration | @unified_do_while_stmt | @unified_equality_type_constraint | @unified_expr_equality_pattern | @unified_for_each_stmt | @unified_function_declaration | @unified_function_expr | @unified_function_type_expr | @unified_generic_type_expr | @unified_guard_if_stmt | @unified_if_expr | @unified_import_declaration | @unified_initializer_declaration | @unified_key_value_pair | @unified_labeled_stmt | @unified_map_literal | @unified_member_access_expr | @unified_name_expr | @unified_name_pattern | @unified_named_type_expr | @unified_operator_syntax_declaration | @unified_or_pattern | @unified_parameter | @unified_pattern_element | @unified_pattern_guard_expr | @unified_return_expr | @unified_switch_case | @unified_switch_expr | @unified_throw_expr | @unified_token | @unified_top_level | @unified_trivia_token | @unified_try_expr | @unified_tuple_expr | @unified_tuple_pattern | @unified_tuple_type_element | @unified_tuple_type_expr | @unified_type_alias_declaration | @unified_type_cast_expr | @unified_type_parameter | @unified_type_test_expr | @unified_type_test_pattern | @unified_unary_expr | @unified_unresolved_operator_sequence | @unified_variable_declaration | @unified_while_stmt
 
 unified_ast_node_location(
   unique int node: @unified_ast_node ref,

unified/ql/test/library-tests/variables/test.swift

@@ -80,7 +80,7 @@ func t10(value: Int) { // name=value1
 // Switch with multiple cases
 func t11(value: Int) { // name=value1
     switch value { // $ access=value1
-    case let x where x > 0: // name=x1
+    case let x where x > 0: // $ access=x1 // name=x1
         print(x) // $ access=x1
     case let x: // name=x2
         print(x) // $ access=x2
@@ -187,6 +187,8 @@ func t22() {
     }
     inner() // $ access=inner1
     print(x) // $ access=x1
+    let inner = 2 // name=inner2
+    print(inner) // $ access=inner2
 }
 
 // Three levels of shadowing
@@ -214,7 +216,7 @@ func t24(optional: Int?) { // name=optional1
     }
 }
 
-// Switch with same variable name in different cases
+// Switch with variable shadowed within body of case
 func t25(value: Int) { // name=value1
     switch value { // $ access=value1
     case let x: // name=x1