Skip to content

Commit db58e33

Browse files
committed
Ruby: allow speculative container qname resolution
1 parent 8c2c28d commit db58e33

File tree

6 files changed

+89
-32
lines changed

6 files changed

+89
-32
lines changed

ruby/ql/lib/codeql/ruby/ast/internal/Module.qll

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
private import codeql.ruby.AST
2+
private import Scope as Scope
23

34
// Names of built-in modules and classes
45
private string builtin() {
@@ -16,6 +17,8 @@ private module Cached {
1617
TResolved(string qName) {
1718
qName = builtin()
1819
or
20+
qName = getAnAssumedGlobalConst()
21+
or
1922
qName = namespaceDeclaration(_)
2023
} or
2124
TUnresolved(Namespace n) { not exists(namespaceDeclaration(n)) }
@@ -65,7 +68,7 @@ private module Cached {
6568
(
6669
m = resolveConstantReadAccess(c.getReceiver())
6770
or
68-
m = enclosingModule(c).getModule() and
71+
m = enclosingModuleNoBlock(c).getModule() and
6972
c.getReceiver() instanceof SelfVariableAccess
7073
) and
7174
result = resolveConstantReadAccess(c.getAnArgument())
@@ -388,11 +391,27 @@ private module ResolveImpl {
388391
result = resolveConstantWriteAccessRec(c, _, _)
389392
}
390393

394+
/**
395+
* Gets the name of a constant `C` that we assume to be defined in the top-level because
396+
* it is referenced in a way that can only resolve to a top-level constant.
397+
*/
398+
string getAnAssumedGlobalConst() {
399+
exists(ConstantAccess access |
400+
not exists(access.getScopeExpr()) and
401+
result = access.getName()
402+
|
403+
access.hasGlobalScope()
404+
or
405+
// At the top-level but not inside a block
406+
enclosingModuleNoBlock(access) instanceof Toplevel
407+
)
408+
}
409+
391410
pragma[nomagic]
392411
private string isDefinedConstantNonRec(string container, string name) {
393412
result = resolveConstantWriteAccessNonRec(_, container, name)
394413
or
395-
result = builtin() and
414+
result = [builtin(), getAnAssumedGlobalConst()] and
396415
name = result and
397416
container = "Object"
398417
}
@@ -447,7 +466,7 @@ private module ResolveImpl {
447466
result = resolveConstantReadAccess(this.getReceiver(), _)
448467
or
449468
exists(ModuleBase encl |
450-
encl = enclosingModule(this) and
469+
encl = enclosingModuleNoBlock(this) and
451470
result = [qualifiedModuleNameNonRec(encl, _, _), qualifiedModuleNameRec(encl, _, _)]
452471
|
453472
this.getReceiver() instanceof SelfVariableAccess
@@ -495,23 +514,31 @@ private module ResolveImpl {
495514
private import ResolveImpl
496515

497516
/**
498-
* A variant of AstNode::getEnclosingModule that excludes
517+
* Gets an enclosing scope of `scope`, stopping at the first module or block.
518+
*
519+
* Includes `scope` itself and the final module/block.
520+
*/
521+
private Scope enclosingScopesNoBlock(Scope scope) {
522+
result = scope
523+
or
524+
not scope instanceof ModuleBase and
525+
not scope instanceof Block and
526+
result = enclosingScopesNoBlock(scope.getOuterScope())
527+
}
528+
529+
/**
530+
* A variant of `AstNode::getEnclosingModule` that excludes
499531
* results that are enclosed in a block. This is a bit wrong because
500532
* it could lead to false negatives. However, `include` statements in
501533
* blocks are very rare in normal code. The majority of cases are in calls
502534
* to methods like `module_eval` and `Rspec.describe` / `Rspec.context`. These
503535
* methods evaluate the block in the context of some other module/class instead of
504536
* the enclosing one.
505537
*/
506-
private ModuleBase enclosingModule(AstNode node) {
507-
result = node.getParent()
508-
or
509-
exists(AstNode mid |
510-
result = enclosingModule(mid) and
511-
mid = node.getParent() and
512-
not mid instanceof ModuleBase and
513-
not mid instanceof Block
514-
)
538+
private ModuleBase enclosingModuleNoBlock(Stmt node) {
539+
// Note: don't rely on AstNode.getParent() here.
540+
// Instead use Scope.getOuterScope() to correctly handle the scoping of things like Namespace.getScopeExpr().
541+
result = enclosingScopesNoBlock(Scope::scopeOfInclSynth(node))
515542
}
516543

517544
private Module getAncestors(Module m) {

ruby/ql/test/library-tests/modules/ancestors.expected

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,14 @@
11
#-----| Class
22
#-----| super -> Module
33

4+
#-----| EsotericInstanceMethods
5+
6+
#-----| MyStruct
7+
8+
#-----| Struct
9+
10+
#-----| UnresolvedNamespace
11+
412
#-----| BasicObject
513

614
#-----| Complex
@@ -244,8 +252,8 @@ unresolved_subclass.rb:
244252
# 1| ResolvableBaseClass
245253
#-----| super -> Object
246254

247-
# 4| ...::Subclass1
255+
# 4| UnresolvedNamespace::Subclass1
248256
#-----| super -> ResolvableBaseClass
249257

250-
# 7| ...::Subclass2
251-
#-----| super -> Object
258+
# 7| UnresolvedNamespace::Subclass2
259+
#-----| super -> UnresolvedNamespace::Subclass1

ruby/ql/test/library-tests/modules/callgraph.expected

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,7 @@ getTarget
281281
| private.rb:104:1:104:20 | call to new | calls.rb:117:5:117:16 | new |
282282
| private.rb:104:1:104:28 | call to call_m1 | private.rb:91:3:93:5 | call_m1 |
283283
| private.rb:105:1:105:20 | call to new | calls.rb:117:5:117:16 | new |
284+
| toplevel_self_singleton.rb:18:12:22:1 | call to new | calls.rb:117:5:117:16 | new |
284285
| toplevel_self_singleton.rb:30:13:30:19 | call to call_me | toplevel_self_singleton.rb:26:9:27:11 | call_me |
285286
| toplevel_self_singleton.rb:31:13:31:20 | call to call_you | toplevel_self_singleton.rb:29:9:32:11 | call_you |
286287
unresolvedCall
@@ -372,7 +373,6 @@ unresolvedCall
372373
| toplevel_self_singleton.rb:8:1:16:3 | call to do_something |
373374
| toplevel_self_singleton.rb:10:9:10:27 | call to ab_singleton_method |
374375
| toplevel_self_singleton.rb:14:9:14:27 | call to ab_singleton_method |
375-
| toplevel_self_singleton.rb:18:12:22:1 | call to new |
376376
| toplevel_self_singleton.rb:20:9:20:27 | call to ab_singleton_method |
377377
privateMethod
378378
| calls.rb:1:1:3:3 | foo |

ruby/ql/test/library-tests/modules/methods.expected

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -602,12 +602,12 @@ lookupMethod
602602
| unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass | new | calls.rb:117:5:117:16 | new |
603603
| unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass | puts | calls.rb:102:5:102:30 | puts |
604604
| unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass | to_s | calls.rb:172:5:173:7 | to_s |
605-
| unresolved_subclass.rb:4:1:5:3 | ...::Subclass1 | new | calls.rb:117:5:117:16 | new |
606-
| unresolved_subclass.rb:4:1:5:3 | ...::Subclass1 | puts | calls.rb:102:5:102:30 | puts |
607-
| unresolved_subclass.rb:4:1:5:3 | ...::Subclass1 | to_s | calls.rb:172:5:173:7 | to_s |
608-
| unresolved_subclass.rb:7:1:8:3 | ...::Subclass2 | new | calls.rb:117:5:117:16 | new |
609-
| unresolved_subclass.rb:7:1:8:3 | ...::Subclass2 | puts | calls.rb:102:5:102:30 | puts |
610-
| unresolved_subclass.rb:7:1:8:3 | ...::Subclass2 | to_s | calls.rb:172:5:173:7 | to_s |
605+
| unresolved_subclass.rb:4:1:5:3 | UnresolvedNamespace::Subclass1 | new | calls.rb:117:5:117:16 | new |
606+
| unresolved_subclass.rb:4:1:5:3 | UnresolvedNamespace::Subclass1 | puts | calls.rb:102:5:102:30 | puts |
607+
| unresolved_subclass.rb:4:1:5:3 | UnresolvedNamespace::Subclass1 | to_s | calls.rb:172:5:173:7 | to_s |
608+
| unresolved_subclass.rb:7:1:8:3 | UnresolvedNamespace::Subclass2 | new | calls.rb:117:5:117:16 | new |
609+
| unresolved_subclass.rb:7:1:8:3 | UnresolvedNamespace::Subclass2 | puts | calls.rb:102:5:102:30 | puts |
610+
| unresolved_subclass.rb:7:1:8:3 | UnresolvedNamespace::Subclass2 | to_s | calls.rb:172:5:173:7 | to_s |
611611
enclosingMethod
612612
| calls.rb:2:5:2:14 | call to puts | calls.rb:1:1:3:3 | foo |
613613
| calls.rb:2:5:2:14 | self | calls.rb:1:1:3:3 | foo |

ruby/ql/test/library-tests/modules/modules.expected

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,14 +35,18 @@ getModule
3535
| file://:0:0:0:0 | BasicObject |
3636
| file://:0:0:0:0 | Class |
3737
| file://:0:0:0:0 | Complex |
38+
| file://:0:0:0:0 | EsotericInstanceMethods |
3839
| file://:0:0:0:0 | FalseClass |
3940
| file://:0:0:0:0 | Float |
41+
| file://:0:0:0:0 | MyStruct |
4042
| file://:0:0:0:0 | NilClass |
4143
| file://:0:0:0:0 | Numeric |
4244
| file://:0:0:0:0 | Proc |
4345
| file://:0:0:0:0 | Rational |
46+
| file://:0:0:0:0 | Struct |
4447
| file://:0:0:0:0 | Symbol |
4548
| file://:0:0:0:0 | TrueClass |
49+
| file://:0:0:0:0 | UnresolvedNamespace |
4650
| hello.rb:1:1:8:3 | EnglishWords |
4751
| hello.rb:11:1:16:3 | Greeting |
4852
| hello.rb:18:1:22:3 | HelloWorld |
@@ -86,8 +90,8 @@ getModule
8690
| toplevel_self_singleton.rb:2:5:5:7 | A::B |
8791
| toplevel_self_singleton.rb:24:1:34:3 | Good |
8892
| unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass |
89-
| unresolved_subclass.rb:4:1:5:3 | ...::Subclass1 |
90-
| unresolved_subclass.rb:7:1:8:3 | ...::Subclass2 |
93+
| unresolved_subclass.rb:4:1:5:3 | UnresolvedNamespace::Subclass1 |
94+
| unresolved_subclass.rb:7:1:8:3 | UnresolvedNamespace::Subclass2 |
9195
getADeclaration
9296
| calls.rb:21:1:34:3 | M | calls.rb:21:1:34:3 | M |
9397
| calls.rb:43:1:58:3 | C | calls.rb:43:1:58:3 | C |
@@ -181,8 +185,8 @@ getADeclaration
181185
| toplevel_self_singleton.rb:2:5:5:7 | A::B | toplevel_self_singleton.rb:2:5:5:7 | B |
182186
| toplevel_self_singleton.rb:24:1:34:3 | Good | toplevel_self_singleton.rb:24:1:34:3 | Good |
183187
| unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass | unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass |
184-
| unresolved_subclass.rb:4:1:5:3 | ...::Subclass1 | unresolved_subclass.rb:4:1:5:3 | Subclass1 |
185-
| unresolved_subclass.rb:7:1:8:3 | ...::Subclass2 | unresolved_subclass.rb:7:1:8:3 | Subclass2 |
188+
| unresolved_subclass.rb:4:1:5:3 | UnresolvedNamespace::Subclass1 | unresolved_subclass.rb:4:1:5:3 | Subclass1 |
189+
| unresolved_subclass.rb:7:1:8:3 | UnresolvedNamespace::Subclass2 | unresolved_subclass.rb:7:1:8:3 | Subclass2 |
186190
getSuperClass
187191
| calls.rb:43:1:58:3 | C | calls.rb:115:1:118:3 | Object |
188192
| calls.rb:65:1:69:3 | D | calls.rb:43:1:58:3 | C |
@@ -239,8 +243,8 @@ getSuperClass
239243
| private.rb:96:1:102:3 | PrivateOverride2 | private.rb:82:1:94:3 | PrivateOverride1 |
240244
| toplevel_self_singleton.rb:2:5:5:7 | A::B | calls.rb:115:1:118:3 | Object |
241245
| unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass | calls.rb:115:1:118:3 | Object |
242-
| unresolved_subclass.rb:4:1:5:3 | ...::Subclass1 | unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass |
243-
| unresolved_subclass.rb:7:1:8:3 | ...::Subclass2 | calls.rb:115:1:118:3 | Object |
246+
| unresolved_subclass.rb:4:1:5:3 | UnresolvedNamespace::Subclass1 | unresolved_subclass.rb:1:1:2:3 | ResolvableBaseClass |
247+
| unresolved_subclass.rb:7:1:8:3 | UnresolvedNamespace::Subclass2 | unresolved_subclass.rb:4:1:5:3 | UnresolvedNamespace::Subclass1 |
244248
getAPrependedModule
245249
| calls.rb:115:1:118:3 | Object | calls.rb:171:1:174:3 | A |
246250
| calls.rb:171:1:174:3 | A | toplevel_self_singleton.rb:2:5:5:7 | A::B |
@@ -318,6 +322,11 @@ resolveConstantReadAccess
318322
| calls.rb:471:5:471:11 | Array | Array |
319323
| calls.rb:477:5:477:9 | Class | Class |
320324
| calls.rb:483:5:483:11 | Array | Array |
325+
| calls.rb:490:1:490:23 | EsotericInstanceMethods | EsotericInstanceMethods |
326+
| calls.rb:491:1:491:23 | EsotericInstanceMethods | EsotericInstanceMethods |
327+
| calls.rb:492:1:492:23 | EsotericInstanceMethods | EsotericInstanceMethods |
328+
| calls.rb:493:1:493:23 | EsotericInstanceMethods | EsotericInstanceMethods |
329+
| calls.rb:494:1:494:23 | EsotericInstanceMethods | EsotericInstanceMethods |
321330
| calls.rb:504:1:504:21 | ExtendSingletonMethod | ExtendSingletonMethod |
322331
| calls.rb:507:12:507:32 | ExtendSingletonMethod | ExtendSingletonMethod |
323332
| calls.rb:510:1:510:22 | ExtendSingletonMethod2 | ExtendSingletonMethod2 |
@@ -383,7 +392,12 @@ resolveConstantReadAccess
383392
| private.rb:100:7:100:22 | PrivateOverride1 | PrivateOverride1 |
384393
| private.rb:104:1:104:16 | PrivateOverride2 | PrivateOverride2 |
385394
| private.rb:105:1:105:16 | PrivateOverride2 | PrivateOverride2 |
395+
| toplevel_self_singleton.rb:18:12:18:17 | Struct | Struct |
396+
| unresolved_subclass.rb:4:7:4:25 | UnresolvedNamespace | UnresolvedNamespace |
386397
| unresolved_subclass.rb:4:40:4:58 | ResolvableBaseClass | ResolvableBaseClass |
398+
| unresolved_subclass.rb:7:7:7:25 | UnresolvedNamespace | UnresolvedNamespace |
399+
| unresolved_subclass.rb:7:40:7:58 | UnresolvedNamespace | UnresolvedNamespace |
400+
| unresolved_subclass.rb:7:40:7:69 | Subclass1 | UnresolvedNamespace::Subclass1 |
387401
resolveConstantWriteAccess
388402
| calls.rb:21:1:34:3 | M | M |
389403
| calls.rb:43:1:58:3 | C | C |

ruby/ql/test/library-tests/modules/superclasses.expected

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,14 @@
11
#-----| Class
22
#-----| -> Module
33

4+
#-----| EsotericInstanceMethods
5+
6+
#-----| MyStruct
7+
8+
#-----| Struct
9+
10+
#-----| UnresolvedNamespace
11+
412
#-----| BasicObject
513

614
#-----| Complex
@@ -235,8 +243,8 @@ unresolved_subclass.rb:
235243
# 1| ResolvableBaseClass
236244
#-----| -> Object
237245

238-
# 4| ...::Subclass1
246+
# 4| UnresolvedNamespace::Subclass1
239247
#-----| -> ResolvableBaseClass
240248

241-
# 7| ...::Subclass2
242-
#-----| -> Object
249+
# 7| UnresolvedNamespace::Subclass2
250+
#-----| -> UnresolvedNamespace::Subclass1

0 commit comments

Comments
 (0)