Rollup of 5 pull requests #63715
Merged
Rollup of 5 pull requests #63715
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
Signed-off-by: Nick Cameron <[email protected]>
Fixed: error: unnecessary trailing semicolon
Object-lifetime-default elision is distinct from other forms of elision; it always refers to some enclosing lifetime *present in the surrounding type* (e.g., `&dyn Bar` expands to `&'a (dyn Bar + 'a)`. If there is no enclosing lifetime, then it expands to `'static`. Therefore, in an `impl Trait<Item = dyn Bar>` setting, we don't expand to create a lifetime parameter for the `dyn Bar + 'X` bound. It will just be resolved to `'static`. Annoyingly, the responsibility for this resolution is spread across multiple bits of code right now (`middle::resolve_lifetimes`, `lowering`). The lowering code knows that the default is for an object lifetime, but it doesn't know what the correct result would be. Probably this should be fixed, but what we do now is a surgical fix: we have it generate a different result for elided lifetimes in a object context, and then we can ignore those results when figuring out the lifetimes that are captured in the opaque type.
Currently the default is "inherited" from context, so e.g. `&impl Foo<Item = dyn Bar>` would default to `&'x impl Foo<Item = dyn Bar + 'x>`, but this triggers an ICE and is not very consistent. This patch doesn't implement what I expect would be the correct semantics, because those are likely too complex. Instead, it handles what I'd expect to be the common case -- where the trait has no lifetime parameters.
Remove recommendation about idiomatic syntax for Arc::clone I believe we should not make this recommendation. I don't want to argue that `Arc::clone` is less idiomatic than `arc.clone`, but that the choice is not clear cut and that we should not be making this kind of call in the docs. The `.clone()` form has advantages too: it is more succinct, it is more likely to be understood by beginners, and it is more uniform with other `clone` calls, indeed with most other method calls. Whichever approach is better, I think that this discussion belongs in a style guide or textbook, rather than the library docs. We don't talk much about idiomatic code in the docs, this place is pretty exceptional. The recommendation is also not followed in this repo. It is hard to figure out how many calls there are of the `.clone()` form, but there are 1550 uses of `Arc` and only 65 uses of `Arc::clone`. The recommendation has existed for over two years. The recommendation was added in rust-lang#42137, as a result of rust-lang/rfcs#1954. However, note that that RFC was closed because it was not necessary to change the docs (the original RFC proposed a new function instead). So I don't think an RFC is necessary here (and I'm not trying to re-litigate the discussion on that RFC (which favoured `Arc::clone` as idiomatic) in any case). cc @nical (who added the docs in the first place; sorry :-) ) r? @alexcrichton (or someone else on @rust-lang/libs )
…7, r=cramertj use different lifetime name for object-lifetime-default elision Introduce a distinct value for `LifetimeName` to use when this is a object-lifetime-default elision. This allows us to avoid creating incorrect lifetime parameters for the opaque types that result. We really need to overhaul this setup at some point! It's getting increasingly byzantine. But this seems like a relatively... surgical fix. r? @cramertj Fixes rust-lang#62517
Use constraint span when lowering associated types Fix rust-lang#63594. r? @Centril
…estebank Fix suggestion from incorrect `move async` to `async move`. PR for rust-lang#61920. Happy with the test. There must be a better implementation though - possibly a MIR visitor to estabilsh a span that doesn't include the `async` keyword?
Fixed: error: unnecessary trailing semicolon
|
@bors r+ p=5 rollup=never |
|
📌 Commit ac34594 has been approved by |
bors
added
the
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Aug 19, 2019
Rollup of 5 pull requests Successful merges: - #63252 (Remove recommendation about idiomatic syntax for Arc::clone) - #63376 (use different lifetime name for object-lifetime-default elision) - #63620 (Use constraint span when lowering associated types) - #63699 (Fix suggestion from incorrect `move async` to `async move`.) - #63704 ( Fixed: error: unnecessary trailing semicolon) Failed merges: r? @ghost
|
☀️ Test successful - checks-azure |
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.
Successful merges:
move asynctoasync move. #63699 (Fix suggestion from incorrectmove asynctoasync move.)Failed merges:
r? @ghost