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

Allow lifetime elision in `Pin<&(mut) Self>` #61207

Merged
merged 12 commits into from Jul 28, 2019

Conversation

@taiki-e
Copy link
Contributor

commented May 26, 2019

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 #60944

Closes #52675

r? @eddyb
cc @cramertj @Centril @withoutboats @scottmcm

@taiki-e

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

This is a breaking change as said by #60944 (comment) and #60944 (comment).

Then, as @cramertj said in #60944 (comment), a beta backport is probably preferable.

@taiki-e taiki-e force-pushed the taiki-e:arbitrary_self_types-lifetime-elision-2 branch from 1d2494f to eee2005 May 26, 2019

@taiki-e taiki-e force-pushed the taiki-e:arbitrary_self_types-lifetime-elision-2 branch 2 times, most recently from 5edeeaa to 46c752a May 26, 2019

@Centril

This comment has been minimized.

Copy link
Member

commented May 26, 2019

Overall I'm quite unsure about in what direction we should take this and I feel I have a lack of information about what the current rules are. I'd appreciate if someone would do a write-up of all the current rules wrt. lifetime elision.

Ideally, I think if we could get away with just special casing &mut? self without an explicitly written Self type then that would be conceptually simplest to understand. I would be interested in seeing a crater run for that, tho I suspect we cannot get away with it.

@taiki-e

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@Centril

Ideally, I think if we could get away with just special casing &mut? self without an explicitly written Self type then that would be conceptually simplest to understand. I would be interested in seeing a crater run for that, tho I suspect we cannot get away with it.

I also would be interested in seeing this. But what I want to do with this PR is to fix #52675. And I hope this will be beta backported to avoid the impact (see #60944 (comment)).

So, what I'm thinking is not to make big changes to the current rules with this PR, but to split that experiment into different PR (if that is not really necessary to fix #52675).

@Centril

This comment has been minimized.

Copy link
Member

commented May 27, 2019

I also would be interested in seeing this. But what I want to do with this PR is to fix #52675. And I hope this will be beta backported to avoid the impact (see #60944 (comment)).

So, what I'm thinking is not to make big changes to the current rules with this PR, but to split that experiment into different PR (if that is not really necessary to fix #52675).

I'm confused by all of this. The current PR seems to permit more code taking us away from the simpler logic of only assigning special meaning to &self and &mut self arguments when written like so (i.e. not self: &Self). I don't see how we move towards accepting less by accepting more in the interim. Hmm... it seems like we are also rejecting some other code... could we try to not accept more code but strictly reject more code without a feature gate and then move the new logic into a feature gate?

@taiki-e

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@Centril
Oh, sorry, I think I said something weird.
Actually, my concern was not a problem, as we can have a warning period.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 27, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.
travis_time:end:02e4fdde:start=1558972063508831994,finish=1558972150928208358,duration=87419376364
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:07:55]    Compiling syntax_ext v0.0.0 (/checkout/src/libsyntax_ext)
[00:08:06] error[E0392]: parameter `F` is never used
[00:08:06]     --> src/librustc/middle/resolve_lifetime.rs:2122:36
[00:08:06]      |
[00:08:06] 2122 |             struct SelfVisitor<'a, F: FnMut(Res) -> bool> {
[00:08:06]      |                                    ^ unused type parameter
[00:08:06]      = help: consider removing `F` or using a marker such as `std::marker::PhantomData`
[00:08:06] 
[00:08:08] error: aborting due to previous error
[00:08:08] 
---
travis_time:end:10f91ec4:start=1558972661728973986,finish=1558972661733999277,duration=5025291
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:01a21a60
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:021e08d7
travis_time:start:021e08d7
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0a73b77a
$ dmesg | grep -i kill

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)

@taiki-e taiki-e force-pushed the taiki-e:arbitrary_self_types-lifetime-elision-2 branch 4 times, most recently from 1cbcbcf to 914e4a3 May 27, 2019

@eddyb

This comment has been minimized.

Copy link
Member

commented May 28, 2019

I like how simple this is! But we should have a crater run, just to be sure:
@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

⌛️ Trying commit 914e4a3 with merge f939092...

bors added a commit that referenced this pull request May 28, 2019

Auto merge of #61207 - taiki-e:arbitrary_self_types-lifetime-elision-…
…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
@bors

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

☀️ Try build successful - checks-travis
Build commit: f939092

@Centril

This comment has been minimized.

Copy link
Member

commented May 28, 2019

@craterbot run mode=check-only

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

👌 Experiment pr-61207 created and queued.
🤖 Automatically detected try build f939092
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot

This comment has been minimized.

Copy link
Collaborator

commented May 28, 2019

🚧 Experiment pr-61207 is now running on agent aws-3-tmp.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@Centril

This comment has been minimized.

Copy link
Member

commented Jul 27, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

📌 Commit 05f67a2 has been approved by Centril

Centril added a commit to Centril/rust that referenced this pull request Jul 27, 2019

Rollup merge of rust-lang#61207 - taiki-e:arbitrary_self_types-lifeti…
…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

Rollup merge of rust-lang#61207 - taiki-e:arbitrary_self_types-lifeti…
…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

bors added a commit that referenced this pull request Jul 27, 2019

Auto merge of #63052 - Centril:rollup-25wl6pf, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #61207 (Allow lifetime elision in `Pin<&(mut) Self>`)
 - #62771 (Break dependencies between `syntax_ext` and other crates)
 - #62813 (rustc: Compile the `fmt_macros` crate as an rlib)
 - #62949 (Re-enable assertions in PPC dist builder)
 - #62996 (tidy: Add a check for inline unit tests)
 - #63050 (ci: download awscli from our mirror)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Jul 28, 2019

Rollup merge of rust-lang#61207 - taiki-e:arbitrary_self_types-lifeti…
…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

bors added a commit that referenced this pull request Jul 28, 2019

Auto merge of #63057 - Centril:rollup-z3a3c6v, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #61207 (Allow lifetime elision in `Pin<&(mut) Self>`)
 - #62074 (squash of all commits for nth_back on ChunksMut)
 - #62771 (Break dependencies between `syntax_ext` and other crates)
 - #62883 (Refactoring use common code between option, result and accum)
 - #62949 (Re-enable assertions in PPC dist builder)
 - #62996 (tidy: Add a check for inline unit tests)
 - #63038 (Make more informative error on outer attribute after inner)
 - #63050 (ci: download awscli from our mirror)

Failed merges:

r? @ghost

@bors bors merged commit 05f67a2 into rust-lang:master Jul 28, 2019

4 checks passed

pr Build #20190727.5 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details

@taiki-e taiki-e deleted the taiki-e:arbitrary_self_types-lifetime-elision-2 branch Jul 28, 2019

taiki-e added a commit to taiki-e/tokio that referenced this pull request Aug 3, 2019

elide lifetimes
lifetime elision in `Pin<&(mut) Self>` has been allowed in rust-lang/rust#61207.

taiki-e added a commit to taiki-e/tokio that referenced this pull request Aug 3, 2019

elide lifetimes
Lifetime elision in `Pin<&(mut) Self>` has been allowed in rust-lang/rust#61207.

bors added a commit that referenced this pull request Aug 4, 2019

Auto merge of #63262 - pietroalbini:beta-rollup, r=pietroalbini
[beta] Rollup backports

Cherry picked:

*  Updated RELEASES.md for 1.37.0 #63147
*  Require a value for configure --debuginfo-level #62906
*  Make the parser TokenStream more resilient after mismatched delimiter recovery #62887
*  ci: move .azure-pipelines to src/ci/azure-pipelines #63242

Rolled up:

*  [BETA] Update cargo #62911
*  [beta] Backport #61207 #63254

r? @ghost

bors added a commit that referenced this pull request Aug 5, 2019

Auto merge of #63262 - pietroalbini:beta-rollup, r=pietroalbini
[beta] Rollup backports

Cherry picked:

*  Updated RELEASES.md for 1.37.0 #63147
*  Require a value for configure --debuginfo-level #62906
*  Make the parser TokenStream more resilient after mismatched delimiter recovery #62887
*  ci: move .azure-pipelines to src/ci/azure-pipelines #63242

Rolled up:

*  [BETA] Update cargo #62911
*  [beta] Backport #61207 #63254

r? @ghost

taiki-e added a commit to taiki-e/tokio that referenced this pull request Aug 10, 2019

elide lifetimes
Lifetime elision in `Pin<&(mut) Self>` has been allowed in rust-lang/rust#61207.

Centril added a commit to Centril/rust that referenced this pull request Aug 12, 2019

Rollup merge of rust-lang#63383 - Centril:async-lifetime-elision-test…
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj

Centril added a commit to Centril/rust that referenced this pull request Aug 13, 2019

Rollup merge of rust-lang#63383 - Centril:async-lifetime-elision-test…
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj

Centril added a commit to Centril/rust that referenced this pull request Aug 13, 2019

Rollup merge of rust-lang#63383 - Centril:async-lifetime-elision-test…
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 14, 2019

Rollup merge of rust-lang#63383 - Centril:async-lifetime-elision-test…
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj

Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019

Rollup merge of rust-lang#63383 - Centril:async-lifetime-elision-test…
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj

Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019

Rollup merge of rust-lang#63383 - Centril:async-lifetime-elision-test…
…s, r=nikomatsakis

`async fn` lifetime elision tests

Add `async fn` version of the tests in rust-lang#61207 per the first checkbox in rust-lang#62121 (comment).
Works towards resolving blockers in rust-lang#63209.

r? @nikomatsakis
cc @cramertj
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.