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

Handle bindings in substructure of patterns with type ascriptions #55274

Merged

Conversation

pnkfelix
Copy link
Member

This attempts to follow the outline described by @nikomatsakis here. Its a bit more complicated than expected for two reasons:

  1. In general it handles sets of type ascriptions, because such ascriptions can be nested within patterns
  2. It has a separate types in the HAIR, PatternTypeProjections and PatternTypeProjection, which are analogues to the corresponding types in the MIR.

The main reason I added the new HAIR types was because I am worried that the current implementation is inefficent, and asymptotically so: It makes copies of vectors as it descends the patterns, even when those accumulated vectors are never used.

Longer term, I would like to used a linked tree structure for the PatternTypeProjections and PatternTypeProjection, and save the construction of standalone vectors for the MIR types. I didn't want to block landing this on that hypoethetical revision; but I figured I could at least make the future change easier by differentiating between the two types now.

Oh, one more thing: This doesn't attempt to handle ref x (in terms of ensuring that any necessary types are ascribed to x in that scenario as well). We should open an issue to investigate supporting that as well. But I didn't want to block this PR on that future work.

Fix #54570

@pnkfelix pnkfelix force-pushed the issue-54570-proj-path-into-pats-with-type-take-2 branch from 813f5aa to e8c1c8b Compare October 22, 2018 21:53
@rust-highfive
Copy link
Collaborator

r? @zackmdavis

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 22, 2018
@pnkfelix
Copy link
Member Author

r? @nikomatsakis

@pnkfelix pnkfelix added the A-NLL Area: Non Lexical Lifetimes (NLL) label Oct 22, 2018
@pnkfelix
Copy link
Member Author

Oh, also: the patterns.rs should be extended with cases that use enum that are analogous to the current ones that use struct.

If I don't get a chance to add that to this PR before its reviewed, then I will file a follow-up E-needs-test issue documenting the desired test.

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 22, 2018

Oh, and I just realized that I should have looked more carefully at @nikomatsakis 's note a couple more times; namely, he anticipated the issue with PlaceProjection carrying the Ty for things like fields; that's why they pointed us at sanitize_projection

My own code has its own hacked up version of that which only covers a subset of the cases, here:

https://github.com/rust-lang/rust/blob/e8c1c8b0c4b0fa89388771dba41532a0d3e1dfa5/src/librustc/mir/tcx.rs#L47-L69

Now, comparing the above to:

fn field_ty(
&mut self,
parent: &dyn fmt::Debug,
base_ty: PlaceTy<'tcx>,
field: Field,
location: Location,
) -> Result<Ty<'tcx>, FieldAccessError> {
let tcx = self.tcx();
let (variant, substs) = match base_ty {
PlaceTy::Downcast {
adt_def,
substs,
variant_index,
} => (&adt_def.variants[variant_index], substs),
PlaceTy::Ty { ty } => match ty.sty {
ty::Adt(adt_def, substs) if !adt_def.is_enum() => (&adt_def.variants[0], substs),
ty::Closure(def_id, substs) => {
return match substs.upvar_tys(def_id, tcx).nth(field.index()) {
Some(ty) => Ok(ty),
None => Err(FieldAccessError::OutOfRange {
field_count: substs.upvar_tys(def_id, tcx).count(),
}),
}
}
ty::Generator(def_id, substs, _) => {
// Try pre-transform fields first (upvars and current state)
if let Some(ty) = substs.pre_transforms_tys(def_id, tcx).nth(field.index()) {
return Ok(ty);
}
// Then try `field_tys` which contains all the fields, but it
// requires the final optimized MIR.
return match substs.field_tys(def_id, tcx).nth(field.index()) {
Some(ty) => Ok(ty),
None => Err(FieldAccessError::OutOfRange {
field_count: substs.field_tys(def_id, tcx).count(),
}),
};
}
ty::Tuple(tys) => {
return match tys.get(field.index()) {
Some(&ty) => Ok(ty),
None => Err(FieldAccessError::OutOfRange {
field_count: tys.len(),
}),
}
}
_ => {
return Ok(span_mirbug_and_err!(
self,
parent,
"can't project out of {:?}",
base_ty
))
}
},
};
if let Some(field) = variant.fields.get(field.index()) {
Ok(self.cx.normalize(&field.ty(tcx, substs), location))
} else {
Err(FieldAccessError::OutOfRange {
field_count: variant.fields.len(),
})
}
}
}

my main takeaway is that ... my hacked up version does not handle closures nor generators. But ... does it need to??? Can those arise in this context (of user type ascriptions)?

  • Update: My hacked up version is also missing the normalization step that the latter applies to the result of looking up the field ty. So okay, I think at this point I'm better off throwing away the hacked up code and finding a way to utilize the code from sanitize_place.

@pnkfelix pnkfelix force-pushed the issue-54570-proj-path-into-pats-with-type-take-2 branch from e8c1c8b to d5afe62 Compare October 22, 2018 22:43
@rust-highfive
Copy link
Collaborator

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.

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)

@pnkfelix
Copy link
Member Author

Hmm i didn’t see that in my (stage1) builds. Will investigate.

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 23, 2018

Okay I've derived a standalone test case from the rustc source. Investigating further.

Minimized test:

#![feature(nll)]

#[derive(Copy, Clone)]
pub struct Spanned<T> {
    pub node: T,
    pub span: Span,
}

pub type Variant = Spanned<VariantKind>;
// #[derive(Clone)] pub struct Variant { pub node: VariantKind, pub span: Span, }

#[derive(Clone)]
pub struct VariantKind { }

#[derive(Copy, Clone)]
pub struct Span;

pub fn variant_to_span(variant: Variant) {
    match variant {
        Variant {
            span: _span,
            ..
        } => { }
    };
}

fn main() { }

@pnkfelix
Copy link
Member Author

Hmm I must be missing a normalization step somewhere in there; my minimized test seems to pivot on whether one is using a type alias or not.

@pnkfelix
Copy link
Member Author

and it also seems to pivot on the particular order I use for the fields in my local definition of struct Spanned<T> ...? I'm starting to think that my hacked up code for projecting out the fields is more buggy than I realized.

@nikomatsakis
Copy link
Contributor

@pnkfelix it wasn't clear to me — is this ready for review? at least partial review, I guess?

@pnkfelix
Copy link
Member Author

@nikomatsakis I think its definitely ready for partial review.

I've already tagged the bit of code that I suspect is most suspicious / in-need-of-revision. Namely, the way I've dealt with field_ty lookup.

But a review of the overall architecture would still be very useful at this point.

@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 23, 2018

(but in case its not clear: It also is slightly buggy. I need to fix it so that the above test works. But my hope is that this will go hand-in-hand with revising the strategy for field_ty it uses)

@pnkfelix
Copy link
Member Author

Oh! I bet the bug is this:
https://github.com/rust-lang/rust/blob/d3c91ae31a0676b2575b83f47252f892b6c25f24/src/librustc_mir/build/matches/mod.rs#L579-L583

namely, the index j is just the index of the subpattern in the HAIR; there's no connection between that and the index of the corresponding field. So dumb!

@pnkfelix
Copy link
Member Author

(and I think the fix is pretty simple; I really should have reviewed HAIR more carefully before I wrote this code.)

@pnkfelix pnkfelix force-pushed the issue-54570-proj-path-into-pats-with-type-take-2 branch from d3c91ae to 1005c56 Compare October 23, 2018 21:46
@pnkfelix
Copy link
Member Author

pnkfelix commented Oct 23, 2018

(implemented aforementioned fix, rebased (so you'll never see what madness I had before), and added the reduced test. Unfortunately I couldn't figure out a way to generalize the test to actually appear relevant to the change here without causing other checks to fire and thus mask the ICE.)

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.

First review — looks good but for the normalization question

src/librustc/mir/mod.rs Show resolved Hide resolved
src/librustc/mir/mod.rs Show resolved Hide resolved
src/librustc/mir/tcx.rs Show resolved Hide resolved
/// `handle_field` callback is used to map a `Field` to its `Ty`;
/// in most cases one can just use the `Ty` stored inline, which
/// is passed as the third parameter to the `handle_field`
/// callback.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we want a convenient wrapper and an "inner" fn that takes the closure?

Copy link
Member Author

@pnkfelix pnkfelix Oct 24, 2018

Choose a reason for hiding this comment

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

probably; will fix.

let mut projected_ty = PlaceTy::from_ty(ty);
for proj in &user_ty.projs {
projected_ty = projected_ty.projection_ty(
tcx, proj, |this, field, &()| this.field_ty(tcx, field));
Copy link
Contributor

Choose a reason for hiding this comment

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

We will need to normalize here, I think

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I thought that might be the case. I haven't tried hard to make a test that exposes the weakness here. (Its a bit hard to make tests that exercise just the code added here...)

but I will look into it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I think this will serve as a basis for a test showing that we need to normalize as we project the fields:

struct Single<T> { value: T }

trait Indirect {
    type U;
}

impl<'a> Indirect for &'a u32 {
    type U = &'a u32;
}

fn static_to_a_to_static_through_struct_2<'a>(_x: &'a u32) -> &'static u32 {
    let Single { value: y }: Single<<&'a u32 as Indirect>::U> = Single { value: &22 };
    y //~ ERROR
}

Copy link
Member Author

Choose a reason for hiding this comment

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

(well, either that, or the example exposes bugs elsewhere. Ha ha ... ha. ha.)

Copy link
Member Author

Choose a reason for hiding this comment

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

right, the previous example looks an awful like an embedding of #54940. So maybe thats not going to get me anywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

(If it comes down to it, I'll happily put in the code to normalize in the callback itself. But I would like to spend a bit longer trying to identify a test case that the current PR breaks on...)

Copy link
Member Author

@pnkfelix pnkfelix Oct 25, 2018

Choose a reason for hiding this comment

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

ah this one looks promising:

trait Indirect { type Assoc; }
struct StaticU32;
impl Indirect for StaticU32 { type Assoc = &'static u32; }

struct Single2<T: Indirect> { value: <T as Indirect>::Assoc }

fn static_no_initializer_2() {
    let x = 22;
    let Single2 { value: mut _y }: Single2<StaticU32>;
    _y = &x; //~ ERROR
}

(it causes an ICE on my PR; and of course it is incorrectly accepted by nightly+NLL)

@pnkfelix pnkfelix force-pushed the issue-54570-proj-path-into-pats-with-type-take-2 branch 3 times, most recently from 3f82689 to b023055 Compare October 25, 2018 13:58
@rust-highfive
Copy link
Collaborator

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:00433623:start=1540476004754957098,finish=1540476061798990958,duration=57044033860
$ 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 SCCACHE_BUCKET=rust-lang-ci-sccache2
---
[00:07:14]    Compiling syntax_ext v0.0.0 (/checkout/src/libsyntax_ext)
[00:07:17] error[E0425]: cannot find value `el` in this scope
[00:07:17]   --> librustc/mir/tcx.rs:83:38
[00:07:17]    |
[00:07:17] 83 |         self.projection_ty_core(tcx, el, |_, _, ty| ty)
[00:07:17]    |                                      ^^ not found in this scope
[00:07:43] error: aborting due to previous error
[00:07:43] 
[00:07:43] For more information about this error, try `rustc --explain E0425`.
[00:07:44] error: Could not compile `rustc`.
[00:07:44] error: Could not compile `rustc`.
[00:07:44] 
[00:07:44] To learn more, run the command again with --verbose.
[00:07:44] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" " jemalloc" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:07:44] expected success, got: exit code: 101
[00:07:44] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1115:9
[00:07:44] travis_fold:end:stage0-rustc

[00:07:44] travis_time:end:stage0-rustc:start=1540476372763269974,finish=1540476535946552647,duration=163183282673


[00:07:44] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:07:44] Build completed unsuccessfully in 0:03:39
[00:07:44] make: *** [all] Error 1
[00:07:44] Makefile:28: recipe for target 'all' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:01ac38e0
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
151412 ./src/tools/clang
149628 ./obj/build/bootstrap/debug/incremental
149120 ./src/llvm-emscripten/test
134172 ./obj/build/bootstrap/debug/incremental/bootstrap-32pr67l4sa8g0
134168 ./obj/build/bootstrap/debug/incremental/bootstrap-32pr67l4sa8g0/s-f61x03xniz-fn1ppn-1wbz4rzqyzyro
107668 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
104700 ./src/tools/lldb
93748 ./src/tools/clang/test
87540 ./obj/build/x86_64-unknown-linux-gnu/stage0-rustc

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
Copy link
Contributor

bors commented Oct 25, 2018

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

@bors
Copy link
Contributor

bors commented Oct 26, 2018

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

It is unused, and would not make sense to maintain in the commits later in this PR.
… *into* a given UserTypeAnnotation.

(That is, it will pull out some component type held or referenced by
the type annotation.)

Note: this still needs to actually do projection itself. That comes in
a later commit
…r type.

I did not think I would need this in the MIR, but in general local
decls are going to need to support this. (That, or we need to be able
define a least-upper-bound for a collection of types encountered via
the pattern compilation.)
Update the existing NLL `patterns.rs` test accordingly.

includes changes addressing review feedback:

 * Added example to docs for `UserTypeProjections` illustrating how we
   build up multiple projections when descending into a pattern with
   type ascriptions.

 * Adapted niko's suggested docs for `UserTypeProjection`.

 * Factored out `projection_ty` from more general `projection_ty_core`
   (as a drive-by, made its callback an `FnMut`, as I discovered later
   that I need that).

 * Add note to docs that `PlaceTy.field_ty(..)` does not normalize its result.

 * Normalize as we project out `field_ty`.
…bed types.

As a drive-by, also added test analogous to existing
static_to_a_to_static_through_tuple, but now apply to a struct instead
of a tuple.
Also added alias `ProjectionKind<'tcx>` for `ProjectionElem<'tcx, (), ()>`.
@pnkfelix pnkfelix force-pushed the issue-54570-proj-path-into-pats-with-type-take-2 branch from b7e3a6c to 639a3ff Compare October 26, 2018 22:01
@pnkfelix
Copy link
Member Author

@bors r=nikomatsakis p=1

@bors
Copy link
Contributor

bors commented Oct 26, 2018

📌 Commit 639a3ff 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 26, 2018
@bors
Copy link
Contributor

bors commented Oct 27, 2018

⌛ Testing commit 639a3ff with merge 10f42cb...

bors added a commit that referenced this pull request Oct 27, 2018
…type-take-2, r=nikomatsakis

Handle bindings in substructure of patterns with type ascriptions

This attempts to follow the outline described by @nikomatsakis [here](#47184 (comment)). Its a bit more complicated than expected for two reasons:

 1. In general it handles sets of type ascriptions, because such ascriptions can be nested within patterns
 2.  It has a separate types in the HAIR, `PatternTypeProjections` and `PatternTypeProjection`, which are analogues to the corresponding types in the MIR.

The main reason I added the new HAIR types was because I am worried that the current implementation is inefficent, and asymptotically so: It makes copies of vectors as it descends the patterns, even when those accumulated vectors are never used.

Longer term, I would like to used a linked tree structure for the `PatternTypeProjections` and `PatternTypeProjection`, and save the construction of standalone vectors for the MIR types. I didn't want to block landing this on that hypoethetical revision; but I figured I could at least make the future change easier by differentiating between the two types now.

Oh, one more thing: This doesn't attempt to handle `ref x` (in terms of ensuring that any necessary types are ascribed to `x` in that scenario as well). We should open an issue to investigate supporting that as well. But I didn't want to block this PR on that future work.

Fix #54570
@bors
Copy link
Contributor

bors commented Oct 27, 2018

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

@bors bors mentioned this pull request Oct 27, 2018
@bors bors merged commit 639a3ff into rust-lang:master Oct 27, 2018
bors added a commit that referenced this pull request Oct 29, 2018
[beta]: Prepare the 1.31.0 beta release

* Update to Cargo's branched 1.31.0 branch
* Update the channel to `beta`

Rolled up beta-accepted PRs:

* #55362: Remove `cargo new --edition` from release notes.
* #55325: Fix link to macros chapter
* #55358: Remove redundant clone (2)
* #55346: Shrink `Statement`.
* #55274: Handle bindings in substructure of patterns with type ascriptions
* #55297: Partial implementation of uniform paths 2.0 to land before beta
* #55192: Fix ordering of nested modules in non-mod.rs mods
* #55185: path suggestions in Rust 2018 should point out the change in semantics
* #55423: back out bogus `Ok`-wrapping suggestion on `?` arm type mismatch

Note that **this does not update the bootstrap compiler** due to #55404
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NLL Area: Non Lexical Lifetimes (NLL) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants