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

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

Merged
merged 1 commit into from
Apr 25, 2015

Conversation

bombless
Copy link
Contributor

Closes #20616
It breaks code such as

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
Copy link
Collaborator

r? @pnkfelix

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

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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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

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 `(`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these tests commented out?

@pnkfelix
Copy link
Member

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?

// except according to those terms.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bombless
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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

@bombless
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Member

@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
Copy link
Contributor Author

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
Copy link
Contributor Author

Nits fixed, @pnkfelix

@bors
Copy link
Contributor

bors commented Apr 22, 2015

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

@bombless
Copy link
Contributor Author

rebased

@bombless
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Member

@bors r+ 0ad48e4 rollup

@bors
Copy link
Contributor

bors commented Apr 25, 2015

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

bors added a commit that referenced this pull request Apr 25, 2015
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 bors merged commit 0ad48e4 into rust-lang:master Apr 25, 2015
@pnkfelix 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
Rust now enforces a comma between lifetime and type, see
rust-lang/rust#24547 for the change.
@pnkfelix
Copy link
Member

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

@pnkfelix
Copy link
Member

accepted for beta backport.

@pnkfelix pnkfelix added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Apr 30, 2015
@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 30, 2015
@brson
Copy link
Contributor

brson commented Apr 30, 2015

Backported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parser accepts bare 'a T in impl header
6 participants