Skip to content

Feature/error equality#1544

Merged
paul-schaaf merged 5 commits into
otter-sec:masterfrom
guibescos:feature/error-equality
Apr 20, 2022
Merged

Feature/error equality#1544
paul-schaaf merged 5 commits into
otter-sec:masterfrom
guibescos:feature/error-equality

Conversation

@guibescos

@guibescos guibescos commented Mar 2, 2022

Copy link
Copy Markdown
Contributor

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.

Comment thread lang/src/error.rs
self.program_error == other.program_error
}
}
impl Eq for ProgramErrorWithOrigin {}

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@guibescos guibescos Mar 4, 2022

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@guibescos guibescos Mar 11, 2022

Copy link
Copy Markdown
Contributor Author

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

paul-schaaf commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

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 otter-sec:master Apr 20, 2022
Otter-0x4ka5h pushed a commit to Otter-0x4ka5h/anchor that referenced this pull request Mar 25, 2026
@jamie-osec jamie-osec added this to the Pre-1.0 milestone Jun 24, 2026
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

4 participants