move closure kind, signature into `ClosureSubsts` #45879

nikomatsakis · 2017-11-08T22:36:46Z

Instead of using side-tables, store the closure-kind and signature in the substitutions themselves. This has two key effects:

It means that the closure's type changes as inference finds out more things, which is very nice.
- As a result, it avoids the need for the freshen_closure_like code (though we still use it for generators).
It avoids cyclic closures calls.
- These were never meant to be supported, precisely because they make a lot of the fancy inference that we do much more complicated. However, due to an oversight, it was previously possible -- if challenging -- to create a setup where a closure directly called itself (see e.g. Stack overflow in rustc using unboxed closure bounds #21410).

We have to see what the effect of this change is, though. Needs a crater run. Marking as [WIP] until that has been assessed.

r? @arielb1

nikomatsakis · 2017-11-08T22:36:56Z

cc @Mark-Simulacrum -- potential breaking change

arielb1 · 2017-11-08T22:59:34Z

src/librustc/infer/freshen.rs

-                    |substs| tcx.mk_closure(def_id, substs)
-                )
-            }
-
            ty::TyGenerator(def_id, substs, interior) => {


Don't you want to do this change for generators too? Generators referencing themselves are just as bad as closures doing so.

Yes, but for purposes of getting crater results, not really needed. Will do, however.

nikomatsakis · 2017-11-09T00:10:26Z

Hmm, the mir-opt tests seem... maybe real? Will have to investigate later.

nikomatsakis · 2017-11-09T22:01:05Z

The mir-opt problem turns out to be an existing bug in inlining.

nikomatsakis · 2017-11-10T11:18:06Z

So should we do a crater run here? In that case, do we want to do a try build?

bors · 2017-11-10T14:12:03Z

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

nikomatsakis · 2017-11-11T11:02:19Z

cc @rust-lang/infra -- what is the procedure for doing a crater run here?

nikomatsakis · 2017-11-11T11:02:24Z

@bors try

bors · 2017-11-11T11:02:36Z

⌛ Trying commit d5a54a9 with merge fb88d08...

@arielb1

[WIP] move closure kind, signature into `ClosureSubsts` Instead of using side-tables, store the closure-kind and signature in the substitutions themselves. This has two key effects: - It means that the closure's type changes as inference finds out more things, which is very nice. - As a result, it avoids the need for the `freshen_closure_like` code (though we still use it for generators). - It avoids cyclic closures calls. - These were never meant to be supported, precisely because they make a lot of the fancy inference that we do much more complicated. However, due to an oversight, it was previously possible -- if challenging -- to create a setup where a closure *directly* called itself (see e.g. #21410). We have to see what the effect of this change is, though. Needs a crater run. Marking as [WIP] until that has been assessed. r? @arielb1

bors · 2017-11-11T12:30:49Z

☀️ Test successful - status-travis
State: approved= try=True

alexcrichton · 2017-11-12T18:35:18Z

r? @arielb1

(assigning a reviewer)

nikomatsakis · 2017-11-13T14:35:00Z

Rebased.

nikomatsakis · 2017-11-13T14:38:07Z

I also decided to start an "old school" crater run.

Root regressions, sorted by rank:

sputnikvm-rlp-0.2.0 (before) (after) -- spurious
elastic_types-0.20.4 (before) (after) -- spurious
tls-api-rustls-0.1.11 (before) (after) -- spurious
google-adexperiencereport1-1.0.6+20170918 (before) (after) -- spurious
match_all-0.2.5 (before) (after) -- spurious
fast_inv_sqrt-1.0.1 (before) (after) -- spurious
env_file-0.1.3 (before) (after) -- spurious
throttler-0.1.3 (before) (after) -- spurious
vec2d-0.1.0 (before) (after) -- spurious
vigenere-0.0.3 (before) (after) -- spurious

nikomatsakis · 2017-11-13T15:21:59Z

@bors try

bors · 2017-11-13T15:22:14Z

⌛ Trying commit 0f6bd17 with merge e3645b5...

@arielb1

[WIP] move closure kind, signature into `ClosureSubsts` Instead of using side-tables, store the closure-kind and signature in the substitutions themselves. This has two key effects: - It means that the closure's type changes as inference finds out more things, which is very nice. - As a result, it avoids the need for the `freshen_closure_like` code (though we still use it for generators). - It avoids cyclic closures calls. - These were never meant to be supported, precisely because they make a lot of the fancy inference that we do much more complicated. However, due to an oversight, it was previously possible -- if challenging -- to create a setup where a closure *directly* called itself (see e.g. #21410). We have to see what the effect of this change is, though. Needs a crater run. Marking as [WIP] until that has been assessed. r? @arielb1

bors · 2017-11-13T18:13:38Z

☀️ Test successful - status-travis
State: approved= try=True

aidanhs · 2017-11-14T17:45:08Z

@nikomatsakis just pinging rust-lang/infra and saying "requesting crater run" will (eventually) get a crater run done.

nikomatsakis · 2017-11-14T18:39:15Z

@aidanhs thanks =)

nikomatsakis · 2017-11-14T18:41:14Z

So, based on the "old school" crater results (no regressions), I see three paths forward:

Improve this branch to issue a nicer error (i.e., one that says "this is a result of a recent fix" and points to an issue describing what happened and how to fix it) and land it. This approach is a good thing for the code but cannot accommodate a warning period.
Close this PR and open another that adds a warning period. Once that is out in the next beta, re-open this PR.
Wait for the "full crater" and then decide.

I'm leaning mildly towards path 2, but we'll see how much time I get for it. I just prefer to give warnings if we can.

arielb1 · 2017-11-18T20:06:51Z

src/librustc/ty/wf.rs

+                    // not known to hold in the creator's context (and
+                    // indeed the closure may not be invoked by its
+                    // creator, but rather turned to someone who *can*
+                    // verify that).


This also doesn't check captured variables from the parent function's generics.

Maybe these don't matter, because within a function the generics are always the function's own - and therefore WF, and outside of it a TyClosure should not appear in the wild. So I'll either add the generics in, or be sure they don't matter and add a comment..

arielb1 · 2017-11-18T20:09:35Z

src/librustc_metadata/encoder.rs

+                EntryKind::Generator(self.lazy(&data))
+            }
+
+            ty::TyClosure(def_id, substs) => {


nit: isn't a closure's signature stored in the closure's item type? storing the data separately feels like it's only likely to cause mimsmatches and trouble (aka can't CrateMetadata::fn_sig call tcx.item_type(def_id).closure_sig(def_id, tcx)?)

Does seem silly. Let me see if it can be readily purged.

It's not really obvious to me how to do this. I'm sure it can be done, but I don't think that item_type will do it. I believe that gives back something with "identity" substitutions, which isn't really what we want here (i.e., the closure_sig parameter will be an unsubstituted type parameter). The other definition of fn_sig uses the typeck_tables to get what we want, but for that we need the HirId of the closure, and I'm not sure how to get that from another crate.

arielb1 · 2017-11-18T20:43:58Z

r=me mod. comments

also, apparently MIR inlining is still somewhat broken with closures. I'll try to open an issue.

[00:57:59] ---- [mir-opt] mir-opt/validate_1.rs stdout ----
[00:57:59] 	
[00:57:59] error: compilation failed!
[00:57:59] status: exit code: 101
[00:57:59] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/mir-opt/validate_1.rs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt" "--target=x86_64-unknown-linux-gnu" "-Zdump-mir=all" "-Zmir-opt-level=3" "-Zdump-mir-exclude-pass-number" "-Zdump-mir-dir=/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/validate_1" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/validate_1.stage2-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-Z" "verbose" "-Z" "mir-emit-validate=1" "-Z" "span_free_formats" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/validate_1.stage2-x86_64-unknown-linux-gnu.aux"
[00:57:59] stdout:
[00:57:59] ------------------------------------------
[00:57:59] 
[00:57:59] ------------------------------------------
[00:57:59] stderr:
[00:57:59] ------------------------------------------
[00:57:59] error: internal compiler error: /checkout/src/librustc_mir/transform/inline.rs:715: Arg operand `1` is (_10.0: &ReErased mut i32), not local

arielb1 · 2017-11-18T21:52:22Z

this is #46086

arielb1 · 2017-11-19T13:21:15Z

Test problem:

[01:06:03] ---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0644 (line 11807) stdout ----
[01:06:03] 	error[E0631]: type mismatch in closure arguments
[01:06:03]   --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:12:3
[01:06:03]    |
[01:06:03] 9  |     let x = |y| {
[01:06:03]    |  ___________-
[01:06:03] 10 | |     // Here, when `x` is called, the parameter `y` is equal to `x`.
[01:06:03] 11 | |   };
[01:06:03]    | |___- found signature of `fn(_) -> _`
[01:06:03] 12 |     fix(&x);
[01:06:03]    |     ^^^ expected signature of `for<'r> fn(&'r [closure@/checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:9:11: 11:4]) -> _`
[01:06:03]    |
[01:06:03]    = note: required by `fix`
[01:06:03] 
[01:06:03] thread 'rustc' panicked at 'couldn't compile the test', /checkout/src/librustdoc/test.rs:288:12
[01:06:03]
[01:06:03] 
[01:06:03] failures:
[01:06:03]     /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0644 (line 11807)

arielb1 · 2017-11-19T15:53:39Z

r=me with test fixed.

arielb1 · 2017-11-20T19:13:36Z

@bors r+

bors · 2017-11-20T19:13:37Z

📌 Commit 9af5a06 has been approved by arielb1

nikomatsakis · 2017-11-20T21:54:14Z

@bors r=arielb1

bors · 2017-11-20T21:54:14Z

📌 Commit b9c766c has been approved by arielb1

Before we were assuming that *every* `fn_sig` must pertain to a local closure.

nikomatsakis · 2017-11-21T15:05:44Z

@bors r=arielb1

bors · 2017-11-21T15:05:45Z

📌 Commit 00732a3 has been approved by arielb1

bors · 2017-11-21T22:52:27Z

⌛ Testing commit 00732a3 with merge d6d09e0...

@arielb1

move closure kind, signature into `ClosureSubsts` Instead of using side-tables, store the closure-kind and signature in the substitutions themselves. This has two key effects: - It means that the closure's type changes as inference finds out more things, which is very nice. - As a result, it avoids the need for the `freshen_closure_like` code (though we still use it for generators). - It avoids cyclic closures calls. - These were never meant to be supported, precisely because they make a lot of the fancy inference that we do much more complicated. However, due to an oversight, it was previously possible -- if challenging -- to create a setup where a closure *directly* called itself (see e.g. #21410). We have to see what the effect of this change is, though. Needs a crater run. Marking as [WIP] until that has been assessed. r? @arielb1

bors · 2017-11-22T01:54:30Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing d6d09e0 to master...

nikomatsakis added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 8, 2017

arielb1 reviewed Nov 8, 2017

View reviewed changes

nikomatsakis force-pushed the nll-kill-cyclic-closures branch from 3a1134d to d5a54a9 Compare November 11, 2017 11:02

kennytm added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 11, 2017

rust-highfive assigned arielb1 Nov 12, 2017

nikomatsakis force-pushed the nll-kill-cyclic-closures branch from d5a54a9 to 0f6bd17 Compare November 13, 2017 14:35

arielb1 reviewed Nov 18, 2017

View reviewed changes

nikomatsakis mentioned this pull request Nov 18, 2017

MIR inlining doesn't work together with MIR validation #46086

Closed

nikomatsakis force-pushed the nll-kill-cyclic-closures branch from 36c7dc0 to c490241 Compare November 19, 2017 01:15

fix closure inlining by spilling arguments to a temporary

df6fdbc

nikomatsakis force-pushed the nll-kill-cyclic-closures branch from c490241 to df6fdbc Compare November 19, 2017 10:37

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

nikomatsakis added 2 commits November 20, 2017 13:34

fix example for E0644

2151e48

extend comment further to explain why we limit wf to upvar_tys

9af5a06

fix example more

b9c766c

check that def_id is a local closure in InferCtxt::fn_sig

00732a3

Before we were assuming that *every* `fn_sig` must pertain to a local closure.

bors merged commit 00732a3 into rust-lang:master Nov 22, 2017

This was referenced Nov 22, 2017

MIR: split Operand::Consume into Copy and Move. #46142

Merged

Validate miri against the HIR const evaluator #45002

Merged

Move collector to librustc_mir::monomorphize #45525

Merged

kennytm mentioned this pull request Feb 5, 2018

E0593 regression when mismatched argument count leads to other errors in Beta/Nightly #47244

Closed

move closure kind, signature into ClosureSubsts #45879

move closure kind, signature into ClosureSubsts #45879

Conversation

nikomatsakis commented Nov 8, 2017

nikomatsakis commented Nov 8, 2017

arielb1 Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Nov 9, 2017

Choose a reason for hiding this comment

nikomatsakis commented Nov 9, 2017

nikomatsakis commented Nov 9, 2017

nikomatsakis commented Nov 10, 2017

bors commented Nov 10, 2017

nikomatsakis commented Nov 11, 2017

nikomatsakis commented Nov 11, 2017

bors commented Nov 11, 2017

bors commented Nov 11, 2017

alexcrichton commented Nov 12, 2017

nikomatsakis commented Nov 13, 2017

nikomatsakis commented Nov 13, 2017 • edited Loading

Root regressions, sorted by rank:

nikomatsakis commented Nov 13, 2017

bors commented Nov 13, 2017

bors commented Nov 13, 2017

aidanhs commented Nov 14, 2017

nikomatsakis commented Nov 14, 2017

nikomatsakis commented Nov 14, 2017

arielb1 Nov 18, 2017

Choose a reason for hiding this comment

arielb1 Nov 18, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Nov 20, 2017

Choose a reason for hiding this comment

nikomatsakis Nov 20, 2017

Choose a reason for hiding this comment

arielb1 commented Nov 18, 2017

arielb1 commented Nov 18, 2017

arielb1 commented Nov 19, 2017

arielb1 commented Nov 19, 2017

arielb1 commented Nov 20, 2017

bors commented Nov 20, 2017

nikomatsakis commented Nov 20, 2017

bors commented Nov 20, 2017

nikomatsakis commented Nov 21, 2017

bors commented Nov 21, 2017

bors commented Nov 21, 2017

bors commented Nov 22, 2017

move closure kind, signature into `ClosureSubsts` #45879

move closure kind, signature into `ClosureSubsts` #45879

arielb1 Nov 8, 2017 •

edited

Loading

nikomatsakis commented Nov 13, 2017 •

edited

Loading

arielb1 Nov 18, 2017 •

edited

Loading