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.

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

Generator Resume Arguments #68524

Merged
merged 25 commits into from
Feb 7, 2020
Merged

Generator Resume Arguments #68524

merged 25 commits into from
Feb 7, 2020

Conversation

jonas-schievink
Copy link
Contributor

@jonas-schievink jonas-schievink commented Jan 24, 2020

cc #43122 and #56974

Blockers:

Follow-up work:

  • Change async/await desugaring to make use of this feature
  • Rewrite box_region.rs to use resume arguments (this shows up in profiles too)

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(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 Jan 24, 2020
@jonas-schievink jonas-schievink added A-coroutines Area: Coroutines F-coroutines `#![feature(coroutines)]` WG-embedded Working group: Embedded systems S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 24, 2020
@rust-highfive

This comment has been minimized.

@petrochenkov
Copy link
Contributor

Glad to see this implemented, but it is not my area of expertise.

r? @Zoxc for review or further reassignment.

@rust-highfive rust-highfive assigned Zoxc and unassigned petrochenkov Jan 24, 2020
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

TerminatorKind::Yield { ref value, .. } => {
TerminatorKind::Yield { ref value, resume_arg: ref place, .. } => {
self.create_move_path(place);
self.gather_init(place.as_ref(), InitKind::Deep);
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the order of gather_init and gather_operand is swapped compared to DropAndReplace, but I'm not sure if it matters.

@Zoxc
Copy link
Contributor

Zoxc commented Jan 25, 2020

I think I'd prefer to make this PR make the argument be a type parameter instead of a associated type before landing to avoid some unnecessary churn in the compiler and for users of generators.

I also don't know how correct this is with regards to drops and references, but those concerns could be delayed by just adding a 'static + Copy bound to the generator argument in typeck. That would be good enough for async/await.

@bjorn3
Copy link
Member

bjorn3 commented Jan 25, 2020

I think an assocated type makes more sense, as it is very unlikely to ever be overloaded for different resume types.

@matthewjasper
Copy link
Contributor

Async generators (should) implement for<'a, 'b> Geneator<&'a Context<'b>, Yield = ()>, which needs a type parameter.

@rust-highfive

This comment has been minimized.

@jonas-schievink
Copy link
Contributor Author

I think I'd prefer to make this PR make the argument be a type parameter instead of a associated type before landing to avoid some unnecessary churn in the compiler and for users of generators.

Done!

I also don't know how correct this is with regards to drops and references, but those concerns could be delayed by just adding a 'static + Copy bound to the generator argument in typeck. That would be good enough for async/await.

True, but I'd prefer to not limit this feature to what async/await needs (that always leaves a sour taste in my mouth). I'm willing to add more tests, if you (or anyone else) have any suggestions for what needs to be tested here. The current tests are fairly minimal, but I did add some drop tests.

@jonas-schievink
Copy link
Contributor Author

There's still a lingering miscompilation somewhere:

    let mut g = |mut d| {
        d = yield;
    };

This ends up dropping d after the yield, but without storing anything in its field in the generator state.

Could this be because the transform assumes there will be at least one Assign statement to a local before it needs to be saved? For locals inside the generator that makes sense, but local _2 is passed in from the outside.

@jonas-schievink
Copy link
Contributor Author

Let's see if the yield lowering changes affect compile times.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@Zoxc
Copy link
Contributor

Zoxc commented Feb 6, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Feb 6, 2020

📌 Commit 732913a has been approved by Zoxc

@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 Feb 6, 2020
@bors
Copy link
Contributor

bors commented Feb 6, 2020

⌛ Testing commit 732913a with merge 003534dfa96de6c33f99b0ce3fc2c16456878039...

@rust-highfive
Copy link
Collaborator

The job test-various of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-02-06T13:52:01.9587157Z ---- [ui] ui/generator/panic-drops-resume.rs stdout ----
2020-02-06T13:52:01.9587212Z 
2020-02-06T13:52:01.9587293Z error: test run failed!
2020-02-06T13:52:01.9587372Z status: exit code: 101
2020-02-06T13:52:01.9588102Z command: "/node-v9.2.0-linux-x64/bin/node" "/checkout/src/etc/wasm32-shim.js" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/generator/panic-drops-resume/a.wasm"
2020-02-06T13:52:01.9588433Z ------------------------------------------
2020-02-06T13:52:01.9588492Z 
2020-02-06T13:52:01.9588691Z ------------------------------------------
2020-02-06T13:52:01.9588781Z stderr:
2020-02-06T13:52:01.9588781Z stderr:
2020-02-06T13:52:01.9588977Z ------------------------------------------
2020-02-06T13:52:01.9589056Z RuntimeError: unreachable
2020-02-06T13:52:01.9589268Z     at __rust_start_panic (wasm-function[67]:1)
2020-02-06T13:52:01.9589494Z     at rust_panic (wasm-function[62]:39)
2020-02-06T13:52:01.9589754Z     at _ZN3std9panicking20rust_panic_with_hook17hce0078af59cd6d68E (wasm-function[57]:279)
2020-02-06T13:52:01.9590209Z     at _ZN3std9panicking11begin_panic17ha296b5396bccef14E (wasm-function[1]:55)
2020-02-06T13:52:01.9590565Z     at _ZN3std9panicking3try7do_call17h8f1195c9f0dbdb86E.llvm.13732214386119929706 (wasm-function[2]:54)
2020-02-06T13:52:01.9590844Z     at __rust_maybe_catch_panic (wasm-function[66]:5)
2020-02-06T13:52:01.9591137Z     at _ZN18panic_drops_resume4main17h1e3758f93406ae49E (wasm-function[6]:104)
2020-02-06T13:52:01.9591440Z     at _ZN3std2rt10lang_start28_$u7b$$u7b$closure$u7d$$u7d$17h2edd8568de8eb571E (wasm-function[9]:25)
2020-02-06T13:52:01.9591780Z     at _ZN3std10sys_common9backtrace28__rust_begin_short_backtrace17heaab70e414a5f765E (wasm-function[46]:8)
2020-02-06T13:52:01.9592063Z     at _ZN3std9panicking3try7do_call17ha70e32415386bc04E (wasm-function[55]:20)
2020-02-06T13:52:01.9592333Z     at __rust_maybe_catch_panic (wasm-function[66]:5)
2020-02-06T13:52:01.9592625Z     at _ZN3std2rt19lang_start_internal17h3636b7b998119f22E (wasm-function[63]:250)
2020-02-06T13:52:01.9592854Z     at main (wasm-function[7]:46)
2020-02-06T13:52:01.9593119Z     at Object.<anonymous> (/checkout/src/etc/wasm32-shim.js:20:20)
2020-02-06T13:52:01.9593192Z     at Module._compile (module.js:641:30)
2020-02-06T13:52:01.9593285Z     at Object.Module._extensions..js (module.js:652:10)
2020-02-06T13:52:01.9593351Z     at Module.load (module.js:560:32)
2020-02-06T13:52:01.9593429Z     at tryModuleLoad (module.js:503:12)
2020-02-06T13:52:01.9593494Z     at Function.Module._load (module.js:495:3)
2020-02-06T13:52:01.9593580Z     at Function.Module.runMain (module.js:682:10)
2020-02-06T13:52:01.9594250Z ------------------------------------------
2020-02-06T13:52:01.9594292Z 
2020-02-06T13:52:01.9594322Z 
2020-02-06T13:52:01.9594580Z 
---
2020-02-06T13:52:01.9608846Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:348:22
2020-02-06T13:52:01.9609117Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
2020-02-06T13:52:01.9620689Z 
2020-02-06T13:52:01.9621623Z 
2020-02-06T13:52:01.9624749Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/wasm32-unknown-unknown/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-wasm32-unknown-unknown" "--mode" "ui" "--target" "wasm32-unknown-unknown" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--nodejs" "/node-v9.2.0-linux-x64/bin/node" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/wasm32-unknown-unknown/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--llvm-version" "9.0.1-rust-1.43.0-nightly\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2020-02-06T13:52:01.9625434Z 
2020-02-06T13:52:01.9625470Z 
2020-02-06T13:52:01.9640853Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target wasm32-unknown-unknown src/test/run-make src/test/ui src/test/compile-fail src/test/mir-opt src/test/codegen-units src/libcore
2020-02-06T13:52:01.9640994Z Build completed unsuccessfully in 1:23:16
2020-02-06T13:52:01.9640994Z Build completed unsuccessfully in 1:23:16
2020-02-06T13:52:01.9726413Z == clock drift check ==
2020-02-06T13:52:01.9747262Z   local time: Thu Feb  6 13:52:01 UTC 2020
2020-02-06T13:52:02.2444294Z   network time: Thu, 06 Feb 2020 13:52:02 GMT
2020-02-06T13:52:02.2447529Z == end clock drift check ==
2020-02-06T13:52:03.0771375Z 
2020-02-06T13:52:03.0863751Z ##[error]Bash exited with code '1'.
2020-02-06T13:52:03.0905866Z ##[section]Starting: Checkout rust-lang/rust@auto to s
2020-02-06T13:52:03.0908159Z ==============================================================================
2020-02-06T13:52:03.0908227Z Task         : Get sources
2020-02-06T13:52:03.0908305Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Feb 6, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 6, 2020
It does not have unwinding support
@jonas-schievink
Copy link
Contributor Author

@bors r=Zoxc

@bors
Copy link
Contributor

bors commented Feb 6, 2020

📌 Commit 9d7b214 has been approved by Zoxc

@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 Feb 6, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 6, 2020
…guments, r=Zoxc

Generator Resume Arguments

cc rust-lang#43122 and rust-lang#56974

Blockers:
* [x] Fix miscompilation when resume argument is live across a yield point (rust-lang#68524 (comment))
* [x] Fix 10% compile time regression in `await-call-tree` benchmarks (rust-lang#68524 (comment))
  * [x] Fix remaining 1-3% regression (rust-lang#68524 (comment)) - resolved (rust-lang#68524 (comment))
* [x] Make dropck rules account for resume arguments (rust-lang#68524 (comment))

Follow-up work:
* Change async/await desugaring to make use of this feature
* Rewrite [`box_region.rs`](https://github.com/rust-lang/rust/blob/3d8778d767f0dde6fe2bc9459f21ead8e124d8cb/src/librustc_data_structures/box_region.rs) to use resume arguments (this shows up in profiles too)
bors added a commit that referenced this pull request Feb 7, 2020
Rollup of 6 pull requests

Successful merges:

 - #67359 (Rename -Zexternal-macro-backtrace to -Zmacro-backtrace and clean up implementation.)
 - #68524 (Generator Resume Arguments)
 - #68791 (implement proper linkchecker hardening)
 - #68886 (Mark fn map_or() as eagerly evaluated.)
 - #68888 (error code examples: replace some more ignore with compile_fail)
 - #68894 (Update E0565 examples)

Failed merges:

r? @ghost
@bors bors merged commit 9d7b214 into rust-lang:master Feb 7, 2020
@semtexzv
Copy link

@jonas-schievink Thanks for the change, I'd be interested about how it relates to the discussion in rust-lang/rfcs#2781 , I'd love to have your input :)

@jonas-schievink
Copy link
Contributor Author

@semtexzv I don't really have the energy to partake in RFC discussions, but I seem to have implemented the second option described in rust-lang/rfcs#2781 (comment) in this PR

@pitaj
Copy link
Contributor

pitaj commented Jun 4, 2020

@jonas-schievink as far as I can tell, this PR doesn't introduce any generator syntax for defining the types of resume arguments and yield values. It still relies on type inference to get those types, right?

What would a maximally qualified generator look like now? Something like this?:

let g = || -> u32 {
  yield "hello";
  5
};

@jonas-schievink jonas-schievink deleted the generator-resume-arguments branch June 4, 2020 19:45
@jonas-schievink
Copy link
Contributor Author

@pitaj The resume argument works like a closure argument, you can give it an explicit type like |arg: Type|. From what I can tell, you're right about the yield type though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-coroutines Area: Coroutines F-coroutines `#![feature(coroutines)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. WG-embedded Working group: Embedded systems
Projects
None yet
Development

Successfully merging this pull request may close these issues.