[WIP] Rework impl-trait-in-bindings feature #55807

alexreg · 2018-11-09T02:31:17Z

Not nearly ready for merge yet; just putting up here for feedback.

rust-highfive · 2018-11-09T02:50:37Z

The job x86_64-gnu-llvm-5.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:199fc618:start=1541730792869377648,finish=1541730848583495443,duration=55714117795
$ 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
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:07:28]    Compiling syntax_ext v0.0.0 (/checkout/src/libsyntax_ext)
[00:13:01]    Compiling rustc_mir v0.0.0 (/checkout/src/librustc_mir)
[00:13:01]    Compiling rustc_allocator v0.0.0 (/checkout/src/librustc_allocator)
[00:13:46]    Compiling rustc_typeck v0.0.0 (/checkout/src/librustc_typeck)
[00:13:48] error: unused import: `DefIdTree`
[00:13:48]   --> librustc_typeck/check/coercion.rs:71:23
[00:13:48]    |
[00:13:48] 71 | use rustc::ty::{self, DefIdTree, TypeAndMut, Ty, ClosureSubsts};
[00:13:48]    |
[00:13:48]    = note: `-D unused-imports` implied by `-D warnings`
[00:13:48] 
[00:13:53] error: aborting due to previous error

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)

rust-highfive · 2018-11-11T05:27:55Z

The job x86_64-gnu-llvm-5.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:1acb5570:start=1541913243595427232,finish=1541913298056340580,duration=54460913348
$ 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
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:03:41] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:41] tidy error: /checkout/src/librustc_mir/borrow_check/nll/type_check/mod.rs:1938: TODO is deprecated; use FIXME
[00:03:43] some tidy checks failed
[00:03:43] 
[00:03:43] 
[00:03:43] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:43] 
[00:03:43] 
[00:03:43] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:43] Build completed unsuccessfully in 0:00:46
[00:03:43] Build completed unsuccessfully in 0:00:46
[00:03:43] make: *** [tidy] Error 1
[00:03:43] Makefile:79: recipe for target 'tidy' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:335d7356
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Nov 11 05:18:51 UTC 2018
---
travis_time:end:0b775ad0:start=1541913531904230504,finish=1541913531909762286,duration=5531782
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:163fe61e
$ 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:3693e253
travis_time:start:3693e253
$ 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:04cc5c51
$ 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 · 2018-11-11T09:11:34Z

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

nikomatsakis

The NLL type-checker code looks "close but not quite right". The coercion may be being inserted in the wrong place, though, as I describe in my comment.

nikomatsakis · 2018-11-14T21:29:37Z

src/librustc/mir/mod.rs

    UnsafeFnPointer,

+    // "Hide" -- convert a value to an opaque type, i.e. `impl Trait`,
+    // thus hiding information about its conrete type.


Nit: s/conrete/concrete/

nikomatsakis · 2018-11-14T21:42:28Z

src/librustc_mir/borrow_check/nll/type_check/mod.rs

+                        let predicates = match ty.sty {
+                            ty::Opaque(def_id, substs) => {
+                                let bounds = tcx.predicates_of(def_id);
+                                let result = bounds.instantiate(tcx, substs);


use this function instead

fn normalize_and_prove_instantiated_predicates( &mut self, instantiated_predicates: ty::InstantiatedPredicates<'tcx>, locations: Locations, ) {

(it's elsewhere in the file)

Okay looks good.

nikomatsakis · 2018-11-14T21:45:10Z

src/librustc_mir/borrow_check/nll/type_check/mod.rs

+                                // as is done in coercion.rs?
+                                result
+                            }
+                            _ => bug!(),


Will the opaque type always be at the top level? I imagine there might be something like

let x: (impl Debug, impl Debug) = (22, 44);

I think what we want to do here instead is to use instantiate_opaque_types to replace the ty::Opaque with type variables. This will give us back a type that we can unify with the type of the value being casted. It will also give us back the predicates to prove that you can prove using prove_predicates as below.

src/librustc_typeck/check/coercion.rs

nikomatsakis · 2018-11-14T21:49:49Z

src/librustc_typeck/check/coercion.rs

+    fn coerce_hidden(&self, source: Ty<'tcx>, target: Ty<'tcx>) -> CoerceResult<'tcx> {
+        debug!("coerce_hidden(source={:?}, target={:?})", source, target);
+
+        let target_predicates = if let ty::Opaque(def_id, substs) = target.sty {


Hmm, I'm not sure if this is where I think this code should go or not. It depends on whether we want to support e.g. (impl Foo, impl Bar) etc.

One way to go about this would be to look to see whether the type includes any ty::Opaque for which this location is a defining use. I don't remember off the top of my head what the code here looks like, @oli-obk may be able to point us at the right place, else I can dig harder.

But we may want to -- for now, at least -- move this out of the "main coercion" path and instead up to the code that handles a let x: T = ... statement. We would in that case look to see whether T contains any impl Trait statements, much as we do for a function return: if so, we would use instantiate_opaque_types to create a type with fresh variables, and then coerce from the source type into that type. Then we'd take the user's type annotation and return that as the type for the underlying pattern. Something like that.

I feel like this is very analogous to normalization. Ultimately, we'll probably want to be handling it in a "lazy fashion" somehow. Have to think about what that means I guess.

The think that makes me nervous here:

Can this coercion code trigger at times when it shouldn't?

In particular, opaque types play this "dual role", and I'm worried we'll confuse it here.

Example:

fn foo() -> impl Debug { } fn main() { let x = if true { foo() // return type will be a TyOpaque } else { 22 // I think this can "coerce" to the type from the `then` arm }; }

Here, I imagine we should get an error, but we might wind up going down this path and trying to add a Hide annotation or something?

XAMPPRocky · 2018-11-25T15:42:13Z

Triage; @alexreg Hello, have you been able to get back to this PR?

alexreg · 2018-11-30T21:38:55Z

@Aaronepower Working on it slowly... it's a tricky one.

Dylan-DPC-zz · 2019-01-07T07:15:29Z

Ping from triage @alexreg any updates on this? you have some conflicts to deal with.

alexreg · 2019-01-07T23:46:21Z

@Dylan-DPC Been distracted by other PRs lately; I'll be back on this soon. :-)

matprec · 2019-01-17T11:39:24Z

Does this also affect #53984 #53457?

jonhoo · 2019-01-21T17:46:26Z

And #57807?

Centril · 2019-01-25T16:38:45Z

Thanks for the PR @alexreg. Since you're distracted with other PRs, I'll close this one out for now; feel free to reopen once you have time to work on it. :)

alexreg · 2019-01-25T17:22:13Z

@Centril Sure. Indeed I am busy with others right now, but I'll be getting back to it soon (especially when @nikomatsakis gets time to collate his notes and our discussions on this subject).

rust-highfive assigned nikomatsakis Nov 9, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2018

Alexander Regueiro added 3 commits November 11, 2018 05:12

WIP

faed02c

WIP

8dc79e0

Stuff works!

98cec39

alexreg force-pushed the fix-impl-trait-in-bindings branch from 423ec56 to 98cec39 Compare November 11, 2018 05:12

Alexander Regueiro added 2 commits November 12, 2018 01:01

WIP

61b6e45

WIP

31de69a

nikomatsakis requested changes Nov 14, 2018

View reviewed changes

XAMPPRocky added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 25, 2018

Centril added the S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. label Jan 25, 2019

Centril closed this Jan 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Rework impl-trait-in-bindings feature #55807

[WIP] Rework impl-trait-in-bindings feature #55807

alexreg commented Nov 9, 2018

rust-highfive commented Nov 9, 2018

rust-highfive commented Nov 11, 2018

bors commented Nov 11, 2018

nikomatsakis left a comment

nikomatsakis Nov 14, 2018

nikomatsakis Nov 14, 2018

alexreg Nov 14, 2018

nikomatsakis Nov 14, 2018

nikomatsakis Nov 14, 2018

nikomatsakis Nov 14, 2018

nikomatsakis Nov 14, 2018

XAMPPRocky commented Nov 25, 2018

alexreg commented Nov 30, 2018

Dylan-DPC-zz commented Jan 7, 2019

alexreg commented Jan 7, 2019

matprec commented Jan 17, 2019 •

edited

Loading

jonhoo commented Jan 21, 2019

Centril commented Jan 25, 2019

alexreg commented Jan 25, 2019

[WIP] Rework impl-trait-in-bindings feature #55807

[WIP] Rework impl-trait-in-bindings feature #55807

Conversation

alexreg commented Nov 9, 2018

rust-highfive commented Nov 9, 2018

rust-highfive commented Nov 11, 2018

bors commented Nov 11, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Nov 14, 2018

Choose a reason for hiding this comment

nikomatsakis Nov 14, 2018

Choose a reason for hiding this comment

alexreg Nov 14, 2018

Choose a reason for hiding this comment

nikomatsakis Nov 14, 2018

Choose a reason for hiding this comment

nikomatsakis Nov 14, 2018

Choose a reason for hiding this comment

nikomatsakis Nov 14, 2018

Choose a reason for hiding this comment

nikomatsakis Nov 14, 2018

Choose a reason for hiding this comment

XAMPPRocky commented Nov 25, 2018

alexreg commented Nov 30, 2018

Dylan-DPC-zz commented Jan 7, 2019

alexreg commented Jan 7, 2019

matprec commented Jan 17, 2019 • edited Loading

jonhoo commented Jan 21, 2019

Centril commented Jan 25, 2019

alexreg commented Jan 25, 2019

matprec commented Jan 17, 2019 •

edited

Loading