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

Allow error type conversions in ensure! and ensure_eq! #474

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

webmaster128
Copy link
Member

As discussed before.

}

#[test]
fn ensure_can_infer_error_type() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to understand why this works. How does Rust infer the error type in that case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, this compiles???

I guess it uses the matches! segment to determine it is expected to compare with StdError, thus setting the Result<_, StdError>. But this compiler is starting to get a bit too smart....

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Amazed the tests requires such few changes, and the actual contracts could always infer the types.

This is strictly better. 👍

if $a != $b {
return Err($e);
}
ensure!($a == $b, $e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better reuse of code 👍

}

#[test]
fn ensure_can_infer_error_type() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah, this compiles???

I guess it uses the matches! segment to determine it is expected to compare with StdError, thus setting the Result<_, StdError>. But this compiler is starting to get a bit too smart....

#[derive(Debug)]
struct ContractError;

impl From<StdError> for ContractError {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice minimal case 👍

}
}

fn check(a: usize, b: usize) -> Result<(), ContractError> {
Copy link
Member

@ethanfrey ethanfrey Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, here is the real test of the compiler's intelligence:

If you remove -> Result<(), ContractError>, will the compiler infer it from line 97?

Copy link
Member Author

@webmaster128 webmaster128 Oct 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An fn always needs a specific return type. Unset means (). So this is more realistic that the closure test case.

But yes, if you turn it into a closure and remove the return type, compiler can infer it.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty good. Just noticed fail_if! is missing these conversions. Maybe implement fail_if! in terms of ensure!?

@webmaster128
Copy link
Member Author

Just noticed fail_if! is missing these conversions.

Good point

Maybe implement fail_if! in terms of ensure!?

I'm in favour of removing fail_if, which is unused here and one more API to learn, teach and maintain.

@maurolacy
Copy link
Contributor

maurolacy commented Oct 7, 2021

I'm in favour of removing fail_if, which is unused here and one more API to learn, teach and maintain.

I second that. fail_if!() is just !ensure!(); or better ensure!(!).

@webmaster128 webmaster128 merged commit 1f0295e into main Oct 7, 2021
@webmaster128 webmaster128 deleted the allow-conversions-in-ensure branch October 7, 2021 14:23
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.

3 participants