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

trigger unsized coercions keyed on Sized bounds #56219

Merged
merged 5 commits into from
Dec 20, 2018

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Nov 25, 2018

This PR causes unsized coercions to not be disabled by $0: Unsize<dyn Object> coercion obligations when we have an $0: Sized obligation somewhere.

This should be mostly backwards-compatible, because in these cases not doing the unsize coercion should have caused the $0: Sized obligation to fail.

Note that X: Unsize<dyn Object> obligations can't fail as obligations if X: Sized holds, so this still maintains some version of monotonicity (I think that an unsized coercion can't be converted to no coercion by unifying type variables).

Fixes #49593 (unblocking never_type).

r? @eddyb
cc @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 25, 2018
@arielb1 arielb1 force-pushed the never-coerce-box branch 2 times, most recently from 116ecff to 33db7c0 Compare November 25, 2018 17:51
}
}

/// FIXME: impl Trait why u give me lifetime errors?
Copy link
Member

Choose a reason for hiding this comment

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

What did you try? You might've had to use + Captures<'tcx> etc.

@eddyb
Copy link
Member

eddyb commented Nov 25, 2018

LGTM, r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Nov 25, 2018
@Centril Centril added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Nov 25, 2018
src/librustc_typeck/check/mod.rs Outdated Show resolved Hide resolved
src/test/ui/coercion/coerce-issue-49593-box-never.rs Outdated Show resolved Hide resolved
src/test/ui/coercion/coerce-issue-49593-box-never.rs Outdated Show resolved Hide resolved
src/test/ui/coercion/coerce-issue-49593-box-never.rs Outdated Show resolved Hide resolved
src/test/ui/coercion/coerce-issue-49593-box-never.rs Outdated Show resolved Hide resolved
src/test/ui/coercion/coerce-issue-49593-box-never.rs Outdated Show resolved Hide resolved
src/test/ui/coercion/coerce-issue-49593-box-never.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Nov 25, 2018

This should be mostly backwards-compatible, because in these cases not doing the unsize coercion should have caused the $0: Sized obligation to fail.

We should test that with a crater run?

@arielb1
Copy link
Contributor Author

arielb1 commented Nov 25, 2018

What did you try? You might've had to use + Captures<'tcx> etc.

I first tried to have the function return impl Iterator<Item=ty::PolyTraitRef<'tcx>> + 'b. That gave me the following error:

error[E0700]: hidden type for `impl Trait` captures lifetime that does not appear in bounds  
    --> src/librustc_typeck/check/mod.rs:2788:12
     |
2788 |         -> impl Iterator<Item=ty::PolyTraitRef<'tcx>> + 'b
     |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
note: hidden type `std::iter::FilterMap<std::vec::IntoIter<rustc::traits::Obligation<'tcx, rustc::ty::Predicate<'tcx>>>, check::ObligationMapper<'b, 'gcx, 'tcx>>` captures the lifetime 'gcx as defined on the impl at 2786:10

I then tried to use the impl Iterator<Item=ty::PolyTraitRef<'tcx>> + 'a + 'b + 'gcx + 'tcx syntax, which gave me:

error: unsatisfied lifetime constraints==========================>   ] 106/111: rustc_ty...  
    --> src/librustc_typeck/check/mod.rs:2788:12
     |
2786 | impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
     |          ----  ---- lifetime `'tcx` defined here
     |          |
     |          lifetime `'gcx` defined here
2787 |     fn obligations_for_self_ty<'b>(&'b self, self_ty: ty::TyVid)
2788 |         -> impl Iterator<Item=ty::PolyTraitRef<'tcx>> + 'a + 'b + 'gcx + 'tcx
     |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ requires that `'tcx` must outlive `'gcx`

error: unsatisfied lifetime constraints
    --> src/librustc_typeck/check/mod.rs:2795:9
     |
2786 |   impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
     |        --  ---- lifetime `'gcx` defined here
     |        |
     |        lifetime `'a` defined here
...
2795 | /         self.fulfillment_cx
2796 | |             .borrow()
2797 | |             .pending_obligations()
2798 | |             .into_iter()
2799 | |             .filter_map(ObligationMapper(self, ty_var_root))
     | |____________________________________________________________^ returning this value requires that `'a` must outlive `'gcx`

error: unsatisfied lifetime constraints
    --> src/librustc_typeck/check/mod.rs:2795:9
     |
2786 |   impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
     |        -- lifetime `'a` defined here
2787 |       fn obligations_for_self_ty<'b>(&'b self, self_ty: ty::TyVid)
     |                                  -- lifetime `'b` defined here
...
2795 | /         self.fulfillment_cx
2796 | |             .borrow()
2797 | |             .pending_obligations()
2798 | |             .into_iter()
2799 | |             .filter_map(ObligationMapper(self, ty_var_root))
     | |____________________________________________________________^ returning this value requires that `'b` must outlive `'a`

Of course, my original design had closures instead of a struct implementing FnMut, but when I didn't have issues with move they gave a similar set of errors.

@eddyb

It would be nice if you could pinpoint a particular shortcoming with impl Trait that led to this problem.

@eddyb
Copy link
Member

eddyb commented Nov 25, 2018

You need to write something like this (see #34511 (comment) for more details):

impl Iterator<Item=ty::PolyTraitRef<'tcx>> + Captures<'gcx> + 'b

In general, add Captures<'x> for every "hidden type ... captures the lifetime 'x".

@rust-highfive

This comment has been minimized.

@arielb1
Copy link
Contributor Author

arielb1 commented Nov 27, 2018

So does this require a lang-team FCP?

@Centril
Copy link
Contributor

Centril commented Nov 27, 2018

@arielb1 I think if not FCP we should at least discuss it (and so I've nominated it) since this has user observable type system changes.

@nikomatsakis
Copy link
Contributor

We had a discussion in the @rust-lang/lang meeting and concluded that we should go ahead with this change, at least in some form. Although we are now taking more things into consideration for coercions (namely, pending coercions) and that's a bit risky, it's nice to fix a real problem, and this doesn't seem like a "fundamental complexity change" in the coercion rules. They've always taken into account "information gained from type-checking thus far" and now we are just expanding that to the "complete set of information" (i.e. not just equality constraints, but arbitrary obligations).

I'd still personally like to spend some time tomorrow to think over some examples and try to be careful about precisely which obligations we consider, but the lang team is 👍 on the general direction thus far. =)

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

OK, I read over the changes, which look good.

My only real hesitation is whether we want to try to be "intensional" about the set of obligations we consider for this role -- i.e., to trace the set of obligations that arise from the surrounding context in the same way we trace the 'expected type'. I believe that the existing closure checking code, which also uses obligations, was sort of aiming for this sort of limitation by tying itself to the case where (a) you have an expected type of an inference variable and (b) you have obligations directly referencing that variable. Pretty hacky, though.

I want to experiment a bit with this branch -- which for example checks for unifications of the expected type inference variable -- to see which things work and which things don't, and if there's much real change. I'm building a local copy for this purpose.

proj_predicate
)
})
Some(()).filter(|()| {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose of this construct? I suspect there's a clearer one out there =)

@arielb1
Copy link
Contributor Author

arielb1 commented Dec 1, 2018

My only real hesitation is whether we want to try to be "intensional" about the set of obligations we consider for this role -- i.e., to trace the set of obligations that arise from the surrounding context in the same way we trace the 'expected type'. I believe that the existing closure checking code, which also uses obligations, was sort of aiming for this sort of limitation by tying itself to the case where (a) you have an expected type of an inference variable and (b) you have obligations directly referencing that variable. Pretty hacky, though.

Note that the obligations here are on the "found type" (i.e., the result of Box::new, which will be Box<$0> where $0: Sized), not on the expected type.

Consider cases like

fn let_expansion() -> Box<Error> {
    let x = Box::new(!);
    x /* should this work? */
}

My personal expectation is that this code will work (BTW, it does not work ATM because of subtyping (the "is sized" relationship doesn't propagate through that) - should I change that?), with x getting the type Box<!> - the "found type" information is not destroyed when you go out of a let.

(b) you have obligations directly referencing that variable

I added the ty_var_root normalization to the closure code too. Which type variable is selected as a root when unifying is sufficiently erratic that I don't think relying on it will do any good. If there's an edge case where you do want to rely on it, maybe a test/comment would be better?

@bors
Copy link
Contributor

bors commented Dec 8, 2018

☔ The latest upstream changes (presumably #56578) made this pull request unmergeable. Please resolve the merge conflicts.

@cramertj
Copy link
Member

@nikomatsakis

I want to experiment a bit with this branch -- which for example checks for unifications of the expected type inference variable -- to see which things work and which things don't, and if there's much real change. I'm building a local copy for this purpose.

Ping on this-- did you have a chance to experiment? I'd love to see this land, since it unblocks both never_type and TryFrom/TryInto traits, AFAICT.

@bors
Copy link
Contributor

bors commented Dec 16, 2018

☔ The latest upstream changes (presumably #56875) made this pull request unmergeable. Please resolve the merge conflicts.

This PR causes unsized coercions to not be disabled by `$0: Unsize<dyn
Object>` coercion obligations when we have an `$0: Sized` obligation
somewhere.

Note that `X: Unsize<dyn Object>` obligations can't fail *as
obligations* if `X: Sized` holds, so this still maintains some version
of monotonicity (I think that an unsized coercion can't be converted to
no coercion by unifying type variables).

Fixes rust-lang#49593 (unblocking never_type).
@bors
Copy link
Contributor

bors commented Dec 18, 2018

📌 Commit eeb8c8d has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 18, 2018
@arielb1
Copy link
Contributor Author

arielb1 commented Dec 19, 2018

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 19, 2018

📌 Commit 5b74438 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 19, 2018

⌛ Testing commit 5b74438 with merge 3b1845c...

bors added a commit that referenced this pull request Dec 19, 2018
trigger unsized coercions keyed on Sized bounds

This PR causes unsized coercions to not be disabled by `$0: Unsize<dyn
Object>` coercion obligations when we have an `$0: Sized` obligation somewhere.

This should be mostly backwards-compatible, because in these cases not doing the unsize coercion should have caused the `$0: Sized` obligation to fail.

Note that `X: Unsize<dyn Object>` obligations can't fail *as obligations* if `X: Sized` holds, so this still maintains some version of monotonicity (I think that an unsized coercion can't be converted to no coercion by unifying type variables).

Fixes #49593 (unblocking never_type).

r? @eddyb
cc @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 19, 2018

💔 Test failed - status-travis

@rust-highfive
Copy link
Collaborator

The job dist-i686-freebsd 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.
Building stage2 tool cargo (i686-unknown-freebsd)
[01:03:50]  Downloading crates ...
[01:04:10] warning: spurious network error (2 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
[01:04:30] warning: spurious network error (1 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
[01:04:50] error: failed to download from `https://crates.io/api/v1/crates/openssl-src/111.1.0+1.1.1a/download`
[01:04:50] Caused by:
[01:04:50]   [6] Couldn't resolve host name (Could not resolve host: crates.io)
[01:04:50] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "i686-unknown-freebsd" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/checkout/src/tools/cargo/Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json"
[01:04:50] expected success, got: exit code: 101
---
travis_time:end:0a564182:start=1545201916212762147,finish=1545201916220909275,duration=8147128
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0da8fb4e
$ 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:0e53a404
travis_time:start:0e53a404
$ 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:05b40c91
$ 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)

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 19, 2018
@kennytm
Copy link
Member

kennytm commented Dec 19, 2018

@bors retry travis-ci/travis-ci#9696

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2018
@bors
Copy link
Contributor

bors commented Dec 19, 2018

⌛ Testing commit 5b74438 with merge cb5e1d32cf5d87c40fa31cf876fd394b12861a1b...

@bors
Copy link
Contributor

bors commented Dec 19, 2018

💔 Test failed - status-travis

@rust-highfive
Copy link
Collaborator

The job dist-x86_64-apple-alt 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.
[00:02:24]       Memory: 8 GB
[00:02:24]       Boot ROM Version: VMW71.00V.0.B64.1704110547
[00:02:24]       Apple ROM Info: [MS_VM_CERT/SHA1/27d66596a61c48dd3dc7216fd715126e33f59ae7]Welcome to the Virtual Machine
[00:02:24]       SMC Version (system): 2.8f0
[00:02:24]       Serial Number (system): VMFGEGuV3OlF
[00:02:24] 
[00:02:24] hw.ncpu: 4
[00:02:24] hw.byteorder: 1234
[00:02:24] hw.memsize: 8589934592
---
Building stage2 tool cargo (x86_64-apple-darwin)
[00:46:48]  Downloading crates ...
[00:46:48] warning: spurious network error (2 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
[00:46:48] warning: spurious network error (1 tries remaining): [6] Couldn't resolve host name (Could not resolve host: crates.io)
[00:46:48] error: failed to download from `https://crates.io/api/v1/crates/openssl-src/111.1.0+1.1.1a/download`
[00:46:48] Caused by:
[00:46:48]   [6] Couldn't resolve host name (Could not resolve host: crates.io)
[00:46:48] command did not execute successfully: "/Users/travis/build/rust-lang/rust/build/x86_64-apple-darwin/stage0/bin/cargo" "build" "--target" "x86_64-apple-darwin" "-j" "4" "--release" "--locked" "--color" "always" "--manifest-path" "/Users/travis/build/rust-lang/rust/src/tools/cargo/Cargo.toml" "--features" "rustc-workspace-hack/all-static" "--message-format" "json"
[00:46:48] expected success, got: exit code: 101
[00:46:48] expected success, got: exit code: 101
[00:46:48] failed to run: /Users/travis/build/rust-lang/rust/build/bootstrap/debug/bootstrap build
[00:46:48] Build completed unsuccessfully in 0:43:24
[00:46:48] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:12ae13ad
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Wed Dec 19 18:36:16 GMT 2018
---
travis_fold:start:after_failure.2
travis_time:start:0f182781
$ ls -lat $HOME/Library/Logs/DiagnosticReports/
total 0
drwx------+ 15 travis  staff  510 Jan 25  2018 ..
drwx------   2 travis  staff   68 Dec  6  2017 .
travis_fold:end:after_failure.2
travis_fold:start:after_failure.3
travis_time:start:16177b9b
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
$ find $HOME/Library/Logs/DiagnosticReports -type f -name '*.crash' -not -name '*.stage2-*.crash' -not -name 'com.apple.CoreSimulator.CoreSimulatorService-*.crash' -exec printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" {} \; -exec head -750 {} \; -exec echo travis_fold":"end:crashlog \; || true
travis_time:end:16177b9b:start=1545244582136754000,finish=1545244582150623000,duration=13869000
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1e679175
$ 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:05a04a88
travis_time:start:05a04a88
$ 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:0a9c5e6e
$ dmesg | grep -i kill
$ dmesg | grep -i kill
Unable to obtain kernel buffer: Operation not permitted
usage: sudo dmesg
travis_fold:end:after_failure.6

Done. Your build exited with 1.

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)

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 19, 2018
@arielb1
Copy link
Contributor Author

arielb1 commented Dec 19, 2018

[00:46:48] error: failed to download from `https://crates.io/api/v1/crates/openssl-src/111.1.0+1.1.1a/download`
[00:46:48] Caused by:
[00:46:48]   [6] Couldn't resolve host name (Could not resolve host: crates.io)

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 19, 2018
@bors
Copy link
Contributor

bors commented Dec 20, 2018

⌛ Testing commit 5b74438 with merge e42247f...

bors added a commit that referenced this pull request Dec 20, 2018
trigger unsized coercions keyed on Sized bounds

This PR causes unsized coercions to not be disabled by `$0: Unsize<dyn
Object>` coercion obligations when we have an `$0: Sized` obligation somewhere.

This should be mostly backwards-compatible, because in these cases not doing the unsize coercion should have caused the `$0: Sized` obligation to fail.

Note that `X: Unsize<dyn Object>` obligations can't fail *as obligations* if `X: Sized` holds, so this still maintains some version of monotonicity (I think that an unsized coercion can't be converted to no coercion by unifying type variables).

Fixes #49593 (unblocking never_type).

r? @eddyb
cc @nikomatsakis
@bors
Copy link
Contributor

bors commented Dec 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing e42247f to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants