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 user type annotations in NLL for complex patterns #54570

Closed
nikomatsakis opened this issue Sep 25, 2018 · 6 comments
Closed

handle user type annotations in NLL for complex patterns #54570

nikomatsakis opened this issue Sep 25, 2018 · 6 comments
Assignees
Labels
A-NLL Area: Non-lexical lifetimes (NLL) NLL-sound Working towards the "invalid code does not compile" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

One of the major remaining items for #47184 is that we do not handle user-type annotations around "complex patterns", like let (x, y): (T, U). I described my plan for how to handle this in a comment earlier. The basic idea is that a user type annotation should be tracked as not only a type but a series of projection elements that can be applied to that type. Opening this issue to track this particular change.

@nikomatsakis nikomatsakis added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-NLL Area: Non-lexical lifetimes (NLL) labels Sep 25, 2018
@nikomatsakis nikomatsakis added this to the Edition 2018 RC 2 milestone Sep 25, 2018
@pnkfelix
Copy link
Member

triage of unassigned milestone issues at NLL meeting, assigning to self

@pnkfelix pnkfelix self-assigned this Sep 25, 2018
@pnkfelix pnkfelix added the NLL-sound Working towards the "invalid code does not compile" goal label Oct 2, 2018
@pnkfelix
Copy link
Member

pnkfelix commented Oct 9, 2018

(I'm still working on this. My prototype was hitting an ICE that I believe has been recently resolved by #54757 )

@pnkfelix
Copy link
Member

I am still working on this; my mysteriously stopped working after the rebase that introduced visit_user_ty so I think I just messed up my rebase and need to investigate that properly.

@pnkfelix
Copy link
Member

Still working on this. Decided midway through a rebase that I needed to redo it by hand, but hopefully adopting a less invasive implementation strategy. Will try to report back more over weekend.

@pnkfelix
Copy link
Member

demoting to 2018 release, in accordance with #47184

@pnkfelix
Copy link
Member

pnkfelix commented Oct 22, 2018

(my branch is working again, huzzah. Will hopefully post PR soonish. Its a big one so I'm not sure if it will land in time for the release.)

((As a fun taste of what's to come, here's the main change so far to the relevant ui test:))

src/test/ui/nll/user-annotations/patterns.rs
--- INDEX/src/test/ui/nll/user-annotations/patterns.rs
+++ WORKDIR/src/test/ui/nll/user-annotations/patterns.rs
@@ -9,11 +9,11 @@ fn variable_no_initializer() {
 }

 fn tuple_no_initializer() {
-    // FIXME(#47187): We are not propagating ascribed type through tuples.
+

     let x = 22;
     let (y, z): (&'static u32, &'static u32);
-    y = &x;
+    y = &x; //~ ERROR
 }

 fn ref_with_ascribed_static_type() -> u32 {
@@ -34,11 +34,11 @@ fn ref_with_ascribed_any_type() -> u32 {
 struct Single<T> { value: T }

 fn struct_no_initializer() {
-    // FIXME(#47187): We are not propagating ascribed type through patterns.
+

     let x = 22;
     let Single { value: y }: Single<&'static u32>;
-    y = &x;
+    y = &x; //~ ERROR
 }

 fn variable_with_initializer() {
@@ -91,26 +91,26 @@ fn struct_double_field_underscore_with_initializer() {
 }

 fn static_to_a_to_static_through_variable<'a>(x: &'a u32) -> &'static u32 {
-    // The error in this test is inconsistency with
-    // `static_to_a_to_static_through_tuple`, but "feels right" to
-    // me. It occurs because we special case the single binding case
-    // and force the type of `y` to be `&'a u32`, even though the
-    // right-hand side has type `&'static u32`.
+
+
+
+
+

     let y: &'a u32 = &22;
     y //~ ERROR
 }
 fn static_to_a_to_static_through_tuple<'a>(x: &'a u32) -> &'static u32 {
-    // FIXME(#47187): The fact that this type-checks is perhaps surprising.
-    // What happens is that the right-hand side is constrained to have
-    // type `&'a u32`, which is possible, because it has type
-    // `&'static u32`. The variable `y` is then forced to have type
-    // `&'static u32`, but it is constrained only by the right-hand
-    // side, not the ascribed type, and hence it passes.
+
+
+
+
+
+

     let (y, _z): (&'a u32, u32) = (&22, 44);
-    y
+    y //~ ERROR
 }

 fn a_to_static_then_static<'a>(x: &'a u32) -> &'static u32 {

bors added a commit that referenced this issue 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
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) NLL-sound Working towards the "invalid code does not compile" goal T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

2 participants