Skip to content

Rust: extract generic parameters, arguments and resolve bound type variables #19237

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
merged 10 commits into from
Apr 17, 2025

Conversation

aibaars
Copy link
Contributor

@aibaars aibaars commented Apr 7, 2025

This PR aims to support the extraction of generic parameters, arguments, and the resolution of bound type variables in Rust.

@Copilot Copilot AI review requested due to automatic review settings April 7, 2025 19:31
@github-actions github-actions bot added the Rust Pull requests that update Rust code label Apr 7, 2025
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 aims to support the extraction of generic parameters, arguments, and the resolution of bound type variables in Rust. It introduces two new generic structs (LocalKey and Thing) and implements a From trait conversion for Thing instantiated with type X.

Files not reviewed (3)
  • rust/ql/lib/codeql/rust/elements/internal/LifetimeImpl.qll: Language not supported
  • rust/ql/test/extractor-tests/crate_graph/modules.expected: Language not supported
  • rust/ql/test/extractor-tests/crate_graph/modules.ql: Language not supported
Comments suppressed due to low confidence (2)

rust/ql/test/extractor-tests/crate_graph/module.rs:47

  • [nitpick] The struct name 'LocalKey' is ambiguous; consider renaming it or adding a doc comment to clarify its purpose.
pub struct LocalKey<T: 'static> {

rust/ql/test/extractor-tests/crate_graph/module.rs:48

  • [nitpick] Consider renaming the field 'inner' to a more descriptive name that reflects its functionality to improve code readability.
    inner: fn(Option<&mut Option<T>>) -> *const T,

@aibaars aibaars force-pushed the aibaars/crate-graph-type-variables branch 4 times, most recently from 5c67429 to df5f003 Compare April 10, 2025 20:36
@aibaars aibaars force-pushed the aibaars/crate-graph-type-variables branch from a2844cc to 77d8ae8 Compare April 15, 2025 08:45
@aibaars aibaars requested a review from a team as a code owner April 15, 2025 08:45
@aibaars aibaars force-pushed the aibaars/crate-graph-type-variables branch from 77d8ae8 to 7bfd5f1 Compare April 15, 2025 15:26
Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

I'll dive into it more in depth tomorrow, but a generic comment I have is that crate_graph.rs is growing even more in complexity, and as far as I can tell it's really using very unstable parts of rust-analyzer. In my rust-analyzer updates experiences, this is the code that has been breaking on every update and needs quite some effort to keep up to date (see the next update in the queue).

That said, is that something we can conceivably avoid? Is running some limited form of the regular generated extractor on dependency source out of the question because of performance? I'm starting to fear this code is creeping out of control a bit, and risks becoming the biggest maintenance burden of the whole extractor.

Copy link
Contributor

@redsun82 redsun82 left a comment

Choose a reason for hiding this comment

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

Hoestly did not have a deep look at the new crate graph code, but we can work on that in the next days.

@aibaars aibaars merged commit 48f9e5a into main Apr 17, 2025
17 checks passed
@aibaars aibaars deleted the aibaars/crate-graph-type-variables branch April 17, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants