Skip to content

Rust: Fix performance issue with additionalExternalFile #21671

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.

Sign up for GitHub

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
geoffw0 merged 2 commits into from
Apr 10, 2026
Merged

Rust: Fix performance issue with additionalExternalFile #21671

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
95681bf
Rust: Fix performance issue with File.fromSource.
geoffw0 Mar 9, 2026
e72c116
Rust: Proposed improved solution.
geoffw0 Apr 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion rust/ql/lib/codeql/files/FileSystem.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,16 @@ extensible predicate additionalExternalFile(string relativePath);

/** A file. */
class File extends Container, Impl::File {
pragma[nomagic]
private string getRelativePath0() { result = this.getRelativePath() }

/**
* Holds if this file was extracted from the source code of the target project
* (rather than another location such as inside a dependency).
*/
predicate fromSource() {
exists(ExtractorStep s | s.getAction() = "Extract" and s.getFile() = this) and
not additionalExternalFile(this.getRelativePath())
not additionalExternalFile(this.getRelativePath0())
Copy link
Copy Markdown
Contributor

@hvitved hvitved Apr 9, 2026

Choose a reason for hiding this comment

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

Would it work if you instead do

pragma[nomagic]
private string isAdditionalExternalFile() { additionalExternalFile(this.getRelativePath()) }

and then call not this.isAdditionalExternalFile() here?

Copy link
Copy Markdown
Contributor

@MathiasVP MathiasVP Apr 9, 2026

Choose a reason for hiding this comment

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

To explain a bit as to why Tom is suggesting this: When the compiler encounters a negation it needs to generate an antijoin which subtracts something from the pipeline. That something must be a materialized relation. So when you do not additionalExternalFile(this.getRelativePath()) the evaluator will:

  • Evaluate additionalExternalFile(this.getRelativePath())
  • Materialize that relation (so it can be used in an antijoin)
  • Generate an antijoin to subtract this materialized relation from the pipeline

With your change, the only thing that changes is that we now evaluate and materialize additionalExternalFile(this.getRelativePath0()) instead. Is that better? From your 3% comment it sounds like it is, but there's no guarantee that it is better 🤷 There can still be bad magic creeping into the materialized relation.

With Tom's suggestion the evaluator:

  • Evaluates the (non-magic'd!) isAdditionalExternalFile
  • Materialize it (we have to because it's a non-magic'd predicate. So it cannot be inlined)
  • Generate an antijoin to subtract this materialized relation from the pipeline

Now the entire materialized body of the negation is guaranteed to not contain magic

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Apr 9, 2026

Choose a reason for hiding this comment

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

It's definitely better as it is, but what you propose may well be more robust. I'll switch to that and run DCA again to confirm.

Copy link
Copy Markdown
Contributor Author

@geoffw0 geoffw0 Apr 9, 2026

Choose a reason for hiding this comment

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

The new run LGTM.

}

/**
Expand Down
Loading