loadOutDirsFromCheck / proc macro ergonomics

[flodiebold]

We regularly get bug reports from people that are fixed by enabling loadOutDirsFromCheck or proc macros. I'm wondering what the "endgame" is here.

I think there are two reasons why we can't just enable these settings by default:

• we don't want to run code without confirmation
• we want to be able to function without cargo check.

But the current situation where things just silently don't work or we produce wrong diagnostics isn't really tenable, right? I think we should ideally try to detect the situations where things aren't working because a build script or proc macro wasn't run, and warn the user.

Also maybe there should long-term be a way to have cargo run build scripts and compile proc macros without doing a full check? (Edit: Ah, this is rust-lang/cargo#7178.)

Shorter-term, here are some ideas:

• detect usage of env!("OUT_DIR") and produce a diagnostic and/or a popup warning that things may not work?
• suppress follow-up name resolution errors from unresolved #[path] modules?
• we already resolve proc macros without them being enabled, so we should probably be able to produce diagnostics about their usage? There's probably no way to detect follow-up errors from proc macros not being run though.

Long-term, maybe we want a pop-up that says "your project requires build scripts and/or proc macros to be built correctly, do you want to run them?" and/or a whitelist?

[matklad]

Yeah, it's about time we think about this...

I don't think there really is a nice end-game here, short of banning build.rs and moving all proc macros to wasm.

Short-term, I think we should just bite the bullet and enable this by default: this would be better for new users, because new users typically don't have gigantic projects. And we have checkOnSave enabled by default as well (I think?), so not-enabling loadOutDirsFromCheck is a no-op.

We do need to improve quality of implementation though, before we flip the switch.

Firstly, we need to make loading asynchronous. We first should execute basic cargo metadata, get something working, and only then run/wait for cargo check to fetch proc macros and build scripts. This might lead to some double work though, not sure what's the best way out here. Stupid solution (before we even confirmed that this is a problem): use trimmed-down crate graph at the start, by only adding sysroot deps. That way, we'll pre-process sysroot, but wont prime the world only to be re-checked again with proc macros. Though, this probably is overengineering --

Secondly, we need to add better observability for cargo check: capturing of errors, tracking the time, reporting progress.

Thirdly, we should expose this observability to users: make sure that errors are properly shown, add command to re-query cargo check. Maybe add some pop-ups to enable/disable the features, but I can't see a useful condition here (showing popup on the start would be a no-go, for example :-) ). I guess, we can try to special-guess diagnostics, but my gut feeling that the result would be "Yes" for any non-trivial project -- at least one crate in the crate graph will use build.rs

[lnicola]

I agree that we should enable them by default, like we do for all features.

One thing that's missing is detecting a proc macro server that crashes and disabling the feature. This is useful when there's an ABI mismatch, like we had before. I'm not sure we can support multiple ABIs, though.

[matklad]

I wonder... what is the behavior of rustc if you feed rustc A with proc-macro crate compiled with rustc B when:

• A and B are different versions, but proc-macro-abi compatible?
• A and B are different versions, and macro abi's are incompatible?

[lnicola]

See also #6174, though I'm not sure why it's needed.

[matklad]

Ok, so this is 1) but not 2). This means that we need to map rustc version to ABI version ourselves (basically, we need to remember the latest version of rustc we are compatible with, which is either Some(version) if there's a known incompat, or None otherwise)

[bjorn3]

I wonder... what is the behavior of rustc if you feed rustc A with proc-macro crate compiled with rustc B when:

If the version differs, rustc behaves the same as with any other crate, which is reporting an error. Rustc decodes the full crate metadata for proc macros, which is much more unstable than the ABI of just libproc_macro.

This means that we need to map rustc version to ABI version ourselves (basically, we need to remember the latest version of rustc we are compatible with, which is either Some(version) if there's a known incompat, or None otherwise)

Maybe libproc_macro could export an ABI version just for use by rust-analyzer? This ABI version would then be incremented manually every time there is an ABI incompatible change to libproc_macro.

[matklad]

Maybe libproc_macro could export an ABI version just for use by rust-analyzer? This ABI version would then be incremented manually every time there is an ABI incompatible change to libproc_macro.

Yeah, that would plausible, but that means some manual and hard to remember work in the compiler. This is the right solution theoretically, but, organizationally, I think it would be easier to maintain this ABI version on our side (by mapping from rustc version to ABI version).

[jyn514]

I wonder... what is the behavior of rustc if you feed rustc A with proc-macro crate compiled with rustc B when:

* A and B are different versions, but proc-macro-abi compatible?

* A and B are different versions, and macro abi's are incompatible?

error[E0514]: found crate `chalk_derive` compiled by an incompatible version of rustc
  --> /home/joshua/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/chalk-ir-0.36.0/src/lib.rs:13:5
   |
13 | use chalk_derive::{Fold, HasInterner, SuperVisit, Visit, Zip};
   |     ^^^^^^^^^^^^
   |
   = help: please recompile that crate using this compiler (rustc 1.51.0-dev)
   = note: the following crate versions were found:
           crate `chalk_derive` compiled by rustc 1.49.0-beta.1 (21dea46d8 2020-11-18): /home/joshua/rustc3/build/x86_64-unknown-linux-gnu/stage1-rustc/release/deps/libchalk_derive-843b3d0b9b865ca6.so

[lnicola]

We should also take a look at #3155 before enabling build script support.

[flodiebold]

@lnicola I think that issue is independent of loadOutDirsFromCheck, since it affects the diagnostics we get from check-on-save.

[flodiebold]

Is there any reason not to enable loadOutDirsFromCheck right now already? Also I think we should find a better name for the setting while we're at it.

My best idea so far is cargo.checkOnStartup. It doesn't mention build.rs or proc macros, but I can't think of a good name that makes it clear that this is necessary to support them, and at least it makes clear what actually happens.

[matklad]

The big reason is security: running `build.rs` when you just "read" the code opens you to attacks. I believe our strategy here would be 🤷, but I opened https://internals.rust-lang.org/t/running-build-rs-in-rust-analyzer/14154 to discuss. I've already started renaming settings yesterday, but apparently lost the code :( I want local history back ;(

…

On Wed, 3 Mar 2021 at 14:00, Florian Diebold ***@***.***> wrote: Is there any reason not to enable loadOutDirsFromCheck right now already? Also I think we should find a better name for the setting while we're at it. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#6448 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANB3M5R37JSXPVO6XJQDJ3TBYJGHANCNFSM4TIUZN3A> .

[flodiebold]

Yeah, but we have checkOnSave enabled by default anyway already.

[flodiebold]

We could maybe only enable it by default if checkOnSave is enabled? 🤔

[matklad]

Status: we now set runBuildScripts to true by default.

[edwin0cheng]

Since proc-macro is enabled by default for now, maybe we close this one ?

[bluenote10]

Status: we now set runBuildScripts to true by default.

Since proc-macro is enabled by default for now, maybe we close this one ?

Does that imply that loadOutDirsFromCheck has been removed? According to the VSCode config linter it is an "unknown configuration setting"

but on the other hand there is still some mention of it in the manual.

I'm asking, because the godot-rust FAQ recommends to use "rust-analyzer.cargo.loadOutDirsFromCheck": true to get around the non-working rust-analyzer support. It doesn't seem to be working though. The question whether loadOutDirsFromCheck is still a thing also came up in #5040 (comment).

[Veykril]

I think we renamed it to rust-analyzer.cargo.runBuildScripts(Note that these are enabled by default now). Judging from the screenshot in that FAQ I imagine the problem for auto completion here is now the use of attribute/derive macros and them failing expansion on invalid syntax(an unfinished "foo." is not a valid expression) and as such preventing us from offering . completions properly.
cc #10520

[hgomersall]

I'm puzzled by this. The fix for my proc macro issues was as per #7497 which I implemented 10 minutes ago, but this thread suggests I should be having macros working properly now. Is this an interaction with CoC? I'm regularly updating rust-analyzer whenever prompted.

[flodiebold]

Both settings should be on by default. Maybe CoC turns them off by default from the client side.