Implement the new desugaring from `try_trait_v2` #84767

scottmcm · 2021-04-30T23:41:48Z

~~Currently blocked on #84782, which has a PR in #84811~~ Rebased atop that fix.

try_trait_v2 tracking issue: #84277

Unfortunately this is already touching a ton of things, so if you have suggestions for good ways to split it up, I'd be happy to hear them. (The combination between the use in the library, the compiler changes, the corresponding diagnostic differences, even MIR tests mean that I don't really have a great plan for it other than trying to have decently-readable commits.

r? @ghost

~~(This probably shouldn't go in during the last week before the fork anyway.)~~ Fork happened.

scottmcm · 2021-04-30T23:49:37Z

src/test/ui/inference/cannot-infer-async-enabled-impl-trait-bindings.stderr

+   |         --- consider giving `fut` the explicit type `impl Future`, where the type parameter `E` is specified
+...
+LL |         Ok(())
+   |         ^^ cannot infer type for type parameter `E` declared on the enum `Result`


@estebank I got some of the places fixed up, but there are some others like this where I've regressed the diagnostics, losing some of the special ? notes. If you have any pointers to share on how to get them fixed up, I'd appreciate it.

To be fair, the existing ? diagnostic was already incorrect: this seems to imply that the async block behaves similarly to try, which it doesn't. We might have to proactively look for ? expressions in async block bodies and give a specific error for them, or collect their spans and see if any E0282s happen in the same block as one of them, to treat them explicitly. Don't think these should block landing this, but needs to be addressed before these errors land on stable.

scottmcm · 2021-04-30T23:50:44Z

src/test/ui/try-trait/bad-interconversion.stderr

+   |
+LL | / fn option_to_result() -> Result<u64, String> {
+LL | |     Some(3)?;
+   | |            ^ use `.ok_or(...)?` to provide an error compatible with `Result<u64, String>`


❤️ rustc_on_unimplemented

scottmcm · 2021-05-03T04:24:54Z

@bors try @rust-timer queue

rust-timer · 2021-05-03T04:24:56Z

Awaiting bors try build completion.

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

bors · 2021-05-03T04:25:03Z

⌛ Trying commit b1cf4cec0ab0c51b45db9c665ade7419c7cd85f0 with merge 9e1fbcd2d92fbe606de781106bb28b5c1e981c0a...

bors · 2021-05-03T05:09:04Z

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

rust-timer · 2021-05-03T05:09:05Z

Queued 9e1fbcd2d92fbe606de781106bb28b5c1e981c0a with parent 59f551a, future comparison URL.

rust-timer · 2021-05-03T10:52:42Z

Finished benchmarking try commit (9e1fbcd2d92fbe606de781106bb28b5c1e981c0a): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

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

lcnr

nits, then r=me

src/test/codegen/try_identity.rs

src/test/mir-opt/simplify-arm.rs

src/test/mir-opt/simplify_try.rs

lcnr · 2021-05-18T18:16:57Z

@bors rollup=never

Thanks for the suggestions, lcnr! Co-authored-by: lcnr <rust@lcnr.de>

scottmcm · 2021-05-18T19:00:00Z

@bors r=lcnr

bors · 2021-05-18T19:00:01Z

📌 Commit e2edee4 has been approved by lcnr

bors · 2021-05-18T20:50:08Z

⌛ Testing commit e2edee4 with merge 4e3e6db...

bors · 2021-05-18T23:09:11Z

☀️ Test successful - checks-actions
Approved by: lcnr
Pushing 4e3e6db to master...

rust-highfive · 2021-05-18T23:09:40Z

📣 Toolstate changed by #84767!

Tested on commit 4e3e6db.
Direct link to PR: #84767

💔 rls on windows: test-pass → build-fail (cc @Xanewok).
💔 rls on linux: test-pass → build-fail (cc @Xanewok).

@Xanewok

Tested on commit rust-lang/rust@4e3e6db. Direct link to PR: <rust-lang/rust#84767> 💔 rls on windows: test-pass → build-fail (cc @Xanewok). 💔 rls on linux: test-pass → build-fail (cc @Xanewok).

Implement the new desugaring from `try_trait_v2` ~~Currently blocked on rust-lang#84782, which has a PR in rust-lang#84811 Rebased atop that fix. `try_trait_v2` tracking issue: rust-lang#84277 Unfortunately this is already touching a ton of things, so if you have suggestions for good ways to split it up, I'd be happy to hear them. (The combination between the use in the library, the compiler changes, the corresponding diagnostic differences, even MIR tests mean that I don't really have a great plan for it other than trying to have decently-readable commits. r? `@ghost` ~~(This probably shouldn't go in during the last week before the fork anyway.)~~ Fork happened.

rylev · 2021-05-25T16:37:57Z

@lcnr @scottmcm this looks like it caused some bigger perf issues after merging than in the original perf run.

A cursory look points to mir_built being the source of most of regression. Is more mir being produced by this change?

scottmcm · 2021-05-25T17:38:44Z

There's actually less MIR being produced now.

The red at https://github.com/rust-lang/rust/pull/84767/files#diff-d93592ecc730e2061ac31cad11e78e3bb7cdc7ca3257a85f04bbd3f48c0c6521L1938 is because it only needs to make one call (from_residual(r)) in the short-circuiting branch, not the two (from_err(From::from(e))) that it used to. That thus results in one fewer basic block in MIR: https://github.com/rust-lang/rust/pull/84767/files#diff-a7c7df8af59063804b05ac13069f523e405cd5ecce0f157de356b37922461f34L71

My guess would be more stress on the type solvers. jyn pointed out that diesel is hitting type_op_prove_predicate pretty hard, apparently, so it might be that the trait structure is harder for the compiler to prove than the old one.

scottmcm mentioned this pull request Apr 30, 2021

Tracking Issue for try_trait_v2, A new design for the ? desugaring (RFC#3058) #84277

Open

23 tasks

scottmcm commented Apr 30, 2021

View reviewed changes

scottmcm force-pushed the try_trait_actual branch 2 times, most recently from c65047a to 1f5d2ee Compare May 1, 2021 00:15

This comment has been minimized.

Sign in to view

scottmcm force-pushed the try_trait_actual branch from 4164dce to 431c0d8 Compare May 1, 2021 02:22

scottmcm added the F-try_trait_v2 Tracking issue for RFC#3058 label May 1, 2021

This comment has been minimized.

Sign in to view

scottmcm force-pushed the try_trait_actual branch from 1631f15 to bf408ca Compare May 1, 2021 03:48

This comment has been minimized.

Sign in to view

scottmcm force-pushed the try_trait_actual branch from bf408ca to 51bc4f8 Compare May 1, 2021 05:08

This comment has been minimized.

Sign in to view

scottmcm mentioned this pull request May 1, 2021

RustDoc: Links to trait aliases go to trait.Foo.html but the pages are traitalias.Foo.html #84782

Closed

scottmcm force-pushed the try_trait_actual branch from 51bc4f8 to b97e2b4 Compare May 3, 2021 02:39

This comment has been minimized.

Sign in to view

scottmcm force-pushed the try_trait_actual branch from b97e2b4 to b1cf4ce Compare May 3, 2021 03:50

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 3, 2021

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 3, 2021

scottmcm changed the title ~~[STILL WORKING ON IT] Implement the new desugaring from try_trait_v2~~ Implement the new desugaring from try_trait_v2 May 3, 2021

lcnr reviewed May 18, 2021

View reviewed changes

src/test/codegen/try_identity.rs Outdated Show resolved Hide resolved

src/test/mir-opt/simplify-arm.rs Outdated Show resolved Hide resolved

src/test/mir-opt/simplify_try.rs Outdated Show resolved Hide resolved

Mention the issue number for the new mir-opt in the FIXMEs

6f7dea7

Thanks for the suggestions, lcnr! Co-authored-by: lcnr <rust@lcnr.de>

This comment has been minimized.

Sign in to view

No matter how trivial the change, tidy always finds a way to complain...

e2edee4

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 May 18, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label May 18, 2021

bors merged commit 4e3e6db into rust-lang:master May 18, 2021

rustbot added this to the 1.54.0 milestone May 18, 2021

scottmcm deleted the try_trait_actual branch May 18, 2021 23:09

rust-highfive mentioned this pull request May 18, 2021

rls no longer builds after rust-lang/rust#84767 #85453

Closed

yaahc mentioned this pull request May 19, 2021

#[cfg(bootstrap)] out NoneError and other v1 try_trait stuff #85482

Merged

scottmcm mentioned this pull request May 23, 2021

Tracking issue for ControlFlow enum, for use with try_fold and in Try #75744

Closed

9 tasks

Skepfyr mentioned this pull request Jun 28, 2021

-Z print-type-sizes: internal error: entered unreachable code: FieldsShape::offset: Primitives have no fields' #86528

Closed

indygreg mentioned this pull request Aug 12, 2021

Rust 1.54.0 optimized compilation overwrites stack variable, causing segfault #87947

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the new desugaring from `try_trait_v2` #84767

Implement the new desugaring from `try_trait_v2` #84767

scottmcm commented Apr 30, 2021 •

edited

Loading

scottmcm Apr 30, 2021

estebank May 12, 2021

scottmcm Apr 30, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

scottmcm commented May 3, 2021

rust-timer commented May 3, 2021

bors commented May 3, 2021

bors commented May 3, 2021

rust-timer commented May 3, 2021

rust-timer commented May 3, 2021

lcnr left a comment

lcnr commented May 18, 2021

This comment has been minimized.

scottmcm commented May 18, 2021

bors commented May 18, 2021

bors commented May 18, 2021

bors commented May 18, 2021

rust-highfive commented May 18, 2021

rylev commented May 25, 2021

scottmcm commented May 25, 2021

Implement the new desugaring from try_trait_v2 #84767

Implement the new desugaring from try_trait_v2 #84767

Conversation

scottmcm commented Apr 30, 2021 • edited Loading

scottmcm Apr 30, 2021

Choose a reason for hiding this comment

estebank May 12, 2021

Choose a reason for hiding this comment

scottmcm Apr 30, 2021

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

scottmcm commented May 3, 2021

rust-timer commented May 3, 2021

bors commented May 3, 2021

bors commented May 3, 2021

rust-timer commented May 3, 2021

rust-timer commented May 3, 2021

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented May 18, 2021

This comment has been minimized.

scottmcm commented May 18, 2021

bors commented May 18, 2021

bors commented May 18, 2021

bors commented May 18, 2021

rust-highfive commented May 18, 2021

rylev commented May 25, 2021

scottmcm commented May 25, 2021

Implement the new desugaring from `try_trait_v2` #84767

Implement the new desugaring from `try_trait_v2` #84767

scottmcm commented Apr 30, 2021 •

edited

Loading