Skip to content

Ruby: add overlay annotations to AST/CFG/SSA layers #19989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nickrolfe
Copy link
Contributor

@nickrolfe nickrolfe commented Jul 7, 2025

Almost everything in those libraries is marked as overlay[local], with the exception that everything depending on resolution of constants and call targets is overlay[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.qls and 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.

@nickrolfe nickrolfe force-pushed the nickrolfe/ruby-annotations branch 3 times, most recently from 4a725ba to 3c4a528 Compare July 7, 2025 14:57
@nickrolfe nickrolfe marked this pull request as ready for review July 7, 2025 16:06
@Copilot Copilot AI review requested due to automatic review settings July 7, 2025 16:06
@nickrolfe nickrolfe requested review from a team as code owners July 7, 2025 16:06
@nickrolfe nickrolfe added the no-change-note-required This PR does not need a change note label Jul 7, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@jbj
Copy link
Contributor

jbj commented Jul 7, 2025

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.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, some suggestions for predicates that can be made local.

@@ -38,6 +41,7 @@ class Call extends Expr instanceof CallImpl {
* foo :bar "baz", qux: 123
* ```
*/
overlay[global]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pragma still needs to be removed.

Copy link
Contributor Author

@nickrolfe nickrolfe Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this can be made local.

@@ -469,19 +474,24 @@ class ParameterExt extends TParameterExt {
}
}

overlay[global]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is it that makes this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😌

@nickrolfe nickrolfe force-pushed the nickrolfe/ruby-annotations branch from 3c4a528 to 21dc396 Compare July 8, 2025 11:49
@@ -375,10 +378,12 @@ private module Cached {
Impl::uncertainWriteDefinitionInput(def, result)
}

overlay[global]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can remove it here, but the BarrierGuards module below needs to be global (at least to make the compiler happy).

@nickrolfe nickrolfe force-pushed the nickrolfe/ruby-annotations branch from 21dc396 to 1eda55d Compare July 8, 2025 12:14
@nickrolfe nickrolfe force-pushed the nickrolfe/ruby-annotations branch from 1eda55d to e1f2433 Compare July 9, 2025 10:59
@nickrolfe
Copy link
Contributor Author

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.

@nickrolfe nickrolfe removed the no-change-note-required This PR does not need a change note label Jul 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants