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

Feature/error equality #1544

Merged
merged 5 commits into from
Apr 20, 2022
Merged

Conversation

guibescos
Copy link
Contributor

@guibescos guibescos commented Mar 2, 2022

Motivation : Close #1538 . It is useful to compare errors in Rust side unit tests (ran with cargo test), to make sure that my Anchor code fails with the expected error.
Example code :

#[cfg(test)]
pub mod tests {
    use crate::ErrorCode;
    use anchor_lang::prelude::{error, Pubkey, Result};

    #[test]
    fn test_error_2() {
        fn returns_error() -> Result<()> {
            return Err(error!(ErrorCode::MyError));
        }

        assert_eq!(returns_error(), Err(error!(ErrorCode::MyError)))
    }
}

Self-contained example: https://github.com/guibescos/anchor-issue-error-equals

PR : Implements traits Eq and PartialEq for anchor_lang::error:Error it only looks at the actual error codes and not the source.

self.program_error == other.program_error
}
}
impl Eq for ProgramErrorWithOrigin {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add Eq to derive? Same question for AnchorError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question, I personally think that equality shouldn't be based for example on the source (file, line) of the error. The error code (the enum variant) uniquely identifies the 'type' of error so that's what I'm comparing.

Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 but my question was about Eq

impl Eq for ProgramErrorWithOrigin {}

vs

#[derive(Eq)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me check if it works and get back to you. I got inspired by a reference implementation.

Copy link
Contributor Author

@guibescos guibescos Mar 4, 2022

Choose a reason for hiding this comment

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

I tried that syntax (#[derive(Eq)]) and it seems to try to compare the fields one by one (fails because Source doesn't implement Eq) even when PartialEq is implemented manually, which is not the behavior I want. I think the empty impl is needed.

Another option is not implementing Eq at all and only having PartialEq which allows comparison already, but I don't know enough about rust to understand what is better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you decide that Derive(Eq) doesn't work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I you check the expanded code for Eq, you can see that it reverts to comparing all 3 fields. The behavior I want is the one I implemented for PartialEq : only comparing 1 field, the error code. Therefore Deriv(Eq) doesn't achieve what I want.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean these 3 lines?

                let _: ::core::cmp::AssertParamIsEq<ProgramError>;
                let _: ::core::cmp::AssertParamIsEq<Option<Source>>;
                let _: ::core::cmp::AssertParamIsEq<Option<String>>;

It's not fields comparing. It only checks that type implement Eq.
https://stdrs.dev/nightly/x86_64-unknown-linux-gnu/std/cmp/struct.AssertParamIsEq.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok . I misunderstood and thought this was actually also performing a comparison.

Still, I have no interest in some of these fields implementing Eq or PartialEq, because my comparison is based on only 1 of the fields.

I don't think adding Deriv(Eq) to all the structs and substructs is a cleaner solution than what I have now.

Copy link
Contributor Author

@guibescos guibescos Mar 11, 2022

Choose a reason for hiding this comment

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

The current solution is implementing PartialEq explicitly and empty Eq in the top level structure to explicitly mark it as Eq.

@paul-schaaf
Copy link
Contributor

paul-schaaf commented Apr 4, 2022

please add a test in lang/tests and a changelog entry

(or lmk if I should do it, if you dont have the time)

@paul-schaaf paul-schaaf merged commit 0916361 into coral-xyz:master Apr 20, 2022
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.

= not implemented for anchor_lang::error::Error
3 participants