Change the Fn traits to use inheritance, not bridge impls
#23282
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nikomatsakis
changed the title
Fn traits to use inheritance, not marker implsFn traits to use inheritance, not bridge impls
| (FnMutClosureKind, FnMutClosureKind) => true, | ||
| (FnMutClosureKind, FnOnceClosureKind) => true, | ||
| (FnOnceClosureKind, FnOnceClosureKind) => true, | ||
| _ => false, |
There was a problem hiding this comment.
Could you just use self <= other instead of the big match?
|
@bors: r+ aaaba925a2d52bcced92f78ae12df7dd6bb1704f (though there is one comment, I don't think it is too important) |
|
☔ The latest upstream changes (presumably #23104) made this pull request unmergeable. Please resolve the merge conflicts. |
nikomatsakis
force-pushed
the
fn-trait-inheritance
branch
from
b10efa9
to
79ee2b2
Compare
impls. This requires: 1. modifying trait selection a bit so that when we synthesize impls for fn pointers and closures; 2. adding code to trans so that we can synthesize a `FnMut`/`FnOnce` impl for a `Fn` closure and so forth.
impls. This is a [breaking-change] (for gated code) in that when you implement `Fn` (`FnMut`) you must also implement `FnOnce`. This commit demonstrates how to fix it.
nikomatsakis
force-pushed
the
fn-trait-inheritance
branch
from
79ee2b2
to
9330bae
Compare
|
⌛ Testing commit 9330bae with merge f4d9386... |
|
💔 Test failed - auto-linux-64-opt |
|
@bors: retry On Tue, Mar 24, 2015 at 3:44 PM, bors notifications@github.com wrote:
|
alexcrichton
added a commit
to alexcrichton/rust
that referenced
this pull request
Mar 24, 2015
The primary motivation here is to sidestep rust-lang#19032 -- for a time, I thought that we should improve coherence or otherwise extend the language, but I now think that any such changes will require more time to bake. In the meantime, inheritance amongst the fn traits is both logically correct *and* a simple solution to that obstacle. This change introduces inheritance and modifies the compiler so that it can properly generate impls for closures and fns. Things enabled by this PR (but not included in this PR): 1. An impl of `FnMut` for `&mut F` where `F : FnMut` (rust-lang#23015). 2. A better version of `Thunk` I've been calling `FnBox`. I did not include either of these in the PR because: 1. Adding the impls in 1 currently induces a coherence conflict with the pattern trait. This is interesting and merits some discussion. 2. `FnBox` deserves to be a PR of its own. The main downside to this design is (a) the need to write impls by hand; (b) the possibility of implementing `FnMut` with different semantics from `Fn`, etc. Point (a) is minor -- in particular, it does not affect normal closure usage -- and could be addressed in the future in many ways (better defaults; convenient macros; specialization; etc). Point (b) is unfortunate but "just a bug" from my POV, and certainly not unique to these traits (c.f. Copy/Clone, PartialEq/Eq, etc). (Until we lift the feature-gate on implementing the Fn traits, in any case, there is room to correct both of these if we find a nice way.) Note that I believe this change is reversible in the future if we decide on another course of action, due to the feature gate on implementing the `Fn` traits, though I do not (currently) think we should reverse it. Fixes rust-lang#18835. r? @nrc
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The primary motivation here is to sidestep #19032 -- for a time, I thought that we should improve coherence or otherwise extend the language, but I now think that any such changes will require more time to bake. In the meantime, inheritance amongst the fn traits is both logically correct and a simple solution to that obstacle. This change introduces inheritance and modifies the compiler so that it can properly generate impls for closures and fns.
Things enabled by this PR (but not included in this PR):
FnMutfor&mut FwhereF : FnMut(FnMut not implemented for mutable FnMut implementors #23015).ThunkI've been callingFnBox.I did not include either of these in the PR because:
FnBoxdeserves to be a PR of its own.The main downside to this design is (a) the need to write impls by hand; (b) the possibility of implementing
FnMutwith different semantics fromFn, etc. Point (a) is minor -- in particular, it does not affect normal closure usage -- and could be addressed in the future in many ways (better defaults; convenient macros; specialization; etc). Point (b) is unfortunate but "just a bug" from my POV, and certainly not unique to these traits (c.f. Copy/Clone, PartialEq/Eq, etc). (Until we lift the feature-gate on implementing the Fn traits, in any case, there is room to correct both of these if we find a nice way.)Note that I believe this change is reversible in the future if we decide on another course of action, due to the feature gate on implementing the
Fntraits, though I do not (currently) think we should reverse it.Fixes #18835.
r? @nrc