Allow lifetime elision in arbitrary_self_types #60944
Closed
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
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Centril
reviewed
May 19, 2019
taiki-e
force-pushed
the
arbitrary_self_types-lifetime-elision
branch
from
f85180e
to
6eb1f35
Compare
|
Seems the current #![allow(dead_code)]
#![feature(arbitrary_self_types)]
struct Foo;
impl Foo {
fn b(self: &Box<Foo>, f: &Foo) -> &Foo { f }
}I expected: or This PR rejects (fixes) this. I feel it is right, but I don't know how far it will affect. |
|
@rustbot modify labels: T-compiler. |
rustbot
added
the
T-compiler
|
It can reproduce even the stable: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=76eefccd7414faab0a51733c20d6b205 |
|
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 |
taiki-e
force-pushed
the
arbitrary_self_types-lifetime-elision
branch
from
3ae4dc9
to
19c7915
Compare
|
r? @Centril |
taiki-e
commented
May 20, 2019
taiki-e
force-pushed
the
arbitrary_self_types-lifetime-elision
branch
2 times, most recently
from
c04f346
to
8efcf41
Compare
cramertj
added
the
T-lang
|
Seems like we should do a crater run-- this is certainly a breaking change, but it's also definitely something we want to fix. cc @rust-lang/lang |
|
It also seems highly likely that this will affect code using futures, which is stabilized on nightly and soon-to-be-beta (1.36), so this also probably wants a beta backport. I don't know that there are significant other libraries making extensive use of this feature, so it seems like we might be able to fix this without an extra warning period. |
|
@bors try (for crater) |
bors
added a commit
that referenced
this pull request
May 20, 2019
… r=<try> Allow lifetime elision in arbitrary_self_types Currently, `self` except `&Self` and `&mut Self` is skipped. By this, other `self`s with lifetime is also ignored. This PR changes it to only skip `Self`, `&Self` and `&mut Self`, and to handle other `self`s like normal arguments. Closes #52675
|
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 |
|
☀️ Try build successful - checks-travis |
|
@craterbot run mode=check-only |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
taiki-e
force-pushed
the
arbitrary_self_types-lifetime-elision
branch
from
c799d4d
to
c0e3422
Compare
bors
added a commit
that referenced
this pull request
May 21, 2019
Move arbitrary self types's tests into ui/self #60944 (comment) r? @Centril
|
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 |
taiki-e
force-pushed
the
arbitrary_self_types-lifetime-elision
branch
from
c0e3422
to
76ab65a
Compare
eddyb
reviewed
May 22, 2019
| @@ -2121,6 +2121,10 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> { | |||
| _ => false, | |||
| }; | |||
|
|
|||
| // Only skip `Self`, `&Self` and `&mut Self`, | |||
| // and to handle other `self`s like normal arguments. | |||
| let mut should_skip = false; | |||
There was a problem hiding this comment.
Maybe rename this to skip_self_arg?
|
This was intentional, in that we didn't want If you start relying on the Ideally elision would've been syntactic from the start (since it's meant to be syntactically intuitive), but we dropped the ball on that. If we do a crater run, I suggest also experimenting with this part commented out: rust/src/librustc/middle/resolve_lifetime.rs Lines 2136 to 2152 in 1cc822c @cramertj Could we special-case |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
Looks like we have an ICE in the only "regression". |
|
@eddyb I'm sorry, I'm not sure I understand-- could you write an example or two of the kind of thing that you wanted to support, but which wouldn't be allowed under this PR? |
|
@eddyb from your comment it seems the concern has to do with self types containing multiple lifetimes. I notice in particular you highlight cases where users have opted for using the actual self type instead of the My naive expectation around this behavior:
|
@withoutboats It probably should've been disallowed, yeah. I'd be curious to see the results of a crater run where that is just banned (I suspect we couldn't "just do that", but it would perhaps be useful to know.) @cramertj Our testsuite might be lacking these examples, but this works on stable: struct Foo<'a>(&'a ());
impl<'a> Foo<'a> {
fn foo<'b>(self: &'b Foo<'a>) -> &() { self.0 }
}
type Alias = Foo<'static>;
impl Alias {
fn bar<'a>(self: &Alias, arg: &'a ()) -> &() { arg }
}And AFAICT, this PR changes the behavior in at least the latter case, as it relies on the (rather brittle) It otherwise uses regular rules, even for the So what I think we should do is change the special-case of |
|
I see that the reference says
Is that an accurate statement of the current rule? Does the |
|
@scottmcm Note that the rule applies to |
|
Closing this PR in favor of #61207. |
bors
added a commit
that referenced
this pull request
May 28, 2019
…2, r=<try> Allow lifetime elision in `Pin<&(mut) Self>` This replaces #60944. ~~This PR changes elision rules to apply `self: &(mut) Self` elision rules even if nested in `Pin`.~~ This PR changes `self: &(mut) S` elision rules to instead visit the type of `self` and look for `&(mut) S` (where `is_self_ty(S)`) within it Closes #52675 r? @eddyb cc @cramertj @Centril @withoutboats @scottmcm
2 tasks
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 26, 2019
…me-elision-2, r=Centril Allow lifetime elision in `Pin<&(mut) Self>` This PR changes `self: &(mut) S` elision rules to instead visit the type of `self` and look for `&(mut) S` (where `is_self_ty(S)`) within it Replaces rust-lang#60944 Closes rust-lang#52675 r? @eddyb cc @cramertj @Centril @withoutboats @scottmcm
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 27, 2019
…me-elision-2, r=Centril Allow lifetime elision in `Pin<&(mut) Self>` This PR changes `self: &(mut) S` elision rules to instead visit the type of `self` and look for `&(mut) S` (where `is_self_ty(S)`) within it Replaces rust-lang#60944 Closes rust-lang#52675 r? @eddyb cc @cramertj @Centril @withoutboats @scottmcm
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 27, 2019
…me-elision-2, r=Centril Allow lifetime elision in `Pin<&(mut) Self>` This PR changes `self: &(mut) S` elision rules to instead visit the type of `self` and look for `&(mut) S` (where `is_self_ty(S)`) within it Replaces rust-lang#60944 Closes rust-lang#52675 r? @eddyb cc @cramertj @Centril @withoutboats @scottmcm
Centril
added a commit
to Centril/rust
that referenced
this pull request
Jul 28, 2019
…me-elision-2, r=Centril Allow lifetime elision in `Pin<&(mut) Self>` This PR changes `self: &(mut) S` elision rules to instead visit the type of `self` and look for `&(mut) S` (where `is_self_ty(S)`) within it Replaces rust-lang#60944 Closes rust-lang#52675 r? @eddyb cc @cramertj @Centril @withoutboats @scottmcm
9 tasks
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.
Currently,
selfexcept&Selfand&mut Selfis skipped. By this, otherselfs with lifetime is also ignored.This PR changes it to only skip
Self,&Selfand&mut Self, and to handle otherselfs like normal arguments.Closes #52675