feat: add Nullable deref_or method

[enitrat]

• Adds a deref_or method to Nullable

Closes #3854

---

This change is 

[enitrat]

Hi @orizi @yuvalsw would it be possible to have a review on this?

Also, what's a good rule to follow when inlining functions? My rule of thumb depended on an arbitrary number of lines / complexity of the function (which is why most one-liner into, try_into fns have been inlined) but I would like to know what you think about it.

I just noticed that tests failed but I'm not sure how to interpret that - it seems that it modifies the CASM code generation, which doesn't match the expected data anymore

[orizi]

Generally, don't add inline for single line no side effect extern function calls, as these will be auto inlined mostly.
Regarding Onable - it is an internal trait, don't add things into it. Instead we may add One and Zero traits matching the corresponding rust ones.

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @enitrat)

---

corelib/src/integer.cairo line 1463 at r2 (raw file):

}

impl Felt252IntoFelt252 of Into<felt252, felt252> {

I believe there already is a T into T impl, this would cause duplicated impl.

[enitrat]

Generally, don't add inline for single line no side effect extern function calls, as these will be auto inlined mostly.

Does that mean a function like

    fn try_into(self: felt252) -> Option<u16> {
        u16_try_from_felt252(self)
    }

will be inlined automatically?

Regarding Onable - it is an internal trait, don't add things into it. Instead we may add One and Zero traits matching the corresponding rust ones.

Not sure I understood that - do I need to remove something related to Oneable in my PR?

[yuvalsw]

Reviewed 1 of 3 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @enitrat)

---

corelib/src/nullable.cairo line 21 at r3 (raw file):

    fn deref_or<impl TDrop: Drop<T>>(self: Nullable<T>, default: T) -> T;
    fn new(value: T) -> Nullable<T>;
}

While you are here, please remove and use #[generate_trait] on the impl. Thanks!

Code quote:

trait NullableTrait<T> {
    fn deref(self: Nullable<T>) -> T;
    fn deref_or<impl TDrop: Drop<T>>(self: Nullable<T>, default: T) -> T;
    fn new(value: T) -> Nullable<T>;
}

---

corelib/src/nullable.cairo line 31 at r3 (raw file):

    }

    fn deref_or<impl TDrop: Drop<T>>(self: Nullable<T>, default: T) -> T {

Suggestion:

+Drop<T>

[orizi]

it is - and yes - probably revert all you have done there.
as you added another impl Onable for felt - which we don't want, and added inlines, which are not relevant.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @enitrat)

[enitrat]

as you added another impl Onable for felt

I wasn't able to find the implementation of Oneable<felt252>. Where is it exactly?

[orizi]

It didn't exist - and Onable is an internal trait - i'd rather not have any additional impls for it if we encourage usage of a One and Zero traits instead.

Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @enitrat and @yuvalsw)

[yuvalsw]

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @enitrat)

[enitrat]

It didn't exist - and Onable is an internal trait - i'd rather not have any additional impls for it if we encourage usage of a One and Zero traits instead.

edit: Just noticed this sentence

it is an internal trait, don't add things into it. Instead we may add One and Zero traits matching the corresponding rust ones.

What exactly do you mean by "Internal" trait? Should it not be used externally? And if Zeroable is implemented for felt252, why not for Oneable?

---

I think there's a misunderstanding here - are you talking about Oneable (not Onable), which is not an internal trait an is defined in math.cairo ?

cairo/corelib/src/math.cairo

Lines 91 to 101 in 8977d3b

	// === Oneable ===
	trait Oneable<T> {
	/// Returns the multiplicative identity element of Self, 1.
	fn one() -> T;
	/// Returns whether self is equal to 1, the multiplicative identity element.
	fn is_one(self: T) -> bool;
	/// Returns whether self is not equal to 1, the multiplicative identity element.
	fn is_non_one(self: T) -> bool;
	}

I think the One and Zero traits you're mentioning are actually Oneable and Zeroable - and the change in my PR is the implementation of Oneable<felt252>, similar to Zeroable<felt252> implemented here

cairo/corelib/src/zeroable.cairo

Lines 12 to 24 in 8977d3b

	impl Felt252Zeroable of Zeroable<felt252> {
	fn zero() -> felt252 {
	0
	}
	#[inline(always)]
	fn is_zero(self: felt252) -> bool {
	self == 0
	}
	#[inline(always)]
	fn is_non_zero(self: felt252) -> bool {
	!self.is_zero()
	}
	}

[orizi]

Oneable is an internal trait defined in math - it was added only for the implementation of inv_mod for secp.
We want to have other traits similar to rust for One and Zero - which would not be internal.
therefore i don't want to add addtional impls for traits i do not want to extend.

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @enitrat)

[enitrat]

Ok, then I can continue the work started #3891 in another PR

[enitrat]

Required changes have been applied. This PR now only implements the deref_or method on Nullable.

[orizi]

Reviewed 1 of 3 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @enitrat)