[WIP] Maybe initialized liveness #45794

Nashenas88 · 2017-11-05T22:01:08Z

The constraint generation now only adds a drop live constraint if the lvalue may be initialized.

rust-highfive · 2017-11-05T22:01:22Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Nashenas88 · 2017-11-05T22:02:13Z

This is for #45665

Nashenas88 · 2017-11-05T22:02:20Z

r? @nikomatsakis

Nashenas88 · 2017-11-05T22:10:32Z

src/librustc_mir/borrow_check/nll/constraint_generation.rs

@@ -79,8 +88,12 @@ impl<'cx, 'gcx, 'tcx> ConstraintGeneration<'cx, 'gcx, 'tcx> {
                .drop
                .simulate_block(self.mir, bb, |location, live_locals| {
                    for live_local in live_locals.iter() {
-                        let live_local_ty = self.mir.local_decls[live_local].ty;
-                        self.add_drop_live_constraint(live_local_ty, location);
+                        if let LookupResult::Exact(mpi) = self.move_data.rev_lookup.find(&Lvalue::Local(live_local)) {


@nikomatsakis this should always be true. Your comment in #45665 suggested that move_data.move_path_index was somehow available here, but I'm not sure if it's missing something (looks cut off in the comment there) or only worked in earlier versions of the code. It's only available on MoveOutIndex, and I'm not sure how to get this through a local. Should I create a method on MovePathLookup to avoid the if check, or is there something existing that I could have used and just missed?

Nashenas88 · 2017-11-05T22:11:07Z

src/librustc_mir/dataflow/impls/borrows.rs

@@ -38,7 +38,7 @@ pub struct Borrows<'a, 'gcx: 'tcx, 'tcx: 'a> {
    location_map: FxHashMap<Location, BorrowIndex>,
    region_map: FxHashMap<Region<'tcx>, FxHashSet<BorrowIndex>>,
    region_span_map: FxHashMap<RegionKind, Span>,
-    nonlexical_regioncx: Option<&'a RegionInferenceContext>,
+    nonlexical_regioncx: Option<RegionInferenceContext>,


@nikomatsakis I'm not sure if this will be an issue in future work for NLL. Should I add the comments from gitter here?

The problem, I believe, is that FlowInProgress<T> is invariant with respect to T. That means that it doesn't allow any lifetimes in T to be approximated. This is unfortunate, but ok, not much we can do easily do about it. This is also why you have to introduce 'a above, rather than just relying on 'cx.

In any case, because of that, when we have a FlowInProgress<Borrows<'a...>> and a FlowInProgress<MoveUninits<'a...>> in the same function call (as you do now), then the lifetime 'a for both of them has to be precisely the same. This means it has to live as long as the longest thing, which then lives longer than the variable nonlexical_regioncx.

Anyway, taking ownership is actually fine.

oli-obk · 2017-11-06T11:53:18Z

A whole suite of tests fails now. Looks very related, maybe you forgot a check whether mir-borrowck is activated?

[00:48:12]     [compile-fail] compile-fail/E0506.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-assign-comp.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-closures-mut-and-imm.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-describe-lvalue.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-imm-ref-to-mut-rec-field-issue-3162-c.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-init-in-fru.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-lend-flow-match.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-match-already-borrowed.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-move-in-irrefut-pat.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-move-out-of-overloaded-auto-deref.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-overloaded-index-and-overloaded-deref.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-overloaded-index-ref-index.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-pat-reassign-binding.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-unary-move.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-uninit-ref-chain.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-union-borrow.rs
[00:48:12]     [compile-fail] compile-fail/borrowck/borrowck-vec-pattern-move-tail.rs
[00:48:12]     [compile-fail] compile-fail/coerce-overloaded-autoderef.rs
[00:48:12]     [compile-fail] compile-fail/hrtb-identity-fn-borrows.rs
[00:48:12]     [compile-fail] compile-fail/mut-pattern-internal-mutability.rs
[00:48:12]     [compile-fail] compile-fail/regions-pattern-typing-issue-19997.rs

Nashenas88 · 2017-11-06T13:45:12Z

It's definitely related. I'll have to look into this when I get home later tonight.

nikomatsakis

Looks good! I left a few comments, but the biggest thing is that we need a test. I think a ui/nll test would be good -- we should be able to have two functions, one of which errors and one of which doesn't. Something like this:

struct Wrap<'p> { p: &'p mut i32 }

impl Drop for Wrap {
  fn drop(&mut self) {
    *self.p += 1;
  }
}

fn foo() {
    let mut x = 0;
    let wrap = Wrap { p: &mut x };
    x += 1; //~ ERROR because of dtor
}

fn bar() {
    let mut x = 0;
    let wrap = Wrap { p: &mut x };
    mem::drop(wrap);
    x += 1; // OK, drop is inert
}

It'd be great to add more tests that involve fragments, too. e.g. I'd like the following tests. Imagine we have a struct Foo<'p> { a: String, b: Wrap<'p> }. Then:

where we move from foo.a only. In that case, an error should still result, since the dtor of foo.b will still execute.
where we move from foo.a and foo.b and hence no error should be reported
where we move from foo.b but not foo.a; I believe your current code will report an error, which is fine, but we should be able to make it so that no error occurs in the future

nikomatsakis · 2017-11-06T17:25:12Z

src/librustc_mir/borrow_check/mod.rs

-                    todo.push(sibling);
+macro_rules! has_any_child_of_impl {
+    ($MaybeTLvals:ident) => {
+        impl<'b, 'gcx, 'tcx> FlowInProgress<$MaybeTLvals<'b, 'gcx, 'tcx>> {


I think we could forego the macro and do instead:

impl<'tcx, T: HasMoveData<'tcx>> FlowInProgress<T> { ... }

nikomatsakis · 2017-11-06T17:25:48Z

src/librustc_mir/dataflow/impls/borrows.rs

@@ -38,7 +38,7 @@ pub struct Borrows<'a, 'gcx: 'tcx, 'tcx: 'a> {
    location_map: FxHashMap<Location, BorrowIndex>,
    region_map: FxHashMap<Region<'tcx>, FxHashSet<BorrowIndex>>,
    region_span_map: FxHashMap<RegionKind, Span>,
-    nonlexical_regioncx: Option<&'a RegionInferenceContext>,
+    nonlexical_regioncx: Option<RegionInferenceContext>,


The problem, I believe, is that FlowInProgress<T> is invariant with respect to T. That means that it doesn't allow any lifetimes in T to be approximated. This is unfortunate, but ok, not much we can do easily do about it. This is also why you have to introduce 'a above, rather than just relying on 'cx.

In any case, because of that, when we have a FlowInProgress<Borrows<'a...>> and a FlowInProgress<MoveUninits<'a...>> in the same function call (as you do now), then the lifetime 'a for both of them has to be precisely the same. This means it has to live as long as the longest thing, which then lives longer than the variable nonlexical_regioncx.

Anyway, taking ownership is actually fine.

nikomatsakis · 2017-11-06T17:32:44Z

src/librustc_mir/borrow_check/nll/constraint_generation.rs

-    regioncx: &'cx mut RegionInferenceContext,
-    mir: &'cx Mir<'tcx>,
-    liveness: &'cx LivenessResults,
+struct ConstraintGeneration<'a, 'cx: 'a, 'gcx: 'tcx, 'tcx: 'cx> {


Rather than this, let's rename 'a to 'cx and rename 'cx to 'init. My reasoning:

I prefer not to use 'a or other single-letter names, which I want to reserve for things local to a function signature.

I know most of the compiler doesn't follow this naming scheme. =) Gotta start somewhere though!

I prefer for 'cx to be contravariant by convention.

I know the name stinks but it's the one I've been using, we can rename later.

I prefer then to use 'init for the lifetime associated with the MaybeInitializedLvals value.

nikomatsakis · 2017-11-06T17:33:37Z

src/librustc_mir/borrow_check/nll/constraint_generation.rs

+                        if let LookupResult::Exact(mpi) =
+                            self.move_data.rev_lookup.find(&Lvalue::Local(live_local))
+                        {
+                            if self.flow_inits.has_any_child_of(mpi).is_some() {


👍 IFIUK, this is basically the "simple" version, in which we consider the entire variable droppable if any part of it is droppable. Seems good as a starting point.

bors · 2017-11-07T02:12:38Z

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

Nashenas88 · 2017-11-07T04:59:39Z

@oli-obk the tests that are failing already had mir-borrowck activated. @nikomatsakis the current failing tests are all caused because there's now an error reported on the StorageDead mir statements for the same locals that were already expecting the E0506 error. Is this what you meant by:

where we move from foo.b but not foo.a; I believe your current code will report an error, which is fine, but we should be able to make it so that no error occurs in the future

If so, then would the right fix for now be to make those tests pass by adding in the (temporarily expected) error and adding a comment that this should be fixed by a future NLL story (is there a useful name or description I could add there)?

I will work on adding the new tests tomorrow.

Nashenas88

@nikomatsakis This is what I could finish this morning, I'll circle back after work to follow up on fixes for the broken tests and to clean up the new test. Let me know if anything is going in the wrong direction.

Nashenas88 · 2017-11-07T12:53:40Z

src/test/ui/nll/maybe-initialized-drop.rs

+
+fn move_wrap<'p>(_b: Wrap<'p>) { }
+
+fn baz_a() {


@nikomatsakis Is this along the lines of what you were expecting?

yep, but we need a x += 1 at the end of each test to show where we expect (or do not expect) errors to occur, I think

Nashenas88 · 2017-11-07T12:54:30Z

src/test/ui/nll/maybe-initialized-drop.stderr

+   |                          ------ borrow of `x` occurs here
+24 |     x += 1; //~ ERROR because of dtor
+25 | }
+   |  ^ assignment to borrowed `x` occurs here


@nikomatsakis these are the StorageDead statements that are getting errors, this is also what's breaking in the compile-fail tests.

nikomatsakis · 2017-11-07T15:46:00Z

@Nashenas88 I don't expect any mir-borrowck failures with this code -- aren't we still gating on -Znll before we enable non-lexical lifetimes?

nikomatsakis · 2017-11-07T15:48:44Z

src/librustc_mir/borrow_check/mod.rs

+    // contain non-lexical lifetimes. It will have a lifetime tied
+    // to the inference context.
+    let mut mir: Mir<'tcx> = input_mir.clone();
+    let num_region_variables = {


This should be

let num_region_variables = if !tcx.sess.opts.debugging_opts.nll { 0 } else { ...; };

nikomatsakis · 2017-11-07T15:50:59Z

src/librustc_mir/borrow_check/mod.rs

@@ -1467,8 +1481,8 @@ impl<'b, 'gcx, 'tcx> InProgress<'b, 'gcx, 'tcx> {
    }
 }

-impl<'b, 'gcx, 'tcx> FlowInProgress<MaybeUninitializedLvals<'b, 'gcx, 'tcx>> {
-    fn has_any_child_of(&self, mpi: MovePathIndex) -> Option<MovePathIndex> {
+impl<'tcx, T> FlowInProgress<T> where T: HasMoveData<'tcx> + BitDenotation<Idx = MovePathIndex> {


nikomatsakis · 2017-11-07T15:52:03Z

src/librustc_mir/borrow_check/nll/constraint_generation.rs


 use super::subtype;
 use super::LivenessResults;
 use super::ToRegionIndex;
 use super::region_infer::RegionInferenceContext;

-pub(super) fn generate_constraints<'a, 'gcx, 'tcx>(
-    infcx: &InferCtxt<'a, 'gcx, 'tcx>,
+pub(super) fn generate_constraints<'init, 'gcx, 'tcx>(


thanks for obliging me =)

Any time 😄

nikomatsakis · 2017-11-07T16:01:10Z

src/librustc_mir/borrow_check/nll/constraint_generation.rs

-            self.liveness
-                .drop
-                .simulate_block(self.mir, bb, |location, live_locals| {
+            self.liveness.drop.simulate_block(


Before we call simulate_block, we need to do:

self.flow_inits.reset_to_entry_of(bb);

This will reset the state to the entry of the basic block. Heh, I just realized a kind of annoying problem. The flow_inits wants to walk forward, whereas liveness wants to walk backwards. This means we can't readily "update" the flow-in-progress for each step as we go.

I think then that we what we ought to do is to walk the liveness backwards and collect the live variables into a vector. Something like this:

let mut live_locals: Vec<(Location, Vec<Local>)> = vec![]; self.liveness.drop.simulate_block(self.mir, bb, |location, live_locals| { live_locals.push((location, live_locals.iter().collect())); }); self.flow_inits.reset_to_entry_of(bb); for (index, statement) in block_data.statements.iter() { let location = Location { block: bb, statement: index }; let (location2, live_locals) = live_locals.pop().unwrap(); assert_eq!(location, location2); // code you had before: let mpi = self.move_data.rev_lookup.find_local(live_local); if self.flow_inits.has_any_child_of(mpi).is_some() { let live_local_ty = self.mir.local_decls[live_local].ty; self.add_drop_live_constraint(live_local_ty, location); } self.flow_inits.reconstruct_statement_effect(location, statement); } // same as above, but for terminator, probably can attach some kind of helper function

nikomatsakis · 2017-11-07T16:05:35Z

src/test/ui/nll/maybe-initialized-drop.stderr

+error[E0506]: cannot assign to `x` because it is borrowed (Mir)
+  --> /home/paulf/rust/src/test/ui/nll/maybe-initialized-drop.rs:46:2
+   |
+42 |     let wrap = Wrap { p: &mut x };


I see; this is a bit unfortunate, seems like a separate concern. Not 100% sure what is happening here.

Nashenas88 · 2017-11-08T05:30:41Z

src/test/ui/nll/maybe-initialized-drop.stderr

@@ -0,0 +1,46 @@
+error[E0506]: cannot assign to `x` because it is borrowed (Ast)


So I can't seem to get the ui tests to output the Mir/NLL output. If I add the revisions comment, it fails to even recognize this file and considers every error a new error that wasn't expected. Removing the revisions but keeping the borrowck and nll compiler flags outputs this file. I tried reading the markdown file for tests, but it doesn't say revisions are supported for ui tests. What should I do here?

Wait, why do you want/have revisions in this test? Also, I feel like these tests are a good example of where I would prefer a compile-fail test, at least until such time as the ui tests permit (and enforce) //~ ERROR annotations. In a compile-fail test, we could put (in the test itself) //~ ERROR (Ast) for where the old borrow checker reports an error and //~ ERROR (Mir) for where the new borrow checker -- with NLL enabled -- reports an error. The latter ought to I guess be a subset of the former. It's easier to read this "inline" than to read the stderr.

(In particular, this test is about testing that errors occur, not necessarily what we emit when they do.)

But I guess I'm missing the point. The point is that you only see (Ast) errors here, right?

In which case it seems like a bug in the branch =)

Sorry, I completely misunderstood the output of what was going on. I thought certain errors were only showing up when I had the revisions comment, but it's actually the -Z nll flag that silences the Mir errors. I'll need to look into this more. (My other confusion was that adding in the revisions comment cause the test to ignore the .stderr file, the expected errors section comes out blank, I thought they were related).

Nashenas88 · 2017-11-08T05:32:21Z

src/test/ui/nll/maybe-initialized-drop.rs

+    let foo = Foo { a: s, b: wrap };
+    move_string(foo.a);
+    move_wrap(foo.b);
+    x += 1; // OK, drops are inert


@nikomatsakis is this comment right? Does it make sense?

That comment seems correct to me.

Nashenas88 · 2017-11-08T05:35:00Z

src/librustc_mir/borrow_check/nll/constraint_generation.rs

+            });
+
+            self.flow_inits.reset_to_entry_of(bb);
+            for index in 0 .. self.mir.basic_blocks()[bb].statements.len() {


The statement value isn't used by reconstruct_statement_effect, so I just iterate over the number of statements. Is that still useful as a sanity check, or should we just use the data in all_live_locals?

Ah, I see, let's just re-use the data. Note that there should be one final entry to pop, corresponding to the terminator. If we just collect this into a while let Some((location, live_locals)) = all_live_locals.pop() { .. } loop, as you suggest, then we can cleanly combine those two things. We just need to be careful not to invoke reconstruct_statement_effect when we are on the last item but rather reconstruct_terminator_effect (or nothing at all, since we don't need the final result I don't think).

bors · 2017-11-09T23:20:16Z

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

cc rust-lang#37166

When we get around to resolving regions, we really ought to take region obligations into account. There is one case where they are presently being ignored. Keep ignoring them there for now but leave a TODO.

This helps make its inputs and outputs more clear.

This new way is **slightly** less expressive (I would be shocked if it affects any code, though) when it comes to higher-ranked bounds or a few other weird tricks. But we don't handle those consistently regardless, and the new way does not require normalization and is just wildly simpler.

Instead, just search the param env predicates directly. This is equivalent to what we were doing before but more efficient.

We've kind of got the same information twice in the MIR, between the return-type field and the local-decls. Seems un-great.

The inference README, in particular, was VERY out of date!

This restores the behavior of regionck with respect to the free-region-map: that is, it collects all the relations from the fn and its closures. This feels a bit fishy but it's the behavior we've had for some time, and it will go away with NLL, so seems best to just keep it.

…ata impl, ensure mir is immutably borrowed after renumbering, and add find_local fn to simplify mpi lookup in constraint generation

… bugs, and update tests based on feedback

…the inits bits

nikomatsakis · 2017-11-10T17:19:24Z

Closing this PR. Please re-open it against the nll-master branch as we discussed. Thanks!

rust-highfive assigned nikomatsakis Nov 5, 2017

Nashenas88 commented Nov 5, 2017

View reviewed changes

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 6, 2017

nikomatsakis requested changes Nov 6, 2017

View reviewed changes

kennytm 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 7, 2017

Nashenas88 commented Nov 7, 2017

View reviewed changes

nikomatsakis requested changes Nov 7, 2017

View reviewed changes

nikomatsakis reviewed Nov 7, 2017

View reviewed changes

Nashenas88 commented Nov 8, 2017

View reviewed changes

nikomatsakis force-pushed the maybe-initialized-liveness branch from 5b5c240 to 83d60d8 Compare November 9, 2017 14:30

nikomatsakis added 10 commits November 9, 2017 21:18

fulfill: remove dead code

a339932

move region constraints into inference context

1e32a6b

convert EXTRA_REQUIREMENT_IN_IMPL into a hard error

58d4548

cc rust-lang#37166

assert that we are consuming all of the region obligations

57b08b0

When we get around to resolving regions, we really ought to take region obligations into account. There is one case where they are presently being ignored. Keep ignoring them there for now but leave a TODO.

extract regionck_outlives into a separate helper function

279882f

This helps make its inputs and outputs more clear.

move the region_obligations processing code into InferCtxt

7317a4f

do not invoke required_region_bounds in region_obligations

c811e07

Instead, just search the param env predicates directly. This is equivalent to what we were doing before but more efficient.

regionck: only add implied bounds from root fn to free_region_map

100a347

extract out the implied bounds code from regionck

3d1b78b

nikomatsakis and others added 24 commits November 9, 2017 21:18

formalize giving ownership of region vars to region inf. context

7947173

simplify lifetime annotations for MirBorrowckCtxt

2cd18e3

erase regions in MIR borrowck when checking if type moves by default

2f70705

MIR-dump: print return type from local_decls for _0

48d3d7e

We've kind of got the same information twice in the MIR, between the return-type field and the local-decls. Seems un-great.

renumber: handle ReturnTy better

03e76cc

renumber: debug logs, use visit_region rather than visit_rvalue

410c433

region_infer: improved debug logging

ffd17ea

integrate NLL with MIR type-checker

00205ea

update READMEs to describe the new situation

c1d84ec

The inference README, in particular, was VERY out of date!

WIP infer/outlives: add license

5758d4b

convert TODO in traits into a FIXME

0b7ebac

add FIXME for converting RegionVid to use newtype_index!

b3cad88

fix mir-opt NLL tests -- variable '_#0r is now 'static

d2b4f27

factor out free_region_binding_scope helper

2ccc3c7

Initial attempt at refining liveness with "maybe initialized" analysis

d59af53

Take ownership of RegionInferenceContext in Borrows struct

a164ae0

Fix tidy errors

ce8e837

Rename lifetimes on ConstraintGeneration, move macro impl to HasMoveD…

0bb2b66

…ata impl, ensure mir is immutably borrowed after renumbering, and add find_local fn to simplify mpi lookup in constraint generation

Add initial attempt at ui/nll test for maybe-initialized drop

e966e8c

Disable certain code on nll runs, update constraint generation to fix…

ef236e4

… bugs, and update tests based on feedback

Simplify drop loop and account for terminators

49d53da

WIP add a bunch of debug statements

581b197

Apply local effects for flow_init in constraint generation so we get …

69a0edf

…the inits bits

Nashenas88 force-pushed the maybe-initialized-liveness branch from 83d60d8 to 69a0edf Compare November 10, 2017 02:21

Shorten line lengths to fix tidy errors

71ab17d

nikomatsakis closed this Nov 10, 2017

Nashenas88 mentioned this pull request Nov 11, 2017

Maybe initialized liveness nikomatsakis/rust#12

Merged

Nashenas88 deleted the maybe-initialized-liveness branch November 11, 2017 17:52

		@@ -0,0 +1,46 @@
		error[E0506]: cannot assign to `x` because it is borrowed (Ast)

[WIP] Maybe initialized liveness #45794

[WIP] Maybe initialized liveness #45794

Conversation

Nashenas88 commented Nov 5, 2017

rust-highfive commented Nov 5, 2017

Nashenas88 commented Nov 5, 2017

Nashenas88 commented Nov 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Nov 6, 2017

Nashenas88 commented Nov 6, 2017

nikomatsakis 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

bors commented Nov 7, 2017

Nashenas88 commented Nov 7, 2017

Nashenas88 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

nikomatsakis commented Nov 7, 2017

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

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

bors commented Nov 9, 2017

nikomatsakis commented Nov 10, 2017