Enforce the comma after lifetime arguments and before type arguments #24547

bombless · 2015-04-18T01:35:43Z

Closes #20616
It breaks code such as

rust/src/librustc_typeck/check/method/suggest.rs

Line 367 in c64feb6

borrow: cell::Ref<'a Option<AllTraitsVec>>,

, so this is a [breaking-change], you have to add missing comma after the last lifetime arguement now.

rust-highfive · 2015-04-18T01:35:56Z

r? @pnkfelix

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

bombless · 2015-04-18T01:39:14Z

src/libsyntax/parse/parser.rs

-        self.last_token = if self.token.is_ident() || self.token.is_path() {
+        self.last_token = if self.token.is_ident() ||
+                             self.token.is_path() ||
+                             self.token == token::Comma {


I'm not sure about the performance impact of this bufferring.
I see I can modify parse_lifetimes to avoid this, but that will look unnatural.

pnkfelix · 2015-04-18T06:30:44Z

We may eventually need / want a help in the output reminding that 'a T is not a type, and suggesting either 'a, T or &'a T, since we do not generally know which was intended

pnkfelix · 2015-04-18T06:32:33Z

src/test/compile-fail/issue-20616-1.rs

+type Type_1<'a T> = &'a T; //~  error: expected `,` or `>` after lifetime name, found `T`
+
+
+//type Type_2 = Type_1_<'static ()>; // error: expected `,` or `>` after lifetime name, found `(`


Why are these tests commented out?

pnkfelix · 2015-04-18T06:38:06Z

Why are there nine compile-fail tests, with no header comment, that all appear similar (even duplicates?) based on a quick skim? Was that accidental?

pnkfelix · 2015-04-18T06:41:39Z

src/test/run-pass/issue-20616.rs

+// except according to those terms.
+
+type MyType<'a, T> = &'a T;
+


Hmm, shouldn't these tests be checking (perhaps in addition) all the same cases when parsing MyOtherType<'a>, with no type argument?

Sorry that was phrased poorly; what I meant was that cases like Box <MyOtherType <'static>> etc need checking too, with the >> token after a lifetime, right?

Ugh never mind again, you did check that, I just missed it twice

bombless · 2015-04-18T09:16:13Z

Because you cannot resume from parsing error, so I have to test different errors in separate files.

I'll try suggesting &'a T and 'a, T for 'a T here.

bombless · 2015-04-18T09:23:25Z

Oh yes these 9 compile-fail files are similar intentionally, the difference is just that only one parsing error is uncommented at once.

I'll add comments to make this clear.

Manishearth · 2015-04-19T23:02:29Z

technically a breaking change, might want to edit the PR message to reflect that

bombless · 2015-04-20T01:25:00Z

right. fixed.

I'm trying to make a different approach, which will make catching the > more natural, hope it can help to provide a good diagnostics note message.

Currently I'm using Parser::is_path_start but it doesn't seem to work here. I'll see what's missing here.

I think we cannot pretty-print a type this early, so I will just make the codemap feels right, and use the first token to generate a note, e.g. suggest "do you mean &'a Box... or 'a, Box..." for <'a Box<i32>> in code.

Or am I missing something?

Any suggestions, @pnkfelix ?

pnkfelix · 2015-04-20T09:53:13Z

@bombless oh yes, I do not object to being fairly generic in the help message; this would not be the first place that we've inserted GenericTypeName in a help message rather than trying to reconstruct it from the input.

pnkfelix · 2015-04-20T09:54:48Z

@bombless as for catching the leading >, I was musing: Would it be sensible to add a method to Token that tells us the first character in that token? Then you could just call out to that method...

bombless · 2015-04-20T11:01:56Z

Would it be sensible to add a method to Token that tells us the first character in that token?

Okay I'll do that.

bombless · 2015-04-22T03:14:26Z

Nits fixed, @pnkfelix

bors · 2015-04-22T06:43:25Z

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

bombless · 2015-04-22T07:07:40Z

rebased

bombless · 2015-04-22T10:51:24Z

oh yes, I do not object to being fairly generic in the help message; this would not be the first place that we've inserted GenericTypeName in a help message rather than trying to reconstruct it from the input.

Do you mean that there's other places in parser.rs need to print a generic type? I don't really understand this, sorry.

pnkfelix · 2015-04-23T23:03:52Z

@bombless Yeah, sorry, I can see that my use of the phrase "GenericType" can be easily misinterpreted there; it was the name I was referring to as being generic, not the type ...

That is, I was trying to say this:

What you describe sounds roughly fine to me.
After all, only some of our error messages take pains to try to generate an error message that is specifically tailored to match the names used in the original user code (such as when we suggest adding more explicit lifetimes).

We do that sometimes.
But we haven't always done it; sometimes the info is not available, and other times the developer doesn't have the time to try to make such a message construction work out.

And that is okay.
Here is an example of the kind of short-cut I am talking about; up until very recently, we would happily print out "perhaps you meant box() (foo) instead?" for code that did not mention foo at all, and that is okay.
And therefore, I was saying it would be fine with me if your code took a similar short-cut; it would not be the first instance where we inserted foo in a help message rather than trying to reconstruct the actual expression from the input.

The main thing I would suggest is this: If you were literally suggesting that when <'a Box<i32>> is what is in the original code, your help output would print:

do you mean &'a Box... or 'a, Box...?

then I advise that you not use ellipses in that fashion, but instead use a placeholder name for the type that you cannot print. Like so:

did you mean &'a Box<Type> or 'a, Box<Type>?

where Type is being used (in the help output) as a placeholder name for the type that we have not yet parsed.

Update: Then again, maybe it is not feasible to attempt to try to plug in placeholders into the type arguments for Box in this example. In which case I would go even further and suggest that you render the help message as:

did you mean a single argument type &'a Type, or did you mean the comma-separated arguments 'a, Type

Update 2: In some ways the english text prefixing to the two suggestions in this final example output is just as important, or perhaps more important, than the suggestion itself, in that (I hope) it makes it much clearer why there is an ambiguity here as to what the original intent of the user was.

pnkfelix · 2015-04-25T17:55:35Z

@bors r+ 0ad48e4 rollup

bors · 2015-04-25T21:44:51Z

⌛ Testing commit 0ad48e4 with merge 00c48d3...

Closes #20616 It breaks code such as <https://github.com/rust-lang/rust/blob/c64feb63418fd05bd6e5adc6f9ad763aa6a594b1/src/librustc_typeck/check/method/suggest.rs#L367>, so this is a [breaking-change], you have to add missing comma after the last lifetime arguement now.

bors · 2015-04-25T23:40:09Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

Rust now enforces a comma between lifetime and type, see rust-lang/rust#24547 for the change.

pnkfelix · 2015-04-30T21:13:24Z

(potential future cleanup: maybe just get rid of the commented out cases in all the tests...)

pnkfelix · 2015-04-30T21:13:38Z

accepted for beta backport.

brson · 2015-04-30T22:17:28Z

Backported.

rust-highfive assigned pnkfelix Apr 18, 2015

bombless reviewed Apr 18, 2015
View reviewed changes

pnkfelix reviewed Apr 18, 2015
View reviewed changes

bombless force-pushed the comma branch from f8aa3f1 to 04bcd11 Compare April 22, 2015 03:11

bombless force-pushed the comma branch from 04bcd11 to cb79904 Compare April 22, 2015 07:05

Fix rust-lang#20616

0ad48e4

bombless force-pushed the comma branch from cb79904 to 0ad48e4 Compare April 25, 2015 03:29

bors merged commit 0ad48e4 into rust-lang:master Apr 25, 2015

pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 26, 2015

zsiciarz added a commit to zsiciarz/rust-fuse that referenced this pull request Apr 26, 2015

Update for latest nightly

d852707

Rust now enforces a comma between lifetime and type, see rust-lang/rust#24547 for the change.

zsiciarz mentioned this pull request Apr 26, 2015

Update for latest nightly zargony/fuse-rs#48

Merged

pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 30, 2015

alexcrichton mentioned this pull request Apr 30, 2015

Backport of PRs to Beta #25004

Merged

brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 30, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce the comma after lifetime arguments and before type arguments #24547

Enforce the comma after lifetime arguments and before type arguments #24547

bombless commented Apr 18, 2015

rust-highfive commented Apr 18, 2015

bombless Apr 18, 2015

pnkfelix commented Apr 18, 2015

pnkfelix Apr 18, 2015

pnkfelix commented Apr 18, 2015

pnkfelix Apr 18, 2015

pnkfelix Apr 18, 2015

pnkfelix Apr 18, 2015

bombless commented Apr 18, 2015

bombless commented Apr 18, 2015

Manishearth commented Apr 19, 2015

bombless commented Apr 20, 2015

pnkfelix commented Apr 20, 2015

pnkfelix commented Apr 20, 2015

bombless commented Apr 20, 2015

bombless commented Apr 22, 2015

bors commented Apr 22, 2015

bombless commented Apr 22, 2015

bombless commented Apr 22, 2015

pnkfelix commented Apr 23, 2015

pnkfelix commented Apr 25, 2015

bors commented Apr 25, 2015

bors commented Apr 25, 2015

pnkfelix commented Apr 30, 2015

pnkfelix commented Apr 30, 2015

brson commented Apr 30, 2015

		type Type_1<'a T> = &'a T; //~ error: expected `,` or `>` after lifetime name, found `T`


		//type Type_2 = Type_1_<'static ()>; // error: expected `,` or `>` after lifetime name, found `(`

		// except according to those terms.

		type MyType<'a, T> = &'a T;

Enforce the comma after lifetime arguments and before type arguments #24547

Enforce the comma after lifetime arguments and before type arguments #24547

Conversation

bombless commented Apr 18, 2015

rust-highfive commented Apr 18, 2015

bombless Apr 18, 2015

Choose a reason for hiding this comment

pnkfelix commented Apr 18, 2015

pnkfelix Apr 18, 2015

Choose a reason for hiding this comment

pnkfelix commented Apr 18, 2015

pnkfelix Apr 18, 2015

Choose a reason for hiding this comment

pnkfelix Apr 18, 2015

Choose a reason for hiding this comment

pnkfelix Apr 18, 2015

Choose a reason for hiding this comment

bombless commented Apr 18, 2015

bombless commented Apr 18, 2015

Manishearth commented Apr 19, 2015

bombless commented Apr 20, 2015

pnkfelix commented Apr 20, 2015

pnkfelix commented Apr 20, 2015

bombless commented Apr 20, 2015

bombless commented Apr 22, 2015

bors commented Apr 22, 2015

bombless commented Apr 22, 2015

bombless commented Apr 22, 2015

pnkfelix commented Apr 23, 2015

pnkfelix commented Apr 25, 2015

bors commented Apr 25, 2015

bors commented Apr 25, 2015

pnkfelix commented Apr 30, 2015

pnkfelix commented Apr 30, 2015

brson commented Apr 30, 2015