Skip to content
New issue

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

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

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants