Java: Make Assignment extend BinaryExpr.#21412
Merged
aschackmull merged 4 commits intogithub:mainfrom Mar 5, 2026
Merged
Conversation
aschackmull
force-pushed
the
java/binary-assignment
branch
from
ecc4e47 to
ea77c0d
Compare
hvitved
approved these changes
Mar 5, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Java CodeQL AST/library model so that Assignment is treated as a kind of BinaryExpr, aligning the AST signature with upcoming CFG-library changes and simplifying some existing usages.
Changes:
- Make
AssignmentextendBinaryExprand adjust operator/pretty-printing behavior accordingly. - Update affected security/dataflow/control-flow/arithmetic libraries and test queries to account for assignments now being included under
BinaryExpr. - Add schema upgrade/downgrade steps plus change notes and refreshed expected test outputs.
Reviewed changes
Copilot reviewed 26 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| java/ql/test/query-tests/StringComparison/StringComparison.expected | Updates expected output formatting for string-comparison query tests. |
| java/ql/test/library-tests/guards/GuardsInline.ql | Adjusts guard pretty-printing in tests (spacing/format). |
| java/ql/test-kotlin2/library-tests/exprs/binop.ql | Updates Kotlin library test to avoid treating assignments as generic binary ops. |
| java/ql/test-kotlin1/library-tests/exprs/binop.ql | Same as above for Kotlin1 test pack. |
| java/ql/lib/upgrades/9f6026c400996c13842974b24f076a486ad1f69c/upgrade.properties | Adds an upgrade step describing inclusion of @assignment in @binaryexpr. |
| java/ql/lib/upgrades/9f6026c400996c13842974b24f076a486ad1f69c/semmlecode.dbscheme | Upgrade dbscheme reflecting @assignment included in @binaryexpr. |
| java/ql/lib/upgrades/9f6026c400996c13842974b24f076a486ad1f69c/old.dbscheme | Prior dbscheme for the above upgrade step. |
| java/ql/lib/semmle/code/java/security/RandomQuery.qll | Adjusts predictable-seed logic to avoid unintended assignment handling under BinaryExpr. |
| java/ql/lib/semmle/code/java/security/NumericCastTaintedQuery.qll | Simplifies LHS extraction due to the new type hierarchy. |
| java/ql/lib/semmle/code/java/security/InsecureRandomnessQuery.qll | Avoids treating assignment expressions as generic binary operand flow steps. |
| java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SignAnalysisSpecific.qll | Simplifies operand access after assignment becomes a binary expression. |
| java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll | Switches from a custom BinaryExpr wrapper to J::BinaryExpr. |
| java/ql/lib/semmle/code/java/dataflow/Nullness.qll | Refines unboxing logic to account for assignments now being binary expressions. |
| java/ql/lib/semmle/code/java/controlflow/Guards.qll | Removes local binary-expression wrapper and uses BinaryExpr directly. |
| java/ql/lib/semmle/code/java/arithmetic/Overflow.qll | Simplifies operand retrieval logic now that assignments are modeled as BinaryExpr. |
| java/ql/lib/semmle/code/java/PrettyPrintAst.qll | Consolidates pretty-printing by relying on binary-expr printing for assignments. |
| java/ql/lib/semmle/code/java/Expr.qll | Core change: Assignment extends BinaryExpr; operator strings/printing updated. |
| java/ql/lib/config/semmlecode.dbscheme | Updates global schema union so @binaryexpr includes @assignment. |
| java/ql/lib/change-notes/2026-03-04-binary-assignment.md | Adds a change note documenting the type hierarchy change. |
| java/ql/consistency-queries/BinaryExpr.ql | Updates consistency query to match new operator-string conventions. |
| java/downgrades/de4ded61c8ae83f829aedaf05be73307ba25ca40/upgrade.properties | Adds a downgrade step to remove @assignment from @binaryexpr. |
| java/downgrades/de4ded61c8ae83f829aedaf05be73307ba25ca40/semmlecode.dbscheme | Downgrade dbscheme reflecting removal of @assignment from @binaryexpr. |
| java/downgrades/de4ded61c8ae83f829aedaf05be73307ba25ca40/old.dbscheme | Prior dbscheme for the above downgrade step. |
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
735
to
741
| /** The operands of this binary expression are `e` and `f`, in either order. */ | ||
| predicate hasOperands(Expr e, Expr f) { | ||
| exists(int i | i in [0 .. 1] | | ||
| e.isNthChildOf(this, i) and | ||
| f.isNthChildOf(this, 1 - i) | ||
| ) | ||
| } |
There was a problem hiding this comment.
The docstring for BinaryExpr.hasOperands says the operands are "in either order". Now that Assignment is a BinaryExpr, this wording can be misleading for order-sensitive expressions like assignments. Consider clarifying the comment to note that hasOperands ignores operand order and that callers needing operand order should use getLeftOperand()/getRightOperand() (or Assignment.getDest()/getRhs()).
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * The class `Assignment` now extends `BinaryExpr`. Uses of `BinaryExpr` may in some cases need slight adjustment. |
There was a problem hiding this comment.
The change note is quite vague about what kind of adjustments may be needed after Assignment becomes a BinaryExpr. Consider explicitly calling out that BinaryExpr queries/predicates that intend to target only operator expressions (e.g., using getAnOperand()/hasOperands()) may now also match assignments and may need an explicit exclusion of Assignment/AssignExpr/AssignOp depending on intent.
Suggested change
| * The class `Assignment` now extends `BinaryExpr`. Uses of `BinaryExpr` may in some cases need slight adjustment. | |
| * The class `Assignment` now extends `BinaryExpr`. Uses of `BinaryExpr` may in some cases need slight adjustment: in particular, queries or predicates that operate on `BinaryExpr` and are intended to target only operator expressions (for example, those using `getAnOperand()` or `hasOperands()`) may now also match assignments, and may need to explicitly exclude `Assignment`/`AssignExpr`/`AssignOp` depending on intent. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is in order to align with future additions to the AST signature in the CFG library. It turns out that this change also allows small simplifications in a number of existing use-cases.