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: Generate rust-project.json without explicit target list #907

Closed
wants to merge 18 commits into from

Conversation

djmarcin
Copy link
Contributor

@djmarcin djmarcin commented Aug 23, 2021

This PR refactors the generation of rust-project.json significantly. It's not fully ready to merge and I need some advice on how to test this new design. (@hlopko)

In broad strokes:

  • The aspect now emits a JSON representation of each crate in the dependency graph
  • Crate deduplication has moved into Rust because we no longer aggregate crates in starlark
  • The logic formerly in the rust_analyzer rule has been moved into gen_rust_project

gen_rust_project now manages the parsing of individual crate specs and creating the rust-project.json file. The basic logic is:

  1. Call bazel build using the aspect and a list of targets.
  2. Call bazel aquery using the list of targets to retrieve the list of generated spec files
  3. Parse the individual spec files
  4. Iterate through the list of crates, merging crates into rust-project.json if all dependencies have already been merged into rust-project.json (required due to the self-referential array data structure that rust-project.json uses).
  5. Write the final data structure to rust-project.json in the workspace root.

Compatibility with the existing rust_analyzer rule has been dropped, save for an empty implementation to prevent immediate breakages upon updating rules_rust. In the common integration case where a user just used the defaults, no changes are required and the rust_analyzer rule can be deleted at their convenience.

Unfortunately, the existing tests rely on the rust_analyzer rule, so they are broken by this PR. Additionally, I don't know if this PR can be tested inside bazel because it repeatedly invokes bazel while running. A proper test given this approach would probably require some additional flags, so I'd like to get some feedback before I go down a rabbit hole adding hooks for testing that might end up not being what folks want to see.

/cc @hlopko @UebelAndre

@google-cla google-cla bot added the cla: yes label Aug 23, 2021
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

This looks awesome! You're an absolute legend!!! 😄

I think testing for this one is one would make sense to live more in Rust than in Bazel? I'd expect to see some tests for the functions in gen_rust_project_lib and probably have some rule which explicitly generates a .rust_analyzer_sysroot_src file from the aspect use in the rust_test target for schema/deserialization validation. I feel like that'd provide sufficient coverage for the functionality here?

For testing the starlark code though, I did write some unit tests for Clippy which were the first I'd ever seen written for aspects. Hopefully they can serve as some baseline? I'll keep peeking at this throughout the day and see if I have any new ideas.


return [OutputGroupInfo(rust_analyzer_sysroot_src = depset([sysroot_src_file]))]

rust_analyzer_detect_sysroot = rule(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just so I know I'm on the same page, can you describe what you mean by sysroot for this rule and for the output group field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name comes from the field in the rust-project.json, but essentially it's the root under which all the rust source can be located (e.g. std::, core::, test::, etc). I'll add a comment.

@hlopko hlopko self-requested a review August 24, 2021 21:14
@hlopko
Copy link
Member

hlopko commented Aug 24, 2021

I'm super interested in this work, but I'm also on vacation until 6th. I'll try to find time on evenings, but I apologize in advance for slowness. Thank you!

@djmarcin
Copy link
Contributor Author

@hlopko No rush, I’m on vacation this week.

I am particularly curious if you (or anyone else) has immediate concerns about this breaking backward compatibility with the rule? Since it’s an experimental feature I’m hoping that we can just give migration instructions and not worry about attempting to interpret the existing rule. I think it’s probably possible to use the existing rule to override the default --targets if it’s important, though.

@UebelAndre
Copy link
Collaborator

Hello friends, any updates here? I think at this point everyone is back from vacation but no worries if I miscalculated 😅.

Copy link
Member

@hlopko hlopko left a comment

Choose a reason for hiding this comment

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

I like the approach! Great work!

Correct me if I'm wrong, but this could be trivially scaled down to analyzing a single file (for example currently opened file in the editor). The gen_rust_project can compute the target of the file, and then generate rust-project.json from that.

Well that's not that exciting you may say, and you would be right :)

But imagine rust-analyzer itself detected that the project is using Bazel. It could use the gen_rust_project tool repeatedly for each opened file to fetch all the information it needs, invalidating what needs to be invalidated on changes to BUILD files and bzl files.

IMHO it's worth a shot talking to rust-analyzer folks once this PR lands.

Anyway, this is a great work and I'd be happy to have it merged.

.output()?;

let crate_spec_files =
parse_aquery_output_files(execution_root, String::from_utf8(aquery_output.stdout)?)?;
Copy link
Member

Choose a reason for hiding this comment

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

Can we parse directly from the stream, without allocating the string?

pub edition: String,
pub root_module: String,
pub is_workspace_member: bool,
pub deps: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

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

Would a set perform better when merging duplicates below? Would you consider it an overkill to use union find? :)

},
)

def _rust_analyzer_aspect_impl(target, ctx):
if OutputGroupInfo in target:
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to "re-provide" output groups here?

Copy link
Contributor Author

@djmarcin djmarcin Sep 13, 2021

Choose a reason for hiding this comment

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

This is a little bit of a hack to ensure that the rust_analyzer aspect generates the rust_analyzer_sysroot_src, which doesn't have crate_info so will be skipped by the check on L46. We might be able to use a separate aspect for it though (I'd have to double check if that would be too much duplication).

More generally, we need to re-provide the output groups because otherwise if you did something like bazel build --aspects=@rules_rust//rust:defs.bzl%rust_analyzer_aspect --output_groups=rust_analyzer_crate_spec //path/to/single:target then only the crate_spec file for //path/to/single:target will be generated, but none of its deps. Re-providing OutputGroupInfo ensures that all the dependent crate_spec files are generated as well.

Copy link
Contributor Author

@djmarcin djmarcin left a comment

Choose a reason for hiding this comment

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

Trying to find some time to circle back on these comments and tests sometime this week.

},
)

def _rust_analyzer_aspect_impl(target, ctx):
if OutputGroupInfo in target:
Copy link
Contributor Author

@djmarcin djmarcin Sep 13, 2021

Choose a reason for hiding this comment

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

This is a little bit of a hack to ensure that the rust_analyzer aspect generates the rust_analyzer_sysroot_src, which doesn't have crate_info so will be skipped by the check on L46. We might be able to use a separate aspect for it though (I'd have to double check if that would be too much duplication).

More generally, we need to re-provide the output groups because otherwise if you did something like bazel build --aspects=@rules_rust//rust:defs.bzl%rust_analyzer_aspect --output_groups=rust_analyzer_crate_spec //path/to/single:target then only the crate_spec file for //path/to/single:target will be generated, but none of its deps. Re-providing OutputGroupInfo ensures that all the dependent crate_spec files are generated as well.

@UebelAndre
Copy link
Collaborator

Hello friends, friendly ping here 😅 This feature would be awesome!

@purkhusid
Copy link
Contributor

@djmarcin Does this improve the performance of the analyzer rules as well?

@hlopko
Copy link
Member

hlopko commented Oct 4, 2021

@purkhusid is the current performance problematic? Could you file an issue for that with more information?

@purkhusid
Copy link
Contributor

@hlopko I created an issue here: #962

@hlopko
Copy link
Member

hlopko commented Oct 8, 2021

Thank you! Yeah this PR may be an (constant-factor) improvement. We'll need to measure/profile. But this PR doesn't change the fact that we have to visit all transitive crates...

@UebelAndre
Copy link
Collaborator

Is there anything I can do to help drive this PR forward? What are the remaining steps?

@djmarcin
Copy link
Contributor Author

Sorry for letting this sit, I've been super busy and haven't had a chance to revisit. I think mainly the issues are just around writing the tests. Otherwise, we've been using this internally for a while and it seems to be very stable for us.

It looks like perhaps recursively calling bazel isn't the problem I thought it would be, because it seems that https://github.com/bazelbuild/rules_rust/blob/main/test/rustfmt/rustfmt_failure_test.sh does it. I'll use that as an example and try to write a few tests.

@UebelAndre
Copy link
Collaborator

It looks like perhaps recursively calling bazel isn't the problem I thought it would be, because it seems that https://github.com/bazelbuild/rules_rust/blob/main/test/rustfmt/rustfmt_failure_test.sh does it. I'll use that as an example and try to write a few tests.

Yeah, I think that pattern would be just fine, though I think unit testing could probably solve for identifying subtle regressions. If the calls to bazel were to be moved into their own functions where that's virtually the only thing they do, then I think a lot could be done by mocking what the outputs would be (particularly since I doubt aquery outputs are going to change anytime soon). Tests that actually invoke Bazel seem more integration tests to me but the only thing they could offer that unittesting does not is testing interactions with various Bazel binaries. I leave it up to you and am still happy to do whatever I can to help get this through. I'm really excited about this PR 😄

@@ -107,6 +108,9 @@ rust_common = _rust_common
rust_analyzer_aspect = _rust_analyzer_aspect
# See @rules_rust//rust/private:rust_analyzer.bzl for a complete description.

rust_analyzer_detect_sysroot = _rust_analyzer_detect_sysroot
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this rule need to be public?

@UebelAndre
Copy link
Collaborator

These changes were merged in #1010

@UebelAndre UebelAndre closed this Nov 17, 2021
@djmarcin
Copy link
Contributor Author

Thanks @UebelAndre !

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.

Visibility suggestions for rust_analyzer?
4 participants