Skip to content
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

Rust: expose more rust-analyzer config knobs #18756

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Feb 12, 2025

TODO: expose all or at least some of the new config fields as official extractor options. Depending on how the review goes for this one, I might do that in a follow-up PR.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Feb 12, 2025
@redsun82 redsun82 marked this pull request as ready for review February 13, 2025 16:48
@Copilot Copilot bot review requested due to automatic review settings February 13, 2025 16:48
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.

PR Overview

This pull request exposes more rust-analyzer configuration knobs by adding new deserialization functions for vectors and maps, updating config field definitions, and adjusting workspace loading APIs. Key changes include:

  • Introducing a new module (deserialize.rs) to handle newline/comma-separated strings.
  • Merging duplicate deserialization logic by removing the former deserialize_vec.rs.
  • Expanding the configuration options in the Config struct (e.g. sysroot, rustc_src, build_script_command, extra_includes, and proc_macro_server).
  • Modifying the cargo configuration construction and workspace loading functions in both config.rs and rust_analyzer.rs.
  • Adjusting the procedural macro in macros/src/lib.rs to use the updated deserialization functions.

Changes

File Description
rust/extractor/src/config/deserialize.rs Adds new vector and map deserialization visitors and functions for comma/newline separated strings.
rust/extractor/src/config.rs Updates the Config struct and related functions to expose additional fields and refactors cargo config logic.
rust/extractor/macros/src/lib.rs Updates macro attribute usage to employ the new deserialization functions with adjusted type checks.
rust/extractor/src/main.rs Modifies workspace loading code to pass the new LoadCargoConfig parameter.
rust/extractor/src/rust_analyzer.rs Adjusts the load_workspace API to remove unused proc_macro_server references.
rust/extractor/src/config/deserialize_vec.rs Removes legacy duplicate deserialization logic.

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

rust/extractor/src/config.rs:217

  • [nitpick] Consider renaming 'join_path_buf' to a name such as 'join_abs_path' for better clarity regarding its purpose of joining an absolute path with a UTF-8 path.
fn join_path_buf(lhs: &AbsPath, rhs: &Path) -> AbsPathBuf {

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

Copy link
Contributor

@aibaars aibaars left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a minor question.


fn join_path_buf(lhs: &AbsPath, rhs: &Path) -> AbsPathBuf {
let Ok(path) = Utf8PathBuf::from_path_buf(rhs.into()) else {
panic!("non utf8 input: {}", rhs.display())
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: does display work correctly if rhs contains non-utf8 data? We wouldn't like to cause an additional panic while trying to display an informative error message.

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.

2 participants