handle elision in async fn correctly #63499
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nikomatsakis
force-pushed
the
issuee-63388-async-fn-elision-self-mut-self
branch
from
19c04a7
to
9ed22c8
Compare
rust-highfive
added
the
S-waiting-on-review
cramertj
reviewed
Aug 12, 2019
| f | ||
| } | ||
|
|
||
| // Test using `&Self` explicitly: | ||
| // | ||
| // FIXME(#63388). This behavior *ought* to, but does not, |
There was a problem hiding this comment.
ick, that's pretty gnarly, though I suppose backwards-compatible (since we just disallow it, rather than misinterpreting it)
There was a problem hiding this comment.
Yeah...roughly what I was thinking. Not entirely sure this PR is the right thing to do. I'm just assuming that a more correct fix will take more time -- but I think we should definitely not just leave the code like this long term.
cramertj
reviewed
Aug 12, 2019
src/librustc/hir/lowering.rs
Outdated
| // lifetime from `&self` (same for `&mut self`). This | ||
| // will always be the first one we define. | ||
| hir::ImplicitSelfKind::ImmRef | hir::ImplicitSelfKind::MutRef => | ||
| get_elided_lt_replacement( |
There was a problem hiding this comment.
IMO this call is somewhat cleaner when inlined:
let (_span, param) = self.lifetimes_to_define[lifetime_count_before_args];
LtReplacement::Some(param)
|
r=me. This is... not wonderful, but it seems like a clear and simple fix for the most common case. |
|
I'm seeing some test failures that suggest that my assumption that |
|
The job Click to expand the log.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 |
This comment has been minimized.
This comment has been minimized.
nikomatsakis
force-pushed
the
issuee-63388-async-fn-elision-self-mut-self
branch
2 times, most recently
from
bfc95ee
to
b4bc0f4
Compare
|
@cramertj this is ready for re-review |
nikomatsakis
force-pushed
the
issuee-63388-async-fn-elision-self-mut-self
branch
from
b4bc0f4
to
233daaa
Compare
We now always make fresh lifetimne parameters for all elided
lifetimes, whether they are in the inputs or outputs. But then
we generate `'_` in the case of elided lifetimes from the outputs.
Example:
```rust
async fn foo<'a>(x: &'a u32) -> &u32 { .. }
```
becomes
```rust
type Foo<'a, 'b> = impl Future<Output = &'b u32>;
fn foo<'a>(x: &'a u32) -> Foo<'a, '_>
```
nikomatsakis
force-pushed
the
issuee-63388-async-fn-elision-self-mut-self
branch
from
233daaa
to
03e7b96
Compare
nikomatsakis
changed the title
9 tasks
jackh726
reviewed
Aug 12, 2019
Centril
reviewed
Aug 12, 2019
cramertj
reviewed
Aug 12, 2019
| @@ -2622,9 +2570,18 @@ impl<'a> LoweringContext<'a> { | |||
|
|
|||
| self.allocate_hir_id_counter(opaque_ty_node_id); | |||
|
|
|||
| let input_lifetimes_count = self.in_scope_lifetimes.len() + self.lifetimes_to_define.len(); | |||
There was a problem hiding this comment.
This is pretty clever and probably deserves a comment-- that in_scope_lifetimes contains all the non-elided lifetime names in scope, and lifetimes_to_define says how many lifetimes we have been paired with CreateParameter, so the last lifetime_params.len() - (in_scope_lifetimes + lifetimes_to_define) elements of lifetime_params are all elided lifetimes. Obviously the way I just worded it is probably even more confusing than just reading the code, but I think some explanation would be helpful.
|
r=me with comment-- this seems better than the |
|
@cramertj is that comment roughly what you had in mind? |
|
Looks great, thanks! @bors r+ |
|
📌 Commit ad214fe has been approved by |
bors
added
S-waiting-on-bors
|
The job Click to expand the log.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 |
|
@bors r=cramertj |
|
📌 Commit d7c7c52 has been approved by |
|
@bors p=18 |
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 13, 2019
…elision-self-mut-self, r=cramertj
handle elision in async fn correctly
We now always make fresh lifetimne parameters for all elided
lifetimes, whether they are in the inputs or outputs. But then
we generate `'_` in the case of elided lifetimes from the outputs.
Example:
```rust
async fn foo<'a>(x: &'a u32) -> &u32 { .. }
```
becomes
```rust
type Foo<'a, 'b> = impl Future<Output = &'b u32>;
fn foo<'a>(x: &'a u32) -> Foo<'a, '_>
```
Fixes rust-lang#63388
bors
added a commit
that referenced
this pull request
Aug 13, 2019
Rollup of 11 pull requests Successful merges: - #62760 (Deduplicate error messages in `librsctc_mir`) - #62849 (typeck: Prohibit RPIT types that inherit lifetimes) - #63383 (`async fn` lifetime elision tests) - #63421 (Implement Clone, Display for ascii::EscapeDefault) - #63459 (syntax: account for CVarArgs being in the argument list.) - #63475 (Bring back suggestion for splitting `<-` into `< -`) - #63485 (ci: move mirrors to their standalone bucket) - #63486 (Document `From` trait for `BinaryHeap`) - #63488 (improve DiagnosticBuilder docs) - #63493 (Remove unneeded comment in src/libcore/hash/mod.rs) - #63499 (handle elision in async fn correctly) Failed merges: r? @ghost
|
Failed in #63515 (comment), @bors r- |
bors
added
S-waiting-on-author
|
@bors r=cramertj |
|
📌 Commit 18d69c8 has been approved by |
bors
added
S-waiting-on-bors
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 14, 2019
…elision-self-mut-self, r=cramertj
handle elision in async fn correctly
We now always make fresh lifetimne parameters for all elided
lifetimes, whether they are in the inputs or outputs. But then
we generate `'_` in the case of elided lifetimes from the outputs.
Example:
```rust
async fn foo<'a>(x: &'a u32) -> &u32 { .. }
```
becomes
```rust
type Foo<'a, 'b> = impl Future<Output = &'b u32>;
fn foo<'a>(x: &'a u32) -> Foo<'a, '_>
```
Fixes rust-lang#63388
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 14, 2019
…elision-self-mut-self, r=cramertj
handle elision in async fn correctly
We now always make fresh lifetimne parameters for all elided
lifetimes, whether they are in the inputs or outputs. But then
we generate `'_` in the case of elided lifetimes from the outputs.
Example:
```rust
async fn foo<'a>(x: &'a u32) -> &u32 { .. }
```
becomes
```rust
type Foo<'a, 'b> = impl Future<Output = &'b u32>;
fn foo<'a>(x: &'a u32) -> Foo<'a, '_>
```
Fixes rust-lang#63388
bors
added a commit
that referenced
this pull request
Aug 14, 2019
Rollup of 17 pull requests Successful merges: - #62760 (Deduplicate error messages in `librsctc_mir`) - #62849 (typeck: Prohibit RPIT types that inherit lifetimes) - #63383 (`async fn` lifetime elision tests) - #63421 (Implement Clone, Display for ascii::EscapeDefault) - #63459 (syntax: account for CVarArgs being in the argument list.) - #63475 (Bring back suggestion for splitting `<-` into `< -`) - #63485 (ci: move mirrors to their standalone bucket) - #63486 (Document `From` trait for `BinaryHeap`) - #63488 (improve DiagnosticBuilder docs) - #63493 (Remove unneeded comment in src/libcore/hash/mod.rs) - #63499 (handle elision in async fn correctly) - #63501 (use `ParamName` to track in-scope lifetimes instead of Ident) - #63508 (Do not ICE when synthesizing spans falling inside unicode chars) - #63511 (ci: add a check for clock drift) - #63512 (Provide map_ok and map_err method for Poll<Option<Result<T, E>>>) - #63529 (RELEASES.md: ? is one of three Kleene operators) - #63530 (Fix typo in error message.) Failed merges: r? @ghost
bors
added a commit
that referenced
this pull request
Aug 14, 2019
Rollup of 17 pull requests Successful merges: - #62760 (Deduplicate error messages in `librsctc_mir`) - #62849 (typeck: Prohibit RPIT types that inherit lifetimes) - #63383 (`async fn` lifetime elision tests) - #63421 (Implement Clone, Display for ascii::EscapeDefault) - #63459 (syntax: account for CVarArgs being in the argument list.) - #63475 (Bring back suggestion for splitting `<-` into `< -`) - #63485 (ci: move mirrors to their standalone bucket) - #63486 (Document `From` trait for `BinaryHeap`) - #63488 (improve DiagnosticBuilder docs) - #63493 (Remove unneeded comment in src/libcore/hash/mod.rs) - #63499 (handle elision in async fn correctly) - #63501 (use `ParamName` to track in-scope lifetimes instead of Ident) - #63508 (Do not ICE when synthesizing spans falling inside unicode chars) - #63511 (ci: add a check for clock drift) - #63512 (Provide map_ok and map_err method for Poll<Option<Result<T, E>>>) - #63529 (RELEASES.md: ? is one of three Kleene operators) - #63530 (Fix typo in error message.) Failed merges: r? @ghost
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 20, 2019
…amertj
Stabilize `async_await` in Rust 1.39.0
Here we stabilize:
- free and inherent `async fn`s,
- the `<expr>.await` expression form,
- and the `async move? { ... }` block form.
Closes rust-lang#62149.
Closes rust-lang#50547.
All the blockers are now closed.
<details>
- [x] FCP in rust-lang#62149
- [x] rust-lang#61949; PR in rust-lang#62849.
- [x] rust-lang#62517; PR in rust-lang#63376.
- [x] rust-lang#63225; PR in rust-lang#63501
- [x] rust-lang#63388; PR in rust-lang#63499
- [x] rust-lang#63500; PR in rust-lang#63501
- [x] rust-lang#62121 (comment)
- [x] Some tests for control flow (PR rust-lang#63387):
- `?`
- `return` in `async` blocks
- `break`
- [x] rust-lang#61775 (comment), i.e. tests for rust-lang#60944 with `async fn`s instead). PR in rust-lang#63383
</details>
r? @cramertj
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We now always make fresh lifetimne parameters for all elided
lifetimes, whether they are in the inputs or outputs. But then
we generate
'_in the case of elided lifetimes from the outputs.Example:
becomes
Fixes #63388