Skip to content
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
Merged

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov 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.

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
Copy link
Collaborator

r? @cramertj

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2018
@petrochenkov
Copy link
Contributor Author

cc @rust-lang/lang and @eddyb specifically

@rust-highfive
Copy link
Collaborator

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
Copy link
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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Centril
Copy link
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

Copy link
Contributor

@Centril Centril Sep 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#![no_std]

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

@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.

@petrochenkov petrochenkov added T-lang Relevant to the language team, which will review and decide on the PR/issue. S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 29, 2018
@Centril
Copy link
Contributor

Centril commented Sep 29, 2018

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

@petrochenkov
Copy link
Contributor Author

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
Copy link
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
Copy link
Member

aturon commented Sep 30, 2018

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

@rfcbot
Copy link

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
Copy link
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
Copy link
Contributor

bors commented Oct 1, 2018

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

@nikomatsakis
Copy link
Contributor

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
Copy link
Member

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

@nikomatsakis
Copy link
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)

@bors
Copy link
Contributor

bors commented Oct 24, 2018

⌛ Testing commit d1e337b with merge 8fb01908103f7dea32ce931c73987a3a597f2a02...

@bors
Copy link
Contributor

bors commented Oct 25, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 25, 2018
@kennytm
Copy link
Member

kennytm commented Oct 25, 2018

@bors retry p=1

3 hour timeout again.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 25, 2018
@bors
Copy link
Contributor

bors commented Oct 25, 2018

⌛ Testing commit d1e337b with merge 365b900...

bors added a commit that referenced this pull request Oct 25, 2018
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
Copy link
Contributor

bors commented Oct 25, 2018

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

@XX
Copy link

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
Copy link
Contributor Author

@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
Copy link
Contributor

@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
Copy link
Contributor Author

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.

bors added a commit that referenced this pull request Dec 1, 2018
experiment: Support aliasing local crate root in extern prelude

This PR provides some minimally invasive solution for the 2018 edition migration issue described in #54647 and affecting proc macro crates.

`extern crate NAME as RENAME;` now accepts `NAME`=`self` and interprets it as referring to the local crate.
As with other `extern crate` items, `RENAME` in this case gets into extern prelude in accordance with #54658, thus resolving #54647.
```rust
extern crate self as serde; // Adds local crate to extern prelude as `serde`
```
This solution doesn't introduce any new syntax and has minimal maintenance cost, so it can be easily deprecated if something better is found in the future.

Closes #54647
@petrochenkov petrochenkov deleted the experelude branch June 5, 2019 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.