From 311a0b6b20f19a61cb527e714b5d65d9d0126fde Mon Sep 17 00:00:00 2001 From: Tom Hvitved Date: Tue, 1 Dec 2020 10:24:33 +0100 Subject: [PATCH] Mark more AST predicates as `final` --- ql/src/codeql_ruby/ast/Method.qll | 26 +++---- ql/src/codeql_ruby/ast/Parameter.qll | 66 ++++++++--------- ql/src/codeql_ruby/ast/Pattern.qll | 8 +- ql/src/codeql_ruby/ast/Variable.qll | 61 ++-------------- ql/src/codeql_ruby/ast/internal/Variable.qll | 73 ++++++++++++++++++- .../library-tests/ast/params/params.expected | 24 +++--- .../variables/parameter.expected | 2 +- 7 files changed, 136 insertions(+), 124 deletions(-) diff --git a/ql/src/codeql_ruby/ast/Method.qll b/ql/src/codeql_ruby/ast/Method.qll index 80849be07d7..7581d92ff39 100644 --- a/ql/src/codeql_ruby/ast/Method.qll +++ b/ql/src/codeql_ruby/ast/Method.qll @@ -20,12 +20,12 @@ class Callable extends AstNode { class Method extends Callable, @method { final override Generated::Method generated; - override string describeQlClass() { result = "Method" } + final override string describeQlClass() { result = "Method" } - override string toString() { result = this.getName() } + final override string toString() { result = this.getName() } /** Gets the name of this method. */ - string getName() { + final string getName() { result = generated.getName().(Generated::Token).getValue() or // TODO: use hand-written Symbol class result = generated.getName().(Generated::Symbol).toString() or @@ -42,19 +42,19 @@ class Method extends Callable, @method { * end * ``` */ - predicate isSetter() { generated.getName() instanceof Generated::Setter } + final predicate isSetter() { generated.getName() instanceof Generated::Setter } } /** A singleton method. */ class SingletonMethod extends Callable, @singleton_method { final override Generated::SingletonMethod generated; - override string describeQlClass() { result = "SingletonMethod" } + final override string describeQlClass() { result = "SingletonMethod" } - override string toString() { result = this.getName() } + final override string toString() { result = this.getName() } /** Gets the name of this method. */ - string getName() { + final string getName() { result = generated.getName().(Generated::Token).getValue() or // TODO: use hand-written Symbol class result = generated.getName().(Generated::Symbol).toString() or @@ -71,9 +71,9 @@ class SingletonMethod extends Callable, @singleton_method { class Lambda extends Callable, @lambda { final override Generated::Lambda generated; - override string describeQlClass() { result = "Lambda" } + final override string describeQlClass() { result = "Lambda" } - override string toString() { result = "-> { ... }" } + final override string toString() { result = "-> { ... }" } } /** A block. */ @@ -85,9 +85,9 @@ class Block extends AstNode, Callable { class DoBlock extends Block, @do_block { final override Generated::DoBlock generated; - override string describeQlClass() { result = "DoBlock" } + final override string describeQlClass() { result = "DoBlock" } - override string toString() { result = "| ... |" } + final override string toString() { result = "| ... |" } } /** @@ -99,7 +99,7 @@ class DoBlock extends Block, @do_block { class BraceBlock extends Block, @block { final override Generated::Block generated; - override string describeQlClass() { result = "BraceBlock" } + final override string describeQlClass() { result = "BraceBlock" } - override string toString() { result = "{ ... }" } + final override string toString() { result = "{ ... }" } } diff --git a/ql/src/codeql_ruby/ast/Parameter.qll b/ql/src/codeql_ruby/ast/Parameter.qll index af7748d504a..7f8f79175cf 100644 --- a/ql/src/codeql_ruby/ast/Parameter.qll +++ b/ql/src/codeql_ruby/ast/Parameter.qll @@ -17,10 +17,10 @@ class Parameter extends AstNode { } /** Gets the callable that this parameter belongs to. */ - Callable getCallable() { result.getAParameter() = this } + final Callable getCallable() { result.getAParameter() = this } /** Gets the zero-based position of this parameter. */ - int getPosition() { result = pos } + final int getPosition() { result = pos } } /** @@ -28,17 +28,11 @@ class Parameter extends AstNode { * * This includes both simple parameters and tuple parameters. */ -class PatternParameter extends Parameter, Pattern { - override string toString() { result = Pattern.super.toString() } - - override Location getLocation() { result = Pattern.super.getLocation() } -} +class PatternParameter extends Parameter, Pattern { } /** A parameter defined using a tuple pattern. */ class TuplePatternParameter extends PatternParameter, TuplePattern { - override string toString() { result = TuplePattern.super.toString() } - - override string describeQlClass() { result = "TuplePatternParameter" } + final override string describeQlClass() { result = "TuplePatternParameter" } } /** A named parameter. */ @@ -57,13 +51,13 @@ class NamedParameter extends Parameter { /** A simple (normal) parameter. */ class SimpleParameter extends NamedParameter, PatternParameter, VariablePattern { - override string getName() { result = VariablePattern.super.getName() } + final override string getName() { result = this.getVariableName() } final override Variable getVariable() { result = TLocalVariable(_, _, this) } - override string describeQlClass() { result = "SimpleParameter" } + final override string describeQlClass() { result = "SimpleParameter" } - override string toString() { result = this.getName() } + final override string toString() { result = this.getName() } } /** @@ -75,15 +69,15 @@ class SimpleParameter extends NamedParameter, PatternParameter, VariablePattern * ``` */ class BlockParameter extends @block_parameter, NamedParameter { - override Generated::BlockParameter generated; + final override Generated::BlockParameter generated; final override Variable getVariable() { result = TLocalVariable(_, _, generated.getName()) } - override string describeQlClass() { result = "BlockParameter" } + final override string describeQlClass() { result = "BlockParameter" } - override string toString() { result = "&" + this.getName() } + final override string toString() { result = "&" + this.getName() } - override string getName() { result = generated.getName().getValue() } + final override string getName() { result = generated.getName().getValue() } } /** @@ -96,15 +90,15 @@ class BlockParameter extends @block_parameter, NamedParameter { * ``` */ class HashSplatParameter extends @hash_splat_parameter, NamedParameter { - override Generated::HashSplatParameter generated; + final override Generated::HashSplatParameter generated; final override Variable getVariable() { result = TLocalVariable(_, _, generated.getName()) } - override string describeQlClass() { result = "HashSplatParameter" } + final override string describeQlClass() { result = "HashSplatParameter" } - override string toString() { result = "**" + this.getName() } + final override string toString() { result = "**" + this.getName() } - override string getName() { result = generated.getName().getValue() } + final override string getName() { result = generated.getName().getValue() } } /** @@ -119,13 +113,13 @@ class HashSplatParameter extends @hash_splat_parameter, NamedParameter { * ``` */ class KeywordParameter extends @keyword_parameter, NamedParameter { - override Generated::KeywordParameter generated; + final override Generated::KeywordParameter generated; final override Variable getVariable() { result = TLocalVariable(_, _, generated.getName()) } - override string describeQlClass() { result = "KeywordParameter" } + final override string describeQlClass() { result = "KeywordParameter" } - override string getName() { result = generated.getName().getValue() } + final override string getName() { result = generated.getName().getValue() } /** * Gets the default value, i.e. the value assigned to the parameter when one @@ -133,15 +127,15 @@ class KeywordParameter extends @keyword_parameter, NamedParameter { * have a default value, this predicate has no result. * TODO: better return type (Expr?) */ - AstNode getDefaultValue() { result = generated.getValue() } + final AstNode getDefaultValue() { result = generated.getValue() } /** * Holds if the parameter is optional. That is, there is a default value that * is used when the caller omits this parameter. */ - predicate isOptional() { exists(this.getDefaultValue()) } + final predicate isOptional() { exists(this.getDefaultValue()) } - override string toString() { result = this.getName() } + final override string toString() { result = this.getName() } } /** @@ -154,22 +148,22 @@ class KeywordParameter extends @keyword_parameter, NamedParameter { * ``` */ class OptionalParameter extends @optional_parameter, NamedParameter { - override Generated::OptionalParameter generated; + final override Generated::OptionalParameter generated; final override Variable getVariable() { result = TLocalVariable(_, _, generated.getName()) } - override string describeQlClass() { result = "OptionalParameter" } + final override string describeQlClass() { result = "OptionalParameter" } - override string toString() { result = this.getName() } + final override string toString() { result = this.getName() } - override string getName() { result = generated.getName().getValue() } + final override string getName() { result = generated.getName().getValue() } /** * Gets the default value, i.e. the value assigned to the parameter when one * is not provided by the caller. * TODO: better return type (Expr?) */ - AstNode getDefaultValue() { result = generated.getValue() } + final AstNode getDefaultValue() { result = generated.getValue() } } /** @@ -181,13 +175,13 @@ class OptionalParameter extends @optional_parameter, NamedParameter { * ``` */ class SplatParameter extends @splat_parameter, NamedParameter { - override Generated::SplatParameter generated; + final override Generated::SplatParameter generated; final override Variable getVariable() { result = TLocalVariable(_, _, generated.getName()) } - override string describeQlClass() { result = "SplatParameter" } + final override string describeQlClass() { result = "SplatParameter" } - override string toString() { result = this.getName() } + final override string toString() { result = "*" + this.getName() } - override string getName() { result = generated.getName().getValue() } + final override string getName() { result = generated.getName().getValue() } } diff --git a/ql/src/codeql_ruby/ast/Pattern.qll b/ql/src/codeql_ruby/ast/Pattern.qll index a6464e6be66..7790ef44b36 100644 --- a/ql/src/codeql_ruby/ast/Pattern.qll +++ b/ql/src/codeql_ruby/ast/Pattern.qll @@ -11,15 +11,15 @@ class Pattern extends AstNode { /** A simple variable pattern. */ class VariablePattern extends Pattern { - override Generated::Identifier generated; + final override Generated::Identifier generated; /** Gets the name of the variable used in this pattern. */ - string getName() { result = generated.getValue() } + final string getVariableName() { result = generated.getValue() } /** Gets the variable used in this pattern. */ Variable getVariable() { this = result.getAnAccess() } - override string toString() { result = this.getName() } + override string toString() { result = this.getVariableName() } } /** @@ -36,5 +36,5 @@ class TuplePattern extends Pattern { /** Gets a sub pattern in this tuple pattern. */ final Pattern getAnElement() { result = this.getElement(_) } - override string toString() { result = "(..., ...)" } + final override string toString() { result = "(..., ...)" } } diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll index 9a3acce53da..d55016797b8 100644 --- a/ql/src/codeql_ruby/ast/Variable.qll +++ b/ql/src/codeql_ruby/ast/Variable.qll @@ -8,10 +8,10 @@ private import internal.Variable /** A scope in which variables can be declared. */ class VariableScope extends TScope { /** Gets a textual representation of this element. */ - string toString() { none() } + final string toString() { result = this.(VariableScopeRange).toString() } /** Gets the program element this scope is associated with, if any. */ - AstNode getScopeElement() { none() } + final AstNode getScopeElement() { result = this.(VariableScopeRange).getScopeElement() } /** Gets the location of the program element this scope is associated with. */ final Location getLocation() { result = getScopeElement().getLocation() } @@ -26,60 +26,19 @@ class VariableScope extends TScope { } } -/** A top-level scope. */ -class TopLevelScope extends VariableScope, TTopLevelScope { - final override string toString() { result = "top-level scope" } - - final override AstNode getScopeElement() { TTopLevelScope(result) = this } -} - -/** A module scope. */ -class ModuleScope extends VariableScope, TModuleScope { - final override string toString() { result = "module scope" } - - final override AstNode getScopeElement() { TModuleScope(result) = this } -} - -/** A class scope. */ -class ClassScope extends VariableScope, TClassScope { - final override string toString() { result = "class scope" } - - final override AstNode getScopeElement() { TClassScope(result) = this } -} - -/** A callable scope. */ -class CallableScope extends VariableScope, TCallableScope { - private Callable c; - - CallableScope() { this = TCallableScope(c) } - - final override string toString() { - (c instanceof Method or c instanceof SingletonMethod) and - result = "method scope" - or - c instanceof Lambda and - result = "lambda scope" - or - c instanceof Block and - result = "block scope" - } - - final override Callable getScopeElement() { TCallableScope(result) = this } -} - /** A variable declared in a scope. */ class Variable extends TVariable { /** Gets the name of this variable. */ - string getName() { none() } + final string getName() { result = this.(VariableRange).getName() } /** Gets a textual representation of this variable. */ final string toString() { result = this.getName() } /** Gets the location of this variable. */ - Location getLocation() { none() } + final Location getLocation() { result = this.(VariableRange).getLocation() } /** Gets the scope this variable is declared in. */ - VariableScope getDeclaringScope() { none() } + final VariableScope getDeclaringScope() { result = this.(VariableRange).getDeclaringScope() } /** Gets an access to this variable. */ VariableAccess getAnAccess() { result.getVariable() = this } @@ -93,12 +52,6 @@ class LocalVariable extends Variable { LocalVariable() { this = TLocalVariable(scope, name, i) } - final override string getName() { result = name } - - final override Location getLocation() { result = i.getLocation() } - - final override VariableScope getDeclaringScope() { result = scope } - final override LocalVariableAccess getAnAccess() { result.getVariable() = this } } @@ -112,7 +65,7 @@ class VariableAccess extends AstNode, @token_identifier { /** Gets the variable this identifier refers to. */ Variable getVariable() { result = variable } - override string toString() { result = variable.getName() } + final override string toString() { result = variable.getName() } } /** An access to a local variable. */ @@ -120,6 +73,4 @@ class LocalVariableAccess extends VariableAccess { override LocalVariable variable; override LocalVariable getVariable() { result = variable } - - override string toString() { result = variable.getName() } } diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll index c0975cdad29..c7bbf3d961d 100644 --- a/ql/src/codeql_ruby/ast/internal/Variable.qll +++ b/ql/src/codeql_ruby/ast/internal/Variable.qll @@ -13,7 +13,7 @@ private VariableScope enclosingScope(Generated::AstNode node) { result.getScopeElement() = parent*(node.getParent()) } -private predicate parameterAssignment(CallableScope scope, string name, Generated::Identifier i) { +private predicate parameterAssignment(CallableScopeRange scope, string name, Generated::Identifier i) { assignment(i, true) and scope = enclosingScope(i) and name = i.getValue() @@ -21,7 +21,7 @@ private predicate parameterAssignment(CallableScope scope, string name, Generate /** Holds if `scope` defines `name` in its parameter declaration at `i`. */ private predicate scopeDefinesParameterVariable( - CallableScope scope, string name, Generated::Identifier i + CallableScopeRange scope, string name, Generated::Identifier i ) { parameterAssignment(scope, name, i) and // In case of overlapping parameter names (e.g. `_`), only the first @@ -61,7 +61,7 @@ private predicate strictlyBefore(Location one, Location two) { } /** A scope that may capture outer local variables. */ -private class CapturingScope extends CallableScope { +private class CapturingScope extends VariableScope { CapturingScope() { exists(Callable c | c = this.getScopeElement() | c instanceof Block @@ -140,3 +140,70 @@ private module Cached { } import Cached + +abstract class VariableScopeRange extends TScope { + abstract string toString(); + + abstract AstNode getScopeElement(); +} + +class TopLevelScopeRange extends VariableScopeRange, TTopLevelScope { + override string toString() { result = "top-level scope" } + + override AstNode getScopeElement() { TTopLevelScope(result) = this } +} + +class ModuleScopeRange extends VariableScopeRange, TModuleScope { + override string toString() { result = "module scope" } + + override AstNode getScopeElement() { TModuleScope(result) = this } +} + +class ClassScopeRange extends VariableScopeRange, TClassScope { + override string toString() { result = "class scope" } + + override AstNode getScopeElement() { TClassScope(result) = this } +} + +class CallableScopeRange extends VariableScopeRange, TCallableScope { + private Callable c; + + CallableScopeRange() { this = TCallableScope(c) } + + override string toString() { + (c instanceof Method or c instanceof SingletonMethod) and + result = "method scope" + or + c instanceof Lambda and + result = "lambda scope" + or + c instanceof Block and + result = "block scope" + } + + override Callable getScopeElement() { TCallableScope(result) = this } +} + +class VariableRange extends TVariable { + abstract string getName(); + + string toString() { result = this.getName() } + + abstract Location getLocation(); + + abstract VariableScope getDeclaringScope(); +} + +class LocalVariableRange extends VariableRange { + private VariableScope scope; + private string name; + private Generated::Identifier i; + + LocalVariableRange() { this = TLocalVariable(scope, name, i) } + + final override string getName() { result = name } + + final override Location getLocation() { result = i.getLocation() } + + final override VariableScope getDeclaringScope() { result = scope } +} diff --git a/ql/test/library-tests/ast/params/params.expected b/ql/test/library-tests/ast/params/params.expected index 5f3ac3db14c..9c8c3b511fe 100644 --- a/ql/test/library-tests/ast/params/params.expected +++ b/ql/test/library-tests/ast/params/params.expected @@ -7,13 +7,13 @@ idParams | params.rb:14:11:14:13 | foo | foo | | params.rb:14:16:14:18 | bar | bar | | params.rb:30:23:30:28 | wibble | wibble | -| params.rb:30:31:30:36 | splat | splat | +| params.rb:30:31:30:36 | *splat | splat | | params.rb:30:39:30:52 | **double_splat | double_splat | | params.rb:34:16:34:18 | val | val | -| params.rb:34:21:34:26 | splat | splat | +| params.rb:34:21:34:26 | *splat | splat | | params.rb:34:29:34:42 | **double_splat | double_splat | | params.rb:38:26:38:26 | x | x | -| params.rb:38:29:38:33 | blah | blah | +| params.rb:38:29:38:33 | *blah | blah | | params.rb:38:36:38:43 | **wibble | wibble | | params.rb:41:32:41:32 | x | x | | params.rb:41:35:41:38 | foo | foo | @@ -47,9 +47,9 @@ patternParams | params.rb:25:40:25:54 | (..., ...) | params.rb:25:41:25:45 | third | 0 | | params.rb:25:40:25:54 | (..., ...) | params.rb:25:48:25:53 | fourth | 1 | splatParams -| params.rb:30:31:30:36 | splat | splat | -| params.rb:34:21:34:26 | splat | splat | -| params.rb:38:29:38:33 | blah | blah | +| params.rb:30:31:30:36 | *splat | splat | +| params.rb:34:21:34:26 | *splat | splat | +| params.rb:38:29:38:33 | *blah | blah | hashSplatParams | params.rb:30:39:30:52 | **double_splat | double_splat | | params.rb:34:29:34:42 | **double_splat | double_splat | @@ -73,7 +73,7 @@ paramsInMethods | params.rb:4:1:5:3 | identifier_method_params | 2 | params.rb:4:40:4:42 | baz | SimpleParameter | | params.rb:17:1:18:3 | destructured_method_param | 0 | params.rb:17:31:17:39 | (..., ...) | TuplePatternParameter | | params.rb:30:1:31:3 | method_with_splat | 0 | params.rb:30:23:30:28 | wibble | SimpleParameter | -| params.rb:30:1:31:3 | method_with_splat | 1 | params.rb:30:31:30:36 | splat | SplatParameter | +| params.rb:30:1:31:3 | method_with_splat | 1 | params.rb:30:31:30:36 | *splat | SplatParameter | | params.rb:30:1:31:3 | method_with_splat | 2 | params.rb:30:39:30:52 | **double_splat | HashSplatParameter | | params.rb:41:1:43:3 | method_with_keyword_params | 0 | params.rb:41:32:41:32 | x | SimpleParameter | | params.rb:41:1:43:3 | method_with_keyword_params | 1 | params.rb:41:35:41:38 | foo | KeywordParameter | @@ -88,7 +88,7 @@ paramsInBlocks | params.rb:9:11:11:3 | \| ... \| | 1 | params.rb:9:20:9:24 | value | SimpleParameter | | params.rb:22:12:22:32 | { ... } | 0 | params.rb:22:15:22:20 | (..., ...) | TuplePatternParameter | | params.rb:34:12:35:3 | \| ... \| | 0 | params.rb:34:16:34:18 | val | SimpleParameter | -| params.rb:34:12:35:3 | \| ... \| | 1 | params.rb:34:21:34:26 | splat | SplatParameter | +| params.rb:34:12:35:3 | \| ... \| | 1 | params.rb:34:21:34:26 | *splat | SplatParameter | | params.rb:34:12:35:3 | \| ... \| | 2 | params.rb:34:29:34:42 | **double_splat | HashSplatParameter | | params.rb:49:24:51:3 | \| ... \| | 0 | params.rb:49:28:49:30 | xx | KeywordParameter | | params.rb:49:24:51:3 | \| ... \| | 1 | params.rb:49:33:49:39 | yy | KeywordParameter | @@ -100,7 +100,7 @@ paramsInLambdas | params.rb:25:19:27:1 | -> { ... } | 0 | params.rb:25:23:25:37 | (..., ...) | TuplePatternParameter | | params.rb:25:19:27:1 | -> { ... } | 1 | params.rb:25:40:25:54 | (..., ...) | TuplePatternParameter | | params.rb:38:22:38:47 | -> { ... } | 0 | params.rb:38:26:38:26 | x | SimpleParameter | -| params.rb:38:22:38:47 | -> { ... } | 1 | params.rb:38:29:38:33 | blah | SplatParameter | +| params.rb:38:22:38:47 | -> { ... } | 1 | params.rb:38:29:38:33 | *blah | SplatParameter | | params.rb:38:22:38:47 | -> { ... } | 2 | params.rb:38:36:38:43 | **wibble | HashSplatParameter | | params.rb:53:30:55:1 | -> { ... } | 0 | params.rb:53:34:53:34 | x | SimpleParameter | | params.rb:53:30:55:1 | -> { ... } | 1 | params.rb:53:37:53:38 | y | KeywordParameter | @@ -121,13 +121,13 @@ paramsInLambdas | params.rb:25:23:25:37 | (..., ...) | 0 | TuplePatternParameter | | params.rb:25:40:25:54 | (..., ...) | 1 | TuplePatternParameter | | params.rb:30:23:30:28 | wibble | 0 | SimpleParameter | -| params.rb:30:31:30:36 | splat | 1 | SplatParameter | +| params.rb:30:31:30:36 | *splat | 1 | SplatParameter | | params.rb:30:39:30:52 | **double_splat | 2 | HashSplatParameter | | params.rb:34:16:34:18 | val | 0 | SimpleParameter | -| params.rb:34:21:34:26 | splat | 1 | SplatParameter | +| params.rb:34:21:34:26 | *splat | 1 | SplatParameter | | params.rb:34:29:34:42 | **double_splat | 2 | HashSplatParameter | | params.rb:38:26:38:26 | x | 0 | SimpleParameter | -| params.rb:38:29:38:33 | blah | 1 | SplatParameter | +| params.rb:38:29:38:33 | *blah | 1 | SplatParameter | | params.rb:38:36:38:43 | **wibble | 2 | HashSplatParameter | | params.rb:41:32:41:32 | x | 0 | SimpleParameter | | params.rb:41:35:41:38 | foo | 1 | KeywordParameter | diff --git a/ql/test/library-tests/variables/parameter.expected b/ql/test/library-tests/variables/parameter.expected index 93842339931..0feace65501 100644 --- a/ql/test/library-tests/variables/parameter.expected +++ b/ql/test/library-tests/variables/parameter.expected @@ -5,7 +5,7 @@ parameter | parameters.rb:1:14:1:14 | x | parameters.rb:1:14:1:14 | x | | parameters.rb:1:18:1:18 | y | parameters.rb:1:18:1:18 | y | | parameters.rb:7:17:7:22 | client | parameters.rb:7:17:7:22 | client | -| parameters.rb:7:25:7:31 | pizzas | parameters.rb:7:26:7:31 | pizzas | +| parameters.rb:7:25:7:31 | *pizzas | parameters.rb:7:26:7:31 | pizzas | | parameters.rb:15:15:15:19 | **map | parameters.rb:15:17:15:19 | map | | parameters.rb:16:16:16:18 | key | parameters.rb:16:16:16:18 | key | | parameters.rb:16:21:16:25 | value | parameters.rb:16:21:16:25 | value |