-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Java: Replace SSA wrapper classes with shared implementation. #20761
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
Conversation
e54b277 to
cf8d1bd
Compare
|
|
||
| import java | ||
| private import internal.SsaImpl | ||
| import internal.SsaImpl::Ssa as Ssa |
Check warning
Code scanning / CodeQL
Names only differing by case Warning
| } | ||
|
|
||
| private predicate upcastEnhancedForStmtAux(BaseSsaUpdate v, RefType t, RefType t1, RefType t2) { | ||
| private predicate upcastEnhancedForStmtAux( |
Check warning
Code scanning / CodeQL
Candidate predicate not marked as `nomagic` Warning
upcastEnhancedForStmt
| i = 0 | ||
| ) | ||
| } | ||
| module Ssa = Impl::MakeSsa<SsaInput>; |
Check warning
Code scanning / CodeQL
Names only differing by case Warning
| } | ||
| } | ||
|
|
||
| module Ssa = Impl::MakeSsa<SsaInput>; |
Check warning
Code scanning / CodeQL
Names only differing by case Warning
aad896a to
cb33a68
Compare
|
I've left several deprecation annotations for a followup PR to avoid having to do a submodule bump here. |
There was a problem hiding this 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 modernizes the SSA (Static Single Assignment) API for Java CodeQL by renaming classes and methods to improve clarity and consistency. The changes deprecate old class and method names while introducing clearer alternatives.
- Renames core SSA classes (e.g.,
SsaVariable→SsaDefinition,SsaPhiNode→SsaPhiDefinition) - Renames methods for consistency (e.g.,
getAUse()→getARead(),getCfgNode()→getControlFlowNode()) - Introduces new specialized SSA classes with clearer semantics (
SsaCapturedDefinition,SsaImplicitCallDefinition, etc.)
Reviewed Changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| shared/ssa/codeql/ssa/Ssa.qll | Adds toString methods and renames SsaPhiDefinition class definition |
| java/ql/lib/semmle/code/java/dataflow/SSA.qll | Core SSA API with renamed classes and deprecated old names |
| java/ql/lib/semmle/code/java/dataflow/internal/BaseSSA.qll | Base SSA implementation updated with new class names |
| java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll | Internal SSA implementation restructuring |
| java/ql/test/**/*.ql | Test queries updated to use new API |
| java/ql/test/**/*.expected | Generated test expectations updated with new toString formats |
| java/ql/lib/**/*.qll | Various library files updated to use new API |
| java/ql/src/**/*.qll | Source queries updated to use new API |
| java/ql/lib/semmle/code/java/Expr.qll | Adds new VariableWrite class for SSA integration |
| java/ql/lib/change-notes/2025-10-07-ssa-api-updates.md | Documents the deprecation of old SSA API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ger tracked as SSA variables.
3a30e2a to
4a58a01
Compare
hvitved
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; my main concern is about caching.
| private import Cached | ||
|
|
||
| cached | ||
| private module Cached { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you check that this cached stage is collapsed with the one inside Impl::MakeSsa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. See below.
| } | ||
| } | ||
|
|
||
| module Ssa = Impl::MakeSsa<SsaInput>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question: Did you check that the cached stages are collapsed?
| result = f.getAnAccess() and | ||
| f.isFinal() and | ||
| f.getInitializer() = clearlyNotNullExpr(reason) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of this logic is duplicated inside clearlyNotNull(SsaDefinition v, Expr reason), so perhaps factor out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but we want to cover the case in both predicates, so we can't quite eliminate either of the cases. We could factor out another mutually recursive predicate clearlyNotNullField(Field f, Expr reason) and refer to that twice, but it's a bit borderline whether there's actually enough common code for that, so I'm leaning slightly towards keeping it as-is.
|
Regarding caching: I've compared the outputs of the stageoverlap script and the number of stages is unchanged. Two stages change as expected: BaseSSA stage after: SSA stage before: SSA stage after: |
hvitved
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for verifying that caching works as expected.
This migrates the Java SSA libraries to use the shared SSA wrapper classes.
Commit-by-commit review is very much encouraged.