Optional alternative crate-naming scheme

[cfredric]

Hello!

Google has some Rust in its source repository, and we have realized that rules_rust's current approach to naming crates (i.e., using the target name as the crate name) will not work for us. We need to use a naming scheme that uniquely identifies each first-party crate, and associates it with the entire Bazel label that it was built from; for third-party crates, we can use the crate's actual name (without modification).

We're experimenting with an approach that embeds the Bazel label of a dependency crate in Rust source code, and uses a proc macro to transform the Bazel labels into valid Rust identifiers corresponding to the crates listed in the target's deps (following a particular encoding scheme that must be agreed upon between the proc macro and the rules_rust implementation).

An example of this usage is the following:

import! {
  "//some_project:utils";
  "//some_project:ugly_name" as something_else;
  "//third_party/rust/serde/v1:serde";
  "//third_party/rust/clap/v2:clap" as aliased_clap;
}

This would expand to:

extern crate some_project_COLON_utils as utils;
extern crate some_project_COLON_ugly_name as something_else;
extern crate serde;
extern crate clap as aliased_clap;

Note that the two crates imported from the //third_party directory were not renamed, but otherwise, that the whole Bazel label was visible in source, and encoded in the dependency crate's name.

I've written a prototype of this macro (uploaded in this commit - currently broken due to missing deps, but you can play around with it and see the idea), so now I'd like to add the ability to do this renaming in rules_rust (behind a flag), to continue experimenting.

I plan to add a flag to allow a project to opt-in to this new naming scheme soon, but wanted to open up for discussion :)

[hlopko]

(disclaimer, I work with Chris, so it won't come as a surprise what I write next :)

I FWIW like this approach - it makes Rust fit a little bit better into bigger monorepos, while staying compatible with OSS workflows depending on crates.io (since those names won't be mangled by the macro). We wanted to share this work with other rules_rust users to get feedback and potentially contribute the macro and a build_setting that would enable this mode in rules_rust. We don't plan to remove the existing mode.

There is some work to be done in the rules (to set the right flags to rustc etc.). If this turns out useful, we may try to talk to rust-analyzer folks about some further goodies like automatically adding crates to the macro.

And a small caveat, we will need some time to stabilize the implementation, it should be considered experimental at the moment. But we do plan to use it internally, so it will reach stability soon.

[dfreese]

Can you explain a little more about what sounds like a hard requirement to have:

a naming scheme that uniquely identifies each first-party crate, and associates it with the entire Bazel label that it was built from.

What's the primary driver of that? Potential name collisions? Tooling?

[hlopko]

Yes :)

• Name collisions (strawperson argument is naming the rust_library 'utils' in many instances in the monorepo)
• Automatic update of the BUILD file based on the content of the .rs file (so the tooling knows exactly which target to add into the deps)
• Large scale refactoring tooling (a tool can figure out a target from which a symbol comes from from the information in the source file - it doesn't have to look into the BUILD file)

I'm probably forgetting some other reasons, am I @cfredric?

[cfredric]

I think that covers it! Facilitating automated BUILD file updates (when a new crate is pulled in to the .rs source file), avoiding name collisions (without requiring the use of aliases at each downstream target), and facilitating large scale refactoring (e.g. renaming a utils crate would otherwise require looking at the BUILD files to ignore the "false positives" from other crates also named utils) are the primary motivations.

[UebelAndre]

If #1013 is the tracking issue, could it be a bit more descriptive? 😅 🙏

[cfredric]

Sure! I was thinking that this discussion could be the source of truth, but I'm happy to add more detail there.

[UebelAndre]

Thanks! It doesn't have to be detailed but rename_1p_crates flag feels pretty ambiguous to me 😅

[cfredric]

(done - added this description there, with some rephrasing and a bit more detail)

[dfreese]

Haven't thought through it entirely, but relying on just a particular file seems to be somewhat fragile. What happens for tooling in the multi file rust_library case? Would a user need to specify these macro blocks in each file for them to work?

It also makes rust look quite different between inside and outside of google. Copybara-ing is always a nightmare, but the distinction between first and third party and the macro seem like it would make that quite difficult.

[davidbarsky]

Thanks for your in-depth response. I really appreciate it as it gives me a valuable insight into the thinking behind this approach.

What do you expect the users to do when they need to import two crates with colliding names? Rename one of them locally? But that requires putting that renaming mapping into the build system file, right? It can't be all contained in the source file, I think.

I think a user will be required to rename one of the conflicting crates in the BUCK file, but to be entirely honest, we haven't had many conflicts crop up since there's no commonly used crate called util or whatever (really, the biggest issue are Thrift-generated targets, but even so, it's something we can fix).

Wouldn't that go against your goal of having the users edit the BUCK files less?

Not necessarily. If a user needs to edit a BUCK file to rename a crate, that is still a meaningful improvement over needing to edit a BUCK file every time they want to change their dependencies. I'm not sure if getting to a state of "no manual editing of a BUCK file, ever" is a goal that I can reasonably take on. "Don't interrupt the user's flow by making them switch files to add a dependency" is a far more achievable goal, in my view.

We don't plan to have extra lints. Users are not supposed to spell the unique (mangled) crate name in the source code, but so far we don't feel like there is a need to protect against it because it looks very wrong.

Gotcha—my the question behind my question is "what happens if a user writes use tracing::error" inside, e.g., a function body, but based on the procedural macro expansion you showed, the crate tracing doesn't exist under that name unless the import! macro is written.

When one adds a new dependency to a source file, one must add an extern crate declaration and update the dependencies list in the BUILD.

I apologize for being overly pedantic, but writing extern crate is no longer required in the 2018 and 2021 editions of Rust. use alone is sufficient, but I think my prior comment about needing to write import! still holds.

Thus, it would be ideal for us if the "import" statement in Rust identified the build dependency in a unique way. Then, the dependencies in BUILD files can be managed by tooling completely automatically - they could be generated from the "import" statements in the source file.

Makes sense.

Based on my experience, I'd guess that your users will be satisfied with a fuzzy finder that works 99% of the time. However, the 1% of exceptions multiplied by a huge codebase size will be a huge toil for the infrastructure teams that run large scale changes throughout the codebase. That is, the large scale change tooling (for renaming or moving symbols, merging or splitting crates etc.) will need to include the complexity to resolve these ambiguities (even having to punt to a human in some cases). That means that a 99% solution is as good as a 0% solution for the infra teams. The only way to avoid the extra complexity in the tooling is to make the mapping between imports, Bazel targets, and filesystem paths completely predictable without exceptions.

Part of the reason it took me a second to respond is that I checked with a few infrastructure folks about this, and it's... not an issue that's cropped up for them. While I absolutely believe it's an issue at Google, I'll dig deeper to verify my understanding since this is sort of a one-way door.

One more question: does Google carry patches on rust-analyzer so that the automatic import functionality works with import! or is that not really handled?

[gribozavr]

One more question: does Google carry patches on rust-analyzer so that the automatic import functionality works with import! or is that not really handled?

We have not started to deploy rust-analyzer in the monorepo yet. I do agree that some customization will be needed, since we would want, for example, code completion to complete Bazel target names inside the import! block.

[davidbarsky]

We have not started to deploy rust-analyzer in the monorepo yet. I do agree that some customization will be needed, since we would want, for example, code completion to complete Bazel target names inside the import! block

At the risk of becoming a backseat driver, I believe this functionality might need to exist outside of rust-analyzer itself since it'd require rust-analyzer to be aware of all Rust targets inside of the monorepo. In any case, we've had rust-analyzer deployed for users for several years now and I'd be happy to talk through what we did, what worked, and what didn't.

I know the teams responsible for clangd at our respective employers chat regularly and I've been meaning to set up something regular for Rust as well. Feel free to send me an email at the email I've provided in a previous comment. I think we will face many of the same problems and land on similar solutions and I think it'd be productive to chat and get alignment.

[gribozavr]

Of course, rust-analyzer can't be by itself aware of all targets in the monorepo. But I don't think that means that this feature shouldn't be in rust-analyzer. I think it means that for rust-analyzer to work in a large monorepo it needs to be adjusted to not assume anymore that it can parse the whole repo, and instead use an API to access a prebuilt index. For example, see class SymbolIndex in clangd.

[davidbarsky]

Yeah, I'm decently familiar with clangd's remote index. I think rust-lang/rust-analyzer#4712 is the equivalent issue on rust-analyzer's end, with the most complete summary being this comment.

I was thinking on-and-off about how to support that feature properly, given that the people who'd benefit most from this are people with systems like Glean or Kythe, but I'm hoping that the approach would be something along the lines of "rust-analyzer can consume some standard-ish format that both Glean and Kythe produce (maybe SCIP)?"