Treat dependencies of proc-macro crates like normal dependencies

[Aaron1011]

Split out from #68941

Previously, we special-cased dependencies of proc-macro crates: we didn't load metadata for them, and we excluded them from the sysroot.

With #68941, this will no longer work - we may need to deserialize Spans from proc-macro crates and their dependencies, which requires having metadata available.

This PR removes the special handling of proc-macro crates in resolve_crate_deps, and adjusts the sysroot generation in bootstrap to only exclude build scripts, not proc-macro dependencies.

I don't know of a way to write a test for this. However, the ui-fulldeps testsuite implicitly tests this, since it has several tests containing extern crate rustc (which ends up loading proc-macro dependencies from the sysroot).

[rust-highfive]

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

[Aaron1011]

cc @petrochenkov

[Mark-Simulacrum]

Why does this need a different check from the above crate_types.iter().any(|t| t == "proc-macro")?

Generally it also feels wrong or at least odd that we're putting proc macros seemingly twice, once into the host and once into the target directory. That feels reminiscent of a flag @Zoxc, IIRC, added (-Zdual-macros, or so, I don't recall exactly).

[Aaron1011]

The crate_types.iter().any(|t| t == "proc-macro") check is looking for proc macro crates. This check is looking for dependencies of proc-macros, which may not be proc-macros themselves.

For example, suppose we have the dependency graph normal_crate_one -> proc_macro_crate -> normal_crate_two. The crate normal_crate_two is not a proc macro crate - but since it's a dependency of proc_macro_crate, it will end up in the host dir.

[pietroalbini]

Had to kill the PR build to get CI back up and running, sorry for the trouble.

[petrochenkov]

Nit: this comment looks unnecessary, like "look, nothing unusual is happening here, move along". It's the removed special case that required a comment, but didn't have it.

[Aaron1011]

We still special-case proc macros is some places, so I thought it was worthwhile to explain why we do this (in case someone ever thinks of re-adding this an an optimization).

[Mark-Simulacrum]

There's nothing here about proc macros anymore though? I would agree with @petrochenkov that this comment seems more confusing than helpful. We should have a test that ensures this doesn't get re-added?

[Zoxc]

With #68941, this will no longer work - we may need to deserialize Spans from proc-macro crates and their dependencies, which requires having metadata available.

Why do we need to do this? Can their Spans appear in proc-macro output? or is there some other reason for this?

[Aaron1011]

It turns out that this bootstrap change causes us to include build-dependencies of std (cc and autocfg) when we copy the libstd artifacts into the sysroot. However, cc and autocfg also get included in the sysroot when we copy rustc artifacts, so we end up with multiple versions of cc and autocfg in the sysroot.

This makes it impossible to write extern crate cc when using the sysroot - instead of the usual "this crate is being loaded from the sysroot" message on stable, you get a mesage about incompatible versions of cc.

On one hand, there's no reason to be using cc or autocfg from the sysroot - you can always add them to your Cargo.toml.

On the other hand, this will give a confusing error message for anyone who writes extern crate cc without first adding it to their Cargo.toml.

At the moment, we have no way of filtering out build dependencies from the sysroot when inspecting Cargo's JSON output - they cannot be distinguished from non-build dependencies, which we do want to include.

We have two options:

1. Accept the sub-optimal error message, and adjust the run-make-fulldeps/sysroot-crates-are-unstable test to accept this message for cc and autocfg.
2. Hardcode exclusions for cfg and autocfg when we copy libstd artifacts into the sysroot during bootstrap. All dependency changes to libstd are carefully audited, so this shouldn't be too much of a maintence burden.

The long-term fix is for Cargo to provide more information in its json output, so that we can distinguish between build-dependencies and normal dependencies.

[Aaron1011]

Why do we need to do this? Can their Spans appear in proc-macro output? or is there some other reason for this?

@Zoxc: This issue is unrelated to proc-macro output. It has to do with the actual identifiers of proc-macro functions (e.g. the foo in #[proc_macro_attr] fn foo()). See #68941 (comment) for more details.

I opened this because we were hitting this case in #68941 when compiling rustc_span with the stage1 compiler - I haven't tracked down exactly what's happening, but I suspect that we're re-exporting a proc macro.

See also #68941 (comment)

[bjorn3]

I don't think it is useful to show ever show the span of a proc-macro function. The user of the proc-macro probably doesn't want to modify or inspect the proc-macro.

[Aaron1011]

@bjorn3: I disagree. While it might be unusual for an error to do so, I think we should treat proc macros as just another kind of definition - just as we might show " defined here" or " defined here", we could potentially show "<proc_macro> defined here".

To be clear, whether or not to show the definition of a proc macro would be determined on a per-diagnostic basis. However, unless loading proc-macro dependency metadata has a negative performance impact, I think we should make this change. It will allow any future use of proc-macro Idents to work as expected, without committing us to use them in any particular place.

[Dylan-DPC-zz]

@Aaron1011 if you can fix the failing tests we can proceed with this

[Aaron1011]

@Dylan-DPC: Fixed

[Dylan-DPC-zz]

@Mark-Simulacrum this is ready for review

[Mark-Simulacrum]

Hardcode exclusions for cfg and autocfg when we copy libstd artifacts into the sysroot during bootstrap. All dependency changes to libstd are carefully audited, so this shouldn't be too much of a maintence burden.

I would prefer that we not give users any errors about sysroot crates if possible, and though these two particular crates are probably quite rare to hit on accident I would prefer to add them to a hardcoded list in bootstrap.

It's also a bit unclear to me -- the PR description says this was necessary for #68941 but that has since landed. I suspect what you meant is that #68941 won't really be useful if we're not loading dependency spans? Can you update the PR description?

@bors try @rust-timer queue

Also would like to gather performance diff of this change. I would also like to make sure that @petrochenkov is okay with this, as I'm not really all that familiar with the details of our resolution/crate loading implementation.

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 9dc999e with merge 3bee93ff74bbd046a67c3acfa691de1399929ee2...

[bors]

☀️ Try build successful - checks-azure
Build commit: 3bee93ff74bbd046a67c3acfa691de1399929ee2 (3bee93ff74bbd046a67c3acfa691de1399929ee2)

[rust-timer]

Queued 3bee93ff74bbd046a67c3acfa691de1399929ee2 with parent de6060b, future comparison URL.

[petrochenkov]

@Mark-Simulacrum

I suspect what you meant is that #68941 won't really be useful if we're not loading dependency spans?

#68941 is still useful without this, but incomplete, that PR has a stub producing dummy spans for proc macro crates.

[petrochenkov]

I would also like to make sure that @petrochenkov is okay with this, as I'm not really all that familiar with the details of our resolution/crate loading implementation.

Loading spans from proc macro crates is suboptimal and long term it would be good to inline actually used spans from all build-time dependencies into crates built with their help somehow, but that may be a nontrivial optimization.
For the initial version of span/hygiene serialization I think we can keep the whole proc macro crates around just to retrieve spans from them, if performance allows it.

[rust-timer]

Finished benchmarking try commit 3bee93ff74bbd046a67c3acfa691de1399929ee2, comparison URL.

[bjorn3]

On average there seems to be a <1% regression.

[Mark-Simulacrum]

The perf run had a bunch of crates fail to build (see the log at the bottom); e.g.

hyper-2 failed with:

     Checking futures-util v0.3.1                                                                                                                                            +
 error: /tmp/.tmpuivAPw/target/debug/deps/libfutures_macro-865b1fd2b529a20c.so: undefined symbol: _ZN11unicode_xid6tables16derived_property12XID_Continue17h1c47640b6319d252E+
   --> /home/collector/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-util-0.3.1/src/async_await/join_mod.rs:78:13                                                  +
    |                                                                                                                                                                        +
 78 |     pub use futures_macro::join;                                                                                                                                       +
    |             ^^^^^^^^^^^^^                                                                                                                                              +
                                                                                                                                                                             +
 error: aborting due to previous error                                                                                                                                       +
                                                                                                                                                                             +
 error: could not compile `futures-util`.                                                                                                                                    +
                                                                                                                                                                             +
 To learn more, run the command again with --verbose.                                                                                                                        +
 warning: build failed, waiting for other jobs to finish...                                                                                                                  +
 error: build failed                                                                                                                                                         +
                                                                                                                                                                             +
                                                                                                                                                                             +
  stdout=

Other have similar link failures. We can rerun perf if you think it was spurious, but it feels like errors like that are plausibly caused by this PR somehow?

[Aaron1011]

Closing in favor of #73706, which is much less invasive.