Skip to content

Fix syntax error in the compiler #37278

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

Merged
merged 1 commit into from
Nov 14, 2016
Merged

Conversation

matklad
Copy link
Member

@matklad matklad commented Oct 19, 2016

Currently rustc accepts the following code: fn f<'a>() where 'a {}. This should be a syntax error, shouldn't it?

Not sure if my changes actually compile, waiting for the LLVM to build.

@rust-highfive
Copy link
Contributor

r? @pnkfelix

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

@matklad matklad force-pushed the lone-lifetime branch 2 times, most recently from 0e93af8 to 9d179e6 Compare October 19, 2016 15:58
@matklad
Copy link
Member Author

matklad commented Oct 20, 2016

It would be a good idea to audit all call sites of parse_ty_param_bounds and make sure (refactor) that empty bounds are not allowed. There is at least one more place where the check is missing:

trait T {}

type S = for<'a> T+; // <- plus what? 

fn main() {}

This will be a subject for another PR.

@pnkfelix
Copy link
Member

This change looks fine to me, but we should probably pass it through crater before we land it to just make sure that no one is accidentally relying on this being input without error.

@pnkfelix pnkfelix added S-waiting-on-crater Status: Waiting on a crater run to be completed. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated labels Oct 20, 2016
@matklad
Copy link
Member Author

matklad commented Oct 31, 2016

One more fun example: fn foo<T:>(x: T) -> T { x }

@matklad
Copy link
Member Author

matklad commented Nov 1, 2016

@nikomatsakis can we schedule a crater run here? Or should we try to fix all similar issues at once?

@matklad
Copy link
Member Author

matklad commented Nov 1, 2016

Ok, let's just do everything in #37511 ?

@matklad matklad closed this Nov 1, 2016
@matklad matklad reopened this Nov 12, 2016
@matklad
Copy link
Member Author

matklad commented Nov 12, 2016

Ok, so #37511 turned out to be more complex that it seemed at first: it's not obvious that we want to forbid empty bounds everywhere.

So let's just stick with fixing lifetimes in where clauses: it's already an error not to supply bounds for the type parameters, and it should be an error for lifetimes as well. The crater run in #37511 did not show any regression because of the lifetimes, so I think we can get away without a warning here.

@eddyb eddyb removed the S-waiting-on-crater Status: Waiting on a crater run to be completed. label Nov 13, 2016
self.span_err(span,
"each predicate in a `where` clause must have \
at least one bound in it");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't make this an error until we reach a decision empty bounds lists in #37511.

@jseyfried
Copy link
Contributor

@matklad could we just land the first two bullet points from #37511 (comment) in this PR?
That might just be the eat -> expect change along with the fallout and test cases.

Don't allow lifetimes without any bounds at all
@matklad
Copy link
Member Author

matklad commented Nov 14, 2016

Yep, removing check for bound.is_empty() for now seems prudent!

However, I can't add both where 'a and where 'a 'b tests to a single file even though there is -continue-parse-after-error. Looks like the parser can't recover in this case and skip tokens until the start of the next item.

I think it's ok to have a single test here, however I am very interested in parser recovery. Could you take a look at this question?

@jseyfried
Copy link
Contributor

jseyfried commented Nov 14, 2016

Thanks! I agree that a single test is fine -- @bors r+

Regarding the question, rustc is capable of analyzing incomplete code, but the parser often isn't good enough at error recovery to get to analysis, even with continue-parse-after-error. I'm not sure exactly why we don't recover in this case, but I could help look into it if you'd like to work on improving error recovery (feel free to ping me on IRC).

@bors
Copy link
Collaborator

bors commented Nov 14, 2016

📌 Commit cf9ff2b has been approved by jseyfried

@matklad
Copy link
Member Author

matklad commented Nov 14, 2016

Thanks a lot for the review, @jseyfried !

@bors
Copy link
Collaborator

bors commented Nov 14, 2016

⌛ Testing commit cf9ff2b with merge 8289a89...

bors added a commit that referenced this pull request Nov 14, 2016
Fix syntax error in the compiler

Currently `rustc` accepts the following code: `fn f<'a>() where 'a {}`. This should be a syntax error, shouldn't it?

Not sure if my changes actually compile, waiting for the LLVM to build.
@bors bors merged commit cf9ff2b into rust-lang:master Nov 14, 2016
@matklad matklad deleted the lone-lifetime branch July 9, 2019 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants