From b04391636d6e4a40e16f74b88a58f59b9c616b68 Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Fri, 18 Dec 2020 13:40:53 +0100
Subject: [PATCH 1/8] Fix qldoc comment

---
 ql/src/codeql_ruby/ast/Variable.qll | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll
index 8b524b504db..0870faa9d76 100644
--- a/ql/src/codeql_ruby/ast/Variable.qll
+++ b/ql/src/codeql_ruby/ast/Variable.qll
@@ -177,7 +177,7 @@ class LocalVariableWriteAccess extends LocalVariableAccess, VariableWriteAccess
 /** An access to a local variable where the value is read. */
 class LocalVariableReadAccess extends LocalVariableAccess, VariableReadAccess { }
 
-/** An access to a local variable. */
+/** An access to a global variable. */
 class GlobalVariableAccess extends VariableAccess, @token_global_variable {
   final override GlobalVariableAccess::Range range;
 

From 184d42efe0edf95bafbc7f86ba7dcdb8849c4abb Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Mon, 21 Dec 2020 19:17:52 +0100
Subject: [PATCH 2/8] Remove unnecessary clause

---
 ql/src/codeql_ruby/ast/internal/Variable.qll | 2 --
 1 file changed, 2 deletions(-)

diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll
index 85e0f996c55..3bbf765aec6 100644
--- a/ql/src/codeql_ruby/ast/internal/Variable.qll
+++ b/ql/src/codeql_ruby/ast/internal/Variable.qll
@@ -20,7 +20,6 @@ private predicate parameterAssignment(
 private predicate scopeDefinesParameterVariable(
   CallableScope::Range scope, string name, Generated::Identifier i
 ) {
-  parameterAssignment(scope, name, i) and
   // In case of overlapping parameter names (e.g. `_`), only the first
   // parameter will give rise to a variable
   i =
@@ -115,7 +114,6 @@ private module Cached {
     TLocalVariable(VariableScope scope, string name, Generated::Identifier i) {
       scopeDefinesParameterVariable(scope, name, i)
       or
-      scopeAssigns(scope, name, i) and
       i =
         min(Generated::Identifier other |
           scopeAssigns(scope, name, other)

From e36795c82e3b803cd9f5858f5f75c073176ecef3 Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Mon, 21 Dec 2020 19:12:20 +0100
Subject: [PATCH 3/8] Instance variables boilerplate code

---
 ql/src/codeql_ruby/ast/Variable.qll          | 18 ++++++++++++
 ql/src/codeql_ruby/ast/internal/Variable.qll | 29 ++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll
index 0870faa9d76..b533b814637 100644
--- a/ql/src/codeql_ruby/ast/Variable.qll
+++ b/ql/src/codeql_ruby/ast/Variable.qll
@@ -85,6 +85,13 @@ class GlobalVariable extends Variable, TGlobalVariable {
   final override GlobalVariableAccess getAnAccess() { result.getVariable() = this }
 }
 
+/** An instance variable. */
+class InstanceVariable extends Variable, TInstanceVariable {
+  override InstanceVariable::Range range;
+
+  final override InstanceVariableAccess getAnAccess() { result.getVariable() = this }
+}
+
 /** An access to a variable. */
 class VariableAccess extends Expr {
   override VariableAccess::Range range;
@@ -192,3 +199,14 @@ class GlobalVariableWriteAccess extends GlobalVariableAccess, VariableWriteAcces
 
 /** An access to a global variable where the value is read. */
 class GlobalVariableReadAccess extends GlobalVariableAccess, VariableReadAccess { }
+
+/** An access to an instance variable. */
+class InstanceVariableAccess extends VariableAccess, @token_instance_variable {
+  final override InstanceVariableAccess::Range range;
+
+  /** Gets the variable this identifier refers to. */
+  final override InstanceVariable getVariable() { result = range.getVariable() }
+
+  final override string getAPrimaryQlClass() { result = "InstanceVariableAccess" }
+}
+
diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll
index 3bbf765aec6..d348f41ec70 100644
--- a/ql/src/codeql_ruby/ast/internal/Variable.qll
+++ b/ql/src/codeql_ruby/ast/internal/Variable.qll
@@ -111,6 +111,7 @@ private module Cached {
   cached
   newtype TVariable =
     TGlobalVariable(string name) { name = any(Generated::GlobalVariable var).getValue() } or
+    TInstanceVariable(VariableScope scope, string name, Generated::AstNode decl) { none() } or
     TLocalVariable(VariableScope scope, string name, Generated::Identifier i) {
       scopeDefinesParameterVariable(scope, name, i)
       or
@@ -414,6 +415,24 @@ module GlobalVariable {
   }
 }
 
+module InstanceVariable {
+  private class ClassLikeScope = TClassScope or TModuleScope or TTopLevelScope;
+
+  class Range extends Variable::Range, TInstanceVariable {
+    private ClassLikeScope scope;
+    private string name;
+    private Generated::AstNode decl;
+
+    Range() { this = TInstanceVariable(scope, name, decl) }
+
+    final override string getName() { result = name }
+
+    final override Location getLocation() { result = decl.getLocation() }
+
+    final override VariableScope getDeclaringScope() { result = scope }
+  }
+}
+
 module VariableAccess {
   abstract class Range extends Expr::Range {
     abstract Variable getVariable();
@@ -449,3 +468,13 @@ module GlobalVariableAccess {
     final override GlobalVariable getVariable() { result = variable }
   }
 }
+
+module InstanceVariableAccess {
+  class Range extends VariableAccess::Range, @token_instance_variable {
+    InstanceVariable variable;
+
+    Range() { this.(Generated::InstanceVariable).getValue() = variable.getName() }
+
+    final override InstanceVariable getVariable() { result = variable }
+  }
+}

From 341bc5c888a272380f6c01fdf5bf508870590390 Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Fri, 8 Jan 2021 10:19:59 +0100
Subject: [PATCH 4/8] Implement instance variables

---
 ql/src/codeql_ruby/ast/Variable.qll           |  3 +
 ql/src/codeql_ruby/ast/internal/Variable.qll  | 63 ++++++++++++++++---
 .../variables/instance_variables.rb           | 44 +++++++++++++
 .../variables/varaccess.expected              | 26 ++++++++
 .../library-tests/variables/variable.expected | 10 +++
 .../variables/varscopes.expected              | 13 ++++
 6 files changed, 152 insertions(+), 7 deletions(-)
 create mode 100644 ql/test/library-tests/variables/instance_variables.rb

diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll
index b533b814637..aa7fc50ef51 100644
--- a/ql/src/codeql_ruby/ast/Variable.qll
+++ b/ql/src/codeql_ruby/ast/Variable.qll
@@ -89,6 +89,9 @@ class GlobalVariable extends Variable, TGlobalVariable {
 class InstanceVariable extends Variable, TInstanceVariable {
   override InstanceVariable::Range range;
 
+  /** Holds is this variable is a class instance variable. */
+  final predicate isClassInstanceVariable() { range.isClassInstanceVariable() }
+
   final override InstanceVariableAccess getAnAccess() { result.getVariable() = this }
 }
 
diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll
index d348f41ec70..dc84d1114d5 100644
--- a/ql/src/codeql_ruby/ast/internal/Variable.qll
+++ b/ql/src/codeql_ruby/ast/internal/Variable.qll
@@ -9,6 +9,36 @@ private Generated::AstNode parent(Generated::AstNode n) {
   not n = any(VariableScope s).getScopeElement()
 }
 
+private predicate instanceVariableAccess(
+  Generated::InstanceVariable var, string name, VariableScope scope, boolean instance
+) {
+  name = var.getValue() and
+  scope = enclosingModuleOrClass(var) and
+  if
+    exists(VariableScope method |
+      method = enclosingMethod(var) and scope = enclosingScope(method.getScopeElement())
+    )
+  then instance = true
+  else instance = false
+}
+
+private VariableScope enclosingMethod(Generated::AstNode node) {
+  exists(VariableScope scope, Callable c |
+    scope = outerScope*(enclosingScope(node)) and
+    scope = TCallableScope(c) and
+    (c instanceof Method or c instanceof SingletonMethod) and
+    result = scope
+  )
+}
+
+private VariableScope enclosingModuleOrClass(Generated::AstNode node) {
+  exists(VariableScope scope | scope = enclosingScope(node) |
+    if scope instanceof ModuleOrClassScope
+    then result = scope
+    else result = enclosingModuleOrClass(scope.getScopeElement())
+  )
+}
+
 private predicate parameterAssignment(
   CallableScope::Range scope, string name, Generated::Identifier i
 ) {
@@ -56,6 +86,10 @@ private predicate strictlyBefore(Location one, Location two) {
   one.getStartLine() = two.getStartLine() and one.getStartColumn() < two.getStartColumn()
 }
 
+private VariableScope outerScope(VariableScope scope) {
+  result = enclosingScope(scope.getScopeElement())
+}
+
 /** A scope that may capture outer local variables. */
 private class CapturingScope extends VariableScope {
   CapturingScope() {
@@ -69,7 +103,7 @@ private class CapturingScope extends VariableScope {
   }
 
   /** Gets the scope in which this scope is nested, if any. */
-  private VariableScope getOuterScope() { result = enclosingScope(this.getScopeElement()) }
+  VariableScope getOuterScope() { result = outerScope(this) }
 
   /** Holds if this scope inherits `name` from an outer scope `outer`. */
   predicate inherits(string name, VariableScope outer) {
@@ -111,7 +145,14 @@ private module Cached {
   cached
   newtype TVariable =
     TGlobalVariable(string name) { name = any(Generated::GlobalVariable var).getValue() } or
-    TInstanceVariable(VariableScope scope, string name, Generated::AstNode decl) { none() } or
+    TInstanceVariable(VariableScope scope, string name, boolean instance, Generated::AstNode decl) {
+      decl =
+        min(Generated::InstanceVariable other |
+          instanceVariableAccess(other, name, scope, instance)
+        |
+          other order by other.getLocation().getStartLine(), other.getLocation().getStartColumn()
+        )
+    } or
     TLocalVariable(VariableScope scope, string name, Generated::Identifier i) {
       scopeDefinesParameterVariable(scope, name, i)
       or
@@ -415,18 +456,21 @@ module GlobalVariable {
   }
 }
 
-module InstanceVariable {
-  private class ClassLikeScope = TClassScope or TModuleScope or TTopLevelScope;
+private class ModuleOrClassScope = TClassScope or TModuleScope or TTopLevelScope;
 
+module InstanceVariable {
   class Range extends Variable::Range, TInstanceVariable {
-    private ClassLikeScope scope;
+    private ModuleOrClassScope scope;
+    private boolean instance;
     private string name;
     private Generated::AstNode decl;
 
-    Range() { this = TInstanceVariable(scope, name, decl) }
+    Range() { this = TInstanceVariable(scope, name, instance, decl) }
 
     final override string getName() { result = name }
 
+    final predicate isClassInstanceVariable() { instance = false }
+
     final override Location getLocation() { result = decl.getLocation() }
 
     final override VariableScope getDeclaringScope() { result = scope }
@@ -473,7 +517,12 @@ module InstanceVariableAccess {
   class Range extends VariableAccess::Range, @token_instance_variable {
     InstanceVariable variable;
 
-    Range() { this.(Generated::InstanceVariable).getValue() = variable.getName() }
+    Range() {
+      exists(boolean instance, VariableScope scope, string name |
+        variable = TInstanceVariable(scope, name, instance, _) and
+        instanceVariableAccess(this, name, scope, instance)
+      )
+    }
 
     final override InstanceVariable getVariable() { result = variable }
   }
diff --git a/ql/test/library-tests/variables/instance_variables.rb b/ql/test/library-tests/variables/instance_variables.rb
new file mode 100644
index 00000000000..d35b6b15af9
--- /dev/null
+++ b/ql/test/library-tests/variables/instance_variables.rb
@@ -0,0 +1,44 @@
+@top = 1
+
+def foo
+  @foo = 10
+end
+
+def print_foo
+  puts @foo
+end
+
+puts @top
+
+class X
+  @x = 10
+  def m()
+    @y = 7
+  end
+end
+
+module M
+ @m = 10
+ def n()
+   @n = 7
+ end
+end
+
+puts { 
+  @x = 100
+}
+
+def bar
+ 1.times { @x = 200 }
+end
+
+class C
+  @x = 42
+  def x 
+   def y
+     @x = 10
+   end
+   y
+   p @x
+  end
+ end
\ No newline at end of file
diff --git a/ql/test/library-tests/variables/varaccess.expected b/ql/test/library-tests/variables/varaccess.expected
index 8791c560a5d..82da276bc9f 100644
--- a/ql/test/library-tests/variables/varaccess.expected
+++ b/ql/test/library-tests/variables/varaccess.expected
@@ -1,4 +1,17 @@
 variableAccess
+| instance_variables.rb:1:1:1:4 | @top | instance_variables.rb:1:1:1:4 | @top | instance_variables.rb:1:1:44:4 | top-level scope |
+| instance_variables.rb:4:3:4:6 | @foo | instance_variables.rb:4:3:4:6 | @foo | instance_variables.rb:1:1:44:4 | top-level scope |
+| instance_variables.rb:8:8:8:11 | @foo | instance_variables.rb:4:3:4:6 | @foo | instance_variables.rb:1:1:44:4 | top-level scope |
+| instance_variables.rb:11:6:11:9 | @top | instance_variables.rb:1:1:1:4 | @top | instance_variables.rb:1:1:44:4 | top-level scope |
+| instance_variables.rb:14:3:14:4 | @x | instance_variables.rb:14:3:14:4 | @x | instance_variables.rb:13:1:18:3 | class scope |
+| instance_variables.rb:16:5:16:6 | @y | instance_variables.rb:16:5:16:6 | @y | instance_variables.rb:13:1:18:3 | class scope |
+| instance_variables.rb:21:2:21:3 | @m | instance_variables.rb:21:2:21:3 | @m | instance_variables.rb:20:1:25:3 | module scope |
+| instance_variables.rb:23:4:23:5 | @n | instance_variables.rb:23:4:23:5 | @n | instance_variables.rb:20:1:25:3 | module scope |
+| instance_variables.rb:28:3:28:4 | @x | instance_variables.rb:28:3:28:4 | @x | instance_variables.rb:1:1:44:4 | top-level scope |
+| instance_variables.rb:32:12:32:13 | @x | instance_variables.rb:32:12:32:13 | @x | instance_variables.rb:1:1:44:4 | top-level scope |
+| instance_variables.rb:36:3:36:4 | @x | instance_variables.rb:36:3:36:4 | @x | instance_variables.rb:35:1:44:4 | class scope |
+| instance_variables.rb:39:6:39:7 | @x | instance_variables.rb:39:6:39:7 | @x | instance_variables.rb:35:1:44:4 | class scope |
+| instance_variables.rb:42:6:42:7 | @x | instance_variables.rb:39:6:39:7 | @x | instance_variables.rb:35:1:44:4 | class scope |
 | nested_scopes.rb:5:3:5:3 | a | nested_scopes.rb:5:3:5:3 | a | nested_scopes.rb:4:1:39:3 | class scope |
 | nested_scopes.rb:7:5:7:5 | a | nested_scopes.rb:7:5:7:5 | a | nested_scopes.rb:6:3:37:5 | module scope |
 | nested_scopes.rb:9:7:9:7 | a | nested_scopes.rb:9:7:9:7 | a | nested_scopes.rb:8:5:35:7 | module scope |
@@ -221,6 +234,19 @@ implicitWrite
 | ssa.rb:64:8:64:8 | a |
 | ssa.rb:66:15:66:15 | a |
 readAccess
+| instance_variables.rb:1:1:1:4 | @top |
+| instance_variables.rb:4:3:4:6 | @foo |
+| instance_variables.rb:8:8:8:11 | @foo |
+| instance_variables.rb:11:6:11:9 | @top |
+| instance_variables.rb:14:3:14:4 | @x |
+| instance_variables.rb:16:5:16:6 | @y |
+| instance_variables.rb:21:2:21:3 | @m |
+| instance_variables.rb:23:4:23:5 | @n |
+| instance_variables.rb:28:3:28:4 | @x |
+| instance_variables.rb:32:12:32:13 | @x |
+| instance_variables.rb:36:3:36:4 | @x |
+| instance_variables.rb:39:6:39:7 | @x |
+| instance_variables.rb:42:6:42:7 | @x |
 | nested_scopes.rb:14:16:14:16 | a |
 | nested_scopes.rb:15:11:15:11 | a |
 | nested_scopes.rb:16:13:16:13 | a |
diff --git a/ql/test/library-tests/variables/variable.expected b/ql/test/library-tests/variables/variable.expected
index 54600ae9da1..c7438f8285e 100644
--- a/ql/test/library-tests/variables/variable.expected
+++ b/ql/test/library-tests/variables/variable.expected
@@ -1,5 +1,15 @@
 | file://:0:0:0:0 | $0 |
 | file://:0:0:0:0 | $global |
+| instance_variables.rb:1:1:1:4 | @top |
+| instance_variables.rb:4:3:4:6 | @foo |
+| instance_variables.rb:14:3:14:4 | @x |
+| instance_variables.rb:16:5:16:6 | @y |
+| instance_variables.rb:21:2:21:3 | @m |
+| instance_variables.rb:23:4:23:5 | @n |
+| instance_variables.rb:28:3:28:4 | @x |
+| instance_variables.rb:32:12:32:13 | @x |
+| instance_variables.rb:36:3:36:4 | @x |
+| instance_variables.rb:39:6:39:7 | @x |
 | nested_scopes.rb:5:3:5:3 | a |
 | nested_scopes.rb:7:5:7:5 | a |
 | nested_scopes.rb:9:7:9:7 | a |
diff --git a/ql/test/library-tests/variables/varscopes.expected b/ql/test/library-tests/variables/varscopes.expected
index 104f0ded8b6..e0044e1402b 100644
--- a/ql/test/library-tests/variables/varscopes.expected
+++ b/ql/test/library-tests/variables/varscopes.expected
@@ -1,4 +1,17 @@
 | file://:0:0:0:0 | global scope |
+| instance_variables.rb:1:1:44:4 | top-level scope |
+| instance_variables.rb:3:1:5:3 | method scope |
+| instance_variables.rb:7:1:9:3 | method scope |
+| instance_variables.rb:13:1:18:3 | class scope |
+| instance_variables.rb:15:3:17:5 | method scope |
+| instance_variables.rb:20:1:25:3 | module scope |
+| instance_variables.rb:22:2:24:4 | method scope |
+| instance_variables.rb:27:6:29:1 | block scope |
+| instance_variables.rb:31:1:33:3 | method scope |
+| instance_variables.rb:32:10:32:21 | block scope |
+| instance_variables.rb:35:1:44:4 | class scope |
+| instance_variables.rb:37:3:43:5 | method scope |
+| instance_variables.rb:38:4:40:6 | method scope |
 | nested_scopes.rb:1:1:3:3 | method scope |
 | nested_scopes.rb:1:1:42:1 | top-level scope |
 | nested_scopes.rb:4:1:39:3 | class scope |

From a07e0fb0f7ef38aa3f0b4fc8dfdc7c8d653c61ef Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Tue, 12 Jan 2021 16:52:36 +0100
Subject: [PATCH 5/8] Class variables boilerplate code

---
 ql/src/codeql_ruby/ast/Variable.qll          | 16 ++++++++++++
 ql/src/codeql_ruby/ast/internal/Variable.qll | 27 ++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll
index aa7fc50ef51..fb9e1538814 100644
--- a/ql/src/codeql_ruby/ast/Variable.qll
+++ b/ql/src/codeql_ruby/ast/Variable.qll
@@ -95,6 +95,13 @@ class InstanceVariable extends Variable, TInstanceVariable {
   final override InstanceVariableAccess getAnAccess() { result.getVariable() = this }
 }
 
+/** A class variable. */
+class ClassVariable extends Variable, TClassVariable {
+  override ClassVariable::Range range;
+
+  final override ClassVariableAccess getAnAccess() { result.getVariable() = this }
+}
+
 /** An access to a variable. */
 class VariableAccess extends Expr {
   override VariableAccess::Range range;
@@ -213,3 +220,12 @@ class InstanceVariableAccess extends VariableAccess, @token_instance_variable {
   final override string getAPrimaryQlClass() { result = "InstanceVariableAccess" }
 }
 
+/** An access to a class variable. */
+class ClassVariableAccess extends VariableAccess, @token_class_variable {
+  final override ClassVariableAccess::Range range;
+
+  /** Gets the variable this identifier refers to. */
+  final override ClassVariable getVariable() { result = range.getVariable() }
+
+  final override string getAPrimaryQlClass() { result = "ClassVariableAccess" }
+}
diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll
index dc84d1114d5..fe02d5563b2 100644
--- a/ql/src/codeql_ruby/ast/internal/Variable.qll
+++ b/ql/src/codeql_ruby/ast/internal/Variable.qll
@@ -145,6 +145,7 @@ private module Cached {
   cached
   newtype TVariable =
     TGlobalVariable(string name) { name = any(Generated::GlobalVariable var).getValue() } or
+    TClassVariable(VariableScope scope, string name, Generated::AstNode decl) { none() } or
     TInstanceVariable(VariableScope scope, string name, boolean instance, Generated::AstNode decl) {
       decl =
         min(Generated::InstanceVariable other |
@@ -477,6 +478,22 @@ module InstanceVariable {
   }
 }
 
+module ClassVariable {
+  class Range extends Variable::Range, TClassVariable {
+    private ModuleOrClassScope scope;
+    private string name;
+    private Generated::AstNode decl;
+
+    Range() { this = TClassVariable(scope, name, decl) }
+
+    final override string getName() { result = name }
+
+    final override Location getLocation() { result = decl.getLocation() }
+
+    final override VariableScope getDeclaringScope() { result = scope }
+  }
+}
+
 module VariableAccess {
   abstract class Range extends Expr::Range {
     abstract Variable getVariable();
@@ -527,3 +544,13 @@ module InstanceVariableAccess {
     final override InstanceVariable getVariable() { result = variable }
   }
 }
+
+module ClassVariableAccess {
+  class Range extends VariableAccess::Range, @token_class_variable {
+    ClassVariable variable;
+
+    Range() { this.(Generated::ClassVariable).getValue() = variable.getName() }
+
+    final override ClassVariable getVariable() { result = variable }
+  }
+}

From 2921f72473e470b53d7160359e3425850ee539c9 Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Wed, 13 Jan 2021 15:24:58 +0100
Subject: [PATCH 6/8] Implement class variables

---
 ql/src/codeql_ruby/ast/internal/Variable.qll  | 21 ++++++++++++--
 .../variables/class_variables.rb              | 29 +++++++++++++++++++
 .../variables/varaccess.expected              | 16 ++++++++++
 .../library-tests/variables/variable.expected |  5 ++++
 .../variables/varscopes.expected              |  8 +++++
 5 files changed, 77 insertions(+), 2 deletions(-)
 create mode 100644 ql/test/library-tests/variables/class_variables.rb

diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll
index fe02d5563b2..26cec25d4bd 100644
--- a/ql/src/codeql_ruby/ast/internal/Variable.qll
+++ b/ql/src/codeql_ruby/ast/internal/Variable.qll
@@ -22,6 +22,11 @@ private predicate instanceVariableAccess(
   else instance = false
 }
 
+private predicate classVariableAccess(Generated::ClassVariable var, string name, VariableScope scope) {
+  name = var.getValue() and
+  scope = enclosingModuleOrClass(var)
+}
+
 private VariableScope enclosingMethod(Generated::AstNode node) {
   exists(VariableScope scope, Callable c |
     scope = outerScope*(enclosingScope(node)) and
@@ -145,7 +150,14 @@ private module Cached {
   cached
   newtype TVariable =
     TGlobalVariable(string name) { name = any(Generated::GlobalVariable var).getValue() } or
-    TClassVariable(VariableScope scope, string name, Generated::AstNode decl) { none() } or
+    TClassVariable(VariableScope scope, string name, Generated::AstNode decl) {
+      decl =
+        min(Generated::ClassVariable other |
+          classVariableAccess(other, name, scope)
+        |
+          other order by other.getLocation().getStartLine(), other.getLocation().getStartColumn()
+        )
+    } or
     TInstanceVariable(VariableScope scope, string name, boolean instance, Generated::AstNode decl) {
       decl =
         min(Generated::InstanceVariable other |
@@ -549,7 +561,12 @@ module ClassVariableAccess {
   class Range extends VariableAccess::Range, @token_class_variable {
     ClassVariable variable;
 
-    Range() { this.(Generated::ClassVariable).getValue() = variable.getName() }
+    Range() {
+      exists(VariableScope scope, string name |
+        variable = TClassVariable(scope, name, _) and
+        classVariableAccess(this, name, scope)
+      )
+    }
 
     final override ClassVariable getVariable() { result = variable }
   }
diff --git a/ql/test/library-tests/variables/class_variables.rb b/ql/test/library-tests/variables/class_variables.rb
new file mode 100644
index 00000000000..dd32df7ea14
--- /dev/null
+++ b/ql/test/library-tests/variables/class_variables.rb
@@ -0,0 +1,29 @@
+@@x = 42
+
+p @@x
+
+def print
+ p @@x
+end
+
+class X
+  def b
+    p @@x
+  end
+  def self.s
+   p @@x
+  end
+end 
+
+class Y < BasicObject
+  @@x = 10
+end 
+
+module M
+  @@x = 12
+end
+
+module N
+  include M
+  p @@x
+end
diff --git a/ql/test/library-tests/variables/varaccess.expected b/ql/test/library-tests/variables/varaccess.expected
index 82da276bc9f..568f4aecffd 100644
--- a/ql/test/library-tests/variables/varaccess.expected
+++ b/ql/test/library-tests/variables/varaccess.expected
@@ -1,4 +1,12 @@
 variableAccess
+| class_variables.rb:1:1:1:3 | @@x | class_variables.rb:1:1:1:3 | @@x | class_variables.rb:1:1:29:4 | top-level scope |
+| class_variables.rb:3:3:3:5 | @@x | class_variables.rb:1:1:1:3 | @@x | class_variables.rb:1:1:29:4 | top-level scope |
+| class_variables.rb:6:4:6:6 | @@x | class_variables.rb:1:1:1:3 | @@x | class_variables.rb:1:1:29:4 | top-level scope |
+| class_variables.rb:11:7:11:9 | @@x | class_variables.rb:11:7:11:9 | @@x | class_variables.rb:9:1:16:3 | class scope |
+| class_variables.rb:14:6:14:8 | @@x | class_variables.rb:11:7:11:9 | @@x | class_variables.rb:9:1:16:3 | class scope |
+| class_variables.rb:19:3:19:5 | @@x | class_variables.rb:19:3:19:5 | @@x | class_variables.rb:18:1:20:3 | class scope |
+| class_variables.rb:23:3:23:5 | @@x | class_variables.rb:23:3:23:5 | @@x | class_variables.rb:22:1:24:3 | module scope |
+| class_variables.rb:28:5:28:7 | @@x | class_variables.rb:28:5:28:7 | @@x | class_variables.rb:26:1:29:3 | module scope |
 | instance_variables.rb:1:1:1:4 | @top | instance_variables.rb:1:1:1:4 | @top | instance_variables.rb:1:1:44:4 | top-level scope |
 | instance_variables.rb:4:3:4:6 | @foo | instance_variables.rb:4:3:4:6 | @foo | instance_variables.rb:1:1:44:4 | top-level scope |
 | instance_variables.rb:8:8:8:11 | @foo | instance_variables.rb:4:3:4:6 | @foo | instance_variables.rb:1:1:44:4 | top-level scope |
@@ -234,6 +242,14 @@ implicitWrite
 | ssa.rb:64:8:64:8 | a |
 | ssa.rb:66:15:66:15 | a |
 readAccess
+| class_variables.rb:1:1:1:3 | @@x |
+| class_variables.rb:3:3:3:5 | @@x |
+| class_variables.rb:6:4:6:6 | @@x |
+| class_variables.rb:11:7:11:9 | @@x |
+| class_variables.rb:14:6:14:8 | @@x |
+| class_variables.rb:19:3:19:5 | @@x |
+| class_variables.rb:23:3:23:5 | @@x |
+| class_variables.rb:28:5:28:7 | @@x |
 | instance_variables.rb:1:1:1:4 | @top |
 | instance_variables.rb:4:3:4:6 | @foo |
 | instance_variables.rb:8:8:8:11 | @foo |
diff --git a/ql/test/library-tests/variables/variable.expected b/ql/test/library-tests/variables/variable.expected
index c7438f8285e..239e60aacef 100644
--- a/ql/test/library-tests/variables/variable.expected
+++ b/ql/test/library-tests/variables/variable.expected
@@ -1,3 +1,8 @@
+| class_variables.rb:1:1:1:3 | @@x |
+| class_variables.rb:11:7:11:9 | @@x |
+| class_variables.rb:19:3:19:5 | @@x |
+| class_variables.rb:23:3:23:5 | @@x |
+| class_variables.rb:28:5:28:7 | @@x |
 | file://:0:0:0:0 | $0 |
 | file://:0:0:0:0 | $global |
 | instance_variables.rb:1:1:1:4 | @top |
diff --git a/ql/test/library-tests/variables/varscopes.expected b/ql/test/library-tests/variables/varscopes.expected
index e0044e1402b..1cc0d4913e9 100644
--- a/ql/test/library-tests/variables/varscopes.expected
+++ b/ql/test/library-tests/variables/varscopes.expected
@@ -1,3 +1,11 @@
+| class_variables.rb:1:1:29:4 | top-level scope |
+| class_variables.rb:5:1:7:3 | method scope |
+| class_variables.rb:9:1:16:3 | class scope |
+| class_variables.rb:10:3:12:5 | method scope |
+| class_variables.rb:13:3:15:5 | method scope |
+| class_variables.rb:18:1:20:3 | class scope |
+| class_variables.rb:22:1:24:3 | module scope |
+| class_variables.rb:26:1:29:3 | module scope |
 | file://:0:0:0:0 | global scope |
 | instance_variables.rb:1:1:44:4 | top-level scope |
 | instance_variables.rb:3:1:5:3 | method scope |

From 6a7e3bfc101cf7200e878894f139e794d065a8ba Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Thu, 28 Jan 2021 17:57:47 +0100
Subject: [PATCH 7/8] Address comments

---
 ql/src/codeql_ruby/ast/Variable.qll          |  4 --
 ql/src/codeql_ruby/ast/internal/Variable.qll | 39 +++++++++++---------
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll
index fb9e1538814..e5e9ff44e41 100644
--- a/ql/src/codeql_ruby/ast/Variable.qll
+++ b/ql/src/codeql_ruby/ast/Variable.qll
@@ -163,7 +163,6 @@ class VariableReadAccess extends VariableAccess {
 class LocalVariableAccess extends VariableAccess, @token_identifier {
   final override LocalVariableAccess::Range range;
 
-  /** Gets the variable this identifier refers to. */
   final override LocalVariable getVariable() { result = range.getVariable() }
 
   final override string getAPrimaryQlClass() {
@@ -198,7 +197,6 @@ class LocalVariableReadAccess extends LocalVariableAccess, VariableReadAccess {
 class GlobalVariableAccess extends VariableAccess, @token_global_variable {
   final override GlobalVariableAccess::Range range;
 
-  /** Gets the variable this identifier refers to. */
   final override GlobalVariable getVariable() { result = range.getVariable() }
 
   final override string getAPrimaryQlClass() { result = "GlobalVariableAccess" }
@@ -214,7 +212,6 @@ class GlobalVariableReadAccess extends GlobalVariableAccess, VariableReadAccess
 class InstanceVariableAccess extends VariableAccess, @token_instance_variable {
   final override InstanceVariableAccess::Range range;
 
-  /** Gets the variable this identifier refers to. */
   final override InstanceVariable getVariable() { result = range.getVariable() }
 
   final override string getAPrimaryQlClass() { result = "InstanceVariableAccess" }
@@ -224,7 +221,6 @@ class InstanceVariableAccess extends VariableAccess, @token_instance_variable {
 class ClassVariableAccess extends VariableAccess, @token_class_variable {
   final override ClassVariableAccess::Range range;
 
-  /** Gets the variable this identifier refers to. */
   final override ClassVariable getVariable() { result = range.getVariable() }
 
   final override string getAPrimaryQlClass() { result = "ClassVariableAccess" }
diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll
index 26cec25d4bd..20cce8aeb34 100644
--- a/ql/src/codeql_ruby/ast/internal/Variable.qll
+++ b/ql/src/codeql_ruby/ast/internal/Variable.qll
@@ -14,12 +14,7 @@ private predicate instanceVariableAccess(
 ) {
   name = var.getValue() and
   scope = enclosingModuleOrClass(var) and
-  if
-    exists(VariableScope method |
-      method = enclosingMethod(var) and scope = enclosingScope(method.getScopeElement())
-    )
-  then instance = true
-  else instance = false
+  if exists(enclosingMethod(var)) then instance = true else instance = false
 }
 
 private predicate classVariableAccess(Generated::ClassVariable var, string name, VariableScope scope) {
@@ -27,23 +22,33 @@ private predicate classVariableAccess(Generated::ClassVariable var, string name,
   scope = enclosingModuleOrClass(var)
 }
 
-private VariableScope enclosingMethod(Generated::AstNode node) {
-  exists(VariableScope scope, Callable c |
-    scope = outerScope*(enclosingScope(node)) and
-    scope = TCallableScope(c) and
-    (c instanceof Method or c instanceof SingletonMethod) and
-    result = scope
+private Callable enclosingMethod(Generated::AstNode node) {
+  parentCallableScope*(enclosingScope(node)) = TCallableScope(result) and
+  (
+    result instanceof Method or
+    result instanceof SingletonMethod
   )
 }
 
-private VariableScope enclosingModuleOrClass(Generated::AstNode node) {
-  exists(VariableScope scope | scope = enclosingScope(node) |
-    if scope instanceof ModuleOrClassScope
-    then result = scope
-    else result = enclosingModuleOrClass(scope.getScopeElement())
+private TCallableScope parentCallableScope(TCallableScope scope) {
+  exists(Callable c |
+    scope = TCallableScope(c) and
+    not c instanceof Method and
+    not c instanceof SingletonMethod
+  |
+    result = outerScope(scope)
   )
 }
 
+private VariableScope parentScope(VariableScope scope) {
+  not scope instanceof ModuleOrClassScope and
+  result = outerScope(scope)
+}
+
+private ModuleOrClassScope enclosingModuleOrClass(Generated::AstNode node) {
+  result = parentScope*(enclosingScope(node))
+}
+
 private predicate parameterAssignment(
   CallableScope::Range scope, string name, Generated::Identifier i
 ) {

From c33c3a112468a11d78f045a1769b06a8ad4898ac Mon Sep 17 00:00:00 2001
From: Arthur Baars <aibaars@github.com>
Date: Fri, 29 Jan 2021 12:27:48 +0100
Subject: [PATCH 8/8] Address comments

---
 ql/src/codeql_ruby/ast/Variable.qll          |  3 ++
 ql/src/codeql_ruby/ast/internal/Variable.qll | 52 ++++++++++----------
 2 files changed, 28 insertions(+), 27 deletions(-)

diff --git a/ql/src/codeql_ruby/ast/Variable.qll b/ql/src/codeql_ruby/ast/Variable.qll
index e5e9ff44e41..7ede082bb4d 100644
--- a/ql/src/codeql_ruby/ast/Variable.qll
+++ b/ql/src/codeql_ruby/ast/Variable.qll
@@ -28,6 +28,9 @@ class VariableScope extends TScope {
     result = this.getAVariable() and
     result.getName() = name
   }
+
+  /** Gets the scope in which this scope is nested, if any. */
+  VariableScope getOuterScope() { result = enclosingScope(this.getScopeElement()) }
 }
 
 /** A variable declared in a scope. */
diff --git a/ql/src/codeql_ruby/ast/internal/Variable.qll b/ql/src/codeql_ruby/ast/internal/Variable.qll
index 20cce8aeb34..6128bfa5ef1 100644
--- a/ql/src/codeql_ruby/ast/internal/Variable.qll
+++ b/ql/src/codeql_ruby/ast/internal/Variable.qll
@@ -14,7 +14,7 @@ private predicate instanceVariableAccess(
 ) {
   name = var.getValue() and
   scope = enclosingModuleOrClass(var) and
-  if exists(enclosingMethod(var)) then instance = true else instance = false
+  if hasEnclosingMethod(var) then instance = true else instance = false
 }
 
 private predicate classVariableAccess(Generated::ClassVariable var, string name, VariableScope scope) {
@@ -22,11 +22,10 @@ private predicate classVariableAccess(Generated::ClassVariable var, string name,
   scope = enclosingModuleOrClass(var)
 }
 
-private Callable enclosingMethod(Generated::AstNode node) {
-  parentCallableScope*(enclosingScope(node)) = TCallableScope(result) and
-  (
-    result instanceof Method or
-    result instanceof SingletonMethod
+predicate hasEnclosingMethod(Generated::AstNode node) {
+  exists(Callable method | parentCallableScope*(enclosingScope(node)) = TCallableScope(method) |
+    method instanceof Method or
+    method instanceof SingletonMethod
   )
 }
 
@@ -36,13 +35,13 @@ private TCallableScope parentCallableScope(TCallableScope scope) {
     not c instanceof Method and
     not c instanceof SingletonMethod
   |
-    result = outerScope(scope)
+    result = scope.(VariableScope).getOuterScope()
   )
 }
 
 private VariableScope parentScope(VariableScope scope) {
   not scope instanceof ModuleOrClassScope and
-  result = outerScope(scope)
+  result = scope.getOuterScope()
 }
 
 private ModuleOrClassScope enclosingModuleOrClass(Generated::AstNode node) {
@@ -96,10 +95,6 @@ private predicate strictlyBefore(Location one, Location two) {
   one.getStartLine() = two.getStartLine() and one.getStartColumn() < two.getStartColumn()
 }
 
-private VariableScope outerScope(VariableScope scope) {
-  result = enclosingScope(scope.getScopeElement())
-}
-
 /** A scope that may capture outer local variables. */
 private class CapturingScope extends VariableScope {
   CapturingScope() {
@@ -112,9 +107,6 @@ private class CapturingScope extends VariableScope {
     )
   }
 
-  /** Gets the scope in which this scope is nested, if any. */
-  VariableScope getOuterScope() { result = outerScope(this) }
-
   /** Holds if this scope inherits `name` from an outer scope `outer`. */
   predicate inherits(string name, VariableScope outer) {
     not scopeDefinesParameterVariable(this, name, _) and
@@ -365,6 +357,22 @@ private module Cached {
   predicate isCapturedAccess(LocalVariableAccess::Range access) {
     access.getVariable().getDeclaringScope() != enclosingScope(access)
   }
+
+  cached
+  predicate instanceVariableAccess(Generated::InstanceVariable var, InstanceVariable v) {
+    exists(string name, VariableScope scope, boolean instance |
+      v = TInstanceVariable(scope, name, instance, _) and
+      instanceVariableAccess(var, name, scope, instance)
+    )
+  }
+
+  cached
+  predicate classVariableAccess(Generated::ClassVariable var, ClassVariable variable) {
+    exists(VariableScope scope, string name |
+      variable = TClassVariable(scope, name, _) and
+      classVariableAccess(var, name, scope)
+    )
+  }
 }
 
 import Cached
@@ -551,12 +559,7 @@ module InstanceVariableAccess {
   class Range extends VariableAccess::Range, @token_instance_variable {
     InstanceVariable variable;
 
-    Range() {
-      exists(boolean instance, VariableScope scope, string name |
-        variable = TInstanceVariable(scope, name, instance, _) and
-        instanceVariableAccess(this, name, scope, instance)
-      )
-    }
+    Range() { instanceVariableAccess(this, variable) }
 
     final override InstanceVariable getVariable() { result = variable }
   }
@@ -566,12 +569,7 @@ module ClassVariableAccess {
   class Range extends VariableAccess::Range, @token_class_variable {
     ClassVariable variable;
 
-    Range() {
-      exists(VariableScope scope, string name |
-        variable = TClassVariable(scope, name, _) and
-        classVariableAccess(this, name, scope)
-      )
-    }
+    Range() { classVariableAccess(this, variable) }
 
     final override ClassVariable getVariable() { result = variable }
   }