Skip to content

Conversation

@yoff
Copy link
Contributor

@yoff yoff commented May 13, 2025

No description provided.

Copilot AI review requested due to automatic review settings May 13, 2025 14:18
@yoff yoff added the Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish label May 13, 2025
@yoff yoff requested a review from a team as a code owner May 13, 2025 14:18
Copy link
Contributor

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.

Pull Request Overview

This PR extends the liveness analysis for captured variables to include exceptional exits and adds tests to verify this behavior.

  • Remove the bb.isNormal() guard in the capturedExitRead predicate to cover exception-driven exits
  • Add do_twice/retry_once helpers and corresponding tests (get_done_twice, get_retried)
  • Document the new exceptional-exit semantics in a change note

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
ruby/ql/test/query-tests/variables/DeadStoreOfLocal/DeadStoreOfLocal.rb Added do_twice and retry_once helpers and tests to ensure blocks run twice or on retry
ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll Removed the bb.isNormal() guard in capturedExitRead to include exception exits
ruby/ql/lib/change-notes/2025-05-13-captured-variables-live-more-often.md Added a note on captured variables remaining live on exceptional exit
Comments suppressed due to low confidence (1)

ruby/ql/lib/codeql/ruby/dataflow/internal/SsaImpl.qll:106

  • Update the doc comment above this predicate to mention that captured variables are also considered live on exceptional exits (not just normal exits).
private predicate capturedExitRead(Cfg::AnnotatedExitBasicBlock bb, int i, LocalVariable v) {

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.

🎉

@yoff
Copy link
Contributor Author

yoff commented May 14, 2025

Some stage timings increase, but overall everything is slightly faster. And we remove several false positives :-)

@yoff yoff merged commit 3fa563b into github:main May 14, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Awaiting evaluation Do not merge yet, this PR is waiting for an evaluation to finish documentation Ruby

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants