Support `::crate` in paths #45771

petrochenkov · 2017-11-04T22:58:45Z

petrochenkov · 2017-11-04T22:58:54Z

rust-highfive · 2017-11-04T22:58:56Z

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

nikomatsakis

I left various requests around the tests, but my biggest concern is that crate::foo::bar seems to parse in contexts where I would not expect it to.

nikomatsakis · 2017-11-06T18:40:21Z

src/libsyntax/ast.rs

@@ -97,9 +97,12 @@ impl Path {
    }

    pub fn default_to_global(mut self) -> Path {


Pre-existing, but I think a comment on this function would be great, just showing the transformations it does (and maybe explaining the reason for its existence).

nikomatsakis · 2017-11-06T18:44:28Z

src/libsyntax/parse/parser.rs

-                  !self.is_union_item() {
+                  !self.is_union_item() &&
+                  !(self.token.is_keyword(keywords::Crate) &&
+                    self.look_ahead(1, |t| t != &token::ModSep)) {


Nit: I feel like some comments would be nice here too. In fact, I personally am a bit confused about what's going on here. =) Lots of nested nots!

// Parse any path `foo::bar`. But be careful of various corner cases involving contextual keywords: // // - `union Foo { .. }`, a union item. This gets parsed below. Note that `union::foo` is a path. // - Note that `crate::foo` *is* a path, but `crate` followed by any other token is not. (??)

I'm not entirely clear I guess on what is happening. =)

nikomatsakis · 2017-11-06T18:47:06Z

src/test/compile-fail/crate-path-non-absolute.rs

+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license


Can we please collect the tests for this feature in some kind of directory named after the rfc? I would suggest {compile-fail,run-pass,ui}/rfc2126-crate-paths.

nikomatsakis · 2017-11-06T18:48:59Z

src/test/compile-fail/crate-visibility-ambiguity.rs

+    pub struct Z;
+    pub struct S1(crate (::m::Z)); // OK
+    pub struct S2(::crate ::m::Z); // OK
+    pub struct S3(crate ::m::Z); //~ ERROR undeclared type or module `crate`


Huh, this error seems to indicate that it is parsing a path crate::m::Z which then fails to resolve... is that correct? If so, that seems sort of wrong, right? Today at least you get an error, and if we were going to favor any interpretation here, I would expect us to treat crate as a visibility modifier (wasn't that the whole point of accepting ::crate only?).

Yes, crate ::m::Z is parsed as a single path, I explained this choice in #45477 (comment).
FWIW, it's also the most conservative solution (since crate::anything::else is an error).

wasn't that the whole point of accepting ::crate only?

I thought it was for teachability / symmetry with ::other_crate::x::y (#44660 (comment)).

nikomatsakis · 2017-11-06T18:49:35Z

src/test/compile-fail/use-super-global-path.rs


    pub fn g() {
-        use ::super::main; //~ ERROR global paths cannot start with `super`
+        use ::super::main; //~ ERROR unresolved import `super`


Why the change to the error message here? Seems less good now.

nikomatsakis · 2017-11-06T18:50:12Z

src/test/run-pass/crate-path-absolute.rs

+
+#![feature(crate_in_paths)]
+
+use crate::m::f;


Can we also test this from another module besides the root module?

nikomatsakis · 2017-11-06T18:50:47Z

src/test/run-pass/crate-path-absolute.rs

+}
+
+fn main() {
+    f();


Not that I think it's really needed, bu this test doesn't demonstrate that things resolved correctly in any way. Perhaps have f and g return values, and we can assert that you get the right thing back?

shepmaster · 2017-11-10T15:16:02Z

Hi from triage @petrochenkov — looks like you've got some grade-A feedback; will you be able to act on it soon?

petrochenkov · 2017-11-10T23:15:30Z

will you be able to act on it soon?

I'll address the comments tomorrow.

petrochenkov · 2017-11-11T22:53:24Z

Updated.

bors · 2017-11-12T09:47:55Z

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

nikomatsakis

Looks nice. Two more questions. =)

nikomatsakis · 2017-11-13T17:13:34Z

src/test/compile-fail/rfc-2126-crate-paths/crate-visibility-ambiguity.rs

+    pub struct Z;
+    pub struct S1(crate (::m::Z)); // OK
+    pub struct S2(::crate ::m::Z); // OK
+    pub struct S3(crate ::m::Z); //~ ERROR `crate` can only be used in absolute paths


I'm still a bit surprised that this doesn't work out with crate being a visibility modifier. What is the reason for it not to parse that way?

I answered this earlier (#45771 (comment)), but GitHub has hidden the message.

nikomatsakis · 2017-11-13T17:14:20Z

src/test/compile-fail/use-super-global-path.rs


    pub fn g() {
-        use ::super::main; //~ ERROR global paths cannot start with `super`
+        use ::super::main; //~ ERROR unresolved import `super`
+                           //~^ NOTE global paths cannot start with `super`


The notes are definitely an improvement -- but "unresolved import super" still feels misleading to me. Among other things, super is a keyword and hence could not possibly be an import, right? Can we readily change from adding a note to altering the main message?

bors · 2017-11-20T11:02:09Z

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

nikomatsakis · 2017-11-20T20:14:08Z

@petrochenkov

Regarding the precedence of crate, I don't agree with your conclusion at the moment, but I'd rather not get stuck on it. Let's land the PR as is. I expect us to be revisiting various aspects of the syntax here in any case.

nikomatsakis · 2017-11-20T20:17:24Z

r=me post rebase

petrochenkov · 2017-11-20T21:40:00Z

@bors r=nikomatsakis

bors · 2017-11-20T21:40:01Z

📌 Commit 90f5cfd has been approved by nikomatsakis

petrochenkov · 2017-11-20T21:41:07Z

@nikomatsakis

I expect us to be revisiting various aspects of the syntax here in any case.

FWIW, I still hope this never ends up on stable.

bors · 2017-11-21T12:33:06Z

⌛ Testing commit 90f5cfd with merge f28df20...

Support `::crate` in paths cc #45477 Fixes #45229

bors · 2017-11-21T15:11:24Z

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

@nikomatsakis

Support `extern` in paths Implement the primary alternative to #46613 + #45771, achieving the same effect without requiring changes to other imports. Both need to be experimentally evaluated before making further progress. The PR also adds docs for all these related features into the unstable book. cc #44660 r? @nikomatsakis

rust-highfive assigned nikomatsakis Nov 4, 2017

petrochenkov mentioned this pull request Nov 4, 2017

permit crate in absolute paths #45477

Closed

petrochenkov force-pushed the crate branch from 0b43f6e to bc98326 Compare November 4, 2017 23:19

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2017

nikomatsakis requested changes Nov 6, 2017

View reviewed changes

kennytm 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2017

petrochenkov force-pushed the crate branch from bc98326 to b42bd2b Compare November 11, 2017 22:52

nikomatsakis requested changes Nov 13, 2017

View reviewed changes

petrochenkov force-pushed the crate branch from b42bd2b to 84f2917 Compare November 19, 2017 14:07

petrochenkov added 2 commits November 21, 2017 00:21

Support ::crate in paths

2e9b89d

Report special messages for path segment keywords in wrong positions

90f5cfd

petrochenkov force-pushed the crate branch from 84f2917 to 90f5cfd Compare November 20, 2017 21:34

petrochenkov 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 Nov 20, 2017

bors added a commit that referenced this pull request Nov 21, 2017

Auto merge of #45771 - petrochenkov:crate, r=nikomatsakis

f28df20

Support `::crate` in paths cc #45477 Fixes #45229

bors merged commit 90f5cfd into rust-lang:master Nov 21, 2017

This was referenced Nov 21, 2017

[WIP] Generic Associated Types Name Resolution #46146

Closed

Add nested groups in imports #45846

Merged

Generic Associated Types Parsing & Name Resolution #45904

Merged

petrochenkov mentioned this pull request Jan 3, 2018

Support extern in paths #47156

Merged

petrochenkov mentioned this pull request May 4, 2018

permit crate as a shorthand visibility identifier #45388

Closed

eira-fransham mentioned this pull request Dec 12, 2018

Check type when resuming function wasmi-labs/wasmi#152

Merged

petrochenkov deleted the crate branch June 5, 2019 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `::crate` in paths #45771

Support `::crate` in paths #45771

petrochenkov commented Nov 4, 2017 •

edited

Loading

petrochenkov commented Nov 4, 2017

rust-highfive commented Nov 4, 2017

nikomatsakis left a comment

nikomatsakis Nov 6, 2017

nikomatsakis Nov 6, 2017

nikomatsakis Nov 6, 2017

nikomatsakis Nov 6, 2017

petrochenkov Nov 10, 2017

nikomatsakis Nov 6, 2017

nikomatsakis Nov 6, 2017

nikomatsakis Nov 6, 2017

shepmaster commented Nov 10, 2017

petrochenkov commented Nov 10, 2017

petrochenkov commented Nov 11, 2017

bors commented Nov 12, 2017

nikomatsakis left a comment

nikomatsakis Nov 13, 2017

petrochenkov Nov 19, 2017

nikomatsakis Nov 13, 2017

petrochenkov Nov 19, 2017

bors commented Nov 20, 2017

nikomatsakis commented Nov 20, 2017 •

edited

Loading

nikomatsakis commented Nov 20, 2017

petrochenkov commented Nov 20, 2017

bors commented Nov 20, 2017

petrochenkov commented Nov 20, 2017

bors commented Nov 21, 2017

bors commented Nov 21, 2017

		@@ -97,9 +97,12 @@ impl Path {
		}

		pub fn default_to_global(mut self) -> Path {

Support ::crate in paths #45771

Support ::crate in paths #45771

Conversation

petrochenkov commented Nov 4, 2017 • edited Loading

petrochenkov commented Nov 4, 2017

rust-highfive commented Nov 4, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepmaster commented Nov 10, 2017

petrochenkov commented Nov 10, 2017

petrochenkov commented Nov 11, 2017

bors commented Nov 12, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Nov 20, 2017

nikomatsakis commented Nov 20, 2017 • edited Loading

nikomatsakis commented Nov 20, 2017

petrochenkov commented Nov 20, 2017

bors commented Nov 20, 2017

petrochenkov commented Nov 20, 2017

bors commented Nov 21, 2017

bors commented Nov 21, 2017

Support `::crate` in paths #45771

Support `::crate` in paths #45771

petrochenkov commented Nov 4, 2017 •

edited

Loading

nikomatsakis commented Nov 20, 2017 •

edited

Loading