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

Remove unchecked arithmetic #1831

Merged
merged 14 commits into from Aug 28, 2023
Merged

Remove unchecked arithmetic #1831

merged 14 commits into from Aug 28, 2023

Conversation

athei
Copy link
Contributor

@athei athei commented Jul 2, 2023

We need to remove all unchecked arithmetic from ink! itself so our CI does not fail after use-ink/cargo-contract#1190 is merged.

I trivially transformed all unchecked arithmetic to checked_* + unwrap which preserves the current behaviour which was to panic on overflow but makes it explicit. I had to inline the arrayref macro so that I were able to remove the unchecked arithmetic from it.

@@ -267,7 +267,10 @@ mod erc1155 {

// Given that TokenId is a `u128` the likelihood of this overflowing is pretty
// slim.
self.token_id_nonce += 1;
#[allow(clippy::arithmetic_side_effects)]
Copy link
Contributor

@SkymanOne SkymanOne Jul 2, 2023

Choose a reason for hiding this comment

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

I'm not fun of a this. Is there a reason we can't do checked arithmetic operation here? If the operation is not implemented for the type, then we need to implement it first before introducing the change here and in cargo contract.

Otherwise we are breaking the arithmetic safety guarantees.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can. But it is not necessary here. Overflowing a u128 is impossible by incrementing by 1 per extrinsic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do I understand correctly that we won't be able to use arithmetic operators (i.e. +, -, /, *) on numbers at all? I am just thinking of what impact it will have on the developer experience if we have to tell everyone knows that they need to use checked_ operations specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I understand correctly that we won't be able to use arithmetic operators (i.e. +, -, /, *) on numbers at all?

Correct.

I am just thinking of what impact it will have on the developer experience if we have to tell everyone knows that they need to use checked_ operations specifically.

Or saturating or wrapping. Developers should express the overflow behaviour in-code instead of leaving it open to a compile time switch. We should have done that from the beginning. People have to think about this anyways.

Copy link

Choose a reason for hiding this comment

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

Or saturating or wrapping.

Or use Wrapping and Saturating, so then the arithmetic itself wouldn't become an unreadable soup of function calls, e.g. this compiles:

#![deny(clippy::arithmetic_side_effects)]
use core::num::Wrapping;

fn foo(a: Wrapping<u32>, b: Wrapping<u32>) -> Wrapping<u32> {
    a + b
}

(Saturating is still unstable though, but we could just provide our own wrapper for the time being.)

@athei
Copy link
Contributor Author

athei commented Aug 21, 2023

Should be merged before new cargo contract release or the CI will break.

Copy link
Contributor

@SkymanOne SkymanOne left a comment

Choose a reason for hiding this comment

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

Can be merged now

@athei athei merged commit 3d57446 into master Aug 28, 2023
22 checks passed
@athei athei deleted the at/remove-unchecked-arithmetic branch August 28, 2023 13:02
@SkymanOne SkymanOne mentioned this pull request Mar 4, 2024
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.

None yet

3 participants