Ruby: add overlay annotations to AST/CFG/SSA layers #19989
Conversation
nickrolfe
force-pushed
the
nickrolfe/ruby-annotations
branch
3 times, most recently
from
4a725ba to
3c4a528
Compare
|
I think this is technically a breaking change and therefore needs a change note: if a class in a custom query overrides a predicate from a class that's now local, it's a compiler error. Reviewers should check that no supported extension point is becoming local. |
ruby/ql/lib/codeql/ruby/ast/Call.qll
Outdated
| @@ -38,6 +41,7 @@ class Call extends Expr instanceof CallImpl { | |||
| * foo :bar "baz", qux: 123 | |||
| * ``` | |||
| */ | |||
| overlay[global] | |||
There was a problem hiding this comment.
I think this predicate can reasonably be made local by replacing
p.getKey().getConstantValue().isSymbol(keyword)below with
keyword = p.getKey().(SymbolLiteral).(StringlikeLiteralImpl).getStringValue()(requires an private import internal.Literal import).
| @@ -425,6 +429,7 @@ class StringConcatenation extends Expr, TStringConcatenation { | |||
| * "foo" "bar#{ n }" | |||
| * ``` | |||
| */ | |||
| overlay[global] | |||
There was a problem hiding this comment.
I think this can be made local:
final string getConcatenatedValueText() {
forall(StringLiteral c | c = this.getString(_) |
exists(c.(StringlikeLiteralImpl).getStringValue())
) and
result =
concat(string valueText, int i |
valueText = this.getString(i).(StringlikeLiteralImpl).getStringValue()
|
valueText order by i
)
}(requires private import internal.Literal).
There was a problem hiding this comment.
The pragma still needs to be removed.
There was a problem hiding this comment.
Unfortunately, the test failure shows that this change isn't equivalent. It no longer gives a result for "foo" "bar#{ 1 * 1 }" 'baz', since StringlikeLiteralImpl.getStringValue() doesn't give a result for the string with the interpolation.
The qldoc for getConcatenatedValueText() is actually wrong then, for the current behaviour.
I think I would prefer backing this change out — along with the other, similar changes — and keeping it global. At least in this PR. What do you think?
| @@ -40,18 +43,22 @@ class MethodBase extends Callable, BodyStmt, Scope, TMethodBase { | |||
| * Holds if this method is public. | |||
| * Methods are public by default. | |||
| */ | |||
| overlay[global] | |||
There was a problem hiding this comment.
It feels like all of these should be local, but it is probably not important.
| @@ -203,6 +206,7 @@ class HashPattern extends CasePattern, THashPattern { | |||
| } | |||
|
|
|||
| /** Gets the value for a given key name. */ | |||
| overlay[global] | |||
There was a problem hiding this comment.
Can be made local:
key = this.getKey(i).(StringlikeLiteralImpl).getStringValue()| @@ -310,6 +317,7 @@ module ExprNodes { | |||
| final ExprCfgNode getAnArgument() { result = this.getArgument(_) } | |||
|
|
|||
| /** Gets the keyword argument whose key is `keyword` of this call. */ | |||
| overlay[global] | |||
There was a problem hiding this comment.
Again, this can be made local.
| @@ -469,19 +474,24 @@ class ParameterExt extends TParameterExt { | |||
| } | |||
| } | |||
|
|
|||
| overlay[global] | |||
There was a problem hiding this comment.
What is it that makes this necessary?
There was a problem hiding this comment.
There must have been some intermediate state where I needed it to fix compilation errors, but I can't see any reason why it's needed now. I'll remove it.
nickrolfe
force-pushed
the
nickrolfe/ruby-annotations
branch
from
3c4a528 to
21dc396
Compare
| @@ -375,10 +378,12 @@ private module Cached { | |||
| Impl::uncertainWriteDefinitionInput(def, result) | |||
| } | |||
|
|
|||
| overlay[global] | |||
There was a problem hiding this comment.
I can remove it here, but the BarrierGuards module below needs to be global (at least to make the compiler happy).
nickrolfe
force-pushed
the
nickrolfe/ruby-annotations
branch
from
21dc396 to
1eda55d
Compare
nickrolfe
force-pushed
the
nickrolfe/ruby-annotations
branch
from
1eda55d to
e1f2433
Compare
|
As mentioned in the comment above, I backed out some of the requested changes for getting string-like values, so they are global again. I've also added a change-note. |
Almost everything in those libraries is marked as
overlay[local], with the exception that everything depending on resolution of constants and call targets isoverlay[global].Overlay compilation is currently disabled for Ruby, so the annotations won't currently have any effect on compilation or evaluation.
I've run Ruby DCA experiments on these changes, combined with an additional commit enabling overlay compilation, showing that they give 100% alert accuracy on
nightly.qlsand a curated source suite of ~200 sources.The QL-for-QL changes are just to ensure that the compiler is happy; I haven't tested them with actual overlay analysis.