Skip to content

Rust: upgrade to rust-analyzer 0.0.300 - #20055

Merged
redsun82 merged 29 commits into
mainfrom
redsun82/cargo-upgrade-2
Aug 25, 2025
Merged

Rust: upgrade to rust-analyzer 0.0.300#20055
redsun82 merged 29 commits into
mainfrom
redsun82/cargo-upgrade-2

Conversation

@redsun82

@redsun82redsun82 commented Jul 15, 2025

Copy link
Copy Markdown
Contributor

The upgrade script is particularly complex. To test it out, I have:

  • compiled the extractor on main
  • run the ForTypeRepr, WherePred and TypeBound tests. The only change I observed were locations (for ForBinder instances reconstructed with the upgrade script, the location does not include the for keyword).

@github-actionsgithub-actionsBot added the Rust Pull requests that update Rust code label Jul 15, 2025
Comment threadrust/ql/test/extractor-tests/generated/AsmExpr/AsmExpr.ql Dismissed
toBeTested(x) and not x.isUnknown() and getExtendedCanonicalPath = x.getExtendedCanonicalPath()
}

query predicate getCrateOrigin(AsmExpr x, string getCrateOrigin) {

Check warning

Code scanning / CodeQL

Predicates starting with "get" or "as" should return a value Warning generated test

This predicate starts with 'get' but does not return a value.
Comment threadrust/ql/test/extractor-tests/generated/AsmExpr/AsmExpr.ql Dismissed
@redsun82redsun82 added the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 5, 2025
@redsun82redsun82 changed the title Rust: upgrade to rust 1.88 and rust-analyzer 0.0.294Rust: upgrade to rust-analyzer 0.0.294Aug 12, 2025
@redsun82redsun82 changed the title Rust: upgrade to rust-analyzer 0.0.294Rust: upgrade to rust-analyzer 0.0.300Aug 12, 2025
Comment threadrust/ql/lib/codeql/rust/elements.qll Dismissed
Comment threadrust/ql/test/extractor-tests/generated/AsmExpr/AsmExpr.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/AsmExpr/AsmExpr.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/AsmExpr/AsmExpr.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/ClosureExpr/ClosureExpr.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/ForBinder/ForBinder.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/ForTypeRepr/ForTypeRepr.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/TypeBound/TypeBound.ql Dismissed
Comment threadrust/ql/test/extractor-tests/generated/WherePred/WherePred.ql Dismissed
@redsun82redsun82 removed the depends on internal PR This PR should only be merged in sync with an internal Semmle PR label Aug 15, 2025
@redsun82
redsun82 marked this pull request as ready for review August 15, 2025 08:20
@redsun82
redsun82 requested review from a team as code ownersAugust 15, 2025 08:20
CopilotAI review requested due to automatic review settings August 15, 2025 08:20

CopilotAI left a comment

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.

Pull Request Overview

This PR upgrades the rust-analyzer version from 0.0.299 to 0.0.300 as indicated by the title. The upgrade brings updates to the Rust AST schema and affects test expectations across various CodeQL test suites due to improved location precision and renamed AST nodes.

  • Updates the Rust AST schema with structural changes including renaming ClosureBinder to ForBinder
  • Introduces the ForBinder as a more general construct for closure and type parameter bindings
  • Updates dependency versions and test expectations to match the improved rust-analyzer output

Reviewed Changes

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

FileDescription
shared/tree-sitter-extractor/Cargo.tomlUpdates the rand dependency from version 0.9.1 to 0.9.2
rust/schema/ast.pyMajor AST schema changes: renames ClosureBinder to ForBinder, adds Item inheritance to AsmExpr, and updates field references
rust/schema/annotations.pyUpdates documentation and annotations to reflect the renaming from ClosureBinder to ForBinder
Multiple .expected filesUpdates test expectations to reflect improved location precision from the rust-analyzer upgrade

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@@ -1 +1,2 @@
| gen_asm_clobber_abi.rs:8:14:8:29 | AsmClobberAbi |
| gen_asm_clobber_abi.rs:8:14:8:29 | AsmClobberAbi |

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.

This looks like a duplicate database element. Any idea what this is about?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I think it has to do with how the asm! macro is expanded. Might be good to have a look at the AST

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

seems like asm! expands to a MacroBlockExpr that has both its TailExpr and its Statement populated with a copy of the AsmExpr each 😕

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.

That sounds like a potential bug. The parser is implemented by hand, so it is possible they made a mistake.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

well, it's not the parser populating that, but the asm! special case of macro expansion (so even more ripe for possible errors)

@aibaarsaibaarsAug 19, 2025

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.

Or more likely the tail_expr() and statements() methods are wrong. These are typically implemented as "find first child with matching type" and because AsmExpr is now also an Item the statements() method will pick it up too. See also:

https://github.com/rust-lang/rust-analyzer/blob/58bbdec73138b978dd91f1082110168fcdeb4669/crates/syntax/src/ast/generated/nodes.rs#L1561-L1565

https://github.com/rust-lang/rust-analyzer/blob/58bbdec73138b978dd91f1082110168fcdeb4669/crates/syntax/src/ast/generated/nodes.rs#L949-L954

Not sure if you want to file a bug report, a PR to fix it, or whether to patch things up in the extractor or in QL.

Having duplicated nodes is not great, but asm stuff is fairly rare and we don't do much with it so having a bad AST temporarily shouldn't be too much of a problem.

@redsun82redsun82Aug 19, 2025

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

uh, I think I see what's happening. This is equivalent to that weird situation we have for path segments, where there's a roundabout way of getting the types from it.

When asking for the repeated Stmt children of a MacroBlock, the library just looks at the children for a matching type. Now, because AsmExpr now are also Items, and Items are Stmts, an AsmExpr will match both as the Stmt* children and as the Expr? one. So this stems from a limitation of the ast library in rust-analyzer...

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

:jinx-coke:

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

can fix that in the extractor

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Comment threadrust/ql/test/extractor-tests/generated/ForTypeRepr/gen_for_type_repr.rs Outdated
Comment threadrust/schema/ast.py
@redsun82redsun82 added the no-change-note-required This PR does not need a change note label Aug 18, 2025

@aibaarsaibaars left a comment

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.

Looks good to me. Still have some questions though.

ParseResult {
ast: source_file,
text: input.text(semantics.db),
text: input.text(semantics.db).clone(),

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.

Why do we need to clone now?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

they changed this to return a ref to an Arc, instead of directly a moved Arc like it was before, so we now need to explicitly clone it. It remains an Arc, so cloning is cheap.

| lifetime.rs:767:2:767:13 | ptr | semmle.label | ptr |
| lifetime.rs:766:2:766:11 | &val | semmle.label | &val |
| lifetime.rs:766:2:766:11 | ptr | semmle.label | ptr |
| lifetime.rs:766:2:766:11 | ptr | semmle.label | ptr |

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.

Some of these look like duplicates. Any idea what is going on?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I'm a bit surprised by the previous results. When I replaced the macro with the expanded code, I could see two distinct ptr results here. So these are legit, they are just squashed together in the macro call location.

Comment threadrust/schema/ast.py
@redsun82
redsun82 merged commit 7de34e4 into mainAug 25, 2025
61 checks passed
@redsun82
redsun82 deleted the redsun82/cargo-upgrade-2 branch August 25, 2025 14:26
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-change-note-requiredThis PR does not need a change noteRubyRustPull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@redsun82@aibaars@github-advanced-security