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.

Sign up for GitHub

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

Jump to bottom

Provide a better error and a suggestion for Fn traits with lifetime params #104531

Merged
merged 1 commit into from
Dec 29, 2022
Merged

Provide a better error and a suggestion for Fn traits with lifetime params #104531

merged 1 commit into from
Dec 29, 2022
+176 −3

Conversation

ohno418
Copy link
Contributor

@ohno418 ohno418 commented Nov 17, 2022 •
edited
Loading

Given Fn-family traits with lifetime params in trait bounds like fn f(_: impl Fn<'a>(&'a str) -> bool), we currently produce many unhelpful errors.

This PR allows these situations to suggest simply using Higher-Rank Trait Bounds like for<'a> Fn(&'a str) -> bool.

Fixes #103490.

@rustbot
Copy link
Collaborator

rustbot commented Nov 17, 2022

r? @jackh726

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 17, 2022
@fmease
Copy link
Member

fmease commented Nov 17, 2022

Probably best reviewed by a member of the diagnostics working group.
r? diagnostics

@rustbot rustbot assigned estebank and unassigned jackh726 Nov 17, 2022
fmease
fmease requested changes Nov 17, 2022
View reviewed changes
Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I have some suggestions.

compiler/rustc_parse/src/parser/ty.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/ty.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/ty.rs Show resolved Hide resolved
compiler/rustc_parse/src/parser/ty.rs Outdated Show resolved Hide resolved
@ohno418 ohno418 force-pushed the recover-fn-traits-with-lifetime-params branch from 37f27f8 to 9e692c3 Compare November 18, 2022 03:02
@ohno418
Copy link
Contributor Author

ohno418 commented Nov 18, 2022

@fmease @estebank
Thank you for the reviews! Updated.

@ohno418 ohno418 force-pushed the recover-fn-traits-with-lifetime-params branch from 9e692c3 to 2a07a2e Compare November 24, 2022 10:13
@ohno418
Copy link
Contributor Author

ohno418 commented Dec 5, 2022

@estebank
Hi, could you please take another look at this?

@bors
Copy link
Contributor

bors commented Dec 28, 2022

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

estebank
estebank reviewed Dec 28, 2022
View reviewed changes
compiler/rustc_parse/src/parser/ty.rs
Comment on lines +972 to +1035
let generic_args = if let Some(p_args) = &fn_path_segment.args {
p_args.clone().into_inner()
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can now be

Suggested change
let generic_args = if let Some(p_args) = &fn_path_segment.args {
p_args.clone().into_inner()
} else {
let Some(p_args) = &fn_path_segment.args {
p_args.clone().into_inner()
} else {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this valid syntax? I got the following error:

error: expected one of `(`, `.`, `::`, `;`, `?`, `else`, or an operator, found `{`
    --> compiler/rustc_parse/src/parser/ty.rs:1033:50
     |
1033 |         let Some(p_args) = &fn_path_segment.args {
     |                                                  ^ expected one of 7 possible tokens

error: could not compile `rustc_parse` due to previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `rustc_parse` due to previous error

Copy link
Member

@fmease fmease Dec 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

With let/else syntax, it's

let Some(p_args) = &fn_path_segment.args else {
    // ... ... ...
    return Ok(());
};
let p_args = p_args.clone().into_inner();

or

let Some(p_args) = fn_path_segment.args.cloned().map(|arg| arg) else {
    // ... ... ...
    return Ok(());
};

or

let Some(p_args) = fn_path.segments
    .last_mut()
    .unwrap()
    .args
    .cloned()
    .map(|arg| arg)
else {
    // ... ... ...
    return Ok(());
};

or something similar at your choosing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yeah, I mistyped what I meant. Thanks @fmease for clarifying it

estebank
estebank approved these changes Dec 28, 2022
View reviewed changes
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

r=me after rebase

@ohno418
Provide a better error for Fn traits with lifetime params
e5281c3 
Currently, given `Fn`-family traits with lifetime params like
`Fn<'a>(&'a str) -> bool`, many unhelpful errors show up. These are a
bit confusing.

This commit allows these situations to suggest simply using
higher-ranked trait bounds like `for<'a> Fn(&'a str) -> bool`.
@ohno418 ohno418 force-pushed the recover-fn-traits-with-lifetime-params branch from 2a07a2e to e5281c3 Compare December 29, 2022 06:47
@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Dec 29, 2022

📌 Commit e5281c3 has been approved by estebank

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 29, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2022
@bors
Auto merge of rust-lang#106266 - matthiaskrgr:rollup-cxrdbzy, r=matth…
ad8ae05 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104531 (Provide a better error and a suggestion for `Fn` traits with lifetime params)
 - rust-lang#105899 (`./x doc library --open` opens `std`)
 - rust-lang#106190 (Account for multiple multiline spans with empty padding)
 - rust-lang#106202 (Trim more paths in obligation types)
 - rust-lang#106234 (rustdoc: simplify settings, help, and copy button CSS by not reusing)
 - rust-lang#106236 (docs/test: add docs and a UI test for `E0514` and `E0519`)
 - rust-lang#106259 (Update Clippy)
 - rust-lang#106260 (Fix index out of bounds issues in rustdoc)
 - rust-lang#106263 (Formatter should not try to format non-Rust files)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 81c2b72 into rust-lang:master Dec 29, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 29, 2022
@bors
Copy link
Contributor

bors commented Dec 29, 2022

⌛ Testing commit e5281c3 with merge ad8ae05...

@ohno418 ohno418 deleted the recover-fn-traits-with-lifetime-params branch December 30, 2022 05:45
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
@bors
Auto merge of rust-lang#106266 - matthiaskrgr:rollup-cxrdbzy, r=matth…
972e531 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104531 (Provide a better error and a suggestion for `Fn` traits with lifetime params)
 - rust-lang#105899 (`./x doc library --open` opens `std`)
 - rust-lang#106190 (Account for multiple multiline spans with empty padding)
 - rust-lang#106202 (Trim more paths in obligation types)
 - rust-lang#106234 (rustdoc: simplify settings, help, and copy button CSS by not reusing)
 - rust-lang#106236 (docs/test: add docs and a UI test for `E0514` and `E0519`)
 - rust-lang#106259 (Update Clippy)
 - rust-lang#106260 (Fix index out of bounds issues in rustdoc)
 - rust-lang#106263 (Formatter should not try to format non-Rust files)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 12, 2023
@bors
Auto merge of rust-lang#106266 - matthiaskrgr:rollup-cxrdbzy, r=matth…
cd579d6 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104531 (Provide a better error and a suggestion for `Fn` traits with lifetime params)
 - rust-lang#105899 (`./x doc library --open` opens `std`)
 - rust-lang#106190 (Account for multiple multiline spans with empty padding)
 - rust-lang#106202 (Trim more paths in obligation types)
 - rust-lang#106234 (rustdoc: simplify settings, help, and copy button CSS by not reusing)
 - rust-lang#106236 (docs/test: add docs and a UI test for `E0514` and `E0519`)
 - rust-lang#106259 (Update Clippy)
 - rust-lang#106260 (Fix index out of bounds issues in rustdoc)
 - rust-lang#106263 (Formatter should not try to format non-Rust files)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@fmease fmease mentioned this pull request Oct 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmease fmease fmease requested changes

@estebank estebank estebank approved these changes

Assignees

@estebank estebank

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.68.0
Development

Successfully merging this pull request may close these issues.

Recover from malformed trait bounds of the form Fn<'a>()
6 participants
@ohno418 @rustbot @fmease @bors @estebank @jackh726