Skip to content

Java: Use the shared BasicBlocks library. #19505

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 5 commits into
base: main
Choose a base branch
from

Conversation

aschackmull
Copy link
Contributor

What the title says. Also, since BasicBlock is now no longer cast-compatible with ControlFlowNode there's no reason to use all the bb-prefixed names, so I've deprecated those and switched to the defaults provided by the shared library.

@Copilot Copilot AI review requested due to automatic review settings May 16, 2025 12:28
@aschackmull aschackmull requested review from a team as code owners May 16, 2025 12:28
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

Migration to the shared BasicBlock library and deprecation of legacy bb* methods and predicates, updating call sites to use the new BasicBlock API.

  • Replaced legacy dominance and successor/predecessor methods (bbDominates, getABBSuccessor, etc.) with the shared-library methods (dominates, getASuccessor, etc.).
  • Updated imports, removed unused Dominance imports, and augmented qlpack.yml to depend on codeql/controlflow.
  • Introduced SuccessorType definitions, updated BasicBlocks.qll to use the shared BasicBlock implementation, and added change notes.

Reviewed Changes

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

Show a summary per file
File Description
java/ql/lib/semmle/code/java/security/PathSanitizer.qll Swapped bbDominates for dominates.
java/ql/lib/semmle/code/java/security/AndroidWebViewCertificateValidationQuery.qll Swapped bbPostDominates for postDominates.
java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/SsaReadPositionSpecific.qll Updated idOf to use getFirstNode().getAstNode().
java/ql/lib/semmle/code/java/dataflow/internal/SsaImpl.qll Removed Dom import; switched to immediatelyDominates and getASuccessor.
java/ql/lib/semmle/code/java/dataflow/internal/DataFlowUtil.qll Replaced getABBSuccessor calls with getASuccessor.
java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll Swapped bbIDominates and getABBSuccessor for shared APIs.
java/ql/lib/semmle/code/java/dataflow/internal/BaseSSA.qll Removed Dom import; switched to shared dominator/successor APIs.
java/ql/lib/semmle/code/java/dataflow/TypeFlow.qll Replaced getABBPredecessor and bbDominates with new methods.
java/ql/lib/semmle/code/java/dataflow/RangeAnalysis.qll Updated idOf and successor calls to use getFirstNode/getASuccessor.
java/ql/lib/semmle/code/java/dataflow/Nullness.qll Replaced all getABBSuccessor/getAST* with new basic-block methods.
java/ql/lib/semmle/code/java/controlflow/internal/GuardsLogic.qll Updated dominance checks to use dominates and new successor logic.
java/ql/lib/semmle/code/java/controlflow/UnreachableBlocks.qll Replaced predecessor and dominance calls with shared methods.
java/ql/lib/semmle/code/java/controlflow/SuccessorType.qll Added new SuccessorType hierarchy.
java/ql/lib/semmle/code/java/controlflow/Paths.qll Swapped legacy bb* calls for shared successor/dominance methods.
java/ql/lib/semmle/code/java/controlflow/Guards.qll Updated getTestSuccessor, dominance checks to new form.
java/ql/lib/semmle/code/java/controlflow/Dominance.qll Deprecated old predicates, forwarding to new methods.
java/ql/lib/semmle/code/java/controlflow/BasicBlocks.qll Imported and extended shared BasicBlock implementation.
java/ql/lib/qlpack.yml Added codeql/controlflow dependency.
java/ql/lib/change-notes/2025-05-16-shared-basicblocks.md Documented deprecation and migration to shared BasicBlock.
cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticCFG.qll Renamed bbDominates to dominates.
Comments suppressed due to low confidence (3)

java/ql/lib/semmle/code/java/controlflow/BasicBlocks.qll:6

  • [nitpick] The Dominance import is now redundant since the shared BasicBlock API provides dominance predicates; consider removing this import to clean up unused dependencies.
import Dominance

java/ql/lib/semmle/code/java/controlflow/SuccessorType.qll:1

  • The new SuccessorType hierarchy introduced here doesn’t appear to have dedicated tests; consider adding unit tests to cover NormalSuccessor, ConditionalSuccessor, and ExceptionSuccessor handling.
import java

java/ql/lib/semmle/code/java/dataflow/internal/DataFlowPrivate.qll:86

  • The BasicBlock type isn’t aliased in this module; you should add private import java as J and class BasicBlock = J::BasicBlock; at the top to ensure the correct type is used.
BasicBlock getImmediateBasicBlockDominator(BasicBlock bb) {

@@ -4,91 +4,130 @@

import java
import Dominance
private import codeql.controlflow.BasicBlock as Bb

Check warning

Code scanning / CodeQL

Names only differing by case Warning

Bb is only different by casing from BB that is used elsewhere for modules.
* That is, all paths reaching `bb` from the entry point basic block must
* go through this basic block.
*/
predicate dominates(BasicBlock node) { super.dominates(node) }

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for node, but the QLDoc mentions bb
}

/** Exit points for basic-block control-flow. */
private predicate bbSink(BasicBlock exit) { exit.getLastNode() instanceof ControlFlow::ExitNode }

/** Reversed `bbSucc`. */
private predicate bbPred(BasicBlock post, BasicBlock pre) { post = pre.getABBSuccessor() }
private predicate bbPred(BasicBlock post, BasicBlock pre) { post = pre.getASuccessor() }

Check warning

Code scanning / CodeQL

Missing QLDoc for parameter Warning

The QLDoc has no documentation for post, or pre, but the QLDoc mentions bbSucc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant