Add rust-analyzer to rust-toolchain.toml

[senekor]

Without this, people who rely on the rust-analyzer version provided by rustup have to install it manually to work on helix.

[the-mikedavis]

I would prefer to mention installing rust-analyzer in the contributing docs. Putting this in rust-toolchain.toml means that rust-analyzer is stuck at an older version following our MSRV but the latest rust-analyzer should work

[senekor]

Putting this in rust-toolchain.toml means that rust-analyzer is stuck at an older version

This goes for all the other tooling as well tough, right? For example, pinning clippy to 1.76 prevents the incompatible_msrv lint.

I've seen a couple projects use rust-toolchain.toml to enforce the MSRV policy. My personal preference is to test that in CI and omit the rust-toolchain.toml entirely. Since the above mentioned lint has been stabilized, following MSRV isn't hard anymore when working with the latest toolchain locally.

But I didn't search for existing discussions around this, there may be other reasons for keeping the rust-toolchain.toml.

[the-mikedavis]

Clippy is pinned because if it follows latest stable then CI will fail whenever a new stable Rust is published, same with rustfmt

[senekor]

But aren't clippy and rustfmt already pinned to MSRV in CI? Independent of rust-toolchain.toml?

  lints:
    name: Lints
    runs-on: ubuntu-latest
    if: github.repository == 'helix-editor/helix' || github.event_name != 'schedule'
    steps:
      - name: Checkout sources
        uses: actions/checkout@v4

      - name: Install MSRV toolchain
        uses: dtolnay/rust-toolchain@master
        with:
          toolchain: ${{ env.MSRV }} # <-- here

[the-mikedavis]

Hmm yeah we could remove the rust-toolchain.toml then and check MSRV in CI and rely on Clippy locally. I did some testing though and incompatible_msrv doesn't appear to work very robustly. It correctly catches string.trim_ascii() (1.80) but misses:

let s: Arc<str> = Default::default();
// or even
let s: Arc<str> = Arc::default();
let s = Arc::<str>::default()

(impl Default for Arc<str> was added in 1.80 as well.)

I'm open to dropping the rust-toolchain.toml and relying on clippy but only once the MSRV lint works robustly enough that it's equivalent to running with a rustc at the MSRV.

[senekor]

Hmm, yeah that makes sense. I should also note that the clippy lint doesn't check dependencies at all, even if it was robust. I ran into this once where I updated a depenceny which violated its declared MSRV and that was only caught in CI.

[senekor]

I can add some documentation as you suggested, but what would you suggest people actually do? Running rustup component add rust-analyzer (as I've done for now) will still yield an outdated version, same as putting it in rust-toolchain.toml. Should we suggest people manually download from rust-analyzer/releases?

[the-mikedavis]

We can advise installing rust-analyzer through a system package manager or rustup if that's not an option, giving the caveat that the version might be older

[senekor]

Is that a setup that's known to work for many people? I think this would fail on most setups. Rustup modifies the PATH by prefixing ~/.cargo/bin to it, meaning the rust-analyzer installed by rustup will shadow the one from the package manager. Even if rust-analyzer is not added as a component, rustup will still add a binary that only prints an error message when it's called. I'm on fedora and I assume most linux distros are similar, but I don't know what the situation is with homebrew / nix.

[the-mikedavis]

For anyone already using rustup we could recommend adding the component if it wouldn't work to install it trhough the system package manager. With the Nix development shell we use the rust-analyzer from nixpkgs while rustc, clippy, rustfmt etc. are pinned to the rust-toolchain.toml version

helix/flake.nix

Lines 171 to 174 in 7c907e6

	devShells.default = pkgs.mkShell {
	inputsFrom = builtins.attrValues self.checks.${system};
	nativeBuildInputs = with pkgs;
	[lld_13 cargo-flamegraph rust-analyzer]