Skip to content

Java: Fix assert CFG by properly tagging the false successor. #19883

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

Merged
merged 3 commits into from
Jun 26, 2025

Conversation

aschackmull
Copy link
Contributor

Follow-up for #19573 to fix the small issue with asserts.

@Copilot Copilot AI review requested due to automatic review settings June 26, 2025 08:21
@aschackmull aschackmull requested a review from a team as a code owner June 26, 2025 08:21
@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Jun 26, 2025
@github-actions github-actions bot added the Java label Jun 26, 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.

Pull Request Overview

This PR adds a dedicated CFG node for failing assert statements and updates tests accordingly.

  • Remove stale expected warnings for assert-false paths in Nullness tests
  • Update test comment to reflect that the correlated conditions are now handled
  • Introduce TAssertThrowNode and AssertThrowNode in the CFG and wire up assert failures

Reviewed Changes

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

File Description
java/ql/test/query-tests/Nullness/NullMaybe.expected Remove three outdated expected-warning lines for xs.
java/ql/test/query-tests/Nullness/C.java Change comment on xs[0] = 42; from “FP...” to “OK”.
java/ql/lib/semmle/code/java/ControlFlowGraph.qll Add TAssertThrowNode type and AssertThrowNode class; update CFG rules for assert.
Comments suppressed due to low confidence (1)

java/ql/lib/semmle/code/java/ControlFlowGraph.qll:103

  • [nitpick] The new TAssertThrowNode(AssertStmt s) case in the TNode newtype lacks a brief comment. Please add documentation describing its purpose (i.e., modeling a failing assert).
    TExitNode(Callable c) { exists(c.getBody()) } or

Copy link
Contributor

@owen-mc owen-mc left a comment

Choose a reason for hiding this comment

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

Great to see a new CFG node being added.

@aschackmull aschackmull merged commit 321a4af into github:main Jun 26, 2025
15 checks passed
@aschackmull aschackmull deleted the java/fix-assert-cfg branch June 26, 2025 09:43
@aschackmull
Copy link
Contributor Author

Great to see a new CFG node being added.

I've got more: #19885

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants