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

(Modules) Tracking issue for (use) crate_name:: paths without extern crate #53128

Closed
2 of 3 tasks
Centril opened this issue Aug 6, 2018 · 80 comments
Closed
2 of 3 tasks
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Milestone

Comments

@Centril
Copy link
Contributor

Centril commented Aug 6, 2018

This is a sub-tracking issue for the RFC "Clarify and streamline paths and visibility" (rust-lang/rfcs#2126)
dealing with the deprecation of extern crate as well as having use crate_name::foo::bar and crate_name::foo::bar Just Work™.

Unresolved questions:

None

@Centril Centril added B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. labels Aug 6, 2018
@joshtriplett
Copy link
Member

I feel that the deprecation of extern crate and the automatic inclusion of external crates in the prelude has gotten enough exposure and positive feedback that it's ready for us to commit to as the path we're going down. We shouldn't stabilize it until we stabilize the macro system changes, but we can handle that via a blocking concern; in the meantime, let's confirm that we have consensus.

Note that while we're still talking about the final system for paths in use statements, both of the proposals currently under consideration incorporate this deprecation of extern crate and automatic inclusion of external crates in the prelude.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Aug 6, 2018

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 6, 2018
@joshtriplett
Copy link
Member

@rfcbot concern macros

We shouldn't stabilize this until we first stabilize the macro system changes needed to deprecate #[macro_use] extern crate as well.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 6, 2018
@Centril Centril added this to the Rust 2018 RC milestone Aug 6, 2018
@matklad
Copy link
Member

matklad commented Aug 6, 2018

Not really a concern, but some tradeoffs about automatic inclusion of external crates in the prelude, mostly from internals thread.

In Rust 2015, there are two ways to refer to extern crate foo:

  • add use foo to the top of the file and write foo::Bar in the module's body (special case in the root: extern crate instead of use).
  • write ::foo::Bar in the module body

Extern crates in prelude adds a third one:

  • write foo::Bar in the module body

This third variant is easier to type, but it gives the least amount of information to the reader.

  • With the first option, two readability properties are maintained. It is possible to use text search in a single file to see where the foo identifier comes from. By looking at the list of imports, it is possible to say, based on the imported crates, what the module could do, without reading its source.
  • With the second option, it is immediately clear where the foo comes from, especially in 2018 edition, although it's not possible to scan a list of imports to get an "overview" of the module dependencies.
  • With the the third option, neither text search, not dependencies overview work. Although the set of extern crates is usually not very big, it is project specific, so, seeing a foo::bar in an unfamiliar code base, it might be not trivial to understand where the foo comes from.

It is also seems to be the case that adding extern crates to the prelude is not required to make other module systems changes "work". Specifically, ergonimics is not regressed compared to Rust 2015: ::foo::Bar style paths continue to work. use foo; foo::Bar pairs continue to work, extern crate foo; foo::Bar pair changes to use foo; foo::Bar. The only exception here is std in the root module, whose extern crate std is added implicitly. This is mitigated by the fact that popular types of std are in prelude and that std does not contain top-level items, so submodules are often usually imported anyway.

It is true that some use statements might be dropped with this feature in 2018.

The original motivation for this feature was to fix path confusion: the fact that paths in use statements and everywhere else work differently. This is only a partial fix though:

  • use foo::Bar and foo::Bar in fn body will resolve to the same Bar, but for different reasons: in the first case, because the path is equivalent to ::foo::Bar, in the second case, because foo is in prelude.
  • This does not solve the problem of needing self:: in use to refer to submodules and local enums, and there's a separate proposal to fix that.

TL;DR

extern crates in prelude

  • seem to violate "optimize for the reader" rule,
  • are not necessary to make 2018 systems coherent (and could be added in the next edition),
  • don't quite solve the original problem they were a solution for.

@sanmai-NL
Copy link

@matklad: your critique ultimately hinges on the outcome of #53130, no? AFAICT, the ambiguity does not appear if we choose anchored_use_paths there.

@matklad
Copy link
Member

matklad commented Aug 7, 2018

#53130 is about paths in use statements. The addition of crates to prelude affects paths outside of use statements. I.e., even with anchored_use_path (which is what currently is default with edition 2018 I believe), fn main() { regex::Regex::new(".*").unwrap().expect() } works.

@sanmai-NL
Copy link

Ah, right. I am only commenting as production user of Rust and haven’t paid close attention to the whole discussion/RFCs so please bear with me. 🙂
Why wouldn’t an obvious solution that springs me to mind, a uniform path syntax throughout (use statements and elsewhere), i.e. something along the lines of anchored_paths, work then?

@matklad
Copy link
Member

matklad commented Aug 7, 2018

a uniform path syntax throughout

The devil is in the details, and the most devilish detail here is which syntax to pick to refer to external crates. The two options are

  • foo::bar always refers to an extern crate, unless there's a foo name in local scope. This is achieved via adding extern crates to prelude, and this "unless" bit is what makes me feel uneasy about this approach and the core issue behind my big comment.

  • ::foo::bar always refers to an extern crate, and you have to use that leading :: in use statements as well (currently, it is optional in use, and that leads to two-flavors of paths and the need for self::). To me personally this looks like the obviously right solution, but turns out a lot of folks really don't like the leading ::.

@sanmai-NL
Copy link

sanmai-NL commented Aug 7, 2018

I second that the second solution is obviously right, vis-à-vis. It’s a pity some programmers dislike verbosity so much they would rather introduce ambiguity...

But there’s a discrepancy still, given anchored_use_paths, in use statements referring to items in external crates, paths wouldn’t contain a leading :: whereas they do in paths elsewhere. Just for consistency (and learnability) I prefer to have one single path syntax. Update: as you wrote, they currently may have the leading :: but it isn’t required.

Update
The general idea I’m trying to sketch:

  • Dependencies on crates are made through Cargo.toml (ultimately, as parameters to the compiler invocation), not through code statements. This means, extern crate syntax is abandoned.
  • There’s a single path syntax everywhere. Every path is unambiguous meaning it must be anchored somehow. It seems this notion isn’t fully accepted in the current proposals.
  • use statements only serve to abbreviate paths to shorthand names in an appropriate scope. Any identifier must either be a bound in e.g. let binding or to a path in a use statement.

I’m sure this idea isn’t specified carefully enough yet, but I hope we can have fruitful discussion at least.

@cramertj
Copy link
Member

cramertj commented Aug 7, 2018

@rfcbot concern no_std-std

I just opened #53166. It's not a huge deal (it's a fairly odd corner case), but I'd like to get some amount of resolution on it before starting FCP.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 7, 2018
@SimonSapin
Copy link
Contributor

Would for example use proc_macro::TokenStream; "just work" without extern crate proc_macro;? (For sysroot crates not listed explicitly with --extern.)

@petrochenkov
Copy link
Contributor

@SimonSapin
This depends on the choice made with regards to #53130.

  • In the "uniform path" model use proc_macro::TokenStream won't work unless proc_macro is specifically hardcoded in the compiler, but the disambiguated form use ::proc_macro::TokenStream will.
  • In the "anchored use path" model the starting :: is still added implicitly, so use proc_macro::TokenStream will work.

@eddyb
Copy link
Member

eddyb commented Aug 10, 2018

@SimonSapin @petrochenkov So there's one more concern, that @aturon and @joshtriplett raised when discussing #52923 (that I'm not sure is written down anywhere else), around the fact that ::test (the crate) and self::test (a local module) would both work if we keep the behavior you talk about, but "uniform paths" would have to choose one of:

  • detect an ambiguity for any unqualified import starting with a name for which a crate can be loaded, even when the crate isn't importable through an unqualified path (i.e. it's not in extern_prelude) - this is unacceptable given how many modules are named test across the ecosystem (there are more examples, like syntax or log, but those are rarer)
  • special-case ambiguity detection to only fire if the crate could be imported unqualified (i.e. if it's in the extern_prelude), making it seem inconsistent with the simple interpretation of "only one of ::x and self::x must exist"

Alternatively, we can make ::crate_name not work for any crate not in the extern_prelude, and still require extern crate / --extern for those crates until a better mechanism (preferably involving Cargo.toml) is introduced.

@joshtriplett
Copy link
Member

@eddyb For the record, for the moment I'd favor the solution of having the ::crate_name syntax not work for crates not named in Cargo.toml / provided via --extern. The goal of removing extern crate is to avoid duplication and having to list the external crate twice, but it needs listing at least once.

@SimonSapin
Copy link
Contributor

I haven’t followed the details of the different path proposals, I mostly wanted to remind that there can be more crate than Cargo dependencies and std. If such crates still need extern crate to be used, then extern crate still need to be taught to new users.

@aturon
Copy link
Member

aturon commented Aug 10, 2018

@SimonSapin to clarify, @joshtriplett is talking specifically about "sysroot" crates (like test) -- he mentioned that passing in via --extern is fine. Today these crates are available without saying anything to the compiler or Cargo, and there's an opportunity to clean up this behavior a bit.

@SimonSapin
Copy link
Contributor

@aturon Are you suggesting that all sysroot crates would always be specified through --extern? If so it seems concerning that mentioning a ::proc_macro::Something path anywhere in the crate is all it takes to start linking against the compiler.

@joshtriplett
Copy link
Member

@SimonSapin

Are you suggesting that all sysroot crates would always be specified through --extern?

Not automatically; they'd need to be specified explicitly in order to use them, via some "allow the use of this sysroot crate" option (and equivalent in Cargo.toml).

If so it seems concerning that mentioning a ::proc_macro::Something path anywhere in the crate is all it takes to start linking against the compiler.

That's exactly the concern, and we want to avoid that.

In the short term, we'll probably just continue requiring extern crate proc_macro;. In the long term, we'd like to have some kind of --extern-builtin proc_macro, and a corresponding Cargo.toml dependency line like proc_macro = "builtin".

@Nemo157
Copy link
Member

Nemo157 commented Aug 12, 2018

I just encountered another case where you still require some explicit inclusion of a dependency with this new system: crates which have a public API of only a lang item like panic_implementation, e.g. panic-abort. Without any reference to the dependency it is not linked into the final binary so no implementation is found.

@rpjohnst
Copy link
Contributor

In other languages it's a common requirement that imports are declared before use.

This isn't changing. You still have use or fully-qualified paths; the only thing going away is the redundant extern crate some_name before the more-useful use some_name::actual_item.

@joshtriplett
Copy link
Member

@spearman Because the build system has to know about it, and it shouldn't appear in two places.

Regarding fmod, and other crates that differ from their package names, I wonder if it might be worth going through those and working with their authors to transition to having the two match, for simplicity's sake alone?

@rpjohnst 👍

@sanmai-NL
Copy link

@joshtriplett: Yes please! There’s also the difference in acceptance of - and _ in names. This causes needless friction. I do hope, once everyone is on board, the name rules are actually going to be enforced rather than remedied afterwards in the future.

@spearman
Copy link

@joshtriplett I'm arguing it should appear in both places because they do different things: extern crate in a root module declares an external dependency, and the dependency entry in the cargo manifest tells the build system where to find that dependency (whether it's on crates.io, is a git repository, or a local path, etc.)

@sanmai-NL
Copy link

sanmai-NL commented Oct 20, 2018

@spearman The use statement also declares a dependency, at least it does now with the more explicit anchored paths as implemented in Rust 2018 (#53130).

@spearman
Copy link

@sanmai-NL so does this become this? or this

@sanmai-NL
Copy link

sanmai-NL commented Oct 20, 2018

The latter. It remains unambiguous though that rand is an external module. You can put the use statement on top at top level if you think that’s clearer.

@Centril
Copy link
Contributor Author

Centril commented Dec 22, 2018

@rust-lang/lang From what I can tell there's nothing left to do here? Shall we close this issue?

@spearman
Copy link

spearman commented Dec 22, 2018

@cramertj
Copy link
Member

cramertj commented Dec 26, 2018

@spearman It's correct to generate a warning for that code-- the use of rand::random() does not require use rand; in the 2018 edition.

@spearman
Copy link

Previously warn(unused_extern_crates) was a way to detect unused dependencies. How can I:

  1. declare dependencies in the root module
  2. be warned when those dependencies are not used

As it is now, cargo builds all dependencies and does not warn if they are unused.

@cramertj
Copy link
Member

cramertj commented Dec 26, 2018

@spearman You cannot currently do what you describe.

@mark-i-m
Copy link
Member

mark-i-m commented Jan 2, 2019

That seems unfortunate... perhaps its worth it's own issue?

@alexreg
Copy link
Contributor

alexreg commented Jan 2, 2019

@mark-i-m Yeah, I'd create one. A PR to add a new lint for this probably wouldn't be too hard.

@mark-i-m
Copy link
Member

mark-i-m commented Jan 2, 2019

Opened #57274

@Centril
Copy link
Contributor Author

Centril commented Jan 2, 2019

I believe this is done now so I'll close this out.

@Centril Centril closed this as completed Jan 2, 2019
@mark-i-m
Copy link
Member

mark-i-m commented Jan 2, 2019

@Centril Was this issue supposed to cover getting rid of extern crate altogether? Because we have not done that yet; you still need it for core, std, etc

@cramertj
Copy link
Member

cramertj commented Jan 2, 2019

@mark-i-m Are you referring to how std-surfaced macro intrinsics aren't yet usable by their std paths? e.g. This code only works if you remove std:::

fn main() {
    println!("{}", std::format_args!("{}", 5));
}
error[E0433]: failed to resolve: could not find `format_args` in `std`
 --> src/main.rs:2:25
  |
2 |     println!("{}", std::format_args!("{}", 5));
  |                         ^^^^^^^^^^^ could not find `format_args` in `std`

error: aborting due to previous error

@mark-i-m
Copy link
Member

mark-i-m commented Jan 2, 2019

@cramertj Not sure. It might be related. I was thinking more about how one must do things like extern crate core in no_std crates. You still have to do that for sysroot crates.

@Centril
Copy link
Contributor Author

Centril commented Jan 2, 2019

@mark-i-m Could you create targeted issues for the remaining work if there is any? That way it's easier to triage... :)

@mark-i-m
Copy link
Member

mark-i-m commented Jan 2, 2019

I opened #57288.

@cramertj I don't fully understand the error you brought up. Is it the same one or different? Should I create a new tracking issue for it?

@cramertj
Copy link
Member

cramertj commented Jan 2, 2019

@mark-i-m It is a different issue. Sure, if you would that would be great!

@mark-i-m
Copy link
Member

mark-i-m commented Jan 2, 2019

Done. #57289 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests