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-analyzer: Set a library's display_name when consolidating crate specs #1039

Merged
merged 2 commits into from
Nov 26, 2021

Conversation

reiyw
Copy link
Contributor

@reiyw reiyw commented Nov 26, 2021

This PR addresses the issue #1032.

When there were multiple build targets sharing the same source, their crate specs were consolidated into one, and their display_name depended on the result of aquery. Since the display_name is actually a target name, it caused a mismatch between the display_name and the actual crate name referenced by the Rust source, leading to unresolved import errors.

To eliminate the possibility of the name mismatch, this PR changes to set a library's display_name when consolidating crate specs. This change resolved the unresolved import errors of Rust Analyzer in my reproduction repository of the issue.

@google-cla google-cla bot added the cla: yes label Nov 26, 2021
@hlopko hlopko self-requested a review November 26, 2021 09:14
@hlopko
Copy link
Member

hlopko commented Nov 26, 2021

Hi @reiyw, thanks so much for working on this!

So, what do you think about instead of checking the crate type, we would check for presence of wrapped_crate_type - that would imply that the crate at hand is actually a wrapper like rust_test using crate attribute or potentially other rules that are not part of the rules_rust, but use the integratoin APIs of rules_rust. In case of a merge event, we pick the display name of the crate that does not have wrapped_crate_type.

For a merge event of rust_binary and rust_library, I agree that library should win, but is this actually a situation that happens in practice? I'd expect that rust_binary will have its root in main.rs file, and rust_library in lib.rs file, so there won't be a conflict. I'd be fine with discussing/solving this in a followup PR, since the rust_library/rust_test problem is far more pressing.

@hlopko
Copy link
Member

hlopko commented Nov 26, 2021

Hmm, I changed my mind, let's go with the current PR approach.

rust/private/rust_analyzer.bzl Show resolved Hide resolved
@hlopko hlopko merged commit fe0ffcd into bazelbuild:main Nov 26, 2021
@hlopko
Copy link
Member

hlopko commented Nov 26, 2021

Thank you again!!

@reiyw reiyw deleted the set-libs-display-name branch November 26, 2021 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants