feat [breaking changes]: Extensible VirtualMachineError and removed PartialEq trait #783

zarboq · 2023-01-26T10:47:35Z

Extensible VirtualMachineError and removed PartialEq trait

Description

Linked to this issue #751 I added VirtualMachineError::Other(anyhow::Error) to allow to returing custom errors when using cairo-rs

Also I removed PartialEq trait from VirtualMachineError enum as I found it was a bad practice through my research with @tdelabro help

See: https://users.rust-lang.org/t/help-understanding-io-error-and-lack-of-partialeq/13212

std::io::Error does not implement PartialEq (https://doc.rust-lang.org/std/io/struct.Error.html)

I have replaced assert_eq!() in tests with assert!(matches!()) (https://stackoverflow.com/questions/57234140/how-to-assert-io-errors-in-rust)

Checklist

Linked to Github Issue
Unit tests added
Integration tests added.
This change requires new documentation.
- Documentation has been added/updated.

tdelabro

use assert_matches
fix variable names
try to make your assert in one single line when possible
improve the Other error variant by use of thiserror features

src/hint_processor/builtin_hint_processor/blake2s_utils.rs

src/vm/errors/vm_exception.rs

src/vm/runners/builtin_runner/mod.rs

src/vm/security.rs

src/vm/errors/vm_errors.rs

src/vm/errors/vm_exception.rs

Oppen · 2023-01-27T16:09:35Z

Is there any chance to require the new error to be PartialEq instead? Or is the Any type restrictive about that?

tdelabro · 2023-01-27T18:20:32Z

@Oppen

Is there any chance to require the new error to be PartialEq instead?

It is not something you want to do. It would prevent users to use any error types that do not implement PartialEq, and a lot of them don't. An example here is std::io::Error: https://doc.rust-lang.org/std/io/struct.Error.html. It does not implement PartialEq so users wouldn't be able to pass this type in the Other variant.

The reason why you want PartialEq is because it makes writing your tests slightly easier than using assert_matches!, but nowhere in the actual runtime code is this type of comparison used, you always handle your errors using match, which is the correct way to do. IMO it's not a good enough reason to impair the lib users' ability to pass their custom error types.

Or is the Any type restrictive about that?

Because the Other variant does not have any information about the error by itself, it's the responsibility of the type it contains to carry this information. That's why we want it to implement the trait Error (anyhow::Error being kind of superset for this trait, adding thread safety, and other nice features. More about it here: https://docs.rs/anyhow/latest/anyhow/struct.Error.html). This way, we can use the #[transparent] feature of thiserror and display the inner error directly.

In conclusion, there is not really a way around it. If we want an escape variant for users' custom errors, it has to contain an Error type, and we cannot expect it to implement PartialEq without impairing their ability to pass their custom types.

tdelabro · 2023-01-27T18:27:43Z

See also this comment: #776 (comment)

Oppen · 2023-01-27T20:26:59Z

It is not something you want to do. It would prevent users to use any error types that do not implement PartialEq, and a lot of them don't. An example here is std::io::Error: https://doc.rust-lang.org/std/io/struct.Error.html. It does not implement PartialEq so users wouldn't be able to pass this type in the Other variant.

Sad, but we'll live.

Oppen

Overall this looks good. There are a few minor comments and we need to update the change log before it can be merged, specially with regards to interface changes.

I'm a bit sad about the added verbosity, but I can't think of any way to avoid it so 🤷

Cargo.toml

src/lib.rs

src/vm/runners/builtin_runner/keccak.rs

src/vm/runners/builtin_runner/mod.rs

Oppen · 2023-01-31T04:28:33Z

src/vm/vm_core.rs

        );
-        assert_eq!(error.unwrap_err().to_string(), "Inconsistent auto-deduction for builtin ec_op, expected 2739017437753868763038285897969098325279422804143820990343394856167768859289, got Some(Int(2778063437308421278851140253538604815869848682781135193774472480292420096757))");
+        //assert_eq!(error.unwrap_err().to_string(), "Inconsistent auto-deduction for builtin ec_op, expected 2739017437753868763038285897969098325279422804143820990343394856167768859289, got Some(Int(2778063437308421278851140253538604815869848682781135193774472480292420096757))");


Why are we no longer checking this? Also, why leave the commented test?

This part has not been addressed.

tests/bitwise_test.rs

tests/pedersen_test.rs

tests/skip_instruction_test.rs

tests/struct_test.rs

src/vm/errors/vm_errors.rs

zarboq · 2023-02-03T15:16:28Z

Review changes applied @Oppen :)

Sorry for the delay

tdelabro · 2023-02-08T12:25:53Z

@fmoletta @Oppen Does it seems mergeable in this current state?

fmoletta

Looks Good!

tdelabro · 2023-02-10T08:50:01Z

@Oppen You can resolve the open conversations and merge?

Oppen · 2023-02-13T14:35:11Z

@tdelabro on it, sorry for the delay.

Oppen

Hi @hurrikaanig @tdelabro. There are a few more things that need changing. They are tiny and should be quick and easy to fix. I talked with Timothée and he said it's OK if @Juan-M-V updates the PR with those changes to get them applied sooner. I think we can get this merged today 🎉

Oppen · 2023-02-13T14:38:59Z

src/hint_processor/builtin_hint_processor/find_element_hint.rs

            run_hint!(vm, ids_data, hint_code::FIND_ELEMENT),
            Err(HintError::Internal(VirtualMachineError::ExpectedInteger(
-                relocatable
+                _relocatable


Shouldn't this also check we have the same relocatable value?

Oppen · 2023-02-13T14:39:18Z

src/hint_processor/builtin_hint_processor/find_element_hint.rs

            run_hint!(vm, ids_data, hint_code::SEARCH_SORTED_LOWER),
-            Err(HintError::ValueOutOfRange(Felt::zero()))
+            Err(HintError::ValueOutOfRange(x)) if x == Felt::zero()


Suggested change

Err(HintError::ValueOutOfRange(x)) if x == Felt::zero()

Err(HintError::ValueOutOfRange(x)) if x.is_zero()

Oppen · 2023-02-13T14:45:21Z

src/hint_processor/builtin_hint_processor/set.rs

            run_hint!(vm, ids_data, HINT_CODE),
            Err(HintError::Internal(VirtualMachineError::ValueNotPositive(
-                int
+                _int


This should compare with the actual value, no?

Oppen · 2023-02-13T14:47:57Z

src/types/relocatable.rs

    }

    #[test]
    fn add_bigint_to_relocatable() {
        let addr = MaybeRelocatable::RelocatableValue(relocatable!(7, 65));
-        let added_addr = addr.add_int(&Felt::new(2));
-        assert_eq!(Ok(MaybeRelocatable::from((7, 67))), added_addr);
+        let _added_addr = addr.add_int(&Felt::new(2));


This value is used, isn't it? Let's remove the underscore.

Oppen · 2023-02-13T14:48:29Z

src/types/relocatable.rs

+        // assert_eq!(
+        //     error.unwrap_err().to_string(),
+        //     "Offset 18446744073709551616 exceeds maximum offset value"
+        // );


This doesn't look like it should be commented.

Oppen · 2023-02-13T15:10:59Z

src/types/relocatable.rs

+            Ok::<MaybeRelocatable, VirtualMachineError>(MaybeRelocatable::RelocatableValue(
+                relocatable!(7, 17)
+            )),
+            _added_addr


Oppen · 2023-02-13T15:11:06Z

src/types/relocatable.rs

+            Ok::<MaybeRelocatable, VirtualMachineError>(MaybeRelocatable::RelocatableValue(
+                relocatable!(7, 14)
+            )),
+            _added_addr


Oppen · 2023-02-13T15:11:22Z

src/types/relocatable.rs

+        let _sub_addr = addr_a.sub(addr_b);
+        assert_matches!(
+            Ok::<MaybeRelocatable, VirtualMachineError>(MaybeRelocatable::from(Felt::new(2))),
+            _sub_addr


Oppen · 2023-02-13T15:11:29Z

src/types/relocatable.rs

+        let _sub_addr = addr_a.sub(addr_b);
+        assert_matches!(
+            Ok::<MaybeRelocatable, VirtualMachineError>(MaybeRelocatable::from(Felt::new(10))),
+            _sub_addr


Oppen · 2023-02-13T15:15:34Z

src/vm/vm_memory/memory.rs

-            mem.get(&MaybeRelocatable::from((1, 0))),
-            _val_clone
-        ));
+        assert_matches!(mem.get(&MaybeRelocatable::from((1, 0))), _val_clone);


I'm not sure I understand this particular test. I don't see a definition of _val_clone, and if it's just a new name for the returned value wouldn't this always match?

codecov · 2023-02-13T18:13:03Z

Codecov Report

Merging #783 (5a28268) into main (ca01985) will decrease coverage by 0.69%.
The diff coverage is 82.83%.

❗ Current head 5a28268 differs from pull request most recent head c0d94ba. Consider uploading reports for the commit c0d94ba to get more accurate results

@@            Coverage Diff             @@
##             main     #783      +/-   ##
==========================================
- Coverage   97.81%   97.13%   -0.69%     
==========================================
  Files          69       69              
  Lines       29380    28827     -553     
==========================================
- Hits        28739    28001     -738     
- Misses        641      826     +185

Impacted Files	Coverage Δ
src/serde/deserialize_program.rs	`97.85% <0.00%> (-0.10%)`	⬇️
...uiltin_hint_processor/cairo_keccak/keccak_hints.rs	`93.53% <20.00%> (-1.85%)`	⬇️
src/vm/trace/mod.rs	`92.50% <33.33%> (-2.80%)`	⬇️
..._processor/builtin_hint_processor/secp/ec_utils.rs	`98.98% <36.36%> (-1.02%)`	⬇️
src/vm/runners/cairo_runner.rs	`97.89% <44.77%> (-1.10%)`	⬇️
...cessor/builtin_hint_processor/secp/bigint_utils.rs	`97.75% <50.00%> (-2.25%)`	⬇️
...processor/builtin_hint_processor/secp/signature.rs	`98.67% <62.50%> (-1.33%)`	⬇️
src/hint_processor/hint_processor_utils.rs	`86.61% <62.50%> (-3.47%)`	⬇️
...t_processor/builtin_hint_processor/dict_manager.rs	`95.72% <66.66%> (-0.77%)`	⬇️
src/vm/security.rs	`98.43% <70.00%> (-1.57%)`	⬇️
... and 61 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…e_virtualmachine_error

Oppen · 2023-02-13T19:02:28Z

src/math_utils.rs

    }

    #[test]
    fn calculate_isqrt_c() {
        let n = biguint_str!(
            "3618502788666131213697322783095070105623107215331596699973092056135872020481"
        );
-        assert_eq!(isqrt(&n.pow(2_u32)), Ok(n));
+        assert_matches!(isqrt(&n.pow(2_u32)), Ok(_n));


Let's compare the value as well.

Resolved: bce4570

Oppen · 2023-02-13T19:02:35Z

src/math_utils.rs

    }

    #[test]
    fn calculate_isqrt_zero() {
        let n = BigUint::zero();
-        assert_eq!(isqrt(&n), Ok(BigUint::zero()));
+        assert_matches!(isqrt(&n), Ok(_n));


Resolved: bce4570

Oppen · 2023-02-13T19:03:04Z

src/types/relocatable.rs

+        let _added_addr = addr.add_int(&Felt::new(2i32));
+        assert_matches!(
+            Ok::<MaybeRelocatable, VirtualMachineError>(MaybeRelocatable::Int(Felt::new(9i32))),
+            _added_addr


Resolved 0247165

Oppen · 2023-02-13T19:03:11Z

src/types/relocatable.rs

-        let added_addr = addr.add_usize(2);
-        assert_eq!(MaybeRelocatable::Int(Felt::new(9_i32)), added_addr);
+        let _added_addr = addr.add_usize(2);
+        assert_eq!(MaybeRelocatable::Int(Felt::new(9_i32)), _added_addr);


Resolved: 0247165

Oppen · 2023-02-13T19:06:25Z

src/vm/runners/builtin_runner/ec_op.rs

@@ -934,7 +934,7 @@ mod tests {
        ];
        let builtin = EcOpBuiltinRunner::new(&EcOpInstanceDef::default(), true);

-        let _error = builtin.deduce_memory_cell(&Relocatable::from((3, 6)), &memory);
+        let _expected_error = builtin.deduce_memory_cell(&Relocatable::from((3, 6)), &memory);
        /*assert_eq!(


This shouldn't be commented out I think. And _expected_error should be used.

Resolved: 220c537

Oppen · 2023-02-13T19:07:10Z

src/math_utils.rs

    }

    #[test]
    fn calculate_isqrt_b() {
        let n = biguint_str!("4573659632505831259480");
-        assert_eq!(isqrt(&n.pow(2_u32)), Ok(n));
+        assert_matches!(isqrt(&n.pow(2_u32)), Ok(_n));


Compare here.

Resolved: c0d94ba

zarboq requested review from igaray, Oppen, fmoletta, entropidelic, HermanObst, juanbono, Jrigada, pefontana and Juan-M-V as code owners January 26, 2023 10:47

tdelabro suggested changes Jan 26, 2023

View reviewed changes

zarboq force-pushed the extensible_virtualmachine_error branch 2 times, most recently from 81ff3ba to 148140e Compare January 27, 2023 12:09

tdelabro approved these changes Jan 27, 2023

View reviewed changes

src/vm/errors/vm_exception.rs Outdated Show resolved Hide resolved

zarboq force-pushed the extensible_virtualmachine_error branch 2 times, most recently from 1afc853 to a6e35f9 Compare January 27, 2023 15:27

zarboq force-pushed the extensible_virtualmachine_error branch from a6e35f9 to edbd652 Compare January 27, 2023 16:52

Oppen requested changes Jan 31, 2023

View reviewed changes

fmoletta reviewed Jan 31, 2023

View reviewed changes

src/vm/errors/vm_errors.rs Outdated Show resolved Hide resolved

zarboq force-pushed the extensible_virtualmachine_error branch from edbd652 to 2d6f092 Compare February 3, 2023 12:43

feat: Extensible VirtualMachinError and removed PartialEq trait

bef7716

zarboq force-pushed the extensible_virtualmachine_error branch from 2d6f092 to bef7716 Compare February 3, 2023 14:26

fmoletta approved these changes Feb 9, 2023

View reviewed changes

Oppen requested changes Feb 13, 2023

View reviewed changes

Fix matches in tests

c61d8f8

Juanma added 3 commits February 13, 2023 15:21

Fix match in find_element_hint test

6e50def

Merge branch 'main' of github.com:lambdaclass/cairo-rs into extensibl…

a3e3e1e

…e_virtualmachine_error

Merge with main; remove assert_eq

5a28268

Oppen requested changes Feb 13, 2023

View reviewed changes

Juanma added 4 commits February 13, 2023 16:11

Fix math_utils tests

bce4570

Fix relocatable tests

0247165

Remove uneeded test

220c537

Fix calculate_isqrt_b test

c0d94ba

Oppen approved these changes Feb 13, 2023

View reviewed changes

Juan-M-V added this pull request to the merge queue Feb 13, 2023

Merged via the queue into lambdaclass:main with commit 8e6c953 Feb 13, 2023

	Err(HintError::ValueOutOfRange(x)) if x == Felt::zero()
	Err(HintError::ValueOutOfRange(x)) if x.is_zero()

feat [breaking changes]: Extensible VirtualMachineError and removed PartialEq trait #783

feat [breaking changes]: Extensible VirtualMachineError and removed PartialEq trait #783

Conversation

zarboq commented Jan 26, 2023

Extensible VirtualMachineError and removed PartialEq trait

Description

Checklist

tdelabro left a comment • edited Loading

Choose a reason for hiding this comment

Oppen commented Jan 27, 2023

tdelabro commented Jan 27, 2023 • edited Loading

tdelabro commented Jan 27, 2023

Oppen commented Jan 27, 2023

Oppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zarboq commented Feb 3, 2023

tdelabro commented Feb 8, 2023

fmoletta left a comment

Choose a reason for hiding this comment

tdelabro commented Feb 10, 2023 • edited Loading

Oppen commented Feb 13, 2023

Oppen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdelabro left a comment •

edited

Loading

tdelabro commented Jan 27, 2023 •

edited

Loading

tdelabro commented Feb 10, 2023 •

edited

Loading

codecov bot commented Feb 13, 2023 •

edited

Loading