C++: Make node.asExpr() instanceof ClassAggregateLiteral satisfiable
#19501
+71
−8
Conversation
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
… instanceof ClassAggregateLiteral' holds for this new node subclass.
There was a problem hiding this comment.
Pull Request Overview
This PR enables node.asExpr() instanceof ClassAggregateLiteral by mapping the final post-update node of a class aggregate initialization back to its aggregate literal, and updates tests and change notes accordingly.
- Introduces
ClassAggregateInitializerPostUpdateNodeand related predicates inExprNodes.qllto link post-update nodes toClassAggregateLiteral. - Updates existing IR-based dataflow tests to expect
{...}nodes for class aggregate literals. - Adds new test cases in
asExpr/test.cppfor class aggregate literal expressions and documents missing support for array aggregates.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cpp/ql/test/library-tests/dataflow/fields/ir-path-flow.expected | Swapped out post-update labels for {...} to reflect the class aggregate literal. |
| cpp/ql/test/library-tests/dataflow/dataflow-tests/localFlow-ir.expected | Inserted {...} expectation for class aggregate literal in local flow tests. |
| cpp/ql/test/library-tests/dataflow/asExpr/test.cpp | Added asExpr={...} checks for struct aggregate literals; marked missing for arrays. |
| cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/ExprNodes.qll | Implemented post-update mapping for ClassAggregateLiteral and extended expr nodes. |
| cpp/ql/lib/change-notes/2025-05-15-class-aggregate-literals.md | Documented the fix for asExpr() on class aggregate literals. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jketema
approved these changes
May 16, 2025
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.
The C++ IR represents an aggregate literal
x = {.a = 1, .b = 2}as a sequence of field writes:and as such there is no instruction for which
getUnconvertedResultExpression()gives the aggregate literal. And sinceNode.asExpr()piggybacks ongetUnconvertedResultExpression()for most nodesnode.asExpr() instanceof AggregateLiteralalso never holds. This gotcha has bitten users several times.This PR solves the problem for class aggregate literals. Since class aggregate literals are represented as a sequence of field writes (see above), we can pick the last field write's post-update node as the node to represent the aggregate literal expression. Semantically this makes sense since the last post-update node represents the state of the object after the last field has been initialized.
Sadly, since we don't model array writes using post-update nodes we can't use the same trick for
ArrayAggregateLiterals. However, I think we should do that eventually. Once we do that we can apply a similar trick to that case. For now I think this solution at least solves half of the problems that's been reported.