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

async-await initial reference material #635

Merged
merged 1 commit into from
Aug 27, 2019

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 10, 2019

I'm not sure what the best reviewing process is here.

cc @cramertj @withoutboats @Centril @joshtriplett @pnkfelix @scottmcm @pnkfelix @eddyb (I think that's the lang team, did I miss anybody?)

cc #634 rust-lang/rust#62566

src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/expressions/await-expr.md Outdated Show resolved Hide resolved
src/expressions/await-expr.md Show resolved Hide resolved
src/expressions/await-expr.md Outdated Show resolved Hide resolved
src/expressions/await-expr.md Outdated Show resolved Hide resolved
src/expressions/async-block-expr.md Outdated Show resolved Hide resolved
@Centril Centril requested a review from ehuss July 10, 2019 22:43
@nikomatsakis
Copy link
Contributor Author

So I get a bunch of errors from the examples like

error[E0670]: `async fn` is not permitted in the 2015 edition
 --> /tmp/mdbookaDvS38/items/functions.md:198:1
  |
3 | async fn regular_example() { }
  | ^^^^^
error: expected one of `!`, `.`, `::`, `;`, `?`, `{`, `}`, or an operator, found `unsafe`
 --> /tmp/mdbookaDvS38/items/functions.md:199:7
  |
4 | async unsafe fn unsafe_example() { }
  |       ^^^^^^ expected one of 8 possible tokens here
error[E0425]: cannot find value `async` in this scope
 --> /tmp/mdbookaDvS38/items/functions.md:199:1
  |
4 | async unsafe fn unsafe_example() { }
  | ^^^^^ not found in this scope
error[E0658]: async fn is unstable
 --> /tmp/mdbookaDvS38/items/functions.md:198:1
  |
3 | async fn regular_example() { }
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: for more information, see https://github.com/rust-lang/rust/issues/50547
  = help: add #![feature(async_await)] to the crate attributes to enable

Any tips on how to address these? :)

@nikomatsakis
Copy link
Contributor Author

(Especially the edition flag)

@Centril
Copy link
Contributor

Centril commented Jul 10, 2019

I'm not sure what the best reviewing process is here.

Not sure either, but the current process is basically: "@Centril and @ehuss reviews, we approve, and then we merge".

@Centril
Copy link
Contributor

Centril commented Jul 10, 2019

(Especially the edition flag)

I think you can follow these examples: https://github.com/rust-lang-nursery/reference/search?q=edition2018&unscoped_q=edition2018

@Centril
Copy link
Contributor

Centril commented Jul 10, 2019

Speaking of editions; I think it was not mentioned in the PR that the new stuff only works in edition 2018. Would be good to work that in (see https://github.com/rust-lang-nursery/reference/search?q=edition&unscoped_q=edition for examples).


More specifically, a `f.await` expression has the following effect.

1. evaluate `f` to a future `tmp` using the `IntoFuture:into_future` method;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. evaluate `f` to a future `tmp` using the `IntoFuture:into_future` method;
1. evaluate `f` to a future temporary variable `tmp` using the `IntoFuture:into_future` method;

I don't like "future" there, but don't really see a better way to succinctly convey the information. It should probably link to the Future trait as well.

@Havvy
Copy link
Contributor

Havvy commented Jul 11, 2019

Given that the semantics of move and ? are affected the same in a closure as in an async block, we could get a name for these psuedo-function boundaries and put information on capturing groups in there, the propagation boundary for ? can refer to it, and we can just say closures and async blocks are both whatever we call these psuedo-function boundaries, linking to that.

If we don't want to do that, then we should say move behaves the same way as on closures, linking to that, and in the capturing groups section on the closures page, say that the section applies to both closures and async blocks, linking back to async blocks.

@eddyb
Copy link
Member

eddyb commented Jul 11, 2019

Nit: the title doesn't mention async/await.

@nikomatsakis nikomatsakis changed the title initial reference material async-await initial reference material Jul 11, 2019
@nikomatsakis
Copy link
Contributor Author

Given that the semantics of move and ? are affected the same in a closure as in an async block, we could get a name for these psuedo-function boundaries and put information on capturing groups in there, the propagation boundary for ? can refer to it, and we can just say closures and async blocks are both whatever we call these psuedo-function boundaries, linking to that.

I did link to closures to explain capture modes. I did not do so for ? but I agree that there is some concept that is similar between them.

@nikomatsakis
Copy link
Contributor Author

Note to self: document async unsafe behavior

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

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

I pushed some changes which I think should be uncontroversial, though feel free to revert/change it.

I think we'll need to add a note that trait functions/methods cannot be async (probably somewhere in items/traits.md?). FunctionQualifiers is reused and I think it should be explicit where it is and is not allowed there.

Also, this causes a problem with the definition of BareFunctionType. Syntactically it cannot have async (AFAIK), so that will need to stop using FunctionQualifiers.

We normally only merge once things are stable on master, so the broken test should pass once that happens. I left a note on the other broken test.

FYI, we're still stuck on mdbook 0.1 here. The links are relative to the root of the book. Hopefully we'll get to upgrade soon.

Also, we use relative links between books (like std), so that they point to the correct version (on doc.rust-lang.org).

src/expressions/block-expr.md Outdated Show resolved Hide resolved
src/items/functions.md Show resolved Hide resolved

```rust,ignore
loop {
let tmp = /* <expr> */;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this line be outside of the loop? tmp isn't re-evaluated every single time, right? Also, should probably call it awaited and not tmp.

@ehuss
Copy link
Contributor

ehuss commented Jul 16, 2019

Just a heads up, I have rebased this PR on latest master to resolve the conflicts, so be careful updating locally.

The reference has migrated to mdbook 0.3. This means that links should use the .md extension, and are relative to the page they are on (previously they were relative to the root of the book). Please don't hesitate to ask if you have any questions or any trouble updating. Sorry for the interruption!

@cramertj
Copy link
Member

Are there remaining blockers to landing this?

@ehuss
Copy link
Contributor

ehuss commented Jul 23, 2019

Are there remaining blockers to landing this?

The reference only includes stable features, so normally we wait until it is stabilized on nightly. IIUC, it is currently waiting on FCP in rust-lang/rust#62149?

There are also some unresolved comments above. I'll take care of the formatting issue, but I don't know enough about async to do much else. Is there someone who can pick it up since Niko isn't around?

There's an unresolved issue about "unsafe" being undocumented, but I personally think it would be ok to land that separately. Others may disagree, though.

@nikomatsakis
Copy link
Contributor Author

I pushed two commits -- one addresses @Havvy's nit, the other documents async unsafe. I'm not sure what remaining things are left to be done.

src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/expressions/await-expr.md Outdated Show resolved Hide resolved
src/items/functions.md Outdated Show resolved Hide resolved
src/expressions.md Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Aug 16, 2019

@nikomatsakis Btw, you can apply suggestions in bulk as a single commit if you go to the "Files changed" tab. Use the "Add suggestion to batch" button when you are there.

@Centril
Copy link
Contributor

Centril commented Aug 21, 2019

@nikomatsakis Since the stabilization PR landed, want to make the finishing touches and then squash? Let's merge after that.

@nikomatsakis
Copy link
Contributor Author

@Centril addressed (I think) all nits, squashed, and rebased

@Centril
Copy link
Contributor

Centril commented Aug 27, 2019

Yep, thanks! We'll have to remember to adjust this if we adjust the lowering wrt. the temporary lifetime stuff.

@Centril Centril merged commit b1844a2 into rust-lang:master Aug 27, 2019
bors added a commit to rust-lang/rust that referenced this pull request Sep 4, 2019
Update cargo, books

## cargo

8 commits in 22f7dd0495cd72ce2082d318d5a9b4dccb9c5b8c..fe0e5a48b75da2b405c8ce1ba2674e174ae11d5d
2019-08-27 16:10:51 +0000 to 2019-09-04 00:51:27 +0000
- Rename `--all` to `--workspace` (rust-lang/cargo#7241)
- Basic standard library support. (rust-lang/cargo#7216)
- Allow using 'config.toml' instead of just 'config' files. (rust-lang/cargo#7295)
- Retry on SSL Connect Error. (rust-lang/cargo#7318)
- minimal-copy `deserialize` for `InternedString` (rust-lang/cargo#7310)
- Fix typo in cargo vendor examples (rust-lang/cargo#7320)
- Fixes around multiple `[patch]` per crate (rust-lang/cargo#7303)
- Improve error messages on mkdir failure (rust-lang/cargo#7306)

## reference

7 commits in d191a0c..090c015
2019-08-15 08:42:23 +0200 to 2019-09-03 13:59:28 -0700
- Fix rust-lang/reference#664: Review Oxford comma usage. (rust-lang/reference#668)
- Fix some links. (rust-lang/reference#667)
- Remove trait object warning. (rust-lang/reference#666)
- Specify pattern types in `let` statements and `for` expressions (rust-lang/reference#663)
- Fix loop expression link. (rust-lang/reference#662)
- async-await initial reference material (rust-lang/reference#635)
- Correct errors in the reference of extern functions definitions and declarations (rust-lang/reference#652)

## rust-by-example

1 commits in 580839d90aacd537f0293697096fa8355bc4e673..e76be6b2dc84c6a992e186157efe29d625e29b94
2019-08-17 23:17:50 -0300 to 2019-09-03 07:42:26 -0300
- Change link to russian translation repository (rust-lang/rust-by-example#1245)

## embedded-book

1 commits in 432ca26686c11d396eed6a59499f93ce1bf2433c..5ca585c4a7552efb546e7681c3de0712f4ae4fdc
2019-08-09 23:20:22 +0000 to 2019-08-27 13:39:14 +0000
- Fixup book CI  (rust-embedded/book#205)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants