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

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

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@alexreg
Contributor

alexreg commented Nov 9, 2018

Fixes #54600.

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

r? @nikomatsakis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 9, 2018

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)

alexreg added some commits Nov 9, 2018

@alexreg alexreg force-pushed the alexreg:fix-impl-trait-in-bindings branch from 423ec56 to 98cec39 Nov 11, 2018

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 11, 2018

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

This comment has been minimized.

Contributor

bors commented Nov 11, 2018

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

alexreg added some commits Nov 12, 2018

WIP
@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.

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

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 14, 2018

Contributor

Nit: s/conrete/concrete/

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

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 14, 2018

Contributor

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)

This comment has been minimized.

@alexreg

alexreg Nov 14, 2018

Contributor

Okay looks good.

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

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 14, 2018

Contributor

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.

Show resolved Hide resolved 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 {

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 14, 2018

Contributor

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.

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 14, 2018

Contributor

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.

This comment has been minimized.

@nikomatsakis

nikomatsakis Nov 14, 2018

Contributor

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?

@Aaronepower

This comment has been minimized.

Contributor

Aaronepower commented Nov 25, 2018

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

@alexreg

This comment has been minimized.

Contributor

alexreg commented Nov 30, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment