Skip to content

Bound.qll - Replace utility for range analysis duplicate across java and cs with shared file#21900

Open
BazookaMusic wants to merge 14 commits into
mainfrom
bazookamusic/range-analysis-bound-move-to-shared
Open

Bound.qll - Replace utility for range analysis duplicate across java and cs with shared file#21900
BazookaMusic wants to merge 14 commits into
mainfrom
bazookamusic/range-analysis-bound-move-to-shared

Conversation

@BazookaMusic
Copy link
Copy Markdown

We have this doc of identical files across languages: https://github.com/github/codeql/blob/main/config/identical-files.json

As an exercise, I created a shared library and removed the duplication between CS and Java.

@BazookaMusic BazookaMusic marked this pull request as ready for review June 1, 2026 16:10
Copilot AI review requested due to automatic review settings June 1, 2026 16:10
@BazookaMusic BazookaMusic requested review from a team as code owners June 1, 2026 16:10
Copy link
Copy Markdown
Contributor

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 reduces Java/C# duplication in range analysis by introducing a shared Bound library under codeql/rangeanalysis and switching the language-specific libraries to instantiate it via per-language definition modules.

Changes:

  • Added shared/rangeanalysis/codeql/rangeanalysis/Bound.qll as the shared implementation.
  • Updated Java and C# bound libraries to use the shared implementation via BoundSpecific::BoundDefs.
  • Updated packaging/config to support the new shared dependency and removed the now-obsolete “identical files” entry.
Show a summary per file
File Description
shared/rangeanalysis/codeql/rangeanalysis/Bound.qll Introduces shared bound abstractions and a parameterized Bound module.
java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/BoundSpecific.qll Defines Java bindings (BoundDefs) implementing the shared bound signature.
java/ql/lib/semmle/code/java/dataflow/Bound.qll Replaces the Java-specific bound implementation with an instantiation of the shared module.
csharp/ql/lib/semmle/code/csharp/dataflow/internal/rangeanalysis/BoundSpecific.qll Defines C# bindings (BoundDefs) implementing the shared bound signature.
csharp/ql/lib/semmle/code/csharp/dataflow/Bound.qll Replaces the C#-specific bound implementation with an instantiation of the shared module.
csharp/ql/lib/qlpack.yml Adds the codeql/rangeanalysis dependency required by the new shared import.
config/identical-files.json Removes the Java/C# “Bound” identical-files entry since the code is now shared.

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 2

Comment thread shared/rangeanalysis/codeql/rangeanalysis/Bound.qll Outdated
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

Very nice! Well done 😄
We should also run DCA after the review process is done.

interestingExprBound(e) and
not exists(SsaVariable v | e = v.getAUse())
}
private module BoundImpl = SharedBound::Bound<CS::Location, BoundSpecific::BoundDefs>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
private module BoundImpl = SharedBound::Bound<CS::Location, BoundSpecific::BoundDefs>;
import SharedBound::Bound<CS::Location, BoundSpecific::BoundDefs>

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, there's a reason for naming the resulting module before importing it. It can help provide shorter names in RA/DIL. There's a mechanism in the compiler that makes use of such module aliases to try to shorten names, so we could have BoundImpl::foo as opposed to SharedBound::Bound<CS::Location, BoundSpecific::BoundDefs>::foo.
And it's actually not insignificant - before introducing that mechanism we had a brief period of time where the evaluator log associated with certain runs of the data flow library could yield OOM - simply due to very long names.

Copy link
Copy Markdown
Author

@BazookaMusic BazookaMusic Jun 3, 2026

Choose a reason for hiding this comment

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

Interesting that this mechanism doesn't do something like SharedBound[stable hash of generic arguments appended] so we don't have to name it ourselves. But I will add it back.

I guess we'd need to keep a separate mapping from aliases to modules if we did name generation and there's already one for aliases.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Uh, I didn't know. @aschackmull : Since this is not insignificant, do you know, whether the foundations team will address the issue (either by somehow shortening the names or making it impossible to import without first making an alias)?

Comment thread csharp/ql/lib/semmle/code/csharp/dataflow/Bound.qll Outdated
Comment thread java/ql/lib/semmle/code/java/dataflow/internal/rangeanalysis/BoundSpecific.qll Outdated
Comment thread java/ql/lib/semmle/code/java/dataflow/Bound.qll Outdated
Comment thread shared/rangeanalysis/codeql/rangeanalysis/Bound.qll Outdated
Comment thread java/ql/lib/semmle/code/java/dataflow/Bound.qll Fixed
Comment thread shared/rangeanalysis/change-notes/released/1.0.52.md Outdated
@BazookaMusic BazookaMusic added the no-change-note-required This PR does not need a change note label Jun 3, 2026
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel left a comment

Choose a reason for hiding this comment

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

LGTM 👍
We should run DCA for Java and C# before merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C# 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.

5 participants