Skip to content

Commit c5f3661

Browse files
hmachmac
committed
Ruby: Refactor method visibility modeling
1 parent 88baf08 commit c5f3661

File tree

3 files changed

+148
-47
lines changed

3 files changed

+148
-47
lines changed

‎ruby/ql/lib/codeql/ruby/ast/Method.qll

Lines changed: 76 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,25 @@ class MethodBase extends Callable, BodyStmt, Scope, TMethodBase {
3636
result = BodyStmt.super.getAChild(pred)
3737
}
3838

39+
/**
40+
* Holds if this method is public.
41+
* Methods are public by default.
42+
*/
43+
predicate isPublic() {
44+
forall(VisibilityModifier m | m = this.getVisibilityModifier() | m.getVisibility() = "public")
45+
}
46+
3947
/** Holds if this method is private. */
40-
predicate isPrivate() { none() }
48+
predicate isPrivate() { this.getVisibilityModifier().getVisibility() = "private" }
49+
50+
/** Holds if this method is protected. */
51+
predicate isProtected() { this.getVisibilityModifier().getVisibility() = "protected" }
52+
53+
/**
54+
* Gets the visibility modifier that defines the visibility of this method, if
55+
* one exists.
56+
*/
57+
VisibilityModifier getVisibilityModifier() { none() }
4158
}
4259

4360
/**
@@ -47,45 +64,40 @@ class MethodBase extends Callable, BodyStmt, Scope, TMethodBase {
4764
private class MethodModifier extends MethodCall {
4865
/** Gets the name of the method that this call applies to. */
4966
Expr getMethodArgument() { result = this.getArgument(0) }
50-
51-
/** Holds if this call modifies a method with name `name` in namespace `n`. */
52-
pragma[noinline]
53-
predicate modifiesMethod(Namespace n, string name) {
54-
this = n.getAStmt() and
55-
[
56-
this.getMethodArgument().getConstantValue().getStringlikeValue(),
57-
this.getMethodArgument().(MethodBase).getName()
58-
] = name
59-
}
6067
}
6168

62-
/** A call to `private` or `private_class_method`. */
63-
private class Private extends MethodModifier {
69+
/** A method call that sets the visibility of other methods. */
70+
private class VisibilityModifier extends MethodModifier {
6471
private Namespace namespace;
6572
private int position;
6673

67-
Private() { this.getMethodName() = "private" and namespace.getStmt(position) = this }
74+
VisibilityModifier() {
75+
this.getMethodName() =
76+
[
77+
"public", "private", "protected", "public_class_method", "private_class_method",
78+
"protected_class_method"
79+
] and
80+
namespace.getStmt(position) = this
81+
}
6882

69-
override predicate modifiesMethod(Namespace n, string name) {
70-
n = namespace and
71-
(
72-
// def foo
73-
// ...
74-
// private :foo
75-
super.modifiesMethod(n, name)
76-
or
77-
// private
78-
// ...
79-
// def foo
80-
not exists(this.getMethodArgument()) and
81-
exists(MethodBase m, int i | n.getStmt(i) = m and m.getName() = name and i > position)
82-
)
83+
/**
84+
* Holds if this modifier changes the "ambient" visibility - i.e. the default
85+
* visibility of any subsequent method definitions.
86+
*/
87+
predicate modifiesAmbientVisibility() {
88+
this.getMethodName() = ["public", "private", "protected"] and
89+
this.getNumberOfArguments() = 0
8390
}
84-
}
8591

86-
/** A call to `private_class_method`. */
87-
private class PrivateClassMethod extends MethodModifier {
88-
PrivateClassMethod() { this.getMethodName() = "private_class_method" }
92+
string getVisibility() {
93+
this.getMethodName() = ["public", "public_class_method"] and result = "public"
94+
or
95+
this.getMethodName() = ["private", "private_class_method"] and
96+
result = "private"
97+
or
98+
this.getMethodName() = ["protected", "protected_class_method"] and
99+
result = "protected"
100+
}
89101
}
90102

91103
/** A normal method. */
@@ -140,28 +152,41 @@ class Method extends MethodBase, TMethod {
140152
* ```
141153
*/
142154
override predicate isPrivate() {
143-
exists(Namespace n, string name |
144-
any(Private p).modifiesMethod(n, name) and
145-
isDeclaredIn(this, n, name)
146-
)
155+
this.getVisibilityModifier().getVisibility() = "private"
147156
or
148157
// Top-level methods are private members of the Object class
149158
this.getEnclosingModule() instanceof Toplevel
150159
}
151160

161+
override predicate isPublic() { super.isPublic() and not this.isPrivate() }
162+
152163
final override Parameter getParameter(int n) {
153164
toGenerated(result) = g.getParameters().getChild(n)
154165
}
155166

156167
final override string toString() { result = this.getName() }
157-
}
158168

159-
/**
160-
* Holds if the method `m` has name `name` and is declared in namespace `n`.
161-
*/
162-
pragma[noinline]
163-
private predicate isDeclaredIn(MethodBase m, Namespace n, string name) {
164-
n = m.getEnclosingModule() and name = m.getName()
169+
override VisibilityModifier getVisibilityModifier() {
170+
result.getEnclosingModule() = this.getEnclosingModule() and
171+
(
172+
result.getMethodArgument() = this
173+
or
174+
result.getMethodArgument().getConstantValue().getStringlikeValue() = this.getName()
175+
)
176+
or
177+
exists(Namespace n, int methodPos | n.getStmt(methodPos) = this |
178+
// The relevant visibility modifier is the closest call that occurs before
179+
// the definition of `m` (typically this means higher up the file).
180+
result =
181+
max(int modifierPos, VisibilityModifier modifier |
182+
modifier.modifiesAmbientVisibility() and
183+
n.getStmt(modifierPos) = modifier and
184+
modifierPos < methodPos
185+
|
186+
modifier order by modifierPos
187+
)
188+
)
189+
}
165190
}
166191

167192
/** A singleton method. */
@@ -216,10 +241,14 @@ class SingletonMethod extends MethodBase, TSingletonMethod {
216241
* end
217242
* ```
218243
*/
219-
override predicate isPrivate() {
220-
exists(Namespace n, string name |
221-
any(PrivateClassMethod p).modifiesMethod(n, name) and
222-
isDeclaredIn(this, n, name)
244+
override predicate isPrivate() { super.isPrivate() }
245+
246+
override VisibilityModifier getVisibilityModifier() {
247+
result.getEnclosingModule() = this.getEnclosingModule() and
248+
(
249+
result.getMethodArgument() = this
250+
or
251+
result.getMethodArgument().getConstantValue().getStringlikeValue() = this.getName()
223252
)
224253
}
225254
}

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -324,3 +324,73 @@ privateMethod
324324
| private.rb:63:11:65:5 | m1 |
325325
| private.rb:67:11:69:5 | m2 |
326326
| private.rb:77:11:81:5 | m1 |
327+
publicMethod
328+
| calls.rb:7:1:9:3 | bar |
329+
| calls.rb:13:1:15:3 | bar |
330+
| calls.rb:22:5:24:7 | instance_m |
331+
| calls.rb:25:5:27:7 | singleton_m |
332+
| calls.rb:51:5:57:7 | baz |
333+
| calls.rb:66:5:68:7 | baz |
334+
| calls.rb:92:5:92:23 | bit_length |
335+
| calls.rb:93:5:93:16 | abs |
336+
| calls.rb:97:5:97:23 | capitalize |
337+
| calls.rb:102:5:102:30 | puts |
338+
| calls.rb:106:5:106:24 | module_eval |
339+
| calls.rb:107:5:107:20 | include |
340+
| calls.rb:108:5:108:20 | prepend |
341+
| calls.rb:109:5:109:20 | private |
342+
| calls.rb:114:5:114:16 | new |
343+
| calls.rb:119:5:119:31 | [] |
344+
| calls.rb:124:3:124:29 | [] |
345+
| calls.rb:126:3:126:17 | length |
346+
| calls.rb:128:3:134:5 | foreach |
347+
| calls.rb:163:5:165:7 | s_method |
348+
| calls.rb:169:5:170:7 | to_s |
349+
| calls.rb:174:5:175:7 | to_s |
350+
| calls.rb:188:5:191:7 | singleton_a |
351+
| calls.rb:193:5:196:7 | singleton_b |
352+
| calls.rb:198:5:200:7 | singleton_c |
353+
| calls.rb:202:5:205:7 | singleton_d |
354+
| calls.rb:207:5:212:7 | instance |
355+
| calls.rb:208:9:210:11 | singleton_e |
356+
| calls.rb:215:9:217:11 | singleton_f |
357+
| calls.rb:220:5:222:7 | call_singleton_g |
358+
| calls.rb:233:1:235:3 | singleton_g |
359+
| calls.rb:240:1:242:3 | singleton_g |
360+
| calls.rb:248:5:250:7 | singleton_g |
361+
| calls.rb:264:1:266:3 | singleton_g |
362+
| calls.rb:278:5:280:7 | singleton_h |
363+
| calls.rb:291:5:293:7 | singleton_i |
364+
| calls.rb:300:5:302:7 | singleton_j |
365+
| calls.rb:308:5:311:7 | instance |
366+
| calls.rb:313:5:315:7 | singleton |
367+
| calls.rb:323:5:325:7 | instance |
368+
| calls.rb:329:5:331:7 | instance |
369+
| calls.rb:335:5:337:7 | instance |
370+
| calls.rb:365:5:367:7 | instance |
371+
| calls.rb:376:9:378:11 | singleton1 |
372+
| calls.rb:380:9:382:11 | call_singleton1 |
373+
| calls.rb:385:5:387:7 | singleton2 |
374+
| calls.rb:389:5:391:7 | call_singleton2 |
375+
| calls.rb:401:9:403:11 | singleton1 |
376+
| calls.rb:406:5:408:7 | singleton2 |
377+
| calls.rb:416:9:418:11 | m1 |
378+
| calls.rb:421:5:433:7 | m2 |
379+
| calls.rb:424:9:430:11 | m3 |
380+
| calls.rb:427:13:429:15 | m4 |
381+
| calls.rb:437:13:439:15 | m5 |
382+
| calls.rb:478:5:480:7 | singleton |
383+
| hello.rb:2:5:4:7 | hello |
384+
| hello.rb:5:5:7:7 | world |
385+
| hello.rb:13:5:15:7 | message |
386+
| hello.rb:19:5:21:7 | message |
387+
| modules.rb:9:5:10:7 | method_in_foo_bar |
388+
| modules.rb:16:3:17:5 | method_in_foo |
389+
| modules.rb:27:3:28:5 | method_in_another_definition_of_foo |
390+
| modules.rb:38:3:39:5 | method_a |
391+
| modules.rb:41:3:42:5 | method_b |
392+
| modules.rb:52:3:53:5 | method_in_another_definition_of_foo_bar |
393+
| private.rb:5:3:6:5 | public |
394+
| private.rb:20:3:21:5 | public2 |
395+
| private.rb:46:3:47:5 | public |
396+
| private.rb:71:3:73:5 | call_m1 |

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,3 +5,5 @@ query Callable getTarget(Call call) { result = call.getATarget() }
55
query predicate unresolvedCall(Call call) { not exists(call.getATarget()) }
66

77
query predicate privateMethod(MethodBase m) { m.isPrivate() }
8+
9+
query predicate publicMethod(MethodBase m) { m.isPublic() }

0 commit comments

Comments
 (0)