Implements the Eq trait for Option

[brandonsurh]

Description

Closes #3542
Implements the Eq trait for Option. Also adds tests for the implementation.

Checklist

• I have linked to any relevant issues.
• I have commented my code, particularly in hard-to-understand areas.
• I have updated the documentation where relevant (API docs, the reference, and the Sway book).
• I have added tests that prove my fix is effective or that my feature works.
• I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
• I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
• I have requested a review from the relevant team or maintainers.

[brandonsurh]

Will need help with adding assignees, adding labels, and requesting reviewers due to permissions.

[brandonsurh]

Not sure why the cargo-run-e2e-test check is failing. I don't think it's related to these changes, but I'm not certain.

[bitzoic]

Not sure why the cargo-run-e2e-test check is failing. I don't think it's related to these changes, but I'm not certain.

Looks like this is a breaking change. You will need to replace the contract address in the e2e test script with 0xe2f370fb01c8c36a521093494a023ce2d7232398e5e88722164c42ec0303f381.

[Braqzen]

I don't believe that we need to check the permutations anymore and all of these negations can probably be cleaned up too.

Besides that, I need to block this until all primitives are tested. An integer and bool are insufficient here despite the trait constraint.

Once all types are tested this will be good to merge

[Braqzen]

Not sure why the cargo-run-e2e-test check is failing. I don't think it's related to these changes, but I'm not certain.

Looks like this is a breaking change. You will need to replace the contract address in the e2e test script with 0xe2f370fb01c8c36a521093494a023ce2d7232398e5e88722164c42ec0303f381.

I don't believe this to be a breaking change. It extends the functionality but does not change the API.
Something somewhere else breaks, that's not a breaking change. That's a side effect of how we wrote the test suite. Once the API is changed for the user (ex. swap the order of parameters in a function) then that qualifies as a breaking change.

[bitzoic]

Not sure why the cargo-run-e2e-test check is failing. I don't think it's related to these changes, but I'm not certain.

Looks like this is a breaking change. You will need to replace the contract address in the e2e test script with 0xe2f370fb01c8c36a521093494a023ce2d7232398e5e88722164c42ec0303f381.

I don't believe this to be a breaking change. It extends the functionality but does not change the API. Something somewhere else breaks, that's not a breaking change. That's a side effect of how we wrote the test suite. Once the API is changed for the user (ex. swap the order of parameters in a function) then that qualifies as a breaking change.

Any changes that can change an existing contract's bytecode and hash must be labeled as a breaking change.

[Braqzen]

Not sure why the cargo-run-e2e-test check is failing. I don't think it's related to these changes, but I'm not certain.

Looks like this is a breaking change. You will need to replace the contract address in the e2e test script with 0xe2f370fb01c8c36a521093494a023ce2d7232398e5e88722164c42ec0303f381.

I don't believe this to be a breaking change. It extends the functionality but does not change the API. Something somewhere else breaks, that's not a breaking change. That's a side effect of how we wrote the test suite. Once the API is changed for the user (ex. swap the order of parameters in a function) then that qualifies as a breaking change.

Any changes that can change an existing contract's bytecode and hash must be labeled as a breaking change.

I'm unsure whether I agree with that. Can you tell me where that reasoning comes from and where this was brought up?

[brandonsurh]

I don't believe that we need to check the permutations anymore and all of these negations can probably be cleaned up too.

Besides that, I need to block this until all primitives are tested. An integer and bool are insufficient here despite the trait constraint.

Once all types are tested this will be good to merge

Cleaned up the permutations and negations. Tests were rewritten and now include the following types that were listed as primitives in the Sway Book:

• u8
• u16
• u32
• u64
• u256
• str
• str[]
• bool
• b256

The only problem is with Eq trait when testing the str[] (String Array) type. The last commit gives the following error:

 error
             --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:119:24
              |
          117 |
          118 |     // Eq tests
          119 |     assert(str_option1 == str_option1);
              |                        ^^ Trait "Eq" is not implemented for type "str[4]".
          120 |     assert(str_option1 == str_option2);
              |
          ____

Not sure what is causing this, but will look into it. It's possible that I am just handling it incorrectly.

[brandonsurh]

Decided to skip over the fixed-length string tests and remove them. They are not within the constraints of the Eq trait. Tests should be good now. Still need to address the failing cargo-run-e2e-test check.

[Braqzen]

Struct, enum, tuple and array tests?

[brandonsurh]

Struct, enum, tuple and array tests?

Wasn't sure if these were within the scope for testing because they aren't primitive types. Though you're right, it's probably a good idea to test them anyways. Tests with them pass, however, implementing the eq method for them come with some warnings about them not being used for some reason. They are most definitely called because if you remove them then the tests break. Because of this, I added expected_warnings = 5 to the test.toml file.

          warning
            --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:14:5
             |
          12 |
          13 |   impl core::ops::Eq for StringArray {
          14 |       fn eq(self, other: Self) -> bool {
             |  _____-
          15 | |         from_str_array(self) != from_str_array(other)
          16 | |     }
             | |_____- This method is never called.
          17 |   }
             |
          ____

          warning
             --> /home/brandon/sway/sway-lib-core/src/ops.sw:536:5
              |
          534 |
          535 |       /// ```
          536 |       fn neq(self, other: Self) -> bool {
              |  _____-
          537 | |         (self.eq(other)).not()
          538 | |     }
              | |_____- This method is never called.
          539 |   }
              |
          ____

          warning
            --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:21:5
             |
          19 |
          20 |   impl core::ops::Eq for Array {
          21 |       fn eq(self, other: Self) -> bool {
             |  _____-
          22 | |         self[0] == other[0] && self[1] == other[1]
          23 | |     }
             | |_____- This method is never called.
          24 |   }
             |
          ____

          warning
            --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:27:5
             |
          25 |
          26 |   impl core::ops::Eq for MyStruct {
          27 |       fn eq(self, other: Self) -> bool {
             |  _____-
          28 | |         self.x == other.x
          29 | |     }
             | |_____- This method is never called.
          30 |   }
             |
          ____

          warning
            --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:34:5
             |
          32 |
          33 |   impl core::ops::Eq for Tuple {
          34 |       fn eq(self, other: Self) -> bool {
             |  _____-
          35 | |         self.0 == other.0 && self.1 == other.1
          36 | |     }
             | |_____- This method is never called.
          37 |   }
             |
          ____

            Compiled script "option_eq" with 5 warnings.

[Braqzen]

Struct, enum, tuple and array tests?

Wasn't sure if these were within the scope for testing because they aren't primitive types. Though you're right, it's probably a good idea to test them anyways. Tests with them pass, however, implementing the eq method for them come with some warnings about them not being used for some reason. They are most definitely called because if you remove them then the tests break. Because of this, I added expected_warnings = 5 to the test.toml file.

          warning
            --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:14:5
             |
          12 |
          13 |   impl core::ops::Eq for StringArray {
          14 |       fn eq(self, other: Self) -> bool {
             |  _____-
          15 | |         from_str_array(self) != from_str_array(other)
          16 | |     }
             | |_____- This method is never called.
          17 |   }
             |
          ____

          warning
             --> /home/brandon/sway/sway-lib-core/src/ops.sw:536:5
              |
          534 |
          535 |       /// ```
          536 |       fn neq(self, other: Self) -> bool {
              |  _____-
          537 | |         (self.eq(other)).not()
          538 | |     }
              | |_____- This method is never called.
          539 |   }
              |
          ____

          warning
            --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:21:5
             |
          19 |
          20 |   impl core::ops::Eq for Array {
          21 |       fn eq(self, other: Self) -> bool {
             |  _____-
          22 | |         self[0] == other[0] && self[1] == other[1]
          23 | |     }
             | |_____- This method is never called.
          24 |   }
             |
          ____

          warning
            --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:27:5
             |
          25 |
          26 |   impl core::ops::Eq for MyStruct {
          27 |       fn eq(self, other: Self) -> bool {
             |  _____-
          28 | |         self.x == other.x
          29 | |     }
             | |_____- This method is never called.
          30 |   }
             |
          ____

          warning
            --> /home/brandon/sway/test/src/e2e_vm_tests/test_programs/should_pass/stdlib/option_eq/src/main.sw:34:5
             |
          32 |
          33 |   impl core::ops::Eq for Tuple {
          34 |       fn eq(self, other: Self) -> bool {
             |  _____-
          35 | |         self.0 == other.0 && self.1 == other.1
          36 | |     }
             | |_____- This method is never called.
          37 |   }
             |
          ____

            Compiled script "option_eq" with 5 warnings.

yeah that's my bad. I meant built-ins but said primitives

[brandonsurh]

The recent PR #5210 that changes the way impl trait functions are handled breaks this PR. Will address this.

[brandonsurh]

The recent PR #5210 that changes the way impl trait functions are handled breaks this PR. Will address this.

Huh, nevermind. Seems like a change within #5313 fixed the issue. Checks passing again!

[brandonsurh]

Seems like this PR doesn't pass the tests on master. The last passing check was probably a false positive since removing the impl block for the Eq trait in option.sw fixes the check. Will check this out.

EDIT: no more errors on master