Infer whether a closure implements `Fn`, `FnMut`, etc based on what actions it takes #21805

nikomatsakis · 2015-01-31T12:39:30Z

Currently, we only infer the kind of a closure based on the expected type or explicit annotation. If neither applies, we currently report an error. This pull request changes that case to defer the decision until we are able to analyze the actions of the closure: closures which mutate their environment require FnMut, closures which move out of their environment require FnOnce.

This PR is not the end of the story:

It does not remove the explicit annotations nor disregard them. The latter is the logical next step to removing them (we'll need a snapshot before we can do anything anyhow). Disregarding explicit annotations might expose more bugs since right now all closures in libstd/rustc use explicit annotations or the expected type, so this inference never kicks in.
The interaction with instantiating type parameter fallbacks leaves something to be desired. This is mostly just saying that the algorithm from Finalize defaulted type parameters rfcs#213 needs to be implemented, which is a separate bug. There are some semi-subtle interactions though because not knowing whether a closure is Fn vs FnMut prevents us from resolving obligations like F : FnMut(...), which can in turn prevent unification of some type parameters, which might (in turn) lead to undesired fallback. We can improve this situation however -- even if we don't know whether (or just how) F : FnMut(..) holds or not for some closure type F, we can still perform unification since we do know the argument and return types. Once kind inference is done, we can complete the F : FnMut(..) analysis -- which might yield an error if (e.g.) the F moves out of its environment.

r? @nick29581

rust-highfive · 2015-01-31T12:39:34Z

r? @pcwalton

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

nikomatsakis · 2015-01-31T12:40:04Z

cc @aturon

nikomatsakis · 2015-01-31T12:41:31Z

Oh, a note to the eventual reviewer -- the commits are mostly independent, but there's a bit of back-and-forth because I realized my initial strategy was overly complex. In particular, I envisioned the "deferred resolutions" introduced in 9a85a17 as a more general thing, but I realized that because we don't have to worry about recursive closures for kind inference, we can simplify everything if we specialize the deferred resolutions to calls, which is done in f9e46a8.

eddyb · 2015-01-31T13:26:36Z

src/librustc/metadata/common.rs

-pub const tag_closure: uint = 0x96;
-pub const tag_closure_type: uint = 0x97;
-pub const tag_closure_kind: uint = 0x98;
+// GAP 0x94...0x98


Could we not have this?

I'm not sure I follow. Not have a gap? Not comment on the gap? I'm not sure how to avoid the gap except for renumbering everything below, which seems kind of pointless? (Or is there something obvious I'm overlooking?)

Forget about it, I'll remove it (the gap) whenever I touch this code again.

eddyb · 2015-01-31T13:50:53Z

I took the liberty of finding a few nits - feel free to r=me unless you want a second review.

Any reason why this PR doesn't disregard explicit kinds? Maybe error if inference and the explicit kind mismatches.

nikomatsakis · 2015-02-01T01:17:59Z

Any reason why this PR doesn't disregard explicit kinds? Maybe error if inference and the explicit kind mismatches.

No, I just ran out of time and decided to post it as is. Maybe I'll try it locally and see what happens. But it doesn't make sense to error: I actually expect inference to get different results than the expectation on a fairly regular basis. For example, if the expected kind is FnMut, but the closure is |x| x*2, then the inference would yield Fn -- but that's perfectly legal.

eddyb · 2015-02-01T07:05:14Z

I was actually talking about |&mut: x| x*2, where one could change to |&: x| x*2 so it still compiles at stage0.
I am thinking that errors would potentially catch some bugs before they're fully removed, like a corner case that causes some form of mutation to be ignored, for example.

nikomatsakis · 2015-02-01T10:27:54Z

@eddyb as an experiment, I tried removing all inference of expected kinds beyond what's implemented in this PR. I immediately hit some (minor) bugs around move closures, which I have fixed. After that, everything bootstraps, but I get some failures in run-pass tests due to the problem I described in bullet 2 of the header: "We can improve this situation however -- even if we don't know whether (or just how) F : FnMut(..) holds or not for some closure type F, we can still perform unification since we do know the argument and return types. Once kind inference is done, we can complete the F : FnMut(..) analysis -- which might yield an error if (e.g.) the F moves out of its environment. "

nikomatsakis · 2015-02-01T10:28:28Z

So I'm personally inclined to land it as is and patch those other problems later.

doing the final checking for closure calls until after trait inference is performed. This isn't important now, but it's essential if we are to delay inferring the closure kind.

…ity that its value is not yet known.

…they are borrowed or by value.

upvar inference. Upvar inference can cause some obligations to be deferred, notably things like `F : Sized` where `F` is a closure type, or `F : FnMut`. Adjust the ordering therefore so that we process all traits and apply fallback, do upvar inference, and only then start reporting errors for outstanding obligations.

generate the closure type and closure kind separately.

Trickier cases not yet handled.

specialized to closures, and invoke them as soon as we know the closure kind. I thought initially we would need a fixed-point inference algorithm but it appears I was mistaken, so we can do this.

…e weren't properly adjusting the closure kind in that case.

…s not yet known.

nikomatsakis · 2015-02-01T11:17:39Z

Rebased, cleaned up @eddyb's nits, and fixed the easy bugs that were exposed by relying entirely on closure inference, but not the harder ones.

nikomatsakis · 2015-02-01T11:39:41Z

@eddyb want to look over those last two commits?

nikomatsakis · 2015-02-01T11:40:40Z

reassigning to eddyb since he did a review already

eddyb · 2015-02-01T12:48:51Z

@bors r+ 870aea2

bors · 2015-02-01T13:01:58Z

⌛ Testing commit 870aea2 with merge 0ab8d5d...

@nick29581

…ddyb Currently, we only infer the kind of a closure based on the expected type or explicit annotation. If neither applies, we currently report an error. This pull request changes that case to defer the decision until we are able to analyze the actions of the closure: closures which mutate their environment require `FnMut`, closures which move out of their environment require `FnOnce`. This PR is not the end of the story: - It does not remove the explicit annotations nor disregard them. The latter is the logical next step to removing them (we'll need a snapshot before we can do anything anyhow). Disregarding explicit annotations might expose more bugs since right now all closures in libstd/rustc use explicit annotations or the expected type, so this inference never kicks in. - The interaction with instantiating type parameter fallbacks leaves something to be desired. This is mostly just saying that the algorithm from rust-lang/rfcs#213 needs to be implemented, which is a separate bug. There are some semi-subtle interactions though because not knowing whether a closure is `Fn` vs `FnMut` prevents us from resolving obligations like `F : FnMut(...)`, which can in turn prevent unification of some type parameters, which might (in turn) lead to undesired fallback. We can improve this situation however -- even if we don't know whether (or just how) `F : FnMut(..)` holds or not for some closure type `F`, we can still perform unification since we *do* know the argument and return types. Once kind inference is done, we can complete the `F : FnMut(..)` analysis -- which might yield an error if (e.g.) the `F` moves out of its environment. r? @nick29581

bors · 2015-02-01T15:49:18Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

@nikomatsakis

This needs a snapshot that includes #21805 before it can be merged. There are some places where type inference regressed after I removed the annotations (see `FIXME`s). cc @nikomatsakis. r? @eddyb or anyone (I'll remove the `FIXME`s before merging, as they are only intended to point out regressions)

rust-highfive assigned pcwalton Jan 31, 2015

nikomatsakis assigned nrc and unassigned pcwalton Jan 31, 2015

eddyb reviewed Jan 31, 2015
View reviewed changes

nikomatsakis force-pushed the closure-inference-refactor-1 branch from f9e46a8 to b31a3df Compare February 1, 2015 11:10

nikomatsakis added 10 commits February 1, 2015 06:13

Introduce the notion of deferred resolutions and use it to hold off on

53c1956

doing the final checking for closure calls until after trait inference is performed. This isn't important now, but it's essential if we are to delay inferring the closure kind.

Make unboxed_closure_kind return Option to allow for the possibil…

bc41cc0

…ity that its value is not yet known.

Do not make fake types for upvars if we haven't yet inferred whether …

f5281e2

…they are borrowed or by value.

Separate out the unboxed closure table into two tables, so that we can

2f46586

generate the closure type and closure kind separately.

Implement basic inference for closure kinds and some simple tests.

e1f54f0

Trickier cases not yet handled.

To handle more complex cases, modify the deferred call handler to be

5a39a0d

specialized to closures, and invoke them as soon as we know the closure kind. I thought initially we would need a fixed-point inference algorithm but it appears I was mistaken, so we can do this.

Tweak comments per eddyb's suggestion.

e778ea4

Fix handling of move closures -- since they have one fewer deref, w…

a9c3841

…e weren't properly adjusting the closure kind in that case.

Do not ICE when e.g. call_mut() is called on a closure whose kind i…

870aea2

…s not yet known.

nikomatsakis force-pushed the closure-inference-refactor-1 branch from b31a3df to 870aea2 Compare February 1, 2015 11:13

nikomatsakis assigned eddyb and unassigned nrc Feb 1, 2015

bors merged commit 870aea2 into rust-lang:master Feb 1, 2015

japaric mentioned this pull request Feb 1, 2015

Obsolete the closure kind syntax #21843

Merged

nikomatsakis mentioned this pull request Feb 2, 2015

Closure kind inference: infer whether a closure is Fn, FnMut, etc #16640

Closed

nikomatsakis deleted the closure-inference-refactor-1 branch March 30, 2016 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Infer whether a closure implements `Fn`, `FnMut`, etc based on what actions it takes #21805

Infer whether a closure implements `Fn`, `FnMut`, etc based on what actions it takes #21805

nikomatsakis commented Jan 31, 2015

rust-highfive commented Jan 31, 2015

nikomatsakis commented Jan 31, 2015

nikomatsakis commented Jan 31, 2015

eddyb Jan 31, 2015

nikomatsakis Feb 1, 2015

eddyb Feb 1, 2015

eddyb commented Jan 31, 2015

nikomatsakis commented Feb 1, 2015

eddyb commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

eddyb commented Feb 1, 2015

bors commented Feb 1, 2015

bors commented Feb 1, 2015

Infer whether a closure implements Fn, FnMut, etc based on what actions it takes #21805

Infer whether a closure implements Fn, FnMut, etc based on what actions it takes #21805

Conversation

nikomatsakis commented Jan 31, 2015

rust-highfive commented Jan 31, 2015

nikomatsakis commented Jan 31, 2015

nikomatsakis commented Jan 31, 2015

eddyb Jan 31, 2015

Choose a reason for hiding this comment

nikomatsakis Feb 1, 2015

Choose a reason for hiding this comment

eddyb Feb 1, 2015

Choose a reason for hiding this comment

eddyb commented Jan 31, 2015

nikomatsakis commented Feb 1, 2015

eddyb commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

nikomatsakis commented Feb 1, 2015

eddyb commented Feb 1, 2015

bors commented Feb 1, 2015

bors commented Feb 1, 2015

Infer whether a closure implements `Fn`, `FnMut`, etc based on what actions it takes #21805

Infer whether a closure implements `Fn`, `FnMut`, etc based on what actions it takes #21805