Skip to content

Java: Assume normal termination in post-dominance. #20163

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

aschackmull
Copy link
Contributor

This is a followup for #19885, which made this possible.

The concept of post-dominance may either assume normal termination of a callable, or try to account for exceptions. This PR switches Java to the former, which is in line with the shared CFG library. Indeed, trying to account for exceptions in post-dominance usually isn't what's desired, and it's also somewhat meaningless, since many potential exception edges are elided in the CFG if there's no enclosing try-block. As such, I'll consider this to be a bug-fix (and a somewhat minor one given that post-dominance is only rarely used).

@aschackmull aschackmull requested a review from a team as a code owner August 4, 2025 13:15
@Copilot Copilot AI review requested due to automatic review settings August 4, 2025 13:15
Copilot

This comment was marked as outdated.

@github-actions github-actions bot added the Java label Aug 4, 2025
@aschackmull
Copy link
Contributor Author

I've started dca with the only two queries using post-dominance: java/lazy-initialization (LazyInitStaticField.ql), java/improper-webview-certificate-validation (ImproperWebViewCertificateValidation.ql)

@aschackmull
Copy link
Contributor Author

Dca shows zero impact (as could be expected).

@aschackmull aschackmull requested a review from a team as a code owner August 5, 2025 06:58
@github-actions github-actions bot added the Kotlin label Aug 5, 2025
@aschackmull aschackmull force-pushed the java/postdom-normal branch from 85d7b7a to 273429d Compare August 5, 2025 08:33
@Copilot Copilot AI review requested due to automatic review settings August 5, 2025 08:33
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 modifies Java's post-dominance calculation to assume normal termination rather than accounting for exceptions. This aligns Java with the shared CFG library approach and fixes a bug where exception handling was inconsistent due to elided exception edges in the CFG when no try-block is present.

  • Changes post-dominance exit nodes from general ExitNode to NormalExitNode only
  • Updates test expectations to reflect the new post-dominance relationships

Reviewed Changes

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

File Description
java/ql/lib/semmle/code/java/controlflow/BasicBlocks.qll Changes post-dominance calculation to use normal exit nodes only
java/ql/test-kotlin1/library-tests/controlflow/basic/strictPostDominance.expected Updated test expectations for new post-dominance behavior
java/ql/test-kotlin2/library-tests/controlflow/basic/strictPostDominance.expected Updated test expectations for new post-dominance behavior

@aschackmull aschackmull added the no-change-note-required This PR does not need a change note label Aug 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Java Kotlin 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.

1 participant