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

Suggest returning tail expressions that match return type #81769

Merged
merged 5 commits into from
Feb 23, 2021
Merged

Suggest returning tail expressions that match return type #81769

merged 5 commits into from
Feb 23, 2021

Conversation

estebank
Copy link
Contributor

@estebank estebank commented Feb 5, 2021

Some newcomers are confused by the behavior of tail expressions,
interpreting that "leaving out the ; makes it the return value".
To help them go in the right direction, suggest using return instead
when applicable.

@rust-highfive
Copy link
Collaborator

r? @lcnr

(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 Feb 5, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
lcnr
lcnr reviewed Feb 6, 2021
View reviewed changes
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
lcnr
lcnr approved these changes Feb 6, 2021
View reviewed changes
Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

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

small nits, otherwise LGTM

compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
@lcnr lcnr added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 11, 2021
@bors

This comment has been minimized.

Sign in to view
@estebank
Suggest returning tail expressions that match return type
796ce9f 
Some newcomers are confused by the behavior of tail expressions,
interpreting that "leaving out the `;` makes it the return value".
To help them go in the right direction, suggest using `return` instead
when applicable.
@estebank
reword ; suggestions to have consistent wording
020edd9
@estebank
Do not suggest ; if expression is side effect free
d669882 
When a tail expression isn't unit, we previously always suggested adding
a trailing `;` to turn it into a statement. This suggestion isn't
appropriate for any expression that doesn't have side-effects, as the
user will have likely wanted to call something else or do something with
the resulting value, instead of just discarding it.
@estebank
tidy
86b3f3f
@estebank estebank force-pushed the tail-expr-as-potential-return branch from 5f3a1ce to 86b3f3f Compare February 22, 2021 00:35
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank estebank force-pushed the tail-expr-as-potential-return branch from 88fd526 to bb73759 Compare February 22, 2021 05:33
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@estebank estebank force-pushed the tail-expr-as-potential-return branch from bb73759 to fc6c19e Compare February 22, 2021 07:16
@estebank
Copy link
Contributor Author

@bors r=lcnr

@bors
Copy link
Contributor

bors commented Feb 22, 2021

📌 Commit fc6c19e has been approved by lcnr

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 22, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 23, 2021
@Dylan-DPC
Rollup merge of rust-lang#81769 - estebank:tail-expr-as-potential-ret…
ea7a90d 
…urn, r=lcnr

Suggest `return`ing tail expressions that match return type

Some newcomers are confused by the behavior of tail expressions,
interpreting that "leaving out the `;` makes it the return value".
To help them go in the right direction, suggest using `return` instead
when applicable.
This was referenced Feb 23, 2021
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2021
@bors
Auto merge of rust-lang#82430 - Dylan-DPC:rollup-nu4kfyc, r=Dylan-DPC
a4e595d 
Rollup of 12 pull requests

Successful merges:

 - rust-lang#79423 (Enable smart punctuation)
 - rust-lang#81154 (Improve design of `assert_len`)
 - rust-lang#81235 (Improve suggestion for tuple struct pattern matching errors.)
 - rust-lang#81769 (Suggest `return`ing tail expressions that match return type)
 - rust-lang#81837 (Slight perf improvement on char::to_ascii_lowercase)
 - rust-lang#81969 (Avoid `cfg_if` in `std::os`)
 - rust-lang#81984 (Make WASI's `hard_link` behavior match other platforms.)
 - rust-lang#82091 (use PlaceRef abstractions more consistently)
 - rust-lang#82128 (add diagnostic items for OsString/PathBuf/Owned as well as to_vec on slice)
 - rust-lang#82166 (add s390x-unknown-linux-musl target)
 - rust-lang#82234 (Remove query parameters when skipping search results)
 - rust-lang#82255 (Make `treat_err_as_bug` Option<NonZeroUsize>)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5d90e89 into rust-lang:master Feb 23, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 23, 2021
@notriddle notriddle mentioned this pull request Feb 23, 2021
Closed
This was referenced Feb 28, 2021
Closed
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 2, 2021
@JohnTitor
Rollup merge of rust-lang#82627 - JohnTitor:issue-82612, r=estebank
2c40e13 
Erase late bound regions to avoid ICE

Fixes rust-lang#82612, which is caused by rust-lang#81769.

r? `@estebank`
@d3v3us
Copy link

d3v3us commented Mar 7, 2021

or pattern matching way

fn authenticate(name: String, password:String)
if let Some(user) = items.pop() {
if...{
return validateHash();
}
}
=> false;
=> false //this also allowed
}

@estebank
Copy link
Contributor Author

estebank commented Mar 7, 2021

@thedeveus sorry, I'm not sure I follow. Could you expand?

ThePuzzlemaker added a commit to ThePuzzlemaker/rust that referenced this pull request Dec 29, 2021
@ThePuzzlemaker
Add regression test for rust-lang#92308
ab7c446 
This amends off of an existing test introduced in rust-lang#81769, if you think I
should make a separate test I will.
@ThePuzzlemaker ThePuzzlemaker mentioned this pull request Dec 29, 2021
Merged
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 14, 2022
@matthiaskrgr
Rollup merge of rust-lang#92381 - ThePuzzlemaker:issue-92308, r=estebank
347c744 
Suggest `return`ing tail expressions in async functions

This PR fixes rust-lang#92308.

Previously, the suggestion to `return` tail expressions (introduced in rust-lang#81769) did not apply to `async` functions, as the suggestion checked whether the types were equal disregarding `impl Future<Output = T>` syntax sugar for `async` functions. This PR changes that in order to fix a potential papercut.

I'm not sure if this is the "right" way to do this, so if there is a better way then please let me know.

I amended an existing test introduced in rust-lang#81769 to add a regression test for this, if you think I should make a separate test I will.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr approved these changes

Assignees

@lcnr lcnr

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.
7 participants
@estebank @rust-highfive @rust-log-analyzer @bors @d3v3us @lcnr @rustbot