Skip to content

fix: parsing of ? opt-out trait bounds #12444

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
Jun 2, 2022
Merged

Conversation

xffxff
Copy link
Contributor

@xffxff xffxff commented Jun 1, 2022

fixes #12442

} else if p.at(T![for]) {
// test question_for_type_trait_bound
// fn f<T>() where T: ?for<> Sized {}
types::for_type(p, false);
Copy link
Contributor Author

@xffxff xffxff Jun 2, 2022

Choose a reason for hiding this comment

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

Hmm, this also allows fn f<T>() where T: ~const for<> Sized {}.

One question: does our parser need to be 100% equivalent to rustc's? Or do we just need to make sure that those that can be parsed by rustc can be parsed by ra. For those syntaxes that can be parsed by ra but not by rustc, flycheck should be able to help us diagnose

Copy link
Member

Choose a reason for hiding this comment

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

We want to be as independent from flycheck as possible so we should reject this ideally yes, given that ~const is unstable atm I don't think this would be too bad, but I also don't think doing it proper here should be too difficult

@xffxff
Copy link
Contributor Author

xffxff commented Jun 2, 2022

@Veykril Now I think the side effects have been eliminated but some duplicate code was introduced, if there is a better way to do this, please let me know.

Comment on lines 136 to 141
T![?] => {
p.bump_any();
if p.at(T![for]) {
// test question_for_type_trait_bound
// fn f<T>() where T: ?for<> Sized {}
types::for_type(p, false)
} else if paths::is_use_path_start(p) {
types::path_type_(p, false);
} else {
m.abandon(p);
return false;
}
}
Copy link
Member

@Veykril Veykril Jun 2, 2022

Choose a reason for hiding this comment

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

We could do the following here

Suggested change
T![?] => {
p.bump_any();
if p.at(T![for]) {
// test question_for_type_trait_bound
// fn f<T>() where T: ?for<> Sized {}
types::for_type(p, false)
} else if paths::is_use_path_start(p) {
types::path_type_(p, false);
} else {
m.abandon(p);
return false;
}
}
// test question_for_type_trait_bound
// fn f<T>() where T: ?for<> Sized {}
T![?] if p.nth_at(1, T![for]) => {
p.bump_any();
types::for_type(p, false)
}

and keep the T![?] match arm on line 138 as is I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better, thanks!

@Veykril
Copy link
Member

Veykril commented Jun 2, 2022

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2022

📌 Commit df67bbd has been approved by Veykril

@bors
Copy link
Contributor

bors commented Jun 2, 2022

⌛ Testing commit df67bbd with merge 88024c7...

@bors
Copy link
Contributor

bors commented Jun 2, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 88024c7 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsing of ? opt-out trait bounds doesn't match rustc
3 participants