Skip to content

Python: Improve performance of FileNotClosed query by using an explicit fastTC #19411

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

Conversation

joefarebrother
Copy link
Contributor

Thanks to @nickrolfe and @alexet for help on slack.

@Copilot Copilot AI review requested due to automatic review settings April 29, 2025 12:27
@joefarebrother joefarebrother requested a review from a team as a code owner April 29, 2025 12:27
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 improves performance of the FileNotClosed query by replacing the built-in transitive closure calls with an explicit fastTC implementation.

  • Replaces getASuccessor* calls with dedicated predicates (cfgGetASuccessorStar and cfgGetASuccessorPlus) using fastTC
  • Introduces helper predicates (cfgGetASuccessor, cfgGetASuccessorPlus, cfgGetASuccessorStar) to clearly define control flow transitions
Comments suppressed due to low confidence (1)

python/ql/src/Resources/FileNotAlwaysClosedQuery.qll:67

  • [nitpick] Consider adding a brief inline comment to explain the purpose of cfgGetASuccessorPlus and how fastTC improves performance in this context.
pragma[inline] private predicate cfgGetASuccessorPlus(ControlFlowNode src, ControlFlowNode sink) = fastTC(cfgGetASuccessor/2)(src, sink)

fastTC(cfgGetASuccessor/2)(src, sink)

pragma[inline]
private predicate cfgGetASuccessorStar(ControlFlowNode src, ControlFlowNode sink) {
Copy link
Preview

Copilot AI Apr 29, 2025

Choose a reason for hiding this comment

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

[nitpick] It may be helpful to add documentation clarifying the base condition 'src = sink' in cfgGetASuccessorStar to outline its role in implementing the transitive closure.

Suggested change
private predicate cfgGetASuccessorStar(ControlFlowNode src, ControlFlowNode sink) {
private predicate cfgGetASuccessorStar(ControlFlowNode src, ControlFlowNode sink) {
// Base condition: A node is always reachable from itself, ensuring reflexivity.

Copilot uses AI. Check for mistakes.

@joefarebrother joefarebrother added the no-change-note-required This PR does not need a change note label Apr 29, 2025
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

Looks good to me (assuming the performance test is okay).

@joefarebrother joefarebrother merged commit 7106475 into github:main Apr 30, 2025
15 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants