-
Notifications
You must be signed in to change notification settings - Fork 217
Description
Brief summary
After some discussions about Yokeable
(and Yoke
) and after reading some related issues, there's a bunch I want to tackle. I could have a PR for each change, but it seems tedious, when some changes need two PR's (one non-breaking and one followup breaking PR). I intend to tackle five issues (#3507, #5368, #6057, this one, and #6786) in two PR's (non-breaking and breaking).
Edit: I'll only be doing the non-breaking one, but will leave the list of tasks for a breaking PR for future reference.
Immediate issue
The Yokeable::make
and Yokeable::transform_mut
methods each require that implementations are equivalent to a transmute, and equivalent to passing the result of a transmute to a callback, respectively. In other words, there is only one implementation which fulfills the safety contract for implementors, and they should not be trait methods.
We should instead provide helper free functions for these uses. I don't believe that any part of the implementation safety contracts for make
or transform_mut
would need to remain in Yokeable
; the existing safety section on Yokeable
should be sufficient, since the functions take Yokeable
s as input. make
keeps its safety contract for users.
Initially discussed in #5368 (comment).
Related issues
Closing this issue would be a breaking change to Yokeable
, removing two of its methods. It might be wise, then, to also make other breaking changes in the same update, and provide related functionality.
Another helper function
If we provide make
as a free function, we could tackle #3507 at the same time, and provide something like the upgrade_output
function discussed there (essentially, a safe version of make
for when 'a
is 'static
).
GATs
We could close #6057 by removing the <'a>
from Yokeable
, and making its output type be Output<'a>
.
Bikeshedding
If Yokeable::make
and Yokeable::transform_mut
are made standalone functions, it seems like a good idea to rename them to make_yokeable
and transform_mut_yokeable
, or similar.
I don't know if the function from #3507 should have "upgrade" in its name, when the safety contract says that if Y: Yokeable<'static>
, then Y::Output
is Y
. Maybe cast_yokeable
, since it should be a practically no-op type cast from Y::Output
to Y
(if the compiler notices the types are the same). Or, from the GAT version of a 'static
Y::Output
to Y
.
Should a Yokeable::Output<'a>
GAT still be called "Output", when it's no longer the output of feeding a lifetime to a Yokeable<'a>
? There are probably better options, possibly WithLifetime<'a>
.
Other decisions to make
I assume we want to publicly export the three helper functions for dealing with Yokeable
s. Do we want to export them in the root of yoke
, or put them in a public module?
Should map_project
(and friends) be renamed to map
in the next major release?
Plan
I'll write a non-breaking PR (handles things I'm personally interested in).
Not sure if I'll be around for the breaking PR, but someone else might find the plan useful.
-
Bikeshedding for first PR
- Name the three free functions (currently:
make_yokeable
,transform_mut_yokeable
, andcast_yokeable
) - Decide where to publicly export them from. For now I'm going to put them in a private
yokeable_utils
module and publicly reexport them from theyoke
root.
- Name the three free functions (currently:
-
Bikeshedding for second PR
- Consider changing the name of
Yokeable::Output<'a>
. Currently:Yokeable::WithLifetime<'a>
.
- Consider changing the name of
-
Non-breaking PR
- Implement the three publicly-exported free functions, which would close Add Yokeable::upgrade_output function #3507.
- Implement the temporary
Yoke::with_mut_return
method, related to Add return type toYoke::with_mut
andYokeable::transform_mut
#5368 though it doesn't close the issue. - Internally transition
yoke
to use the free functions forYokeable
s instead of the corresponding trait methods. - Update documentation. Make it clear that
Yoke::with_mut_return
will be removed on the next major update. - Add
#[deprecated(...)]
toYokeable::make
andYokeable::transform_mut
. - Add another
allow
in the derive macro to handlederive(Yokeable)
can triggerclippy::mem_forget
#6786.
-
Breaking PR
- Remove
Yokeable::make
andYokeable::transform_mut
. Possibly, add checks to the derive macro that the free functions work for the typeYokeable
is derived on. - Close Add return type to
Yoke::with_mut
andYokeable::transform_mut
#5368 by replacingYoke::with_mut
with the implementation ofYoke::with_mut_return
, and remove the latter. (Maybe the change toYoke::with_mut
could be done in one step and be considered sufficiently nonbreaking. Adding a return type certainly supersedes the old behavior, but I think that breaks type inference in some silly edge cases. Maybe the slight breakage would be considered worth avoiding the temporaryYoke::with_mut_return
, but I'm defaulting to putting it in the breaking PR.) - Close Move Yoke over to GATs #6057 by making
Yokeable::Output
a GAT. (Renamed toYokeable::WithLifetime<'a>
or whatever it's bikeshed to.) - Update anything in
yoke
affected by above changes, e.g. trait bounds, the derive macro, documentation.
- Remove