Allow release version of Rust Analyzer to work on the code about buck2 HOT 6 OPEN

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?

from buck2.

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.

from buck2.

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)]

from buck2.

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!).

from buck2.

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).

from buck2.

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.

from buck2.