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

rustc_resolve: don't allow paths starting with ::crate. #53347

Merged
merged 1 commit into from
Aug 17, 2018

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Aug 14, 2018

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 14, 2018
@rust-highfive

This comment has been minimized.

@eddyb
Copy link
Member Author

eddyb commented Aug 14, 2018

cc @nikomatsakis If we go down this path, chalk at least needs to be updated.
AFAIK crate::... without a leading :: should work before this PR too.

@petrochenkov
Copy link
Contributor

Implementation looks good to me, but I'd like to wait for other people's opinions.

I've seen an argument that ::crate may be useful in scenarios like

macro m($crate_name: ident) {
   ::$crate_name::something();
}

m!(my_crate); // OK
m!(crate); // OK

@joshtriplett
Copy link
Member

joshtriplett commented Aug 14, 2018

@petrochenkov Doesn't seem that hard to write m!(::my_crate) or m!(crate), in that case.

I don't have any fundamental objection to ::crate::foo working, but since crate:: is already unambiguous, it seems preferable to have one way to write it rather than two.

@petrochenkov
Copy link
Contributor

@joshtriplett

Doesn't seem that hard to write m!(::my_crate) or m!(crate), in that case.

Yes, this should be possible when #48067 is implemented (paths cannot be easily concatenated in macros right now).

Anyway, prohibiting ::crate is more conservative variant so we can do it now and re-allow again if it becomes necessary.

varkor added a commit to varkor/chalk that referenced this pull request Aug 14, 2018
@eddyb eddyb force-pushed the no-crate-in-root branch from fd8ab07 to 2c4a2fc Compare August 14, 2018 21:41
@rust-highfive

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Do we have a label "ping @rust-lang/lang, wait for a few days and then r+ even if nobody responded"?

@petrochenkov petrochenkov added 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 Aug 15, 2018
@eddyb eddyb added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Aug 15, 2018
@cramertj
Copy link
Member

We discussed this in a previous meeting and I got the impression that there was consensus around ::<cratename>::.. being the one and only way to use leading-:: in a path.

@Centril
Copy link
Contributor

Centril commented Aug 15, 2018

I second @cramertj. ::crate should not be permitted; or at the very least I'd want to be conservative here.

@joshtriplett
Copy link
Member

This still needs its build failures fixed (s/::crate::/crate::/g everywhere), but as soon as that happens, let's merge this.

@joshtriplett joshtriplett added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Aug 16, 2018
@eddyb eddyb removed the I-nominated label Aug 16, 2018
@eddyb eddyb force-pushed the no-crate-in-root branch from 2c4a2fc to 9347bf7 Compare August 16, 2018 23:31
@eddyb
Copy link
Member Author

eddyb commented Aug 16, 2018

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Aug 16, 2018

📌 Commit 9347bf78aaadf67967bd03b144bce9cf99850203 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 16, 2018
@rust-highfive

This comment has been minimized.

@eddyb eddyb force-pushed the no-crate-in-root branch from 9347bf7 to be90514 Compare August 17, 2018 00:36
@eddyb
Copy link
Member Author

eddyb commented Aug 17, 2018

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Aug 17, 2018

📌 Commit be90514154a4202ba27a0be50d64ff4fe193c61c has been approved by petrochenkov

@bors
Copy link
Contributor

bors commented Aug 17, 2018

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout no-crate-in-root (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self no-crate-in-root --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
warning: Cannot merge binary files: src/Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging src/Cargo.lock
CONFLICT (content): Merge conflict in src/Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 17, 2018
@eddyb eddyb force-pushed the no-crate-in-root branch from be90514 to 9b1d3c7 Compare August 17, 2018 10:00
@eddyb
Copy link
Member Author

eddyb commented Aug 17, 2018

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented Aug 17, 2018

📌 Commit 9b1d3c7 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 17, 2018
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Aug 17, 2018
rustc_resolve: don't allow paths starting with `::crate`.

cc @aturon @joshtriplett
r? @petrochenkov
bors added a commit that referenced this pull request Aug 17, 2018
Rollup of 11 pull requests

Successful merges:

 - #52858 (Implement Iterator::size_hint for Elaborator.)
 - #53321 (Fix usage of `wasm_target_feature`)
 - #53326 ([nll] add regression test for issue #27868)
 - #53347 (rustc_resolve: don't allow paths starting with `::crate`.)
 - #53349 ([nll] add tests for #48697 and #30104)
 - #53357 (Pretty print btreemap for GDB)
 - #53358 (`{to,from}_{ne,le,be}_bytes` for unsigned integer types)
 - #53406 (Do not suggest conversion method that is already there)
 - #53407 (make more ported compile fail tests more robust w.r.t. NLL)
 - #53413 (Warn that `#![feature(rust_2018_preview)]` is implied when the edition is set to Rust 2018.)
 - #53434 (wasm: Remove --strip-debug argument to LLD)

Failed merges:

r? @ghost
@bors bors merged commit 9b1d3c7 into rust-lang:master Aug 17, 2018
@eddyb eddyb deleted the no-crate-in-root branch August 17, 2018 19:12
@rpjohnst
Copy link
Contributor

Being conservative here is fine, especially now that :: has become far less necessary, but it seems more consistent to me to allow ::crate because crate plays the same role as crate_name.

@eddyb
Copy link
Member Author

eddyb commented Aug 18, 2018

@rpjohnst The current interpretation isn't that, but rather that crate is a keyword just like self and super, that only appears at the start of a path, and changes it from the default mode (whichever it may be, Rust 2015 vs Rust 2015, with and without uniform_paths) to the root of the local crate.

@rpjohnst
Copy link
Contributor

I get that interpretation, but the ::crate/"crate is a crate name" mental model is a great way to explain the new system, and it would be a shame to lose it.

@eddyb
Copy link
Member Author

eddyb commented Aug 18, 2018

@rpjohnst Note that we can't make that work in Rust 2015, because there is no place (in the "module tree", so to speak) where you can just "access crates by their name".

In Rust 2018 that's the topmost path level, but in Rust 2015 it doesn't exist, and if we allow crate to resolve in the crate root (which is what ::crate means in Rust 2015), then self::crate, super::crate, etc. should resolve too, if the prefix of the path reaches the crate root, to remain consistent (unless we special-case it further).

@rpjohnst
Copy link
Contributor

rpjohnst commented Aug 18, 2018

Yeah, 2015 does make it messier. But perhaps allowing crate to resolve in the crate root makes sense there? That's where all the extern crate declarations go anyway, and crate really only needs to make sense in 2015 insofar as it's part of the transition path to 2018. :P

We could also allow ::crate only in 2018, by allowing it to resolve in the 2018 root, if that feels less messy. Or maybe it makes it worse via interaction with the new prelude-like mechanism, I'm not sure how that works.

Regardless, :: plays a smaller role in 2018, and that role doesn't intersect with crate, so maybe it doesn't matter to the mental model.

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.

8 participants