Ruby: add overlay annotations to AST/CFG/SSA layers #19989
Conversation
nickrolfe
force-pushed
the
nickrolfe/ruby-annotations
branch
2 times, most recently
from
770dd30 to
4a725ba
Compare
nickrolfe
force-pushed
the
nickrolfe/ruby-annotations
branch
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. |
| @@ -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).
| @@ -41,6 +44,7 @@ class IntegerLiteral extends NumericLiteral instanceof IntegerLiteralImpl { | |||
| /** Gets the numerical value of this integer literal. */ | |||
| final int getValue() { result = super.getValue() } | |||
|
|
|||
| overlay[global] | |||
There was a problem hiding this comment.
I would have thought that these were not needed when the root def has overlay[global]?
| @@ -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?
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.