rust-lang / wg-cargo-std-aware

Repo for working on "std aware cargo"
133 stars 8 forks source link

Using `--extern` is apparently not equivalent to `--sysroot` #40

Closed alexcrichton closed 4 years ago

alexcrichton commented 4 years ago

Testing out building a crate graph with -Z build-std=std,alloc (note that the need to pass alloc is a separate bug) ends up yielding:

   Compiling crossbeam-utils v0.6.6
error: macro-expanded `extern crate` items cannot shadow names passed with `--extern`
  --> /home/alex/.cargo/registry/src/github.com-1ecc6299db9ec823/crossbeam-utils-0.6.6/src/lib.rs:43:9
   |
43 |         extern crate std as alloc;
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

error: Could not compile `crossbeam-utils`.

To learn more, run the command again with --verbose.

Sure enough this code:

macro_rules! a {
    () => (extern crate std as alloc;)
}
a!();

fails to compile:

$ rustc +nightly foo.rs --crate-type rlib --extern alloc -Z unstable-options
error: macro-expanded `extern crate` items cannot shadow names passed with `--extern`
 --> foo.rs:2:12
  |
2 |     () => (extern crate std as alloc;)
  |            ^^^^^^^^^^^^^^^^^^^^^^^^^^
3 | }
4 | a!();
  | ----- in this macro invocation

error: aborting due to previous error

I'm not entirely sure the origin of this error, but it seems that we either need to:

Ericson2314 commented 4 years ago

Hmm what happens if you pass an empty sysroot? (See https://github.com/rust-lang/wg-cargo-std-aware/issues/31 where it mentions that). Perhaps there is an issue of search priority?

ehuss commented 4 years ago

Ouch. Looks like this stems from https://github.com/rust-lang/rust/pull/54658#issuecomment-432030209.

@Ericson2314 redirecting --sysroot doesn't matter here.

I don't really understand why they are not allowed to shadow. Might be good to better understand that (and why sysroot is allowed to be shadowed but not --extern).

ehuss commented 4 years ago

@petrochenkov can you explain why a macro-expanded extern crate is allowed to shadow a sysroot crate, but not one from --extern? Is this restriction intended to be permanent?

For context, this project is to add standard library support to Cargo. My original strategy was to use --extern flags to tell rustc where to load the newly built std crates. However, it runs afoul of the check added in https://github.com/rust-lang/rust/pull/54658 for things like this idiom in crossbeam-utils.

petrochenkov commented 4 years ago

@ehuss Sysroot crates do not exist from name resolution point of view, so they cannot be shadowed. extern crate my_sysroot; is the only entity that pulls a crate from sysroot (or a crate from -L directories) into name resolution. (Note, that std and core specifically behave as if --extern std/core is passed implicitly, so they exist from name resolution point of view, and can be affected by shadowing restrictions.)

The shadowing restriction can be relaxed (by using may_appear_after instead of expansion != ExpnId::root(), https://github.com/rust-lang/rust/pull/54658 was a bit lazy in this regard), but not removed entirely.

alexcrichton commented 4 years ago

Hm so now this is sort of an interesting question. I think that we want -Z build-std to have the same semantics both if you use and if you don't. It looks like both core and std are implicit names in name resolution today, but both proc_macro and alloc, the other two stable crates, need to be implicitly exported. By using --extern alloc=... and --extern proc_macro=... we're actually changing the semantics of -Z build-std code because they don't need to say extern crate alloc. That seems bad!

Now the "solution" to this is probably to use --sysroot, but I'm realizing now I don't actually think that this is an easily viable option. The reason for this is that crates have implicit access to all crates in the sysroot, including the dependencies of std itself. For example you can do extern crate backtrace; today on nightly but it just gives you a warning about the usage of the rustc_private unstable feature. If we don't build crates with -Zforce-unstable-if-unmarked then crates will have implicit stable access to backtrace, for example, using -Zbuild-std.

So I think that our main goal should be parity between "normal mode" and -Z build-std mode. We can't today achieve that with --extern both because of this bug and also because today stable Rust requires extern crate alloc, even though alloc is stable. We also can't achieve it today with --sysroot because we have no way of building the dependencies of libstd as unstable without a -Z flag.

I think the best way forward, in general, is to "basically do the same thing" as rust-lang/rust. I think that we should use --sysroot (or -L) to load libstd crates, and we should also figure out how to mark them as inaccessible (maybe that's with the -Z flag of today, maybe not).

ehuss commented 4 years ago

Using --sysroot is doable but much more complicated, and will require a lot of changes. I'd like to make sure we're certain it is the way to go. I don't mind rewriting it, I just don't want to make this change haphazardly. (Using --sysroot was my initial approach, but I was lured by the simplicity of using --extern.)

I was originally thinking alloc and proc_macro would need to be explicitly mentioned as dependencies in Cargo.toml, otherwise the --extern flag wouldn't be used. However, I see that would be a backwards-incompatible change for dependencies that aren't std-aware.

Hm, it's a bit unfortunate, but I can't think of better options.

alexcrichton commented 4 years ago

My current thinking is we should do one of two things:

I'm a fan personally of avoiding rustc changes if we can, but you're probably not wrong in that this is a much easier rustc change than a Cargo change. I was sort of hoping we could "just change the --out-dir" for libstd crates to the sysroot and don't pass --extern at all for sysroot crates in Cargo, but I suspect that's a bit too naive.

ehuss commented 4 years ago

@alexcrichton I've been blocked for a while trying to think of a way to get the mock test suite to work. The fundamental problem is that there are duplicate core or std crate errors because there is both the mock one and the real one in the link path. Do you have any ideas on how the mock suite can be changed to work?

alexcrichton commented 4 years ago

Oh dear yeah I can see where that would cause problems. Want to gist what you've got and I can try to poke around?

It may be that "mock std" either fundamentally no longer works or we can add name = "something-other-than-std" annotations and hack our way to success. It may be sort of fundamentally a broken idea now though, which would be a bummer!

ehuss commented 4 years ago

Here is a work-in-progress (no test changes): https://github.com/ehuss/cargo/commit/1ed87601630f88cb53c930473d3753454eda599f

I've been trying a variety of things but keep hitting different problems. I was thinking of manually adding --extern flags into the original sysroot, but I keep hitting various issues.

alexcrichton commented 4 years ago

Not my proudest moment, but I think this does the trick - https://github.com/alexcrichton/cargo/commit/dead4d0a34ff899051ed940236fdc263d89a75b5

alexcrichton commented 4 years ago

Fixed in https://github.com/rust-lang/cargo/pull/7421

Ericson2314 commented 4 years ago

--extern-sysroot is a much better fix, I believe, not just easier.