Skip to content

Suggest returning tail expressions that match return type #81769

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
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
Contributor

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.

# to view
lcnr
lcnr reviewed Feb 6, 2021
View reviewed changes
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

@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.

# 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.

# to view
@rust-log-analyzer

This comment has been minimized.

# 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.

# 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
Collaborator

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.
# for free to join this conversation on GitHub. Already have an account? # 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