Ruby: handle private module methods #7340
Conversation
hmac
force-pushed
the
hmac/private-methods
branch
2 times, most recently
from
3c63de1 to
0644d79
Compare
`private` can be used in both classes and modules.
hmac
force-pushed
the
hmac/private-methods
branch
from
0644d79 to
5b99580
Compare
`Module#private_class_method` takes a symbol representing the name of a
method in the current module scope and makes that module private. This
is similar to `private`, but applies only to class (singleton) methods.
Unlike `private`, it must be called with an argument, and does not
change the ambient visibility for any subsequent method definitions.
class Foo
def public
end
def private1
end
private_class_method :private1
# This alternate form works because method definition
# returns its name as a symbol:
private_class_method def private2
end
end
hmac
force-pushed
the
hmac/private-methods
branch
from
5b99580 to
8df5aaa
Compare
At the same time, rename some classes in `private.rb` so they don't interact with identically-named modules in `calls.rb`.
hmac
force-pushed
the
hmac/private-methods
branch
from
8421f05 to
6223b16
Compare
| exists(Namespace n | | ||
| n.getAStmt() = this and | ||
| n.getAStmt() = result and | ||
| result.getName() = this.getMethodArgument().(StringlikeLiteral).getValueText() |
There was a problem hiding this comment.
This looks like a join that is bound to go wrong (i.e., joining all string literals and methods that have matching values/names). The old isRef/isDeclaredIn approach avoided this by making sure that we always join on both name and declaring class/module.
There was a problem hiding this comment.
Ah good catch - I'll see if I can resurrect the old approach whilst keeping these improvements.
| this = c.getStmt(j) and | ||
| j > i | ||
| ) | ||
| any(Private p).modifiesMethod(this.getEnclosingModule(), this.getName()) |
There was a problem hiding this comment.
This can still join on just this.getName(), so it is better to add a top-level predicate
pragma[noinline]
private predicate isDeclaredIn(MethodBase m, Namespace n, string name) {
n = m.getEnclosingModule() and
name = m.getName()
}and then change the above to
exists(Namespace n, string name |
any(Private p).modifiesMethod(n, name) and
isDeclaredIn(this, n, name)
)There was a problem hiding this comment.
Interesting! That seems to work, and from some rough testing it also looks faster as well. But I don't really understand why it is faster. Why is it that the previous version can join on just this.getName(), rather than this.getName() and this.getEnclosingModule()? Are there some docs that I could read to better understand this?
There was a problem hiding this comment.
The code above involves three relations (modifiesMethod, getEnclosingModule, and getName), so the join-orderer has to pick one of the 6 possible join-orders. The join-order modifiesMethod <-> getName <-> getEnclosingModule is the one we want to avoid. By folding getEnclosingModule and getName into a separate predicate, we are effectively making sure that modifiesMethod joins on the two relations simultaneously, and there is only one possible (good) join-order.
You can try to look at the Datalog Intermediate Language (DIL) code before and after (right-click a query run in VS Code and select "View DIL").
There was a problem hiding this comment.
Ah that makes sense! Had I thought about it a bit harder I probably should have guessed something like that 🤔 . Thanks for the explanation.
| @@ -203,7 +204,9 @@ class SingletonMethod extends MethodBase, TSingletonMethod { | |||
| * end | |||
| * ``` | |||
| */ | |||
| override predicate isPrivate() { this = any(PrivateClassMethod p).getMethod() } | |||
| override predicate isPrivate() { | |||
| any(PrivateClassMethod p).modifiesMethod(this.getEnclosingModule(), this.getName()) | |||
Extend our modelling of private methods in two ways:
privatemodelling to uses in modules, as well as classes.private_class_method, which makes class methods (def self.foo) private.