proc-macros that call out to cargo metadata break cargo fix

[jplatte]

Problem

It looks like cargo fix sets the environment variable __CARGO_FIX_PLZ which alters behavior of cargo invocations with the same environment in a way that makes otherwise functional cargo metadata invocations fail with

error: could not find .rs file in rustc args

Steps

1. Use a proc-macro that calls cargo metadata (for example to find the workspace root)
2. Run cargo fix

Notes

This came up here: launchbadge/sqlx#1229
The only reason SQLx'es proc-macro code calls cargo metadata is to find the workspace root. If there was an environment variable for that, akin to CARGO_MANIFEST_DIR, that proc-macros wouldn't have to call cargo-metadata in the first place.

Happens both on Stable & Nightly:

• cargo 1.53.0 (4369396ce 2021-04-27)
• 1.55.0-nightly (27277d966 2021-07-16)

[0xPoe]

@rustbot claim

I am working on this.

[0xPoe]

@ehuss Should we ignore cargo runs here that are not wanting cargo-fix (return false if no .rs file is found and execute the correct cargo command), at fix_maybe_exec_rustc we seem to assume it is wanting cargo-fix as soon as FIX_ENV is detected. I am not sure if this is the correct fix? Do you have any suggestions?

[ehuss]

Sorry, I'm a bit confused about the comment about "return false if no .rs file is found". I was assuming this would just entail unsetting the FIX_ENV when executing the real rustc.

[0xPoe]

Sorry, I'm a bit confused about the comment about "return false if no .rs file is found". I was assuming this would just entail unsetting the FIX_ENV when executing the real rustc.

Sorry, I complicated it. It should simply remove that env variable. I was thinking that even if the variable is set, but it turns out that no files are passed in that need fixing, then just go ahead and execute the real cargo command. Thanks for the reply!

[0xPoe]

@ehuss I read the code again and it looks like we can't simply unset FIX_ENV because

cargo/src/bin/cargo/main.rs

Line 35 in b51439f

let result = match cargo::ops::fix_maybe_exec_rustc(&config) {

determines if there is a FIX_ENV here to determine if the real cargo command is executed. So we don't have a proper place to unset FIX_ENV?

[ehuss]

Would it work to unset it around here? That's the process builder for the "real" rustc.

[0xPoe]

Would it work to unset it around here? That's the process builder for the "real" rustc.

It will first return an error here

cargo/src/cargo/ops/fix.rs

Line 326 in b51439f

let args = FixArgs::get()?;

Maybe we can find out here that we didn't find the file we need to fix and just return false and let it run the correct command?

[ehuss]

Sorry, maybe I'm a bit confused. Why would the following not work?

diff --git a/src/cargo/ops/fix.rs b/src/cargo/ops/fix.rs
index ddce0acca..3763da2d2 100644
--- a/src/cargo/ops/fix.rs
+++ b/src/cargo/ops/fix.rs
@@ -329,7 +329,8 @@ pub fn fix_maybe_exec_rustc(config: &Config) -> CargoResult<bool> {
     let workspace_rustc = std::env::var("RUSTC_WORKSPACE_WRAPPER")
         .map(PathBuf::from)
         .ok();
-    let rustc = ProcessBuilder::new(&args.rustc).wrapped(workspace_rustc.as_ref());
+    let mut rustc = ProcessBuilder::new(&args.rustc).wrapped(workspace_rustc.as_ref());
+    rustc.env_remove(FIX_ENV);

     trace!("start rustfixing {:?}", args.file);
     let fixes = rustfix_crate(&lock_addr, &rustc, &args.file, &args, config)?;

[0xPoe]

Because we will first find the file to be fixed at

cargo/src/cargo/ops/fix.rs

Line 326 in b51439f

let args = FixArgs::get()?;

, the error in the issue is also from

cargo/src/cargo/ops/fix.rs

Line 763 in b51439f

let file = file.ok_or_else(|| anyhow::anyhow!("could not find .rs file in rustc args"))?;

So I understand that before we can remove the environment variable, the error is already returned and we exit.

[0xPoe]

Because we will first find the file to be fixed at

cargo/src/cargo/ops/fix.rs

Line 326 in b51439f

let args = FixArgs::get()?;

, the error in the issue is also from

cargo/src/cargo/ops/fix.rs

Line 763 in b51439f

let file = file.ok_or_else(|| anyhow::anyhow!("could not find .rs file in rustc args"))?;

So I understand that before we can remove the environment variable, the error is already returned and we exit.

[ehuss]

Sorry, I'm still not following. AIUI, the issue is running cargo from within a proc-macro. If FIX_ENV is set while the proc-macro is running, then running cargo will fail because cargo thinks it is in "fix" mode. If rustc is run without FIX_ENV set, then I believe that should solve the problem, because the proc-macro is run without FIX_ENV, and then cargo works normally.

So the chain of events is:

1. User launches cargo fix
2. cargo fix starts a build with itself set as a rustc wrapper, and FIX_ENV set.
3. The outer cargo calls the inner cargo wrapper to build some crate which is using a proc-macro.
4. The inner cargo executes rustc to build the crate. Here is where the FIX_ENV should be unset.
5. The real rustc runs, and executes the proc-macro.
6. The proc-macro spawns a cargo process. This should work correctly now that FIX_ENV has been cleared in step 4.

I think the line you are pointing at (FixArgs::get) is done in step 3 here. At step 4, clearing the FIX_ENV should mean any proc-macros executing cargo should no longer believe they are running as a wrapper, and it should execute normally.

And just to be clear, this is the kind of use-case I am understanding this issue to be:

// A proc-macro
use proc_macro::*;

#[proc_macro]
pub fn foo(_input: TokenStream) -> TokenStream {
    let output = std::process::Command::new("cargo").arg("metadata").output().unwrap();
    eprintln!("{}", std::str::from_utf8(&output.stderr).unwrap());
    eprintln!("{}", std::str::from_utf8(&output.stdout).unwrap());
    "".parse().unwrap()
}

Perhaps you could write a cargo integration test to show what does or doesn't work?