Allow release version of Rust Analyzer to work on the code

[philipcraig]

It's not currently possible to use Rust Analyzer at the root of the repo. This holds back open-source collaboration.

This is because Rust Analzyer requires builds to build with one version of the toolchain, and by default, that's the stable version. This repo isn't built with the stable toolchain.

It's possible via Settings to get the Analyzer to use a particular nightly toolchain version, but even that is not sufficient, as there are crates used that themselves specify a different nightly toolchain version to the main toolchain being used.

Suggested fix: converge the code on the stable version of the toolchain, or, failing that, on a particular nightly version, so that Rust Analyzer can work.

[ndmitchell]

For things like Starlark we have converged on a stable version of the toolchain. Looking at the features we still use, none immediately look like being painful to use. Perhaps the hardest one is the try keyword, which we use 31 times, but none seem as hard as they were for Starlark (cc @stepancheg who might have views here).

That said, on a completely fresh Windows computer, I just installed VS Code, Rust, Rust Analyzer, checked out Buck2, started it all up and Rust Analyzer worked for me perfectly at the root of the computer. I didn't configure anything. Go to definition works. Hover docs work. Errors show up when I hit save. What isn't working?

[rbtcollins]

Picking a an arbitrary try block to try and understand why they are in use:

fn aquery_attributes(&self, fs: &ExecutorFs) -> IndexMap<String, String> {
        let res: anyhow::Result<String> = try { String::from_utf8(self.get_contents(fs)?)? };
        // TODO(cjhopman): We should change this api to support returning a Result.
        indexmap! {
            "contents".to_owned() => match res {
                Ok(v) => v,
                Err(e) => format!("ERROR: constructing contents ({})", e)
            }
        }
    }

This doesn't seem like a particularly strong reason for indexmap - and indeed the TODO there gets to the essence of it. You could do something like

fn aquery_attributes(&self, fs: &ExecutorFs) -> IndexMap<String, String> {
        let contents = (|| Ok(String::from_utf8(self.get_contents(fs)?)?))()
            .unwrap_or_else(|e: anyhow::Error| format!("ERROR: constructing contents ({e})"));
        indexmap! { "contents".to_owned() => contents }
    }

Which is shorter, clearer (because the error handling is in one place rather than spread out), and doesn't depend on an unstable feature with unclear future. I haven't checked godbolt but my intuition says it will generate quite similar code to the original, if efficiency is a concern (and if it is, may I suggest using Cow<> rather than String for keys, to avoid allocating teeny strings all over the place when you know in advance what they will be most of the time.

I'd be happy to put a PR up removing any similarly easy transformations if that would be welcome.

[ndmitchell]

I think the advantage of removing some try blocks is relatively little, whereas removing them all and prohibiting new ones has some value - but it's questionable whether this is worth it. I think there are two points in the transformation that add value:

• Works with rust-analyzer out of the box. I think we are there? If not, what errors are people seeing. (If no one is seeing errors, I'll close this task).
• Is fully stable Rust so we can avoid pinning the toolchain. Given the number of features we currently use, I don't see that as feasible without excessive cost, given the benefit is relatively low. Many of the ones we do use are functions that are likely to stabilise in the near future on their own, so I imagine in a few Rust editions we should look more closely at which ones matter. E.g. box_patterns makes some code slightly nicer, so if we can't be stable we might as well have it, but if that's the only thing left we probably want to delete it.

#![feature(absolute_path)]
#![feature(assert_matches)]
#![feature(async_closure)]
#![feature(box_into_pin)]
#![feature(box_patterns)]
#![feature(const_fn_fn_ptr_basics)]
#![feature(const_fn_trait_bound)]
#![feature(const_panic)]
#![feature(control_flow_enum)]
#![feature(cow_is_borrowed)]
#![feature(downcast_unchecked)]
#![feature(entry_insert)]
#![feature(exact_size_is_empty)]
#![feature(exclusive_range_pattern)]
#![feature(exhaustive_patterns)]
#![feature(exit_status_error)]
#![feature(fn_traits)]
#![feature(fs_try_exists)]
#![feature(int_roundings)]
#![feature(io_error_more)]
#![feature(is_sorted)]
#![feature(iter_order_by)]
#![feature(map_entry_replace)]
#![feature(map_try_insert)]
#![feature(maybe_uninit_slice)]
#![feature(min_specialization)]
#![feature(negative_impls)]
#![feature(never_type)]
#![feature(once_cell)]
#![feature(pattern)]
#![feature(plugin)]
#![feature(provide_any)]
#![feature(round_char_boundary)]
#![feature(rustc_private)]
#![feature(termination_trait_lib)]
#![feature(test)]
#![feature(trait_alias)]
#![feature(try_blocks)]
#![feature(try_trait_v2)]
#![feature(type_alias_impl_trait)]
#![feature(type_name_of_val)]

[rbtcollins]

I think there is merit in treating aiming for stable-rust as a feature, and only using unstable features that are truely blocking for the ability to create buck2 effectively.

I do agree that features that are well on the path to stablilisation are quite low risk, but adding in other things just because they are there seems like an unneeded source of churn and risk.

For instance, box_patterns that you mention won't be stabilised.
And it is harder to reason about code bisection on the codebase when the toolchain has to be switched as well at each probe point and code is changing solely to enable newer versions of rust... (fortunately rustup makes switching simple!).

[ndmitchell]

While box_patterns might not be stabilised, they haven't changed in a long time, and in fact they are now very unlikely to ever change until the day they are removed. When they do get removed it will probably be because deref patterns exist, and as soon as those are available unstable, we'll probably move to them. Complicating the code to avoid them doesn't seem to add much benefit.

In practice over the lifetime of Buck2 there has been more churn over the Debug instance of String (which is considered stable but allowed to vary between releases) than any of the features...

Agreed that being purely on stable would have value. But advantages like bisection aren't that useful if you are almost on stable.

Btw, I just audited our features, and found 14 declarations weren't used at all, so I've deleted them (patch will be sync'd after it is approved, probably tomorrow). It might be worth figuring out which features we use are merely functions that are stabilising (e.g. cow_is_borrowed), features that are highly likely to stabilise (e.g. negative_impls maybe?) and those that are likely to be removed (e.g. box_patterns).

[matklad]

This is because Rust Analzyer requires builds to build with one version of the toolchain, and by default, that's the stable version.

This is not the case, there’s no preference for any particular version in rust-analyzer.

By default, we run cargo (or $CARGO), using projects root directory as a cwd, and that picks the same rust version you’d get in the shell.

In terms of support for specific features, stable language features are supported, while unstable are best effort. I’d imagine it would be “good enough” in practice.

One place where compiler version matters is proc macros. Rust analyzer consumes the same .so dynamic libraries with compiled proc macros as rustc. The ABI of these .so is not stable and tied to compiler versions. Rust-analyzer tries to support a range of ABI, but that’s not a particularly long range. However, if you use the nightly corresponding to the latest stable, it has the same ABI as stable, and this is a non-problem.

To sum up, any observable issues with rust-analyzer support are unlikely to be attributed to the usage of nightly. rust-analyzer doesn’t tip the “recent nightly/recent stable” scale either way.

[davidbarsky]

Yeah, @matklad's response is correct here, but of course, he would know! I'm not in a position to dictate what the Buck team should do here with regard to unstable features, despite my personal stance being (perhaps unfairly!) negative towards them. Anyways, most features in rust-analyzer can/do work on Buck2 for the following reasons:

• Procedural macros work properly in Buck2 due to the procedural macro server used by rust-analyzer, as the API between rust-analyzer and the proc macro server is decently stable.
  • While other nightly projects might encounter breakage, the risk to Buck2 is very low since the nightly it uses is effectively a RUSTC_BOOTSTRAP=1'd compiler. A more recently nightly could be used, but I don't think we—the teams supporting Rust at FB—are in a position to do more frequent updates beyond the current "every 6 weeks".
• I don't know how accurate this is now, but most of the features used by Buck2 appear to library features. I wouldn't personally use them, but rust-analyzer handles them without issue.
  • Features like #![feature(trait_alias)], #[type_alias_impl_trait], #![feature(min_specialization)], and #![feature(negative_impls)] are more complicated. As Matklad said, those are best-effort, but just yesterday, I saw a case of a trait alias not being resolved by rust-analyzer. I'm not going to go out of my way to support those features in rust-analyzer because I think I'd need to make changes to Chalk and I'd prefer to wait for the new trait solver instead.

I think this issue can be closed in favor of a more specific "how many unstable features should buck2 use".