[NLL] Check user types are well-formed #54942

matthewjasper · 2018-10-09T22:04:45Z

Also contains a change of span for AscribeUserType.
I'm not quite sure if this was what @nikomatsakis was thinking.

nikomatsakis · 2018-10-09T22:07:06Z

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

@@ -90,8 +90,7 @@ pub(super) fn relate_type_and_user_type<'tcx>(
        NllTypeRelatingDelegate::new(infcx, borrowck_context, locations, category),
        v1,
        b_variables,
-    ).relate(&b_value, &a)?;
-    Ok(())
+    ).relate(&b_value, &a).map_err(From::from)


This is indeed not quite what I had in mind. I was imagining something more like this.

let mut relating = TypeRelating::new(...); relating.relate(&b_value, &a)?; let canonical_var_values = CanonicalVarValues { var_values: relating.canonical_var_values }; Ok(b.substitute(&canonical_var_values))

what this would do is to take the canonical type and replace all the "canonical variables" with the values that we inferred during relate_tys. It would not require the changes to "propagate back" a correct type. Those make me a bit nervous, as it's not really obvious what to return for a <: b. And as a case in point, in the case of handling higher-ranked things, we convert a == b to a <: b and b <: a, so which return value should we give back? etc

nikomatsakis · 2018-10-09T22:08:04Z

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

@@ -482,8 +481,10 @@ where
        // Watch out for the case that we are matching a `?T` against the
        // right-hand side.
        if let ty::Infer(ty::CanonicalTy(var)) = a.sty {
-            self.relate_var(var, b.into())?;
-            Ok(a)
+            self.relate_var(var, b.into()).map(|kind| match kind.unpack() {


I would prefer to use ? here, is there a reason not to?

(I just find it much easier to read)

e.g.,

match self.relate_var(...)?.unpack() { UnpackedKind::Ty(ty) => Ok(ty), ... }

nikomatsakis · 2018-10-09T22:09:13Z

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

@@ -500,8 +501,10 @@ where
        b: ty::Region<'tcx>,
    ) -> RelateResult<'tcx, ty::Region<'tcx>> {
        if let ty::ReCanonical(var) = a {
-            self.relate_var(*var, b.into())?;
-            return Ok(a);
+            return self.relate_var(*var, b.into()).map(|kind| match kind.unpack() {


also here I prefer ?

rust-highfive · 2018-10-09T23:14:13Z

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.

[00:54:02] .................................................................................................... 2200/4578
[00:54:07] ......i............................................................................................. 2300/4578
[00:54:11] .................................................................................................... 2400/4578
[00:54:14] .................................................................................................... 2500/4578
[00:54:18] ...................iiiiiiiii........................................................................ 2600/4578
[00:54:24] .................................................................................................... 2800/4578
[00:54:28] .................................................................................................... 2900/4578
[00:54:31] .......................................i............................................................ 3000/4578
[00:54:34] ...................................................................................................i 3100/4578
---
travis_time:start:test_mir-opt
Check compiletest suite=mir-opt mode=mir-opt (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:07:22] 
[01:07:22] running 47 tests
[01:07:22] ERROR 2018-10-09T23:13:49Z: compiletest::runtest: None
[01:07:41]        AscribeUserType(_4, o, Canonical { variables: [], value: std::option::Option<std::boxed::Box<u32>> });
[01:07:41]        FakeRead(ForLet, _4);
[01:07:41]        StorageLive(_5);
[01:07:41]        StorageLive(_6);
[01:07:41]        _6 = move _4;
[01:07:41]        replace(_5 <- move _6) -> [return: bb2, unwind: bb5];
[01:07:41] ... (elided)
[01:07:41]    bb2: {
[01:07:41]    bb2: {
[01:07:41]        drop(_6) -> [return: bb6, unwind: bb4];
[01:07:41] ... (elided)
[01:07:41]    bb5: {
[01:07:41]        drop(_6) -> bb4;
[01:07:41]    }
[01:07:41]    }
[01:07:41] Actual:
[01:07:41] fn main() -> (){
[01:07:41]     let mut _0: ();
[01:07:41]     scope 1 {
[01:07:41]         scope 3 {
[01:07:41]             scope 5 {
[01:07:41]                 scope 7 {
[01:07:41]                 scope 8 {
[01:07:41]                 scope 8 {
[01:07:41]                     let _5: std::option::Option<std::boxed::Box<u32>>;
[01:07:41]             }
[01:07:41]             scope 6 {
[01:07:41]             scope 6 {
[01:07:41]                 let _4: std::option::Option<std::boxed::Box<u32>> as Canonical { variables: [], value: std::option::Option<std::boxed::Box<u32>> };
[01:07:41]         }
[01:07:41]         scope 4 {
[01:07:41]         scope 4 {
[01:07:41]             let _2: bool;
[01:07:41]     }
[01:07:41]     scope 2 {
[01:07:41]     scope 2 {
[01:07:41]         let _1: bool;
[01:07:41]     }
[01:07:41]     let mut _3: bool;
[01:07:41]     let mut _6: std::option::Option<std::boxed::Box<u32>>;
[01:07:41]     bb0: {                              
[01:07:41]         StorageLive(_1);
[01:07:41]         _1 = const fal1:07:41] expected success, got: exit code: 101
[01:07:41] 
[01:07:41] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:07:41] Build completed unsuccessfully in 0:18:27
[01:07:41] Build completed unsuccessfully in 0:18:27
[01:07:41] Makefile:58: recipe for target 'check' failed
[01:07:41] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:26a38a40
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

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)

Also change the order of the fake read for let and the AscribeUserType, so that we use the better span and message from the fake read in errors.

matthewjasper · 2018-10-10T11:04:22Z

Comments addressed and tests fixed

nikomatsakis

r=me with a FIXME comment

nikomatsakis · 2018-10-10T12:58:30Z

src/test/ui/regions/regions-assoc-type-in-supertrait-outlives-container.rs

@@ -49,7 +47,8 @@ fn with_assoc<'a,'b>() {
    // outlive 'a. In this case, that means TheType<'b>::TheAssocType,
    // which is &'b (), must outlive 'a.

-    let _: &'a WithAssoc<TheType<'b>> = loop { }; //~ ERROR reference has a longer lifetime
+    let _x: &'a WithAssoc<TheType<'b>> = loop { };
+    //~^ ERROR reference has a longer lifetime


Can we ope an issue regarding enforcing WF conditions in dead code and put a FIXME here like this:

FIXME(#XXX) -- NLL doesn't enforce WF conditions (or any type annotations) in dead code.

Also update some tests so that they don't have user types on `_` in unreachable code.

matthewjasper · 2018-10-10T15:25:46Z

@bors r=nikomatsakis

bors · 2018-10-10T15:25:47Z

📌 Commit c312e04 has been approved by nikomatsakis

blitzerr

@matthewjasper
**This is just for the purposes of my understanding :)

Can you explain why this change was required at all? What would be the repercussions, if as part of the issue, the only commit was the second commit ?

blitzerr · 2018-10-12T04:30:07Z

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

@@ -970,15 +970,21 @@ impl<'a, 'gcx, 'tcx> TypeChecker<'a, 'gcx, 'tcx> {
        locations: Locations,
        category: ConstraintCategory,
    ) -> Fallible<()> {
-        relate_tys::relate_type_and_user_type(
+        let ty = relate_tys::relate_type_and_user_type(


It is okay to have a variable name the same as the module name ?

blitzerr · 2018-10-12T04:35:51Z

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

@@ -85,13 +85,24 @@ pub(super) fn relate_type_and_user_type<'tcx>(
    // variance to get the right relationship.
    let v1 = ty::Contravariant.xform(v);

-    TypeRelating::new(
+    let mut type_relating = TypeRelating::new(
        infcx.tcx,
        NllTypeRelatingDelegate::new(infcx, borrowck_context, locations, category),
        v1,
        b_variables,


b_variables
Can you explain to me how this magic happens ? b.variables is written as b_variables ?

b is destructured just above this.

Would you mind pointing me to the line please ?

blitzerr · 2018-10-12T04:37:22Z

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

-    ).relate(&b_value, &a)?;
-    Ok(())
+    );
+    type_relating.relate(&b_value, &a)?;


type_relating.relate(&b_value, &a)?;

Can you explain to me what this line does ?

It checks that a and b_value are the same type apart from lifetimes, if they're not it returns an error to the caller. Otherwise, depending on v1, it adds any outlives constraints that are required for a being a subtype of b, or b being a subtype of a or b being the same type as a to the borrowck_context (if it's Some, which it will be during the NLL type check). Finally it works out what any type variables in b should be based on a.

So if b is &mut '_1 &'_2 _, a is &mut '_3 &'_4 i32 and v1is Covariant, the constraints added are'_1: '_3, '_2: '_4and'_4: '_2(the last constraint comes from&mut Tbeing invariant inT), and the type variable is inferred to be i32`.

@matthewjasper Thanks a lot for the explanation. Really appreciate it.

matthewjasper · 2018-10-12T08:38:22Z

What would be the repercussions, if as part of the issue, the only commit was the second commit ?

The first commit is a somewhat unrelated diagnostics change. If it wasn't there then the error messages would point to the variables instead of the type annotation.

bors · 2018-10-12T21:28:50Z

⌛ Testing commit c312e04 with merge 2c2e2c5...

@nikomatsakis

[NLL] Check user types are well-formed Also contains a change of span for AscribeUserType. I'm not quite sure if this was what @nikomatsakis was thinking. Closes #54620 r? @nikomatsakis

bors · 2018-10-12T23:59:20Z

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

matthewjasper added the A-NLL Area: Non-lexical lifetimes (NLL) label Oct 9, 2018

rust-highfive assigned nikomatsakis Oct 9, 2018

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

matthewjasper mentioned this pull request Oct 9, 2018

NLL doesn't check that user type annotations are well-formed in unreachable code #54943

Closed

nikomatsakis requested changes Oct 9, 2018

View reviewed changes

matthewjasper force-pushed the wf-type-annotations branch from 17596a8 to 77b0d1b Compare October 10, 2018 10:50

Use the span of the user type for AscribeUserType

55ec104

Also change the order of the fake read for let and the AscribeUserType, so that we use the better span and message from the fake read in errors.

matthewjasper force-pushed the wf-type-annotations branch from 77b0d1b to 8c99712 Compare October 10, 2018 11:02

nikomatsakis approved these changes Oct 10, 2018

View reviewed changes

Check user types are well-formed in MIR borrow check

c312e04

Also update some tests so that they don't have user types on `_` in unreachable code.

matthewjasper force-pushed the wf-type-annotations branch from 8c99712 to c312e04 Compare October 10, 2018 15:24

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 Oct 10, 2018

blitzerr reviewed Oct 12, 2018

View reviewed changes

bors merged commit c312e04 into rust-lang:master Oct 12, 2018

matthewjasper deleted the wf-type-annotations branch October 13, 2018 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NLL] Check user types are well-formed #54942

[NLL] Check user types are well-formed #54942

matthewjasper commented Oct 9, 2018

nikomatsakis Oct 9, 2018

nikomatsakis Oct 9, 2018

nikomatsakis Oct 9, 2018

nikomatsakis Oct 9, 2018

nikomatsakis Oct 9, 2018

rust-highfive commented Oct 9, 2018

matthewjasper commented Oct 10, 2018

nikomatsakis left a comment

nikomatsakis Oct 10, 2018

matthewjasper commented Oct 10, 2018

bors commented Oct 10, 2018

blitzerr left a comment

blitzerr Oct 12, 2018

matthewjasper Oct 12, 2018

blitzerr Oct 12, 2018

matthewjasper Oct 12, 2018

blitzerr Oct 12, 2018

matthewjasper Oct 13, 2018

blitzerr Oct 12, 2018

matthewjasper Oct 12, 2018 •

edited

Loading

blitzerr Oct 12, 2018

matthewjasper commented Oct 12, 2018 •

edited

Loading

bors commented Oct 12, 2018

bors commented Oct 12, 2018

[NLL] Check user types are well-formed #54942

[NLL] Check user types are well-formed #54942

Conversation

matthewjasper commented Oct 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rust-highfive commented Oct 9, 2018

matthewjasper commented Oct 10, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjasper commented Oct 10, 2018

bors commented Oct 10, 2018

blitzerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjasper Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewjasper commented Oct 12, 2018 • edited Loading

bors commented Oct 12, 2018

bors commented Oct 12, 2018

matthewjasper Oct 12, 2018 •

edited

Loading

matthewjasper commented Oct 12, 2018 •

edited

Loading