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

Only suggest removing semicolon when expression is compatible with impl Trait #95758

Merged
merged 1 commit into from
Apr 11, 2022

Conversation

compiler-errors
Copy link
Member

#54771 (comment)

It still needs checking that the last statement's expr can actually conform to the trait, but the naïve behavior is there.

Only suggest removing a semicolon when the type behind the semicolon actually implements the trait in an RPIT -> impl Trait. Also upgrade the label that suggests removing the semicolon to a suggestion (should it be verbose?).

cc #54771

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 7, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 7, 2022
@compiler-errors
Copy link
Member Author

oh, hey, it r?'ed the person i was gonna cc in this followup comment... that being said, @estebank, I don't know the context that you have in this FIXME:

// FIXME(estebank): When encountering a method with a trait
// bound not satisfied in the return type with a body that has
// no return, suggest removal of semicolon on last statement.
// Once that is added, close #54771.

What more work needs to be done to improve this? I'd love to investigate more.

Comment on lines +32 to +33
LL | fn bar() -> impl Bar {
| ^^^^^^^^ the trait `Bar` is not implemented for `()`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a span label pointing at the last statement saying "this is a statement, which are always of type ()" and maybe mention the type of the expression in that statement if it were a tail expression, explaining that it doesn't match the trait?

@estebank
Copy link
Contributor

estebank commented Apr 8, 2022

r=me after removing unnecessary comment. Feel free to address some of the nitpicks at your leisure.

@compiler-errors
Copy link
Member Author

Also changed the equiavlent "consider removing semicolon" to just say "remove" (e.g. fn foo() -> &str { ""; } for consistency.

@bors r=estebank

@bors
Copy link
Contributor

bors commented Apr 10, 2022

📌 Commit dfe13db has been approved by estebank

@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 Apr 10, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 10, 2022
…bank

Only suggest removing semicolon when expression is compatible with `impl Trait`

rust-lang#54771 (comment)
> It still needs checking that the last statement's expr can actually conform to the trait, but the naïve behavior is there.

Only suggest removing a semicolon when the type behind the semicolon actually implements the trait in an RPIT `-> impl Trait`. Also upgrade the label that suggests removing the semicolon to a suggestion (should it be verbose?).

cc rust-lang#54771
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 11, 2022
…bank

Only suggest removing semicolon when expression is compatible with `impl Trait`

rust-lang#54771 (comment)
> It still needs checking that the last statement's expr can actually conform to the trait, but the naïve behavior is there.

Only suggest removing a semicolon when the type behind the semicolon actually implements the trait in an RPIT `-> impl Trait`. Also upgrade the label that suggests removing the semicolon to a suggestion (should it be verbose?).

cc rust-lang#54771
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 11, 2022
…bank

Only suggest removing semicolon when expression is compatible with `impl Trait`

rust-lang#54771 (comment)
> It still needs checking that the last statement's expr can actually conform to the trait, but the naïve behavior is there.

Only suggest removing a semicolon when the type behind the semicolon actually implements the trait in an RPIT `-> impl Trait`. Also upgrade the label that suggests removing the semicolon to a suggestion (should it be verbose?).

cc rust-lang#54771
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 11, 2022
…bank

Only suggest removing semicolon when expression is compatible with `impl Trait`

rust-lang#54771 (comment)
> It still needs checking that the last statement's expr can actually conform to the trait, but the naïve behavior is there.

Only suggest removing a semicolon when the type behind the semicolon actually implements the trait in an RPIT `-> impl Trait`. Also upgrade the label that suggests removing the semicolon to a suggestion (should it be verbose?).

cc rust-lang#54771
@bors
Copy link
Contributor

bors commented Apr 11, 2022

⌛ Testing commit dfe13db with merge 6a4851f22a7d3cd2b908b9e3a3cd8daff692da07...

@Dylan-DPC
Copy link
Member

@bors retry yield to rollup containing this pr

@bors
Copy link
Contributor

bors commented Apr 11, 2022

⌛ Testing commit dfe13db with merge d00e770...

@bors bors mentioned this pull request Apr 11, 2022
@bors
Copy link
Contributor

bors commented Apr 11, 2022

☀️ Test successful - checks-actions
Approved by: estebank
Pushing d00e770 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 11, 2022
@bors bors merged commit d00e770 into rust-lang:master Apr 11, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 11, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@compiler-errors compiler-errors deleted the issue-54771 branch August 11, 2023 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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
Development

Successfully merging this pull request may close these issues.

7 participants