Rust: Make crate fallback logic more conservative in path resolution library - #22495
Conversation
e35d861 to
1925c20
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Raw string ordering can select the wrong semantic version, and the uniqueness behavior lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review tier: Balanced
Findings: 1
New issues introduced by this change (2)
| Severity | Finding |
|---|---|
 |
rust/ql/lib/codeql/rust/internal/PathResolution.qll — ver is a raw Cargo version string, so this ordering is lexicographic rather than semantic: for… |
 |
rust/ql/lib/codeql/rust/internal/PathResolution.qll — The existing path-resolution fixture does not contain multiple extracted crate entities sharing the… |
What changed in this PR
Restricts Rust crate dependency fallback to a uniquely identified latest crate version, reducing path-resolution explosions.
Changes:
- Selects the latest crate version for fallback resolution.
- Adds debugging support for crate dependency edges.
| File | Description |
|---|---|
rust/ql/lib/codeql/rust/internal/PathResolution.qll |
Narrows crate fallback resolution and adds debugging support. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d432cd5 to
52e2c72
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
Looks good to me. Two small comments.
Huge speedup on reflaxe-rust. Some of the metrics that we usually like going up are going down. That's to be expected, but I wonder if, for the non-unique crates, it could make sense to pick one of them based on some arbitrary but stable thing (that's not the version)?
| * Pre-release information and build metadata is not yet supported. | ||
| */ | ||
| bindingset[orig] | ||
| string normalizeSemVer(string orig) { |
There was a problem hiding this comment.
The name "normalize" made me initially think this was something about turning 1.023.0 into 1.23.0. But it's not really about normalizing, it's about padding to make make lexicographic order coincide with semver order. What about calling it padSemVer?
There was a problem hiding this comment.
I just kept the name as-is from Go/JS (except I changed casing).
There was a problem hiding this comment.
Yes, but we can still change it?
There was a problem hiding this comment.
OK, I'll change it ;-)
There was a problem hiding this comment.
Done. I also realized that Ruby had its own (more tolerant) implementation, which is now in the shared library.
| module; | ||
|
|
||
| bindingset[str] | ||
| private string leftPad(string str) { result = ("000" + str).suffix(str.length()) } |
There was a problem hiding this comment.
I've seen quite a few version components in Rust at tripple digits. Maybe throw in one more 0 for good measure?
It might make sense to do something based on the folder structure, but I'd prefer to get this in now, and then we can do it later if needed. |
52e2c72 to
9207c4b
Compare
paldepind
left a comment
paldepind
left a comment
There was a problem hiding this comment.
Nice to get the Ruby version de-duplicated as well 😎
f645ecc to
df7d49d
Compare
When doing a QA run for #21795, I noticed that our fallback logic for crate dependencies can sometimes lead to combinatorial explosions. This PR alleviates this by only applying the fallback logic to crates that have a unique latest version. DCA confirms that this PR resolves the performance issue, most notably on the
reflaxe-rustproject, where thehxrtcrate exists in many test copies (e.g. https://github.com/fullofcaffeine/reflaxe.rust/blob/main/test/snapshot/abstracts_conversions/intended/hxrt/Cargo.toml).