New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `extern crate` items to extern prelude #54658

Merged
merged 4 commits into from Oct 25, 2018

Conversation

@petrochenkov
Contributor

petrochenkov commented Sep 29, 2018

With this patch each extern crate orig_name as name item adds name name into the extern prelude, as if it was passed with --extern.

What changes this causes in practice?
Almost none! After all, --extern passed from Cargo was supposed to replace extern crate items in source, so if some code has extern crate item (or had it on 2015 edition), then it most likely uses --extern as well...

... with exception of a few important cases.

  • Crates using proc_macro. proc_macro is not passed with --extern right now and is therefore not in extern prelude.
    Together with 2018 edition import behavior this causes problems like #54418, e.g.

    extern crate proc_macro;
    use proc_macro::TokenStream;

    doesn't work.
    It starts working after this patch.

  • #[no_std] crates using std conditionally, like @aturon described in #53166 (comment), and still wanting to write std instead of crate::std. This PR covers that case as well.
    This allows us to revert placing std into the extern prelude unconditionally, which was, I think, a bad idea.

  • Later extern crate syntax can be extended to support adding an alias to some local path to extern prelude, as it may be required for resolving #54647.

Notes:

  • Only extern crate items from the root module added to the prelude, mostly because this behavior for items from inner modules would look very strange, rather than for technical reasons.
    This means you can opt out from the prelude additions with something like
    mod inner {
        pub(crate) extern crate foo;
    }
    use inner::foo;
  • I haven't updated logic for 2018 import canaries to work fully correctly with this. The cases where it matters are pretty exotic (the extern crate item must be "sufficiently macro expanded") and I'd rather spend the time on eliminating the canaries entirely.
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Sep 29, 2018

r? @cramertj

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

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Sep 29, 2018

cc @rust-lang/lang and @eddyb specifically

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Sep 29, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:47:50] ....................................................................................................
[00:47:53] ....................................................................................................
[00:47:56] ....................................................................................................
[00:47:59] ...................................i................................................................
[00:48:04] .............................F......................................................................
[00:48:10] .................................................................................................i..
[00:48:13] ....................................................................................................
[00:48:16] ....................................................................................................
[00:48:21] ...................................................i................................................
---
[00:48:22] 
[00:48:22] - warning: unused extern crate
[00:48:22] -   --> $DIR/remove-extern-crate.rs:19:1
[00:48:22] -    |
[00:48:22] - LL | extern crate core;
[00:48:22] -    |
[00:48:22] - note: lint level defined here
[00:48:22] -   --> $DIR/remove-extern-crate.rs:17:9
[00:48:22] -    |
[00:48:22] -    |
[00:48:22] - LL | #![warn(rust_2018_idioms)]
[00:48:22] -    |         ^^^^^^^^^^^^^^^^
[00:48:22] -    = note: #[warn(unused_extern_crates)] implied by #[warn(rust_2018_idioms)]
[00:48:22] - 
[00:48:22] - warning: `extern crate` is not idiomatic in the new edition
[00:48:22] -    |
[00:48:22] -    |
[00:48:22] - LL | extern crate core as another_name;
[00:48:22] -    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: convert it to a `use`
[00:48:22] - 
[00:48:22] - warning: `extern crate` is not idiomatic in the new edition
[00:48:22] -    |
[00:48:22] -    |
[00:48:22] - LL |     extern crate core;
[00:48:22] -    |     ^^^^^^^^^^^^^^^^^^ help: convert it to a `use`
[00:48:22] - 
[00:48:22] 
[00:48:22] 
[00:48:22] 
[00:48:22] error: failed to delete `/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/rust-2018/remove-extern-crate/remove-extern-crate.stderr`: No such file or directory (os error 2)
[00:48:22] tdb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:48:22] 
[00:48:22] 
[00:48:22] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:48:22] Build completed unsuccessfully in 0:03:12
[00:48:22] Build completed unsuccessfully in 0:03:12
[00:48:22] Makefile:58: recipe for target 'check' failed
[00:48:22] make: *** [check] Error 1
ckout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:14a4a7ef
travis_time:start:14a4a7ef
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:015cb8ad
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 29, 2018

r? @eddyb

@rust-highfive rust-highfive assigned eddyb and unassigned cramertj Sep 29, 2018

@@ -11,6 +11,8 @@
// run-pass
// edition:2018
extern crate core;

This comment has been minimized.

@Centril

Centril Sep 29, 2018

Contributor

We would want a dedicated test just for this I think a of copy this file with your change and one without.

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 29, 2018

cc @Nemo157 @japaric @ZackPierce re. the #![no_std] behavior.

Later extern crate syntax can be extended to support adding an alias to some local path to extern prelude, [...]

This would actually almost be a way of building a custom prelude, or adding things to it at least.

It does seem interesting; but if there is time we might want to fcp merge this.

if !attr::contains_name(&krate.attrs, "no_std") {
extern_prelude.insert(Ident::from_str("std"), None);
} else {
extern_prelude.insert(Ident::from_str("core"), None);

This comment has been minimized.

@Nemo157

Nemo157 Sep 29, 2018

Contributor

I really preferred having core accessible even in non-no_std builds. I think the rest of these changes, but with this still outside this else gives the best pattern for crates with a std feature:

#![cfg_attr(not(feature = "std"), no_std)]

use core::mem;
#[cfg(feature = "std")]
use std::boxed::Box;

You get the best of both worlds, minimal overhead compared to a normal std using crate, plus compile-time checking that you don’t accidentally reference std in a no_std build (even when your target has std available).

This comment has been minimized.

@Nemo157

Nemo157 Sep 29, 2018

Contributor

(If it’s possible there could be a warning about using core if your crate has no cfg-d out no_std attribute to push people that aren’t using this pattern to use std always, in case they copy-paste some core using code)

This comment has been minimized.

@Centril

Centril Sep 29, 2018

Contributor

I think using core instead of std where you don't have #![no_std] is nothing bad we should lint against because it encourages keeping crates that much closer to working on #![no_std].

This comment has been minimized.

@Nemo157

Nemo157 Sep 29, 2018

Contributor

I agree with you that use std:: should be possible when #![no_std] is active.

This is the current nightly’s behaviour. I think this should not be possible, if it is ergonomic to do so. The only way to currently check whether you accidentally used std is to setup a build for a target like thumbv6m-none-eabi which doesn’t distribute a std lib, this is a real pain point. I think with the 2018 module changes, and this one change to always have core available (except no_core), it becomes ergonomic enough to have #![no_std] use std::*; be a hard error, and require crates to optionally enable no_std when they want to use std with a feature flag.

This comment has been minimized.

@Centril

Centril Sep 29, 2018

Contributor

I agree with you that use std:: should be possible when #![no_std] is active.

I removed this bit when I realized it was wrong ;)

This comment has been minimized.

@Centril

Centril Sep 29, 2018

Contributor

@petrochenkov Sorry; I don't follow what you are trying to say here...

I think the situation should be like so:

flag_std = true;
flag_core = true;

#![no_core] => [flag_core = false,  #![no_std]]
#![no_std] => [flag_std = false]

flag_core => core is in the prelude.
flag_std  => std is in the prelude.

This comment has been minimized.

@petrochenkov

petrochenkov Sep 29, 2018

Contributor

@Centril
That's exactly what I was trying to say.

This comment has been minimized.

@Centril

Centril Sep 29, 2018

Contributor

@petrochenkov oh, ok, great :) let's do that.

This comment has been minimized.

@Centril

Centril Sep 29, 2018

Contributor

Hmm... is it still the case this works?:

#![no_std]

use ::std::process::Command;
//  -- Note the use of `::`.

This comment has been minimized.

@petrochenkov

petrochenkov Sep 29, 2018

Contributor

Not unless you add extern crate std; explicitly.
Starting with #54116 (the first point) absolute paths only look into crates listed in extern prelude, not crates lying around in library search directories.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Sep 29, 2018

@Centril

This would actually almost be a way of building a custom prelude, or adding things to it at least.

If that effect is preferably avoided, then we can use something less general like extern crate self as foo; tailored specifically for #54647 and unable to put anything else in the prelude.

@Centril

This comment has been minimized.

Contributor

Centril commented Sep 29, 2018

Nominating for discussion on next meeting unless it is resolved before.

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Sep 29, 2018

I think the state after this PR can be summarized as a simple guideline:

Extern crate can be specified either through Cargo.toml or through extern crate in source, in both cases it will behave identically and make the extern crate name available in the whole crate.

(With cases being unspecify-able through Cargo.toml being gradually retired, of course.)

@japaric

This comment has been minimized.

Member

japaric commented Sep 29, 2018

cc Nemo157 japaric ZackPierce re. the #![no_std] behavior.

👍 to this #54658 (comment) / thread of having core in the extern prelude of non-no_std crates; and not having std in the extern prelude of no_std crates.

@aturon

This comment has been minimized.

Member

aturon commented Sep 30, 2018

@rfcbot poll Are we OK with this semantics for extern crate?

@rfcbot

This comment has been minimized.

rfcbot commented Sep 30, 2018

Team member @aturon has asked teams: T-lang, for consensus on:

Are we OK with this semantics for extern crate?

@Centril

This comment has been minimized.

Contributor

Centril commented Oct 1, 2018

@aturon I feel we should discuss this on the next meeting since it's not a trivial change; but I think I'm generally favorably inclined.

@bors

This comment has been minimized.

Contributor

bors commented Oct 1, 2018

☔️ The latest upstream changes (presumably #54650) made this pull request unmergeable. Please resolve the merge conflicts.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 4, 2018

Question, @petrochenkov : how does this work with macro expansions? If a macro expansion generates an extern crate at the root, will that be added to the extern prelude? Is there some concern if there is no item in the prelude until the macro expansion executesion? I'm a bit worried because I remember us saying at some point that it was useful to know "up front" the full set of extern paths, and this seems to remove that invariant.

(cc @eddyb)

@joshtriplett

This comment has been minimized.

Member

joshtriplett commented Oct 4, 2018

This seems consistent with the intent of extern crate providing an interim measure for handling sysroot crates.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 4, 2018

@rfcbot concern macro-expansion

I'd like to understand how the macro-expansion interaction works before we do this. If it all "works fine", I have no objection. (See also this)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Oct 4, 2018

Oh, this is a "poll". I guess you can't add concerns to those. =)

@bors

This comment has been minimized.

Contributor

bors commented Oct 24, 2018

⌛️ Testing commit d1e337b with merge 3da4d6e...

bors added a commit that referenced this pull request Oct 24, 2018

Auto merge of #54658 - petrochenkov:experelude, r=nikomatsakis
Add `extern crate` items to extern prelude

With this patch each `extern crate orig_name as name` item adds name `name` into the extern prelude, as if it was passed with `--extern`.

What changes this causes in practice?
Almost none! After all, `--extern` passed from Cargo was supposed to replace `extern crate` items in source, so if some code has `extern crate` item (or had it on 2015 edition), then it most likely uses `--extern` as well...

... with exception of a few important cases.

- Crates using `proc_macro`. `proc_macro` is not passed with `--extern` right now and is therefore not in extern prelude.
Together with 2018 edition import behavior this causes problems like #54418, e.g.
    ```rust
    extern crate proc_macro;
    use proc_macro::TokenStream;
    ```
    doesn't work.
It starts working after this patch.

- `#[no_std]` crates using `std` conditionally, like @aturon described in #53166 (comment), and still wanting to write `std` instead of `crate::std`. This PR covers that case as well.
This allows us to revert placing `std` into the extern prelude unconditionally, which was, I think, a [bad idea](#53166 (comment)).

- Later `extern crate` syntax can be extended to support adding an alias to some local path to extern prelude, as it may be required for resolving #54647.

Notes:
- Only `extern crate` items from the root module added to the prelude, mostly because this behavior for items from inner modules would look very strange, rather than for technical reasons.
This means you can opt out from the prelude additions with something like
    ```rust
    mod inner {
        pub(crate) extern crate foo;
    }
    use inner::foo;
    ```
- I haven't updated logic for 2018 import canaries to work fully correctly with this. The cases where it matters are pretty exotic (the `extern crate` item must be "sufficiently macro expanded") and I'd rather spend the time on eliminating the canaries entirely.
@Mark-Simulacrum

This comment has been minimized.

Member

Mark-Simulacrum commented Oct 24, 2018

@bors retry - prioritize stable

@kennytm

This comment has been minimized.

Member

kennytm commented Oct 24, 2018

@bors p=172

rollup fairness

@bors

This comment has been minimized.

Contributor

bors commented Oct 24, 2018

⌛️ Testing commit d1e337b with merge b5a4b0a...

bors added a commit that referenced this pull request Oct 24, 2018

Auto merge of #54658 - petrochenkov:experelude, r=nikomatsakis
Add `extern crate` items to extern prelude

With this patch each `extern crate orig_name as name` item adds name `name` into the extern prelude, as if it was passed with `--extern`.

What changes this causes in practice?
Almost none! After all, `--extern` passed from Cargo was supposed to replace `extern crate` items in source, so if some code has `extern crate` item (or had it on 2015 edition), then it most likely uses `--extern` as well...

... with exception of a few important cases.

- Crates using `proc_macro`. `proc_macro` is not passed with `--extern` right now and is therefore not in extern prelude.
Together with 2018 edition import behavior this causes problems like #54418, e.g.
    ```rust
    extern crate proc_macro;
    use proc_macro::TokenStream;
    ```
    doesn't work.
It starts working after this patch.

- `#[no_std]` crates using `std` conditionally, like @aturon described in #53166 (comment), and still wanting to write `std` instead of `crate::std`. This PR covers that case as well.
This allows us to revert placing `std` into the extern prelude unconditionally, which was, I think, a [bad idea](#53166 (comment)).

- Later `extern crate` syntax can be extended to support adding an alias to some local path to extern prelude, as it may be required for resolving #54647.

Notes:
- Only `extern crate` items from the root module added to the prelude, mostly because this behavior for items from inner modules would look very strange, rather than for technical reasons.
This means you can opt out from the prelude additions with something like
    ```rust
    mod inner {
        pub(crate) extern crate foo;
    }
    use inner::foo;
    ```
- I haven't updated logic for 2018 import canaries to work fully correctly with this. The cases where it matters are pretty exotic (the `extern crate` item must be "sufficiently macro expanded") and I'd rather spend the time on eliminating the canaries entirely.
@pietroalbini

This comment has been minimized.

Member

pietroalbini commented Oct 24, 2018

@bors retry -- prioritize stable release

@bors

This comment has been minimized.

Contributor

bors commented Oct 24, 2018

⌛️ Testing commit d1e337b with merge 8fb0190...

bors added a commit that referenced this pull request Oct 24, 2018

Auto merge of #54658 - petrochenkov:experelude, r=nikomatsakis
Add `extern crate` items to extern prelude

With this patch each `extern crate orig_name as name` item adds name `name` into the extern prelude, as if it was passed with `--extern`.

What changes this causes in practice?
Almost none! After all, `--extern` passed from Cargo was supposed to replace `extern crate` items in source, so if some code has `extern crate` item (or had it on 2015 edition), then it most likely uses `--extern` as well...

... with exception of a few important cases.

- Crates using `proc_macro`. `proc_macro` is not passed with `--extern` right now and is therefore not in extern prelude.
Together with 2018 edition import behavior this causes problems like #54418, e.g.
    ```rust
    extern crate proc_macro;
    use proc_macro::TokenStream;
    ```
    doesn't work.
It starts working after this patch.

- `#[no_std]` crates using `std` conditionally, like @aturon described in #53166 (comment), and still wanting to write `std` instead of `crate::std`. This PR covers that case as well.
This allows us to revert placing `std` into the extern prelude unconditionally, which was, I think, a [bad idea](#53166 (comment)).

- Later `extern crate` syntax can be extended to support adding an alias to some local path to extern prelude, as it may be required for resolving #54647.

Notes:
- Only `extern crate` items from the root module added to the prelude, mostly because this behavior for items from inner modules would look very strange, rather than for technical reasons.
This means you can opt out from the prelude additions with something like
    ```rust
    mod inner {
        pub(crate) extern crate foo;
    }
    use inner::foo;
    ```
- I haven't updated logic for 2018 import canaries to work fully correctly with this. The cases where it matters are pretty exotic (the `extern crate` item must be "sufficiently macro expanded") and I'd rather spend the time on eliminating the canaries entirely.
@bors

This comment has been minimized.

Contributor

bors commented Oct 25, 2018

💔 Test failed - status-appveyor

@kennytm

This comment has been minimized.

Member

kennytm commented Oct 25, 2018

@bors retry p=1

3 hour timeout again.

@bors

This comment has been minimized.

Contributor

bors commented Oct 25, 2018

⌛️ Testing commit d1e337b with merge 365b900...

bors added a commit that referenced this pull request Oct 25, 2018

Auto merge of #54658 - petrochenkov:experelude, r=nikomatsakis
Add `extern crate` items to extern prelude

With this patch each `extern crate orig_name as name` item adds name `name` into the extern prelude, as if it was passed with `--extern`.

What changes this causes in practice?
Almost none! After all, `--extern` passed from Cargo was supposed to replace `extern crate` items in source, so if some code has `extern crate` item (or had it on 2015 edition), then it most likely uses `--extern` as well...

... with exception of a few important cases.

- Crates using `proc_macro`. `proc_macro` is not passed with `--extern` right now and is therefore not in extern prelude.
Together with 2018 edition import behavior this causes problems like #54418, e.g.
    ```rust
    extern crate proc_macro;
    use proc_macro::TokenStream;
    ```
    doesn't work.
It starts working after this patch.

- `#[no_std]` crates using `std` conditionally, like @aturon described in #53166 (comment), and still wanting to write `std` instead of `crate::std`. This PR covers that case as well.
This allows us to revert placing `std` into the extern prelude unconditionally, which was, I think, a [bad idea](#53166 (comment)).

- Later `extern crate` syntax can be extended to support adding an alias to some local path to extern prelude, as it may be required for resolving #54647.

Notes:
- Only `extern crate` items from the root module added to the prelude, mostly because this behavior for items from inner modules would look very strange, rather than for technical reasons.
This means you can opt out from the prelude additions with something like
    ```rust
    mod inner {
        pub(crate) extern crate foo;
    }
    use inner::foo;
    ```
- I haven't updated logic for 2018 import canaries to work fully correctly with this. The cases where it matters are pretty exotic (the `extern crate` item must be "sufficiently macro expanded") and I'd rather spend the time on eliminating the canaries entirely.
@bors

This comment has been minimized.

Contributor

bors commented Oct 25, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 365b900 to master...

@bors bors merged commit d1e337b into rust-lang:master Oct 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@XX

This comment has been minimized.

XX commented Oct 31, 2018

@petrochenkov @bors Are these changes already released in beta? I'm trying to build my proc macro, but I get this error:

$ cargo +beta build
   Compiling field_types v1.0.0 (/mnt/data/projects/field_types)
error[E0658]: use of extern prelude names introduced with `extern crate` items is unstable (see issue #54658)
 --> src/lib.rs:7:5
  |
7 | use proc_macro::TokenStream;
  |     ^^^^^^^^^^

error: aborting due to previous error
@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Oct 31, 2018

@XX
Technically they are released on beta, yes, but the feature is unstable and gated, so de-facto it's available on nightly until stabilized.

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Nov 1, 2018

@petrochenkov rustc points here for the tracking issue for this feature, but this is a PR. Could you open an issue to track stabilization of this feature, and change rustc so point to that instead?

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 1, 2018

@SimonSapin
Yeah, I was lazy here, sorry. Will open an issue soon.

UPDATE: Done in #55601, the tracking issue is #55599.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment