syntax: Relax path grammar

[petrochenkov]

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]

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

[joshtriplett]

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]

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

[joshtriplett]

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

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

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]

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

• @aturon
• @eddyb
• @nikomatsakis
• @nrc
• @pnkfelix
• @withoutboats

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]

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]

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

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

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]

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

[withoutboats]

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]

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

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

[nikomatsakis]

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

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

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

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

[withoutboats]

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

[rfcbot]

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

[petrochenkov]

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

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]

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

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

[petrochenkov]

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

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

The final comment period is now complete.

[nikomatsakis]

So the code looks fine.

[nikomatsakis]

@bors r+

[bors]

📌 Commit 804459b has been approved by nikomatsakis

[bors]

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

[bors]

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