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

Improve suggestions for returning binding #99539

Merged
merged 5 commits into from
Jul 22, 2022
Merged

Improve suggestions for returning binding #99539

merged 5 commits into from
Jul 22, 2022

Conversation

compiler-errors
Copy link
Member

Fixes #99525

Also reworks the cause codes for match and if a bit, I think cleaning them up in a positive way.
We no longer need to call could_remove_semicolon in successful code, which might save a few cycles?

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

r? @fee1-dead

(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 Jul 21, 2022
@compiler-errors compiler-errors force-pushed the bidirectional-block-suggestions branch from 21ca82f to e6588dc Compare July 21, 2022 01:29
@compiler-errors
Copy link
Member Author

compiler-errors commented Jul 21, 2022 •
edited
Loading

We no longer need to call could_remove_semicolon in successful code, which might save a few cycles?

@bors try @rust-timer queue

will probably just come back with noise..

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 21, 2022
@bors
Copy link
Contributor

bors commented Jul 21, 2022

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout bidirectional-block-suggestions (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self bidirectional-block-suggestions --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message 
Auto-merging compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs
CONFLICT (content): Merge conflict in compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs
Automatic merge failed; fix conflicts and then commit the result.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@compiler-errors compiler-errors force-pushed the bidirectional-block-suggestions branch from e6588dc to d1db3cf Compare July 21, 2022 01:34
@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Jul 21, 2022

⌛ Trying commit d1db3cf40b0bd96f6521ea338dce52457935eab5 with merge e55802f787a29518ae5d5174349d8aa47458e9f8...

compiler-errors
compiler-errors commented Jul 21, 2022
View reviewed changes
src/test/ui/async-await/async-block-control-flow-static-semantics.stderr
@@ -55,6 +47,14 @@ LL | fn return_targets_async_block_not_fn() -> u8 {
| |
| implicitly returns `()` as its body has no tail or `return` expression

error[E0271]: type mismatch resolving `<impl Future<Output = u8> as Future>::Output == ()`
Copy link
Member Author

Choose a reason for hiding this comment

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

The output of this test moved in the last PR, too... I think it's just some FxHash* instability somewhere, but seems unrelated.

@compiler-errors compiler-errors changed the title Bidirectional block suggestions Improve suggestions for returning binding Jul 21, 2022
@bors
Copy link
Contributor

bors commented Jul 21, 2022

☀️ Try build successful - checks-actions
Build commit: e55802f787a29518ae5d5174349d8aa47458e9f8 (e55802f787a29518ae5d5174349d8aa47458e9f8)

@rust-timer
Copy link
Collaborator

Queued e55802f787a29518ae5d5174349d8aa47458e9f8 with parent be9cfb3, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e55802f787a29518ae5d5174349d8aa47458e9f8): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: 😿 relevant regression found
mean1 max count2
Regressions 😿
(primary)		 3.0% 3.0% 1
Regressions 😿
(secondary)		 2.2% 2.2% 1
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 3.0% 3.0% 1

Cycles

Results
  • Primary benchmarks: 😿 relevant regressions found
  • Secondary benchmarks: no relevant changes found
mean1 max count2
Regressions 😿
(primary)		 2.1% 2.4% 2
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) 2.1% 2.4% 2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 21, 2022
@compiler-errors
Copy link
Member Author

Well, at least it's not a regression lol.

@bors rollup-

fee1-dead
fee1-dead reviewed Jul 21, 2022
View reviewed changes
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

only done a few, expect more later

compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/fn_ctxt/_impl.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Show resolved Hide resolved
@compiler-errors compiler-errors force-pushed the bidirectional-block-suggestions branch from d1db3cf to a90010b Compare July 21, 2022 07:09
@compiler-errors compiler-errors force-pushed the bidirectional-block-suggestions branch from a90010b to 8926dac Compare July 21, 2022 07:44
fee1-dead
fee1-dead reviewed Jul 21, 2022
View reviewed changes
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

Overall looks good, left some nits.

compiler/rustc_infer/src/infer/error_reporting/mod.rs Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_hir/src/hir.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved
fee1-dead
fee1-dead approved these changes Jul 21, 2022
View reviewed changes
Copy link
Member

@fee1-dead fee1-dead left a comment

Choose a reason for hiding this comment

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

Great work! Sorry for the one last nit. r= me once that is resolved.

compiler/rustc_infer/src/infer/error_reporting/mod.rs Show resolved Hide resolved
@compiler-errors compiler-errors force-pushed the bidirectional-block-suggestions branch from fb63313 to 3eef023 Compare July 21, 2022 16:43
@compiler-errors
Copy link
Member Author

@bors r=fee1-dead

@bors
Copy link
Contributor

bors commented Jul 21, 2022

📌 Commit 3eef023 has been approved by fee1-dead

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 Jul 21, 2022
estebank
estebank reviewed Jul 21, 2022
View reviewed changes
src/test/ui/suggestions/return-bindings.stderr
Comment on lines +58 to +59
LL + s
LL ~
Copy link
Contributor

Choose a reason for hiding this comment

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

I was gonna make a comment about this and then I realized it is the elided //~ERROR 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it renders everything up to the // on the next line, which is just whitespace...

estebank
estebank reviewed Jul 21, 2022
View reviewed changes
src/test/ui/suggestions/return-bindings.rs
Comment on lines -1 to -2
// run-rustfix

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we keep some rustfix version? What made it no longer apply neatly? I usually split files like these into two, one for things that are rustfix and another for the ones that aren't.

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't that, @fee1-dead just didn't think that this was MachineApplicable worthy and I didn't feel strongly either way.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's a category between MachineApplicable and MaybeIncorrect that's like "probably right given no other options", which this suggestion falls into.

Copy link
Member

Choose a reason for hiding this comment

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

given that this is already in the queue we could split this into two files in a followup PR.

estebank
estebank reviewed Jul 21, 2022
View reviewed changes
src/test/ui/issues/issue-59494.stderr
Comment on lines -10 to -11
= note: expected a closure with arguments `(((_, _), _),)`
found a closure with arguments `(_,)`
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting regression 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not a regression, per se. The original note was nonsense, because the reason that this impl Fn(((_, _), _)) does not implement Fn<(_,)> is unrelated to arg mismatch and has to do with some cyclical closure stuff. This was just revealed because now (((_, _), _),) and (_,) are "same type modulo infer".

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah! I see. Neat.

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Jul 22, 2022
@Dylan-DPC
Rollup merge of rust-lang#99539 - compiler-errors:bidirectional-block…
36478ef 
…-suggestions, r=fee1-dead

Improve suggestions for returning binding

Fixes rust-lang#99525

Also reworks the cause codes for match and if a bit, I think cleaning them up in a positive way.
We no longer need to call `could_remove_semicolon` in successful code, which might save a few cycles?
This was referenced Jul 22, 2022
Closed
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2022
@bors
Auto merge of rust-lang#99592 - Dylan-DPC:rollup-xlw4wax, r=Dylan-DPC
e7a9c11 
Rollup of 6 pull requests

Successful merges:

 - rust-lang#98174 (Rename `<*{mut,const} T>::as_{const,mut}` to `cast_`)
 - rust-lang#98868 (Fix unreachable coverage generation for inlined functions)
 - rust-lang#99393 (feat: omit suffixes in const generics (e.g. `1_i32`))
 - rust-lang#99423 (Group CSS font rule)
 - rust-lang#99539 (Improve suggestions for returning binding)
 - rust-lang#99579 (Add same warning to Result::expect as Result::unwrap)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 92bebac into rust-lang:master Jul 22, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 22, 2022
@Noratrieb Noratrieb mentioned this pull request Aug 17, 2022
Closed
@compiler-errors compiler-errors deleted the bidirectional-block-suggestions branch August 11, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fee1-dead fee1-dead fee1-dead approved these changes

@estebank estebank estebank left review comments

Assignees

@fee1-dead fee1-dead

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.64.0
Development

Successfully merging this pull request may close these issues.

Suggest potentially missing value in empty conditional expression arms with inferred type
7 participants
@compiler-errors @rust-highfive @rust-timer @bors @estebank @fee1-dead @rustbot