Skip to content
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

Don't suggest adding return type for closures with default return type #129260

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

wafarm
Copy link
Contributor

@wafarm wafarm commented Aug 19, 2024

Follow up of #129223

r? @compiler-errors

@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 Aug 19, 2024
@wafarm wafarm force-pushed the dont-suggest-closures branch from bf22d44 to 893f2be Compare August 20, 2024 01:56
@wafarm wafarm marked this pull request as ready for review August 20, 2024 02:59
@@ -832,7 +832,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
err.subdiagnostic(errors::ExpectedReturnTypeLabel::Unit { span });
return true;
}
&hir::FnRetTy::DefaultReturn(span) if expected.is_unit() => {
// Don't suggest adding return types for closures with default return.
Copy link
Member

Choose a reason for hiding this comment

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

There's some redundancy between this check and the can_suggest boolean above. Can you find out if we can just reuse can_suggest in this position, rather than making the check from within the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did some investigations and I found that

hir::ClosureKind::Closure => Some((def_id, fn_decl, true)),

If I set can_suggest to false for closures here, then for tests/ui/closures/add_semicolon_non_block_closure.rs

match &fn_decl.output {
&hir::FnRetTy::DefaultReturn(span) if expected.is_unit() && !can_suggest => {
// `fn main()` must return `()`, do not suggest changing return type
err.subdiagnostic(errors::ExpectedReturnTypeLabel::Unit { span });
return true;
}
&hir::FnRetTy::DefaultReturn(span) if expected.is_unit() => {

code here will go to the upper branch. I think we still need some way to distinguish main and closures or we will get something like this:

 --> testfile.rs:8:12
  |
8 |     foo(|| bar())
  |           -^^^^^ expected `()`, found `i32`
  |           |
  |           expected `()` because of default return type
  |

I don't expect the last line to be printed.

I've thought of some possible solutions:

  • The above result is acceptable?
  • Keep my original solution, which indeed adds to complexity
  • Add is_main to the return values of get_fn_decl, which seems (I think) a bit overkill and may impact performance if it's getting called a lot
  • Refactor some of the code to see if we can find a better solution

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Would appreciate testing out the suggestion I had.

@rustbot rustbot 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 Aug 26, 2024
@wafarm wafarm force-pushed the dont-suggest-closures branch from 893f2be to 9b7739c Compare August 28, 2024 04:39
@wafarm wafarm force-pushed the dont-suggest-closures branch from 9b7739c to 736ab66 Compare August 28, 2024 04:55
@wafarm
Copy link
Contributor Author

wafarm commented Aug 28, 2024

I removed can_suggest from get_fn_decl as the only usage of it is to be passed to suggest_missing_return_type. Instead, I do the check in a new function can_add_return_type.

This also fixed not suggesting adding return type in non-trait impl blocks.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 28, 2024
@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 10, 2024

📌 Commit 736ab66 has been approved by compiler-errors

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 Sep 10, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 11, 2024
…mpiler-errors

Don't suggest adding return type for closures with default return type

Follow up of rust-lang#129223

r? `@compiler-errors`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…kingjubilee

Rollup of 14 pull requests

Successful merges:

 - rust-lang#129260 (Don't suggest adding return type for closures with default return type)
 - rust-lang#129520 (Suggest the correct pattern syntax on usage of unit variant pattern for a struct variant)
 - rust-lang#129696 (update stdarch)
 - rust-lang#129759 (Stabilize `const_refs_to_static`)
 - rust-lang#129835 (enable const-float-classify test, and test_next_up/down on 32bit x86)
 - rust-lang#129866 (Clarify documentation labelling and definitions for std::collections)
 - rust-lang#130052 (Don't leave debug locations for constants sitting on the builder indefinitely)
 - rust-lang#130077 (Fix linking error when compiling for 32-bit watchOS)
 - rust-lang#130123 (Report the `note` when specified in `diagnostic::on_unimplemented`)
 - rust-lang#130156 (Add test for S_OBJNAME & update test for LF_BUILDINFO cl and cmd)
 - rust-lang#130206 (Map `WSAEDQUOT` to `ErrorKind::FilesystemQuotaExceeded`)
 - rust-lang#130207 (Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`)
 - rust-lang#130219 (Fix false positive with `missing_docs` and `#[test]`)
 - rust-lang#130221 (Make SearchPath::new public)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#129260 (Don't suggest adding return type for closures with default return type)
 - rust-lang#129520 (Suggest the correct pattern syntax on usage of unit variant pattern for a struct variant)
 - rust-lang#129866 (Clarify documentation labelling and definitions for std::collections)
 - rust-lang#130123 (Report the `note` when specified in `diagnostic::on_unimplemented`)
 - rust-lang#130161 (refactor merge base logic and fix `x fmt`)
 - rust-lang#130206 (Map `WSAEDQUOT` to `ErrorKind::FilesystemQuotaExceeded`)
 - rust-lang#130207 (Map `ERROR_CANT_RESOLVE_FILENAME` to `ErrorKind::FilesystemLoop`)
 - rust-lang#130219 (Fix false positive with `missing_docs` and `#[test]`)
 - rust-lang#130221 (Make SearchPath::new public)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 76e070f into rust-lang:master Sep 11, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 11, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 11, 2024
Rollup merge of rust-lang#129260 - wafarm:dont-suggest-closures, r=compiler-errors

Don't suggest adding return type for closures with default return type

Follow up of rust-lang#129223

r? ``@compiler-errors``
@wafarm wafarm deleted the dont-suggest-closures branch September 13, 2024 13:13
# for free to join this conversation on GitHub. Already have an account? # to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants