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

Add support for more numeric types #131

Merged
merged 2 commits into from
Nov 30, 2020

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Nov 23, 2020

What was wrong?

We did not support other numeric types as u256.

How was it fixed?

Added support for u8, u16, u32, u64 and u128

To-Do

  • use in storage
  • use as value
  • basic arithmetic
  • abi encoding

@cburgdorf cburgdorf marked this pull request as draft November 23, 2020 14:43
@codecov-io
Copy link

codecov-io commented Nov 23, 2020

Codecov Report

Merging #131 (6fc01aa) into master (7aeca1a) will decrease coverage by 0.50%.
The diff coverage is 72.22%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
- Coverage   84.26%   83.76%   -0.51%     
==========================================
  Files          43       43              
  Lines        2759     2797      +38     
==========================================
+ Hits         2325     2343      +18     
- Misses        434      454      +20     
Impacted Files Coverage Δ
compiler/src/abi/elements.rs 82.53% <0.00%> (-7.12%) ⬇️
semantics/src/traversal/declarations.rs 95.65% <ø> (ø)
semantics/src/namespace/types.rs 59.35% <50.00%> (-3.25%) ⬇️
semantics/src/traversal/expressions.rs 80.72% <80.00%> (-0.29%) ⬇️
compiler/src/abi/builder.rs 89.21% <100.00%> (+0.55%) ⬆️
compiler/src/yul/abi/functions.rs 54.36% <100.00%> (ø)
compiler/src/yul/abi/operations.rs 75.00% <100.00%> (ø)
compiler/src/yul/abi/utils.rs 98.14% <100.00%> (ø)
compiler/src/yul/mappers/assignments.rs 88.37% <100.00%> (ø)
compiler/src/yul/mappers/declarations.rs 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7aeca1a...6fc01aa. Read the comment docs.

@@ -266,6 +267,7 @@ fn test_assert() {
case("return_u256_from_called_fn.fe", vec![], Some(u256_token(42))),
case("return_u256.fe", vec![], Some(u256_token(42))),
case("return_identity_u256.fe", vec![42], Some(u256_token(42))),
case("return_identity_u128.fe", vec![42], Some(u256_token(42))),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@g-r-a-n-t
This test passes but I'm not 100 % sure if I'm testing the right thing because the inputs (vec![42]) are converted to u256token

input
                .clone()
                .into_iter()
                .map(|val| u256_token(val))
                .collect(),

I thought I need to create a u128_token variant of that but ethabi::Token::Uint simply expects a U256 so maybe it's just something I don't understand 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

The ABI encoding for uints is the same for every size, so it shouldn't really matter in practice how we encode uints for these tests. We might just want to rename u256_token to uint_token.

The difference in sizes of uints should only really be relevant internally when it comes to packing values in memory and storage more efficiently.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That makes sense, thanks!

@cburgdorf cburgdorf force-pushed the christoph/feat/numerics branch 3 times, most recently from abb488c to c326d9f Compare November 25, 2020 10:47
@cburgdorf cburgdorf marked this pull request as ready for review November 25, 2020 14:33
@cburgdorf cburgdorf marked this pull request as draft November 26, 2020 10:27
@cburgdorf cburgdorf force-pushed the christoph/feat/numerics branch 3 times, most recently from f46185c to 7ba1118 Compare November 26, 2020 16:08
case("return_u256_from_called_fn_with_args.fe", vec![], Some(uint_token(200))),
case("return_u256_from_called_fn.fe", vec![], Some(uint_token(42))),
case("return_u256.fe", vec![], Some(uint_token(42))),
// case("return_u128.fe", vec![], Some(uint_token(42))),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This seems like a tricky case to me. We currently assume numeric literals to always be of type U256. In the case of something like...

contract Foo:
    pub def bar() -> u128:
        return 42

...it seems the compiler should be able to infer that 42 should be of type u128. But I'm not sure yet how I would do that. It seems impossible to assign a type directly in expr_num without knowing how it is being used. If it is being used as part of a return (e.g. return 42) it would be reasonable to lookup the return value of the function but I'm not sure if expr_num should try to figure out the context of the usage. So, I'm wondering if expr_num should instead just say that this is of numeric type (could be any one at that point) and leave it up to e.g. the return to type check it. E.g. we could introduce Base::AnyNumeric which would be used as a placeholder for situations like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another idea. Instead of introducing a variant such as AnyNumeric. We could do something like this:

pub enum Base {
    Numeric(Option<NumericSize>),
    Bool,
    Byte,
    Address,
}

#[derive(Clone, Debug, PartialEq, PartialOrd, Ord, Eq)]
pub enum NumericSize {
    U256,
    I256,
    U128,
    I128,
    U64,
    I64
    U32,
    I32
    U16,
    I16,
    U8,
    I8
}

So, expr_num would resolve to Base::Numeric(None) which is to say it is a numeric type but it could be any of them at this point. In practice though, it wouldn't be any of them because e.g. if I write return 40000 then it can't fit into u8 or u16 but it could still be anything from u32 (or i32) upward. So, I'm still not sure how exactly this should be dealt with. It feels like what we would like to express in expr_num would be a list of allowed types and then leave it to other code to narrow it down.

Anyway, looking forward to discuss that with you after the holidays.

Copy link
Member

Choose a reason for hiding this comment

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

contract Foo:
    pub def bar() -> u128:
        return 42

I think having 42 cast to a u128 implicitly makes sense, but for now, it might be easier to just require the use of type constructors. So it could instead look like this:

contract Foo:
    pub def bar() -> u128:
        return u128(42)

In the example above, 42 would be given the type u256 (as would be the case with any positive literals) and then the return of the constructor would be given type u128. With this approach, there's no mystery about the type of an expression, so we don't need to use an optional, which I think would simplify things using the numeric type.

When we get around to introducing implicit casts, we could do something similar to move_location, where we would have an attribute like cast_type: Optional<Type> that could be set by the expression's context if it needs to be cast to a certain type (in this case return 42 would cast the return expression to u128). We can worry about this later though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, yes, requiring to go through a type constructor strikes me as a pragmatic solution to keep things simple for now 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've put this into its own issue to keep this PR short and focused #136

@cburgdorf cburgdorf force-pushed the christoph/feat/numerics branch 2 times, most recently from dafe080 to 4c1cf7f Compare November 30, 2020 10:54
@cburgdorf cburgdorf marked this pull request as ready for review November 30, 2020 11:01
U32,
U16,
U8,
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My idea is to let the Integer enum host everyting from u8..u256 to i8..i256. This is to keep the hierarchy shorter and hence simplify matching etc.

U8,
}

pub const U256: Base = Base::Numeric(Integer::U256);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I created this as a shortcut since it is used in quite a bunch of tests etc

@cburgdorf cburgdorf requested a review from g-r-a-n-t November 30, 2020 11:06
@cburgdorf
Copy link
Collaborator Author

I think this is ready for review @g-r-a-n-t

Copy link
Member

@g-r-a-n-t g-r-a-n-t left a comment

Choose a reason for hiding this comment

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

Looks good, just one comment about the FeSized impl.

@@ -192,7 +204,7 @@ impl FixedSize {
impl FeSized for Base {
fn size(&self) -> usize {
match self {
Base::U256 => 32,
Base::Numeric(_) => 32,
Copy link
Member

Choose a reason for hiding this comment

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

We should have FeSized implemented for Integer and then this arm would look something like Base::Numeric(integer) => integer.size(). Each of the sizes would be the same as the ABI data sizes below.

If we don't, there will be issues reading and writing to arrays that have been decoded and packed in memory. It also results in memory savings (e.g. u8[10] would occupy 10 bytes as opposed to 320).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, makes sense! I just pushed the update.

@cburgdorf cburgdorf force-pushed the christoph/feat/numerics branch from 4c1cf7f to 6fc01aa Compare November 30, 2020 16:52
@cburgdorf cburgdorf merged commit adbaeb0 into ethereum:master Nov 30, 2020
@g-r-a-n-t g-r-a-n-t mentioned this pull request Dec 1, 2020
4 tasks
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.

3 participants