diff --git a/ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll b/ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll index 5ff48191534b..8eed5ee6f702 100644 --- a/ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll +++ b/ruby/ql/lib/codeql/ruby/ast/internal/Variable.qll @@ -170,8 +170,6 @@ private module Input implements LocalNameBindingInputSig { class SiblingShadowingDecl extends AstNode { SiblingShadowingDecl() { none() } - AstNode getLhs() { none() } - AstNode getRhs() { none() } AstNode getElse() { none() } diff --git a/shared/namebinding/codeql/namebinding/LocalNameBinding.qll b/shared/namebinding/codeql/namebinding/LocalNameBinding.qll index a467568f9832..57c792027ba8 100644 --- a/shared/namebinding/codeql/namebinding/LocalNameBinding.qll +++ b/shared/namebinding/codeql/namebinding/LocalNameBinding.qll @@ -82,9 +82,6 @@ signature module LocalNameBindingInputSig { * ``` */ class SiblingShadowingDecl extends AstNode { - /** Gets the left-hand side of this declaration. */ - AstNode getLhs(); - /** * Gets the right-hand side of this declaration. * @@ -296,6 +293,31 @@ module LocalNameBinding ) } + /** Holds if `node` should be included in the debug tree. */ + private signature predicate relevantNodeSig(AstNode node); + + module DebugScopeGraph { + private string getANodeAnnotation(AstNode node) { + result = + "[scope=" + + strictconcat(string name | + declInScope(_, name, node) or implicitDeclInScope(name, node) + | + name, "," + ) + "]" + } + + query predicate nodes(AstNode node, string key, string value) { + relevantNode(node) and + key = "semmle.label" and + value = node.toString() + concat(getANodeAnnotation(node)) + } + + query predicate edges(AstNode node1, AstNode node2) { + relevantNode(node2) and node2 = getParentForScoping(node1) + } + } + /** Gets the immediately enclosing variable scope of `n`. */ private Scope getEnclosingScope(AstNode n) { result = getParentForScoping(n) diff --git a/shared/yeast/src/build.rs b/shared/yeast/src/build.rs index a5f454a5f544..6c2a9d4181d7 100644 --- a/shared/yeast/src/build.rs +++ b/shared/yeast/src/build.rs @@ -177,6 +177,37 @@ impl BuildCtx<'_, C> { None => Err("translate() called on a BuildCtx without a translator handle".into()), } } + + /// Translate every node in an iterator with a **fresh** user context + /// (reset to `C::default()`), restoring the previous context afterwards. + /// + /// Use when descending into a subtree — a body, expression, or statement + /// list — that must not inherit any of the surrounding translation + /// context (for example an enclosing binding modifier). Accepts optional + /// (`Option`) and repeated (`Vec`) captures (both `IntoIterator`); + /// for a single `Id`, wrap it in `std::iter::once(id)`. + pub fn translate_reset>( + &mut self, + ids: impl IntoIterator, + ) -> Result, String> + where + C: Default, + { + let saved = std::mem::take(&mut *self.user_ctx); + let mut out = Vec::new(); + let mut result = Ok(()); + for id in ids { + match self.translate(id) { + Ok(v) => out.extend(v), + Err(e) => { + result = Err(e); + break; + } + } + } + *self.user_ctx = saved; + result.map(|()| out) + } } impl std::ops::Deref for BuildCtx<'_, C> { diff --git a/unified/extractor/src/languages/swift/swift.rs b/unified/extractor/src/languages/swift/swift.rs index 5689d930bff3..0f4f80a8f019 100644 --- a/unified/extractor/src/languages/swift/swift.rs +++ b/unified/extractor/src/languages/swift/swift.rs @@ -25,16 +25,13 @@ struct SwiftContext { /// by the outer `function_parameter` rule; read by the `parameter` /// rules. default_value: Option, - /// Translated outer modifiers (e.g. visibility, attributes) to - /// attach to each child of a flattening outer rule. Set by - /// `property_declaration`, `enum_entry`, and - /// `protocol_property_declaration`. + /// Translated outer modifiers to attach to each child of a flattening + /// outer rule. Set by `property_declaration`, `binding_pattern`, + /// `enum_entry`, and `protocol_property_declaration`. For `let`/`var` + /// declarations and `binding_pattern`s the list is led by the binding + /// modifier, which also serves as the "this is a binding" signal for + /// pattern translation (see `in_binding_pattern`). outer_modifiers: Vec, - /// The `let`/`var` binding modifier for a `property_declaration`. - /// Set by `property_declaration`; read by the inner declaration - /// rules (`property_binding` variants, accessor rules) so they - /// emit it as part of the output node's `modifier:` field. - binding_modifier: Option, /// True when the current child of a flattening outer rule is not /// the first one — its inner rule should emit a /// `chained_declaration` modifier so the original grouping can be @@ -42,6 +39,20 @@ struct SwiftContext { is_chained: bool, } +impl SwiftContext { + /// Whether the pattern currently being translated is a binding + /// (the LHS of a `let`/`var` declaration or a `binding_pattern`). + /// + /// True exactly when an enclosing binding has published its modifier into + /// `outer_modifiers`. This is reliable because non-binding subtrees + /// (bodies, initializer values, ...) are translated with a reset context + /// (see `BuildCtx::translate_reset`), so a bare identifier only sees a + /// non-empty `outer_modifiers` when it really is a binding. + fn in_binding_pattern(&self) -> bool { + !self.outer_modifiers.is_empty() + } +} + /// Build a freshly-created `chained_declaration` modifier node if /// `ctx.is_chained`, else `None`. Used by inner declaration rules to /// emit the chained tag for non-first children of a flattening outer @@ -220,16 +231,15 @@ fn translation_rules() -> Vec> { (property_binding name: (pattern bound_identifier: @name) type: _? @ty - computed_value: (computed_property statement: _* @body)) + computed_value: (computed_property statement: _* @@body)) => (accessor_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} name: (identifier #{name}) type: {ty} accessor_kind: (accessor_kind "get") - body: (block stmt: {body})) + body: (block stmt: {ctx.translate_reset(body)?})) ), // Stored property with willSet/didSet observers (initializer // optional) → a `variable_declaration` followed by one @@ -246,13 +256,19 @@ fn translation_rules() -> Vec> { (property_binding name: (pattern bound_identifier: @name) type: _? @ty - value: _? @val + value: _? @@val observers: (willset_didset_block willset: _? @@ws didset: _? @@ds)) => {{ + // The initializer value must not inherit the binding context + // (it may contain patterns, e.g. a switch expression), so + // translate it with a reset context. The observers keep the + // context: each willSet/didSet accessor emits the binding + // modifier and resets its own body. + let val = ctx.translate_reset(val)?; + let var_decl = tree!( (variable_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} pattern: (name_pattern identifier: (identifier #{name})) @@ -275,19 +291,23 @@ fn translation_rules() -> Vec> { ), // property_binding with any pattern name (identifier or // destructuring). Reads outer modifiers / chained tag from `ctx`. + // + // The enclosing `property_declaration` leads `ctx.outer_modifiers` + // with the `let`/`var` binding modifier, so the auto-translated name + // pattern (the LHS) becomes a binding, while the initializer value is + // translated with a reset context (see `translate_reset`). rule!( (property_binding name: @pattern type: _? @ty - value: _? @val) + value: _? @@val) => (variable_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} pattern: {pattern} type: {ty} - value: {val}) + value: {ctx.translate_reset(val)?}) // reset context: the initializer must not see the binding ), // property_declaration: flatten declarators (each may translate // to multiple nodes — variable_declaration and/or @@ -307,8 +327,11 @@ fn translation_rules() -> Vec> { => {{ let binding_text = ctx.ast.source_text(binding_kind); - ctx.binding_modifier = Some(ctx.literal("modifier", &binding_text)); - ctx.outer_modifiers = mods; + let binding = ctx.literal("modifier", &binding_text); + // The `let`/`var` binding modifier leads the declaration's + // modifier list and doubles as the "this is a binding" signal + // for pattern translation (see `in_binding_pattern`). + ctx.outer_modifiers = std::iter::once(binding).chain(mods).collect(); let mut result = Vec::new(); for (i, decl) in decls.into_iter().enumerate() { @@ -403,14 +426,23 @@ fn translation_rules() -> Vec> { rule!((type name: @inner) => {inner}), // `directly_assignable_expression` is just a wrapper; unwrap it rule!((directly_assignable_expression expr: @inner) => {inner}), - // Pattern with bound_identifier → name_pattern - rule!((pattern bound_identifier: @name) => (name_pattern identifier: (identifier #{name}))), - // Pattern with 'let' or 'var' binding: extract the inner pattern - // TODO: Names in a pattern need to be translated to expr_equality_pattern if not under a 'var/let' but we lack a way to pass down context to do this. + // Pattern with bound_identifier → name_pattern. + rule!( + (pattern bound_identifier: @name) + => + (name_pattern identifier: (identifier #{name})) + ), + // Pattern with 'let' or 'var' binding: publish the binding modifier + // into `ctx` and translate the inner pattern under it. rule!( - (pattern kind: (binding_pattern binding: _? pattern: @pattern)) + (pattern kind: (binding_pattern binding: (value_binding_pattern mutability: @@binding_kind) pattern: @@pattern)) => - {pattern} + {{ + let binding_text = ctx.ast.source_text(binding_kind); + let binding = ctx.literal("modifier", &binding_text); + ctx.outer_modifiers = vec![binding]; + ctx.translate(pattern)? + }} ), // case T.foo(x,y) pattern rule!( @@ -436,6 +468,22 @@ fn translation_rules() -> Vec> { rule!((pattern kind: (type_casting_pattern)) => (unsupported_node)), // Wildcard pattern rule!((pattern kind: (wildcard_pattern)) => (ignore_pattern)), + // A bare identifier used as an expression-pattern. Under a `var`/`let` + // binding it introduces a new variable and becomes a `name_pattern`; + // otherwise it matches by equality and is left as an `expr_equality_pattern` + // over the name expression. + rule!( + (pattern kind: (simple_identifier) @name) + => + { + if ctx.in_binding_pattern() { + tree!((name_pattern identifier: (identifier #{name}))) + } else { + let expr = tree!((name_expr identifier: (identifier #{name}))); + tree!((expr_equality_pattern expr: {expr})) + } + } + ), // Expression pattern // We lack a way to check if 'expr' is actually an expression, but due to rule ordering // the 'expression' case is the only remaining possibility when this rule tries to match. @@ -1062,56 +1110,52 @@ fn translation_rules() -> Vec> { // and binding/outer modifiers + chained tag from the outer // property_declaration rule. rule!( - (computed_getter body: (block statement: _* @body)?) + (computed_getter body: (block statement: _* @@body)?) => (accessor_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} name: {ctx.property_name.ok_or("computed_getter outside property_binding context")?} type: {ctx.property_type} accessor_kind: (accessor_kind "get") - body: (block stmt: {body})) + body: (block stmt: {ctx.translate_reset(body)?})) ), // Computed setter with explicit parameter name. rule!( - (computed_setter parameter: @param body: (block statement: _* @body)) + (computed_setter parameter: @param body: (block statement: _* @@body)) => (accessor_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} name: {ctx.property_name.ok_or("computed_setter outside property_binding context")?} type: {ctx.property_type} accessor_kind: (accessor_kind "set") parameter: (parameter pattern: (name_pattern identifier: (identifier #{param}))) - body: (block stmt: {body})) + body: (block stmt: {ctx.translate_reset(body)?})) ), // Computed setter without explicit parameter name; body optional. rule!( - (computed_setter body: (block statement: _* @body)?) + (computed_setter body: (block statement: _* @@body)?) => (accessor_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} name: {ctx.property_name.ok_or("computed_setter outside property_binding context")?} type: {ctx.property_type} accessor_kind: (accessor_kind "set") - body: (block stmt: {body})) + body: (block stmt: {ctx.translate_reset(body)?})) ), // Computed modify → accessor_declaration rule!( - (computed_modify body: (block statement: _* @body)) + (computed_modify body: (block statement: _* @@body)) => (accessor_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} name: {ctx.property_name.ok_or("computed_modify outside property_binding context")?} type: {ctx.property_type} accessor_kind: (accessor_kind "modify") - body: (block stmt: {body})) + body: (block stmt: {ctx.translate_reset(body)?})) ), // willset/didset block — spread to children (only reachable as a // fallback; the outer property_binding manual rule normally @@ -1122,27 +1166,25 @@ fn translation_rules() -> Vec> { // binding/outer modifiers + chained tag from the outer // property_declaration rule. rule!( - (willset_clause body: (block statement: _* @body)?) + (willset_clause body: (block statement: _* @@body)?) => (accessor_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} name: {ctx.property_name.ok_or("willset_clause outside property_binding context")?} accessor_kind: (accessor_kind "willSet") - body: (block stmt: {body})) + body: (block stmt: {ctx.translate_reset(body)?})) ), // didset clause → accessor_declaration (body optional). rule!( - (didset_clause body: (block statement: _* @body)?) + (didset_clause body: (block statement: _* @@body)?) => (accessor_declaration - modifier: {ctx.binding_modifier} modifier: {ctx.outer_modifiers.clone()} modifier: {chained_modifier(&mut ctx)} name: {ctx.property_name.ok_or("didset_clause outside property_binding context")?} accessor_kind: (accessor_kind "didSet") - body: (block stmt: {body})) + body: (block stmt: {ctx.translate_reset(body)?})) ), // Preprocessor conditionals — unsupported rule!((diagnostic) => (unsupported_node)), diff --git a/unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.output b/unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.output new file mode 100644 index 000000000000..3919b875b96a --- /dev/null +++ b/unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.output @@ -0,0 +1,86 @@ +let x = 1 +switch y { +case someConstant: + print("matched") +default: + break +} + +--- + +source_file + statement: + property_declaration + binding: + value_binding_pattern + mutability: let + declarator: + property_binding + name: + pattern + bound_identifier: simple_identifier "x" + value: integer_literal "1" + switch_statement + entry: + switch_entry + pattern: + switch_pattern + pattern: + pattern + kind: simple_identifier "someConstant" + statement: + call_expression + function: simple_identifier "print" + suffix: + call_suffix + arguments: + value_arguments + argument: + value_argument + value: + line_string_literal + text: line_str_text "matched" + switch_entry + default: default_keyword "default" + statement: + control_transfer_statement + kind: break + expr: simple_identifier "y" + +--- + +top_level + body: + block + stmt: + variable_declaration + modifier: modifier "let" + pattern: + name_pattern + identifier: identifier "x" + value: int_literal "1" + switch_expr + case: + switch_case + body: + block + stmt: + call_expr + argument: + argument + value: string_literal "\"matched\"" + callee: + name_expr + identifier: identifier "print" + pattern: + expr_equality_pattern + expr: + name_expr + identifier: identifier "someConstant" + switch_case + body: + block + stmt: break_expr "break" + value: + name_expr + identifier: identifier "y" diff --git a/unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.swift b/unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.swift new file mode 100644 index 000000000000..1338097e8280 --- /dev/null +++ b/unified/extractor/tests/corpus/swift/control-flow/binding-modifier-does-not-leak-to-sibling.swift @@ -0,0 +1,7 @@ +let x = 1 +switch y { +case someConstant: + print("matched") +default: + break +} diff --git a/unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.output b/unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.output new file mode 100644 index 000000000000..4ee195672bb2 --- /dev/null +++ b/unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.output @@ -0,0 +1,100 @@ +var p: Int { + get { + switch y { + case someConstant: + return 1 + default: + return 2 + } + } +} + +--- + +source_file + statement: + property_declaration + binding: + value_binding_pattern + mutability: var + declarator: + property_binding + computed_value: + computed_property + accessor: + computed_getter + body: + block + statement: + switch_statement + entry: + switch_entry + pattern: + switch_pattern + pattern: + pattern + kind: simple_identifier "someConstant" + statement: + control_transfer_statement + kind: return + result: integer_literal "1" + switch_entry + default: default_keyword "default" + statement: + control_transfer_statement + kind: return + result: integer_literal "2" + expr: simple_identifier "y" + specifier: + getter_specifier + name: + pattern + bound_identifier: simple_identifier "p" + type: + type_annotation + type: + type + name: + user_type + part: + simple_user_type + name: type_identifier "Int" + +--- + +top_level + body: + block + stmt: + accessor_declaration + body: + block + stmt: + switch_expr + case: + switch_case + body: + block + stmt: + return_expr + value: int_literal "1" + pattern: + expr_equality_pattern + expr: + name_expr + identifier: identifier "someConstant" + switch_case + body: + block + stmt: + return_expr + value: int_literal "2" + value: + name_expr + identifier: identifier "y" + modifier: modifier "var" + name: identifier "p" + type: + named_type_expr + name: identifier "Int" + accessor_kind: accessor_kind "get" diff --git a/unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.swift b/unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.swift new file mode 100644 index 000000000000..74430bb282f5 --- /dev/null +++ b/unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.swift @@ -0,0 +1,10 @@ +var p: Int { + get { + switch y { + case someConstant: + return 1 + default: + return 2 + } + } +} diff --git a/unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.output b/unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.output new file mode 100644 index 000000000000..65364c1ef918 --- /dev/null +++ b/unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.output @@ -0,0 +1,63 @@ +let x = switch y { +case someConstant: 1 +default: 2 +} + +--- + +source_file + statement: + property_declaration + binding: + value_binding_pattern + mutability: let + declarator: + property_binding + name: + pattern + bound_identifier: simple_identifier "x" + value: + switch_statement + entry: + switch_entry + pattern: + switch_pattern + pattern: + pattern + kind: simple_identifier "someConstant" + statement: integer_literal "1" + switch_entry + default: default_keyword "default" + statement: integer_literal "2" + expr: simple_identifier "y" + +--- + +top_level + body: + block + stmt: + variable_declaration + modifier: modifier "let" + pattern: + name_pattern + identifier: identifier "x" + value: + switch_expr + case: + switch_case + body: + block + stmt: int_literal "1" + pattern: + expr_equality_pattern + expr: + name_expr + identifier: identifier "someConstant" + switch_case + body: + block + stmt: int_literal "2" + value: + name_expr + identifier: identifier "y" diff --git a/unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.swift b/unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.swift new file mode 100644 index 000000000000..2e6224b95019 --- /dev/null +++ b/unified/extractor/tests/corpus/swift/variables/binding-modifier-does-not-leak-into-initializer.swift @@ -0,0 +1,4 @@ +let x = switch y { +case someConstant: 1 +default: 2 +} diff --git a/unified/extractor/tests/corpus/swift/variables/tuple-destructuring-binding.output b/unified/extractor/tests/corpus/swift/variables/tuple-destructuring-binding.output index a0a45eb6d22a..5709911171c1 100644 --- a/unified/extractor/tests/corpus/swift/variables/tuple-destructuring-binding.output +++ b/unified/extractor/tests/corpus/swift/variables/tuple-destructuring-binding.output @@ -38,16 +38,12 @@ top_level element: pattern_element pattern: - expr_equality_pattern - expr: - name_expr - identifier: identifier "a" + name_pattern + identifier: identifier "a" pattern_element pattern: - expr_equality_pattern - expr: - name_expr - identifier: identifier "b" + name_pattern + identifier: identifier "b" value: name_expr identifier: identifier "pair" diff --git a/unified/ql/lib/codeql/unified/Comments.qll b/unified/ql/lib/codeql/unified/Comments.qll deleted file mode 100644 index e839af2dbee2..000000000000 --- a/unified/ql/lib/codeql/unified/Comments.qll +++ /dev/null @@ -1,18 +0,0 @@ -/** Provides classes for working with comments. */ - -private import unified - -/** - * A comment appearing in the source code. - */ -class Comment extends TriviaToken { - // At the moment, comments are the only type trivia token we extract - /** - * Gets the text inside this comment, not counting the delimeters. - */ - string getCommentText() { - result = this.getValue().regexpCapture("//(.*)", 1) - or - result = this.getValue().regexpCapture("(?s)/\\*(.*)\\*/", 1) - } -} diff --git a/unified/ql/lib/codeql/unified/internal/AstExtra.qll b/unified/ql/lib/codeql/unified/internal/AstExtra.qll new file mode 100644 index 000000000000..e14043dc46d2 --- /dev/null +++ b/unified/ql/lib/codeql/unified/internal/AstExtra.qll @@ -0,0 +1,55 @@ +/** + * Provides additional AST-like classes outside the generated tree-sitter classes. + */ + +private import unified + +module Public { + /** + * A logical 'and' expression with short-circuiting. + */ + class LogicalAndExpr extends BinaryExpr { + LogicalAndExpr() { this.getOperator().getValue() = "&&" } + + Expr getAnOperand() { result = [this.getLeft(), this.getRight()] } + } + + /** + * Declaration of a local or top-level variable. + */ + class LocalVariableDeclaration extends VariableDeclaration { + private Block block; + + LocalVariableDeclaration() { this = block.getStmt(_) } + + /** Gets the block in which this variable is declared. */ + Block getDeclaringBlock() { result = block } + } + + /** + * Declaration of a local or top-level function. + */ + class LocalFunctionDeclaration extends FunctionDeclaration { + private Block block; + + LocalFunctionDeclaration() { this = block.getStmt(_) } + + /** Gets the block in which this function is declared. */ + Block getDeclaringBlock() { result = block } + } + + /** + * A comment appearing in the source code. + */ + class Comment extends TriviaToken { + // At the moment, comments are the only type trivia token we extract + /** + * Gets the text inside this comment, not counting the delimiters. + */ + string getCommentText() { + result = this.getValue().regexpCapture("//(.*)", 1) + or + result = this.getValue().regexpCapture("(?s)/\\*(.*)\\*/", 1) + } + } +} diff --git a/unified/ql/lib/codeql/unified/internal/Variables.qll b/unified/ql/lib/codeql/unified/internal/Variables.qll new file mode 100644 index 000000000000..97629f2c57ff --- /dev/null +++ b/unified/ql/lib/codeql/unified/internal/Variables.qll @@ -0,0 +1,295 @@ +/** + * Provides classes for reasoning about lexically scoped variables and references to these. + */ + +private import unified +private import unified as U +private import codeql.namebinding.LocalNameBinding + +private module LocalNameBindingInput implements LocalNameBindingInputSig { + class AstNode = U::AstNode; + + private class LogicalAndRoot extends LogicalAndExpr { + LogicalAndRoot() { not this = any(LogicalAndExpr e).getAnOperand() } + + private Expr getDescendant(string path) { + path = "" and result = this + or + exists(LogicalAndExpr mid, string midPath | mid = this.getDescendant(midPath) | + result = mid.getLeft() and path = midPath + "A" + or + result = mid.getRight() and path = midPath + "B" + ) + } + + Expr getNthLeaf(int n) { + result = + rank[n](Expr e, string path | + e = this.getDescendant(path) and not e instanceof LogicalAndExpr + | + e order by path + ) + } + + Expr getLastLeaf() { result = max(int n | | this.getNthLeaf(n) order by n) } + } + + private class BlockWithGuardStmts extends Block { + BlockWithGuardStmts() { this.getStmt(_) instanceof GuardIfStmt } + + AstNode getTranslatedChild(int n) { + result = + rank[n](AstNode stmt, AstNode child, int i1, int i2 | + stmt = this.getStmt(i1) and + ( + child = stmt.(GuardIfStmt).getCondition().(LogicalAndRoot).getNthLeaf(i2) + or + child = stmt.(GuardIfStmt).getCondition() and + not child instanceof LogicalAndExpr and + i2 = 0 + or + child = stmt.(GuardIfStmt).getElse() and + i2 = -1 // place before condition so its variables are not seen + or + not stmt instanceof GuardIfStmt and + child = stmt and + i2 = 0 + ) + | + child order by i1, i2 + ) + } + } + + private AstNode getChild1(AstNode n, int index) { + result = n.(Block).getStmt(index) and + not n instanceof BlockWithGuardStmts + or + result = n.(BlockWithGuardStmts).getTranslatedChild(index) + or + result = n.(LogicalAndRoot).getNthLeaf(index) + or + exists(PatternGuardExpr guard | n = guard | + index = 0 and result = guard.getPattern() + or + index = 1 and result = guard.getValue() + ) + or + exists(IfExpr expr | n = expr | + index = 0 and result = expr.getCondition() + or + index = 1 and result = expr.getThen() + or + index = 2 and result = expr.getElse() + ) + or + exists(VariableDeclaration decl | n = decl | + index = 0 and result = decl.getPattern() + or + index = 1 and result = decl.getType() + or + index = 2 and result = decl.getValue() + ) + } + + AstNode getChild(AstNode n, int index) { + result = getChild1(n, index) + or + not exists(getChild1(n, _)) and + not n instanceof LogicalAndExpr and // also ignore intermediate nodes within a 'logical and' tree + not n instanceof GuardIfStmt and + index = 0 and + result = n.getAFieldOrChild() + } + + abstract class Conditional extends AstNode { + /** Gets the condition of this conditional. */ + abstract AstNode getCondition(); + + /** Gets the then-branch of this conditional. */ + abstract AstNode getThen(); + + /** Gets the else-branch of this conditional. */ + abstract AstNode getElse(); + } + + private class IfExprConditional extends Conditional instanceof IfExpr { + override AstNode getCondition() { result = IfExpr.super.getCondition() } + + override AstNode getThen() { result = IfExpr.super.getThen() } + + override AstNode getElse() { result = IfExpr.super.getElse() } + } + + private class WhileStmtConditional extends Conditional instanceof WhileStmt { + override AstNode getCondition() { result = WhileStmt.super.getCondition() } + + override AstNode getThen() { result = WhileStmt.super.getBody() } + + override AstNode getElse() { none() } + } + + abstract class SiblingShadowingDecl extends AstNode { + /** + * Gets the right-hand side of this declaration. + * + * Any local declared in the left-hand side of this declaration is _not_ in scope + * in the right-hand side. + */ + abstract AstNode getRhs(); + + /** + * Gets the else-branch of this declaration, if any. + * + * Any local declared in the left-hand side of this declaration is _not_ in scope + * in the else-branch. + */ + abstract AstNode getElse(); + } + + private class LocalVariableDeclarationSiblingShadowingDecl extends SiblingShadowingDecl instanceof LocalVariableDeclaration + { + override AstNode getRhs() { result = LocalVariableDeclaration.super.getValue() } + + override AstNode getElse() { none() } + } + + private class PatternGuardExprSiblingShadowingDecl extends SiblingShadowingDecl instanceof PatternGuardExpr + { + override AstNode getRhs() { result = PatternGuardExpr.super.getValue() } + + override AstNode getElse() { none() } + } + + private predicate bindingContext(AstNode pattern, AstNode scope) { + exists(LocalVariableDeclaration decl | + scope = decl and // LocalVariableDeclaration is a ShadowingSiblingDecl, it must use itself as the scope + pattern = decl.getPattern() + ) + or + exists(LocalFunctionDeclaration func | + scope = func.getDeclaringBlock() and + pattern = func.getName() + ) + or + exists(Parameter param | + scope = param.getParent() and // TODO: add SourceCallable and use .getParameter() instead + pattern = param.getPattern() + ) + or + exists(CatchClause catch | + scope = catch and // ensure both 'body' and 'guard' clause 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?) + pattern = case.getPattern() + ) + or + exists(ForEachStmt stmt | + scope = stmt and // ensure both 'body' and 'guard' are in scope + pattern = stmt.getPattern() + ) + or + exists(TuplePattern pat | + bindingContext(pat, scope) and + pattern = pat.getElement(_).getPattern() + ) + or + exists(ConstructorPattern pat | + bindingContext(pat, scope) and + pattern = pat.getElement(_).getPattern() + ) + or + exists(OrPattern pat | + bindingContext(pat, scope) and + pattern = pat.getPattern(_) + ) + or + exists(PatternGuardExpr expr | + pattern = expr.getPattern() and + scope = expr + ) + } + + /** + * Gets the nearest enclosing `OrPattern` to which variable bindings in `p` should be lifted. + * + * To ensure that `case .foo(let x), .bar(let x)` result in a single definition for + * the variable `x`, the `OrPattern` becomes the `definingNode` for `x`. + * + * At the moment no further checks are needed since the Swift compiler enforces that + * variable names bound in any branch are bound in all branches. + */ + private OrPattern getEnclosingOrPattern(Pattern p) { + p = result.getPattern(_) + or + exists(Pattern parent | result = getEnclosingOrPattern(parent) | + p = parent.(ConstructorPattern).getElement(_).getPattern() + or + p = parent.(TuplePattern).getElement(_).getPattern() + ) + } + + predicate declInScope(AstNode definingNode, string name, AstNode scope) { + exists(AstNode pattern | + bindingContext(pattern, scope) and + ( + pattern.(NamePattern).getIdentifier().getValue() = name + or + pattern.(Identifier).getValue() = name + ) and + ( + definingNode = getEnclosingOrPattern(pattern) + or + not exists(getEnclosingOrPattern(pattern)) and + definingNode = pattern + ) + ) + } + + predicate implicitDeclInScope(string name, AstNode scope) { + none() + // TODO: self + } + + predicate accessCand(AstNode n, string name) { + n.(NameExpr).getIdentifier().getValue() = name + or + n.(NamePattern).getIdentifier().getValue() = name + or + n = any(LocalFunctionDeclaration f).getName() and + n.(Identifier).getValue() = name + } + + predicate lookupStartsAt(AstNode n, AstNode scope) { none() } +} + +module LocalNameBindingOutput = LocalNameBinding; + +module Public { + /** + * A local variable. + */ + class Variable extends LocalNameBindingOutput::Local { + VariableAccess getAnAccess() { result.getVariable() = this } + } + + /** + * An AST node that is a reference to a local variable. + */ + class VariableAccess extends AstNode instanceof LocalNameBindingOutput::LocalAccess { + Variable getVariable() { result = super.getLocal() } + + Identifier getIdentifier() { + result = this.(NameExpr).getIdentifier() + or + result = this.(NamePattern).getIdentifier() + or + result = this + } + + string getName() { result = this.getIdentifier().getValue() } + } +} diff --git a/unified/ql/lib/codeql/unified/internal/dev/debugScopeGraph.ql b/unified/ql/lib/codeql/unified/internal/dev/debugScopeGraph.ql new file mode 100644 index 000000000000..2f9376b1732e --- /dev/null +++ b/unified/ql/lib/codeql/unified/internal/dev/debugScopeGraph.ql @@ -0,0 +1,19 @@ +/** + * @name Debug scope graph + * @description Renders the graph used to perform local variable lookups + * @kind graph + * @id unified/debug-scope-graph + */ + +private import unified +private import codeql.unified.internal.Variables + +/** + * Holds if `node` should be shown in the graph. + */ +predicate relevantNode(AstNode node) { + // Match an ancestor node by location so its whole subtree is shown. + node.getParent*().getLocation().toString().matches("%test.swift@227:%") +} + +import LocalNameBindingOutput::DebugScopeGraph diff --git a/unified/ql/lib/qlpack.yml b/unified/ql/lib/qlpack.yml index 896bf37ac5e5..d67af16271b6 100644 --- a/unified/ql/lib/qlpack.yml +++ b/unified/ql/lib/qlpack.yml @@ -7,5 +7,6 @@ library: true upgrades: upgrades dependencies: codeql/util: ${workspace} + codeql/namebinding: ${workspace} warnOnImplicitThis: true compileForOverlayEval: true diff --git a/unified/ql/lib/unified.qll b/unified/ql/lib/unified.qll index 4f7387ef8f1c..477ac22f3661 100644 --- a/unified/ql/lib/unified.qll +++ b/unified/ql/lib/unified.qll @@ -5,4 +5,5 @@ import codeql.Locations import codeql.files.FileSystem import codeql.unified.Ast::Unified -import codeql.unified.Comments +import codeql.unified.internal.AstExtra::Public +import codeql.unified.internal.Variables::Public diff --git a/unified/ql/test/library-tests/variables/test.swift b/unified/ql/test/library-tests/variables/test.swift new file mode 100644 index 000000000000..0e6c23782b0d --- /dev/null +++ b/unified/ql/test/library-tests/variables/test.swift @@ -0,0 +1,351 @@ +func t1() -> Int { + let x = 42 // name=x1 + return x // $ access=x1 +} + +func t2() -> Int { + let x = 42 // name=x1 + + if case let x = x + 10 { // $ access=x1 // name=x2 + print(x) // $ access=x2 + } + print(x) // $ access=x1 +} + +func t3() -> Int { + guard let x = foo() else { // name=x1 + return 0 + } + print(x) // $ access=x1 +} + +// Function parameters +func t4(x: Int) -> Int { // name=x1 + return x // $ access=x1 +} + +// Multiple parameters +func t5(x: Int, // name=x1 + y: Int) -> Int { // name=y1 + let z = x + // $ access=x1 // name=z1 + y // $ access=y1 + return z // $ access=z1 +} + +// Parameter shadowed by local variable +func t6(x: Int) -> Int { // name=x1 + let x = x * 2 // $ access=x1 // name=x2 + return x // $ access=x2 +} + +// Nested blocks +func t7() { + let x = 1; // name=x1 + do { + print(x) // $ access=x1 + let x = 2 // name=x2 + print(x) // $ access=x2 + } + print(x) // $ access=x1 +} + +// For-in loop +func t8() { + let array = [1, 2, 3] // name=array1 + for x in array { // $ access=array1 // name=x1 + print(x) // $ access=x1 + } +} + +// For-in loop with shadowing +func t9() { + let x = 0 // name=x1 + let array = [1, 2, 3] // name=array1 + for x in array { // $ access=array1 // name=x2 + print(x) // $ access=x2 + } + print(x) // $ access=x1 +} + +// Switch statement with case bindings +func t10(value: Int) { // name=value1 + switch value { // $ access=value1 + case let x: // name=x1 + print(x) // $ access=x1 + default: + break + } +} + +// Switch with multiple cases +func t11(value: Int) { // name=value1 + switch value { // $ access=value1 + case let x where x > 0: // name=x1 + print(x) // $ access=x1 + case let x: // name=x2 + print(x) // $ access=x2 + } +} + +// Tuple unpacking +func t12() { + let tuple = (1, 2) // name=tuple1 + let (x, // name=x1 + y) = tuple // $ access=tuple1 // name=y1 + print(x) // $ access=x1 + print(y) // $ access=y1 +} + +// Tuple unpacking with underscore +func t13() { + let tuple = (1, 2, 3) // name=tuple1 + let (x, // name=x1 + _, + y) = tuple // $ access=tuple1 // name=y1 + print(x) // $ access=x1 + print(y) // $ access=y1 +} + +// Optional binding (if let) +func t14(optional: Int?) { // name=optional1 + if let x = optional { // $ access=optional1 // name=x1 + print(x) // $ access=x1 + } +} + +// Multiple optional bindings +func t15(opt1: Int?, // name=opt11 + opt2: String?) { // name=opt21 + if let x = opt1, // $ access=opt11 // name=x1 + let y = opt2 { // $ access=opt21 // name=y1 + print(x) // $ access=x1 + print(y) // $ access=y1 + } +} + +// Do-catch blocks +func t16() throws { + do { + let x = try foo() // name=x1 + print(x) // $ access=x1 + } catch let error { // name=error1 + print(error) // $ access=error1 + } +} + +// Closure captures +func t17() { + let x = 1 // name=x1 + let closure = { // name=closure1 + print(x) // $ access=x1 + } + closure() // $ access=closure1 +} + +// Closure with parameter shadowing +func t18() { + let x = 1 // name=x1 + let closure = + { (x: Int) -> Void in // name=x2 + print(x) // $ access=x2 + } + closure(2) // $ access=closure + print(x) // $ access=x1 +} + +// While loop +func t19() { + var x = 0 // name=x1 + while x < 10 { // $ access=x1 + x = x + 1 // $ access=x1 + print(x) // $ access=x1 + } +} + +// Repeat-while loop +func t20() { + var x = 0 // name=x1 + repeat { + x = x + 1 // $ access=x1 + print(x) // $ access=x1 + } while x < 10 // $ access=x1 +} + +// Property shadowing (var) +func t21() { + var x = 1 // name=x1 + var x = 2 // name=x2 + print(x) // $ access=x2 +} + +// Nested functions +func t22() { + let x = 1 // name=x1 + func inner() { // name=inner1 + let x = 2 // name=x2 + print(x) // $ access=x2 + } + inner() // $ access=inner1 + print(x) // $ access=x1 +} + +// Three levels of shadowing +func t23() { + let x = 1 // name=x1 + { + let x = 2 // name=x2 + { + let x = 3 // name=x3 + print(x) // $ access=x3 + } + print(x) // $ access=x2 + } + print(x) // $ access=x1 +} + +// If-let followed by regular if +func t24(optional: Int?) { // name=optional1 + if let x = optional { // $ access=optional1 // name=x1 + print(x) // $ access=x1 + } + if true { + let x = 5 // name=x2 + print(x) // $ access=x2 + } +} + +// Switch with same variable name in different cases +func t25(value: Int) { // name=value1 + switch value { // $ access=value1 + case let x: // name=x1 + print(x) // $ access=x1 + let x = 10 // name=x2 + print(x) // $ access=x2 + } +} + +func t26() -> Int { + let x = 42 // name=x1 + guard let x = foo() else { // name=x2 + return x // $ access=x1 + } + print(x) // $ access=x2 +} + +// if with multiple conditions, mixing boolean and optional binding +func t27(opt: Int?) { // name=opt1 + if opt != nil, // $ access=opt1 + let x = opt { // $ access=opt1 + print(x) // $ access=x + } +} + +// if with multiple let bindings and a boolean condition +func t28(a: Int?, b: Int?) { + if let x = a, // $ access=a + let y = b, // $ access=b + x < y { // $ access=x access=y + print(x) // $ access=x + print(y) // $ access=y + } +} + +// if with multiple conditions where a later binding shadows an outer variable +func t29(opt: Int?) { // name=opt1 + let x = 0 // name=x1 + if opt != nil, // $ access=opt1 + let x = opt { // $ access=opt1 + print(x) // $ access=x + } + print(x) // $ access=x1 +} + +// guard with multiple conditions, mixing boolean and optional binding +func t30(opt: Int?) { // name=opt1 + guard opt != nil, // $ access=opt1 + let x = opt else { // $ access=opt1 + return + } + print(x) // $ access=x +} + +// guard with multiple let bindings and a boolean condition +func t31(a: Int?, b: Int?) { + guard let x = a, // $ access=a + let y = b, // $ access=b + x < y else { // $ access=x access=y + return + } + print(x) // $ access=x + print(y) // $ access=y +} + +// guard with multiple conditions where bound variable is used in later condition +func t32(opt: Int?) { // name=opt1 + guard let x = opt, // $ access=opt1 + x > 0 else { // $ access=x + return + } + print(x) // $ access=x +} + +func t33() { + let x = 1 // name=x1 + guard x > 0, // $ access=x1 + let x = x, // $ access=x1 // name=x2 + x > 0 else { // $ access=x2 + return + } + print(x) // $ access=x2 +} + +func t34() { + let x = 1 // name=x1 + if x > 0, // $ access=x1 + let x = x, // $ access=x1 // name=x2 + x > 0 { // $ access=x2 + print(x) // $ access=x2 + } +} + +// While-let optional binding +func t35(optional: Int?) { // name=optional1 + while let x = optional { // $ access=optional1 // name=x1 + print(x) // $ access=x1 + } +} + +// While with a sequence of variable bindings in the condition +func t36(a: Int?, b: Int?) { + while let x = a, // $ access=a + let y = b, // $ access=b + x < y { // $ access=x access=y + print(x) // $ access=x + print(y) // $ access=y + } +} + +// While-let with a sequence of shadowing variable declarations +func t37() { + let x = 1 // name=x1 + while let x = x, // $ access=x1 // name=x2 + let x = x, // $ access=x2 // name=x3 + x > 0 { // $ access=x3 + print(x) // $ access=x3 + } +} + +enum E38 { + case a(Int) + case b(Int) +} + +// Switch with a multi-pattern case that binds 'x' in each pattern +// Note: the testing framework does not make it possible to name the 'x' variable in this case. +func t38(value: E38) { + switch value { // $ access=value + case .a(let x), // $ access=x + .b(let x): // $ access=x + print(x) // $ access=x + } +} diff --git a/unified/ql/test/library-tests/variables/variables.expected b/unified/ql/test/library-tests/variables/variables.expected new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/unified/ql/test/library-tests/variables/variables.ql b/unified/ql/test/library-tests/variables/variables.ql new file mode 100644 index 000000000000..146da296ae74 --- /dev/null +++ b/unified/ql/test/library-tests/variables/variables.ql @@ -0,0 +1,51 @@ +import unified +import utils.test.InlineExpectationsTest + +/** Holds if a comment with `text` appears at `filepath:line`, excluding the text in a `$` section. */ +predicate plainCommentAt(string filepath, int line, string text) { + exists(Comment comment | + comment.getLocation().hasLocationInfo(filepath, line, _, _, _) and + text = comment.getCommentText().regexpReplaceAll("\\$([^/]|/[^/])*", "") + ) +} + +/** Holds if a `key=value` comment appears on `filepath:line` (not in the `$` section). */ +predicate keyValueCommentAt(string filepath, int line, string key, string value) { + exists(string text, string regexp, string match | + plainCommentAt(filepath, line, text) and + regexp = "(\\w+)=(\\w+)" and + match = text.regexpFind(regexp, _, _) and + key = match.regexpCapture(regexp, 1) and + value = match.regexpCapture(regexp, 2) + ) +} + +module VariableAccessTest implements TestSig { + string getARelevantTag() { result = "access" } + + private predicate declAt(Variable v, string filepath, int line) { + v.getLocation().hasLocationInfo(filepath, _, _, line, _) + } + + private predicate decl(Variable v, string alias) { + exists(string filepath, int line | declAt(v, filepath, line) | + keyValueCommentAt(filepath, line, "name", alias) + or + not keyValueCommentAt(filepath, line, "name", _) and + alias = v.getName() + ) + } + + predicate hasActualResult(Location location, string element, string tag, string value) { + exists(VariableAccess va, Variable v | + v = va.getVariable() and + not va = v.getDefiningNode() and + location = va.getLocation() and + element = va.toString() and + decl(v, value) and + tag = "access" + ) + } +} + +import MakeTest