Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the types in one of the FromResidual implementations #1645

Closed
wants to merge 2 commits into from

Conversation

scottmcm
Copy link

Without this change, ? on Result doesn't actually work as intended.

The compiler error from the new test I added, before fixing the types:

error[E0277]: the `?` operator can only be used in a function that returns `Result` or `Option` (or another type that implements `FromResidual`)
   --> core\lib\src\outcome.rs:665:25
    |
664 | /         fn demo() -> Outcome<i128, String, f32> {
665 | |             Err("hello")?;
    | |                         ^ cannot use the `?` operator in a function that returns `outcome::Outcome<i128, std::string::String, f32>`
666 | |             unreachable!()
667 | |         }
    | |_________- this function should return `Result` or `Option` to accept `?`
    |
    = help: the trait `FromResidual<std::result::Result<Infallible, &str>>` is not implemented for `outcome::Outcome<i128, std::string::String, f32>`
    = note: required by `from_residual`

And while I was there, I tweaked the code to remove the need for some #[allow]s.

And while I was there, tweak the code to remove the need for some `#[allow]`s.
@SergioBenitez
Copy link
Member

Ah, that's interesting; I assumed the std-lib impl used !, but that's clearly not the case. Thanks for catching this.

@SergioBenitez
Copy link
Member

Could you add a test for Outcome propagation, forward and failure, as well? Ideally we'd check that the variant and values match.

@scottmcm
Copy link
Author

Hopefully the current work to stabilize ! will make it finally stick, and then it will, but for now it doesn't. (The plan is for Infallible to become a type alias for ! at that point, so people don't have to migrate.)

Context for why, if you're curious: rust-lang/rust#84092 (comment)

@scottmcm
Copy link
Author

Added additional tests.

@SergioBenitez
Copy link
Member

SergioBenitez commented May 19, 2021

Awesome. Merged in f2a56f4. Thank you!

@scottmcm scottmcm deleted the v0.4 branch May 19, 2021 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants