syntax: Relax path grammar #43540

petrochenkov · 2017-07-29T12:12:26Z

TLDR: Accept the disambiguator :: in "type" paths (Type::<Args>), accept the disambiguator :: before parenthesized generic arguments (Fn::(Args)).

The "turbofish" disambiguator ::<> in expression paths is a necessary evil required for path parsing to be both simple and to give reasonable results.
Since paths in expressions usually refer to values (but not necessarily, e.g. Struct::<u8> { field: 0 } is disambiguated, but refers to a type), people often consider ::<> to be inherent to values, and not expressions and want to write disambiguated paths for values even in contexts where disambiguation is not strictly necessary, for example when a path is passed to a macro m!(Vec::<i32>::new).
The problem is that currently, if the disambiguator is not required, then it's prohibited. This results in confusion - see #41740, https://internals.rust-lang.org/t/macro-path-uses-novel-syntax/5561.

This PR makes the disambiguator optional instead of prohibited in contexts where it's not strictly required, so people can pass paths to macros in whatever form they consider natural (e.g. disambiguated form for value paths).
This PR also accepts the disambiguator in paths with parenthesized arguments (Fn::(Args)) for consistency and to simplify testing of stuff like #41856 (comment).

Closes #41740

cc @rust-lang/lang
r? @nikomatsakis

nikomatsakis · 2017-07-29T16:33:33Z

This seems OK -- I can certainly see the advantage of having a single path grammar that works in any context.

joshtriplett · 2017-07-29T16:42:29Z

My only concern here would be whether it's possible to use the "optional" turbofish with a type and have that type ignored. Will the optional turbofish's specified type always get used? Because if it's ever possible to pass Vec::<u32> or Vec::<u32>::new to something and have it treated as though the user had just passed Vec or Vec::new, and then get used for a non-Vec<u32> type, that kind of "optional" seems bad.

Is that ever possible here?

eddyb · 2017-07-29T17:26:47Z

@joshtriplett No, the two syntax forms result in the same AST/HIR, it's only a disambiguation.

joshtriplett · 2017-07-29T17:35:42Z

@eddyb Oh, I see; this isn't making the whole ::<T> optional, this is making the :: optional in contexts where <T> is already allowed? That makes much more sense, thank you for the clarification.

(Now if only we could do the reverse, and allow <T> in contexts that currently require the turbofish.)

nikomatsakis · 2017-07-31T18:54:53Z

@rfcbot fcp merge

I propose that we accept this change. To summarize: the idea is to accept Foo::<T>in all contexts, but in type contexts, we also accept Foo<T>. This makes it easier to have a generic path syntax that will work everything (e.g., for use in macros). I think a motivating example is wanting to have the path to a struct constructor that can double as the type (where the path may include type parameters). I'm also nominating just to bring this to people's attention (and/or discuss at lang meeting).

I confess I have some reservations: this doesn't seem actively harmful, but it introduces "more than one way to do it", and I'm not sure how strong the motivation is. But I'm nonetheless proposing to merge in order to spur conversation forward.

joshtriplett · 2017-07-31T19:18:19Z

This is probably a FAQ somewhere, but do we have documentation for why there are contexts where we only accept the turbofish (Vec::<T>) and can't accept the non-turbofish syntax (Vec<T>)? What grammar problem would it create if we allowed omitting the :: everywhere we currently allow the turbofish?

rfcbot · 2017-07-31T19:29:49Z

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

No concerns currently listed.

Once these reviewers reach consensus, 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.

withoutboats · 2017-07-31T19:35:28Z

This makes it easier to have a generic path syntax that will work everything (e.g., for use in macros).

Is there an example of a use case for this? Trying to understand #41740, it seems like the macro path matcher could (should) be changed, but that doesn't imply changing the grammar entirely.

So far the comments haven't really made this change feel well-motivated. The grammar being simpler doesn't necessarily make the language easier to use. Is anyone actually going to be writing let x: Vec::<i32>; or would that be considered unidiomatic? (I would think the latter).

petrochenkov · 2017-08-01T21:59:57Z

@withoutboats

it seems like the macro path matcher could (should) be changed, but that doesn't imply changing the grammar entirely.

Yes, we could add one more path flavor with optional disambiguator and use it only in macros, but I'd prefer to reduce the number of flavors rather than to increase it.
Simplification is a good enough motivation for me.

Is anyone actually going to be writing let x: Vec::<i32>;

I don't think so, maybe copypaste can produce it.

or would that be considered unidiomatic? (I would think the latter).

I guess yes, why writing more symbol noise if you can write less symbol noise. This is true for other optional stuff like trailing separators or in in pub(in super).

petrochenkov · 2017-08-01T22:12:22Z

@joshtriplett

This is probably a FAQ somewhere, but do we have documentation for why there are contexts where we only accept the turbofish (Vec::) and can't accept the non-turbofish syntax (Vec)? What grammar problem would it create if we allowed omitting the :: everywhere we currently allow the turbofish?

Arbitrarily large lookahead is required to discern between generic arguments and comparisons in expressions in general case.
So far Rust managed to parse everything with very local rules, maybe with a couple of tokens of lookahead.
And even if everything is disambiguated succesfully during parsing, there are still semantic ambiguities like f(a < b, c > - d) that would need to be resolved as well.
I wasn't here when the ::< syntax was introduced, but I suspect the logic was that disambiguation efforts don't worth it, let's require users to disambiguate instead, it shouldn't be very common because expressions have type inference and generic arguments are more often deduced than written explicitly.

joshtriplett · 2017-08-03T19:22:08Z

Quick note: could we detect use of this outside a macro, and warn that you should drop the ::? We don't want new users thinking this is the preferred syntax.

aturon · 2017-08-03T19:30:06Z

(Checking off @pnkfelix's box while he's on PTO).

withoutboats · 2017-08-03T19:32:07Z

I'd like to have an error-by-default lint against this which is turned off in macros, to discourage it ever appearing in surface code. @petrochenkov how burdensome would that be to implement?

petrochenkov · 2017-08-03T21:06:29Z

@withoutboats
Not burdensome.
Do you necessarily want it in rustc, or clippy will work too?
(Deny-by-default seems dire, it's on the same territory as unused parens or unnecessary type annotations, but I don't care much.)

withoutboats · 2017-08-03T21:54:30Z

@petrochenkov I'd prefer in rustc, and I'm fine with warn by default.

nikomatsakis · 2017-08-04T06:51:21Z

@withoutboats I would expect to accept both silently, but normalize in rustfmt. I am skeptical of the very concept of an error-by-default lint, and certainly skeptical of using it for style issues.

nikomatsakis · 2017-08-04T06:52:12Z

@petrochenkov

I wasn't here when the ::< syntax was introduced, but I suspect the logic was that disambiguation efforts don't worth it, let's require users to disambiguate instead, it shouldn't be very common because expressions have type inference and generic arguments are more often deduced than written explicitly.

It also predates me, but yes -- the constraint against infinite lookahead was (and still is, so far) taken as a hard constraint, and the assumption (which is true) was that ::< would be rarely needed.

I suspect that without :: there are actually true ambiguous cases too - i.e., things that could parse either way. I feel like we've come up with examples in the past, though I can't think of any right now (particularly since < and > do not combine without parentheses...).

nikomatsakis · 2017-08-04T06:53:58Z

@withoutboats I agree the motivation is not spelled out very strongly. I think the use case would be: "a macro that accepts a path that will be used for both a type and a path to the constructor". This may in general require type parameters. Put another way, without this, macros really need "type path" and "item path" as two distinct things. They may need that anyway, mind you.

aturon · 2017-08-09T02:46:49Z

Ping @withoutboats, are you ready to tick your box?

withoutboats · 2017-08-09T04:22:54Z

I'll tick my box. Really want to see a lint against this though!

rfcbot · 2017-08-09T04:46:20Z

🔔 This is now entering its final comment period, as per the review above. 🔔

petrochenkov · 2017-08-10T23:52:04Z

@withoutboats
I've added a hard-coded warning to avoid creating a new stable lint name.
When adoption of rustfmt increases it could be freely removed.

nikomatsakis · 2017-08-16T20:17:07Z

Hmm, a hard-coded warning seems .. unfortunate, no? i.e., the whole point of allowing it is that we expect it to sometimes be used, e.g. in a macro expansion? (But I guess I have to look more closely at when the warning fires...)

petrochenkov · 2017-08-16T23:16:35Z

@nikomatsakis

we expect it to sometimes be used, e.g. in a macro expansion

The warning is not reported when a path is passed to a macro, only when we are certainly parsing a type path.
(Personally, I'd rather follow #43540 (comment) too, remove the warning and leave this to rustfmt.)

nikomatsakis · 2017-08-17T19:06:12Z

@petrochenkov but would the warning fire after macro expansion, presuming that a Foo::<> path winds up in type position?

petrochenkov · 2017-08-17T20:48:21Z

@nikomatsakis
No, the warning is reported during parsing. During expansion we have only complete ast::Path and AST doesn't keep information about presence of ::, so we don't even know how this ast::Path was written in the source code.

nikomatsakis · 2017-08-17T21:27:10Z

@petrochenkov I see. OK. I'm not a big fan of unconditional warnings, and I tend to agree with you that developers will not intentionally write extra :: (though I guess new users might), so I'm of mixed minds about the warning.

rfcbot · 2017-08-19T05:09:13Z

The final comment period is now complete.

nikomatsakis

So the code looks fine.

nikomatsakis · 2017-08-21T22:56:27Z

@bors r+

bors · 2017-08-21T22:56:28Z

📌 Commit 804459b has been approved by nikomatsakis

bors · 2017-08-21T23:04:03Z

⌛ Testing commit 804459b with merge 8df670b...

@nikomatsakis

syntax: Relax path grammar TLDR: Accept the disambiguator `::` in "type" paths (`Type::<Args>`), accept the disambiguator `::` before parenthesized generic arguments (`Fn::(Args)`). The "turbofish" disambiguator `::<>` in expression paths is a necessary evil required for path parsing to be both simple and to give reasonable results. Since paths in expressions usually refer to values (but not necessarily, e.g. `Struct::<u8> { field: 0 }` is disambiguated, but refers to a type), people often consider `::<>` to be inherent to *values*, and not *expressions* and want to write disambiguated paths for values even in contexts where disambiguation is not strictly necessary, for example when a path is passed to a macro `m!(Vec::<i32>::new)`. The problem is that currently, if the disambiguator is not *required*, then it's *prohibited*. This results in confusion - see #41740, https://internals.rust-lang.org/t/macro-path-uses-novel-syntax/5561. This PR makes the disambiguator *optional* instead of prohibited in contexts where it's not strictly required, so people can pass paths to macros in whatever form they consider natural (e.g. disambiguated form for value paths). This PR also accepts the disambiguator in paths with parenthesized arguments (`Fn::(Args)`) for consistency and to simplify testing of stuff like #41856 (comment). Closes #41740 cc @rust-lang/lang r? @nikomatsakis

bors · 2017-08-22T01:34:48Z

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

Changelog: Version 1.21.0 (2017-10-12) ========================== Language -------- - [You can now use static references for literals.][43838] Example: ```rust fn main() { let x: &'static u32 = &0; } ``` - [Relaxed path syntax. Optional `::` before `<` is now allowed in all contexts.][43540] Example: ```rust my_macro!(Vec<i32>::new); // Always worked my_macro!(Vec::<i32>::new); // Now works ``` Compiler -------- - [Upgraded jemalloc to 4.5.0][43911] - [Enabled unwinding panics on Redox][43917] - [Now runs LLVM in parallel during translation phase.][43506] This should reduce peak memory usage. Libraries --------- - [Generate builtin impls for `Clone` for all arrays and tuples that are `T: Clone`][43690] - [`Stdin`, `Stdout`, and `Stderr` now implement `AsRawFd`.][43459] - [`Rc` and `Arc` now implement `From<&[T]> where T: Clone`, `From<str>`, `From<String>`, `From<Box<T>> where T: ?Sized`, and `From<Vec<T>>`.][42565] Stabilized APIs --------------- [`std::mem::discriminant`] Cargo ----- - [You can now call `cargo install` with multiple package names][cargo/4216] - [Cargo commands inside a virtual workspace will now implicitly pass `--all`][cargo/4335] - [Added a `[patch]` section to `Cargo.toml` to handle prepublication dependencies][cargo/4123] [RFC 1969] - [`include` & `exclude` fields in `Cargo.toml` now accept gitignore like patterns][cargo/4270] - [Added the `--all-targets` option][cargo/4400] - [Using required dependencies as a feature is now deprecated and emits a warning][cargo/4364] Misc ---- - [Cargo docs are moving][43916] to [doc.rust-lang.org/cargo](https://doc.rust-lang.org/cargo) - [The rustdoc book is now available][43863] at [doc.rust-lang.org/rustdoc](https://doc.rust-lang.org/rustdoc) - [Added a preview of RLS has been made available through rustup][44204] Install with `rustup component add rls-preview` - [`std::os` documentation for Unix, Linux, and Windows now appears on doc.rust-lang.org][43348] Previously only showed `std::os::unix`. Compatibility Notes ------------------- - [Changes in method matching against higher-ranked types][43880] This may cause breakage in subtyping corner cases. [A more in-depth explanation is available.][info/43880] - [rustc's JSON error output's byte position start at top of file.][42973] Was previously relative to the rustc's internal `CodeMap` struct which required the unstable library `libsyntax` to correctly use. - [`unused_results` lint no longer ignores booleans][43728] [42565]: rust-lang/rust#42565 [42973]: rust-lang/rust#42973 [43348]: rust-lang/rust#43348 [43459]: rust-lang/rust#43459 [43506]: rust-lang/rust#43506 [43540]: rust-lang/rust#43540 [43690]: rust-lang/rust#43690 [43728]: rust-lang/rust#43728 [43838]: rust-lang/rust#43838 [43863]: rust-lang/rust#43863 [43880]: rust-lang/rust#43880 [43911]: rust-lang/rust#43911 [43916]: rust-lang/rust#43916 [43917]: rust-lang/rust#43917 [44204]: rust-lang/rust#44204 [cargo/4123]: rust-lang/cargo#4123 [cargo/4216]: rust-lang/cargo#4216 [cargo/4270]: rust-lang/cargo#4270 [cargo/4335]: rust-lang/cargo#4335 [cargo/4364]: rust-lang/cargo#4364 [cargo/4400]: rust-lang/cargo#4400 [RFC 1969]: rust-lang/rfcs#1969 [info/43880]: rust-lang/rust#44224 (comment) [`std::mem::discriminant`]: https://doc.rust-lang.org/std/mem/fn.discriminant.html

@nikomatsakis

syntax: Remove warning for unnecessary path disambiguators `rustfmt` is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in rust-lang#43540 (where it was introduced). One hardcoded warning less. Closes rust-lang#58055 r? @nikomatsakis

@nikomatsakis

syntax: Remove warning for unnecessary path disambiguators `rustfmt` is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in rust-lang#43540 (where it was introduced). One hardcoded warning less. Closes rust-lang#58055 r? @nikomatsakis

@nikomatsakis

syntax: Remove warning for unnecessary path disambiguators `rustfmt` is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in rust-lang#43540 (where it was introduced). One hardcoded warning less. Closes rust-lang#58055 r? @nikomatsakis

@nikomatsakis

syntax: Remove warning for unnecessary path disambiguators `rustfmt` is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in rust-lang#43540 (where it was introduced). One hardcoded warning less. Closes rust-lang#58055 r? @nikomatsakis

4667: GRAM: fix type path parsing r=vlad20012 a=Undin After rust-lang/rust#43540 disambiguator `::` is allowed in "type" paths. But our grammar forbade it previously because it had been written before the corresponding changes in rustc. These changes fix the grammar. Also, they highlight redundant `::` in type paths and provide fix to remove it ![2019-11-21 11 25 22](https://user-images.githubusercontent.com/2539310/69320139-a5193b80-0c51-11ea-85ba-2ddfe0a41fcb.gif) Fixes #4430 Co-authored-by: Arseniy Pendryak <a.pendryak@yandex.ru>

rust-highfive assigned nikomatsakis Jul 29, 2017

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 31, 2017

nikomatsakis added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jul 31, 2017

rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 31, 2017

withoutboats removed the I-nominated label Aug 4, 2017

rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Aug 9, 2017

syntax: Relax path grammar

7d21f21

petrochenkov force-pushed the pathrelax branch from 23322cc to 804459b Compare August 10, 2017 23:50

carols10cents 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 14, 2017

nikomatsakis approved these changes Aug 21, 2017

View reviewed changes

bors merged commit 804459b into rust-lang:master Aug 22, 2017

bors mentioned this pull request Aug 22, 2017

Ensure that generic arguments don't end up in attribute paths. #43948

Merged

petrochenkov deleted the pathrelax branch August 26, 2017 00:19

petrochenkov mentioned this pull request Sep 10, 2017

Updated RELEASES.md for 1.21.0 #44481

Merged

petrochenkov mentioned this pull request Jan 13, 2019

syntax: Remove warning for unnecessary path disambiguators #57565

Merged

zackmdavis mentioned this pull request Feb 1, 2019

unnecessary path disambiguator slips past deny(warnings) #58055

Closed

Undin mentioned this pull request Nov 21, 2019

GRAM: fix type path parsing intellij-rust/intellij-rust#4667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

syntax: Relax path grammar #43540

syntax: Relax path grammar #43540

petrochenkov commented Jul 29, 2017

nikomatsakis commented Jul 29, 2017

joshtriplett commented Jul 29, 2017

eddyb commented Jul 29, 2017

joshtriplett commented Jul 29, 2017

nikomatsakis commented Jul 31, 2017

joshtriplett commented Jul 31, 2017

rfcbot commented Jul 31, 2017 •

edited by withoutboats

withoutboats commented Jul 31, 2017 •

edited

petrochenkov commented Aug 1, 2017

petrochenkov commented Aug 1, 2017

joshtriplett commented Aug 3, 2017 •

edited

aturon commented Aug 3, 2017

withoutboats commented Aug 3, 2017

petrochenkov commented Aug 3, 2017 •

edited

withoutboats commented Aug 3, 2017

nikomatsakis commented Aug 4, 2017

nikomatsakis commented Aug 4, 2017 •

edited

nikomatsakis commented Aug 4, 2017

aturon commented Aug 9, 2017

withoutboats commented Aug 9, 2017

rfcbot commented Aug 9, 2017

petrochenkov commented Aug 10, 2017

nikomatsakis commented Aug 16, 2017 •

edited

petrochenkov commented Aug 16, 2017

nikomatsakis commented Aug 17, 2017

petrochenkov commented Aug 17, 2017

nikomatsakis commented Aug 17, 2017

rfcbot commented Aug 19, 2017

nikomatsakis left a comment

nikomatsakis commented Aug 21, 2017

bors commented Aug 21, 2017

bors commented Aug 21, 2017

bors commented Aug 22, 2017

syntax: Relax path grammar #43540

syntax: Relax path grammar #43540

Conversation

petrochenkov commented Jul 29, 2017

nikomatsakis commented Jul 29, 2017

joshtriplett commented Jul 29, 2017

eddyb commented Jul 29, 2017

joshtriplett commented Jul 29, 2017

nikomatsakis commented Jul 31, 2017

joshtriplett commented Jul 31, 2017

rfcbot commented Jul 31, 2017 • edited by withoutboats

withoutboats commented Jul 31, 2017 • edited

petrochenkov commented Aug 1, 2017

petrochenkov commented Aug 1, 2017

joshtriplett commented Aug 3, 2017 • edited

aturon commented Aug 3, 2017

withoutboats commented Aug 3, 2017

petrochenkov commented Aug 3, 2017 • edited

withoutboats commented Aug 3, 2017

nikomatsakis commented Aug 4, 2017

nikomatsakis commented Aug 4, 2017 • edited

nikomatsakis commented Aug 4, 2017

aturon commented Aug 9, 2017

withoutboats commented Aug 9, 2017

rfcbot commented Aug 9, 2017

petrochenkov commented Aug 10, 2017

nikomatsakis commented Aug 16, 2017 • edited

petrochenkov commented Aug 16, 2017

nikomatsakis commented Aug 17, 2017

petrochenkov commented Aug 17, 2017

nikomatsakis commented Aug 17, 2017

rfcbot commented Aug 19, 2017

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis commented Aug 21, 2017

bors commented Aug 21, 2017

bors commented Aug 21, 2017

bors commented Aug 22, 2017

rfcbot commented Jul 31, 2017 •

edited by withoutboats

withoutboats commented Jul 31, 2017 •

edited

joshtriplett commented Aug 3, 2017 •

edited

petrochenkov commented Aug 3, 2017 •

edited

nikomatsakis commented Aug 4, 2017 •

edited

nikomatsakis commented Aug 16, 2017 •

edited