From 71aef4088005b485d0723b66ce41c2d7c88b58b4 Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Tue, 30 Mar 2021 09:50:51 -0700 Subject: [PATCH] Move the accidental interconversion support to an alternative, given crater results. --- text/0000-try-trait-v2.md | 132 +++++++++++++++++++++++++------------- 1 file changed, 87 insertions(+), 45 deletions(-) diff --git a/text/0000-try-trait-v2.md b/text/0000-try-trait-v2.md index 88910cc17a7..ffe5f75fc6e 100644 --- a/text/0000-try-trait-v2.md +++ b/text/0000-try-trait-v2.md @@ -10,9 +10,10 @@ Replace [RFC #1859, `try_trait`](https://rust-lang.github.io/rfcs/1859-try-trait with a new design for the currently-unstable [`Try` trait](https://doc.rust-lang.org/nightly/std/ops/trait.Try.html) and corresponding desugaring for the `?` operator. -The new design supports all the currently-stable conversions (including the accidental ones), -while addressing the discovered shortcomings of the currently-implemented solution, -as well as enabling new scenarios. +The new design includes support for all *intentional* interconversions. +It proposes removing the *accidental* interconversions, as a crater run +demonstrated that would be feasible, however includes an alternative system +that can support them as a low-support-cost edition mechanism if needed. *This is [forward-looking](#future-possibilities) to be compatible with other features, like [`try {}`](https://doc.rust-lang.org/nightly/unstable-book/language-features/try-blocks.html) blocks @@ -27,7 +28,7 @@ The motivations from the previous RFC still apply (supporting more types, and re However, new information has come in since the previous RFC, making people wish for a different approach. - Using the "error" terminology is a poor fit for other potential implementations of the trait. -- The ecosystem has started to see error types which are `From`-convertible from *any* type implementing `Debug`, which makes the previous RFC's method for controlling interconversions ineffective. +- The previous RFC's mechanism for controlling interconversions proved ineffective, with inference meaning that people did it unintentionally. - It's no longer clear that `From` should be part of the `?` desugaring for _all_ types. It's both more flexible -- making inference difficult -- and more restrictive -- especially without specialization -- than is always desired. - An [experience report](https://github.com/rust-lang/rust/issues/42327#issuecomment-366840247) in the tracking issue mentioned that it's annoying to need to make a residual type in common cases. @@ -554,34 +555,6 @@ impl ops::FromResidual for ControlFlow { } ``` -## Making the accidental `Option` interconversion continue to work - -This is done with an extra implementation: -```rust -mod sadness { - use super::*; - - /// This is a remnant of the old `NoneError` which is never going to be stabilized. - /// It's here as a snapshot of an oversight that allowed this to work in the past, - /// so we're stuck supporting it even though we'd really rather not. - #[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)] - pub struct PleaseCallTheOkOrMethodToUseQuestionMarkOnOptionsInAMethodThatReturnsResult; - - impl ops::FromResidual> for Result - where - E: From, - { - fn from_residual(x: Option) -> Self { - match x { - None => Err(From::from( - PleaseCallTheOkOrMethodToUseQuestionMarkOnOptionsInAMethodThatReturnsResult, - )), - } - } - } -} -``` - ## Use in `Iterator` The provided implementation of `try_fold` is already just using `?` and `try{}`, so doesn't change. The only difference is the name of the associated type in the bound: @@ -855,14 +828,6 @@ In those cases where a separate type *is* needed, it's still easier to make a re This RFC uses `!` to be concise. It would work fine with `convert::Infallible` instead if `!` has not yet stabilized, though a few more match arms would be needed in the implementations. (For example, `Option::from_residual` would need `Some(c) => match c {}`.) -## Moving away from the `Option`→`Result` interconversion - -We could consider doing an edition switch to make this no longer allowed. - -For example, we could have a different, never-stable `Try`-like trait used in old editions for the `?` desugaring. It could then have a blanket impl, plus the extra interconversion one. - -It's unclear that that's worth the effort, however, so this RFC is currently written to continue to support it going forward. Notably, removing it isn't enough to solve the annotation requirements, so the opportunity cost feels low. - ## Why `FromResidual` is the supertrait It's nicer for `try_fold` implementations to just mention the simpler `Try` name. It being the subtrait means that code needing only the basic scenario can just bound on `Try` and know that both `from_output` and `from_residual` are available. @@ -890,6 +855,85 @@ impl FromResidual for LogAndIgnoreErrors { And, ignoring the coherence implications, a major difference between the two sides is that the target type is typically typed out visibly (in a return type) whereas the source type (going into the `?`) is often the result of some called function. So it's preferable for any behaviour extensions to be on the type that can more easily be seen in the code. +## Can we just remove the accidental interconversions? + +This depends on how we choose to read the rules around breaking changes. + +A [crater run on a prototype implementation](https://github.com/rust-lang/rust/pull/82322#issuecomment-792299734) found that some people are doing this. PRs have been sent to the places that broke, and generally it was agreed that removing the mixing improved the code: + +> Definitely a good change. + +> Thanks for spotting that, that was indeed a confusing mix + +However another instance is in an abandoned project where the repository has been archived, so will not be fixed. And of course if it happened 3 times, there might be more instances in the wild. + +The interesting pattern boils down to this: + +```rust +.map(|v| Ok(something_returning_option(v)?)) +``` + +That means it's using `?` on an `Option`, but the closure ends up returning `Result<_, NoneError>` without needing to name the type as trait resolution discovers that it's the only possibility. It seems reasonable that this could happen accidentally while refactoring. That does mean, however, that the breakage could also be considered "allowed" as an inference change, and hypothetically additional implementations could make it ambiguous in the future. (It's like the normal `AsRef` breakage, and fits the pattern of "there's a way it could be written that works before and after", though in this case the disambiguated form requires naming an unstable type.) + +This RFC thus proposes removing the accidental interconversions. + +### Compatibility with accidental interconversions (if needed) + +If something happens that turns out they need to be supported, the following approach can work. + +This would take a multi-step approach: +- Add a new never-stable `FromResidualLegacy` trait +- Have a blanket implementation so that users interact only with `FromResidual` +- Add implementations for the accidental interconversions +- Use `FromResidualLegacy` in the desugaring, [perhaps only for old editions](https://github.com/scottmcm/rust/commit/do-or-do-not-edition) + +This keeps them from being visible in the trait system on stable, as `FromResidual` (the only form that would ever stabilize, or even be mentionable) would not include them. + +```rust +mod sadness { + use super::*; + + /// This includes all of the [`ops::FromResidual`] conversions, but + /// also adds the two interconversions that work in 2015 & 2018. + /// It will never be stable. + pub trait FromResidualLegacy { + fn from_residual_legacy(r: R) -> Self; + } + + impl, R> FromResidualLegacy for T { + fn from_residual_legacy(r: R) -> Self { + >::from_residual(r) + } + } + + /// This is a remnant of the old `NoneError` which is never going to be stabilized. + /// It's here as a snapshot of an oversight that allowed this to work in the past, + /// so we're stuck supporting it even though we'd really rather not. + /// This will never be stabilized; use [`Option::ok_or`] to mix `Option` and `Result`. + #[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)] + pub struct LegacyNoneError; + + impl ops::FromResidual> for Result + where + E: From, + { + fn from_residual(x: Option) -> Self { + match x { + None => Err(From::from(LegacyNoneError)), + } + } + } + + + #[unstable(feature = "try_trait_v2", issue = "42327")] + impl FromResidualLegacy> for Option + { + fn from_residual_legacy(_: Result) -> Self { + None + } + } +} +```