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

Ensure integer type constructor reject anything but numeric literals #163

Merged

Conversation

cburgdorf
Copy link
Collaborator

@cburgdorf cburgdorf commented Dec 14, 2020

What was wrong?

Currently, our integer type constructor are too permissive. E.g. the following is allowed today even though it isn't guaranteed that val will fit into the u16)

    pub def bar(val: u8) -> u16:
        return u16(val)

I believe that our integer type constructor should simply not allow being used with anything other than a numeric literal such as 1 or -3.

Numeric literals can be checked on compile time whereas arbitrary variables can't.

How was it fixed?

  • Added a function validate_is_numeric_literal and used it inside expr_call to validate
  • Added a new error type NumericLiteralExpected
  • Added a test to prove bad code is rejected.

Notice that this does not yet entail #145 but it is paving the way for it.

To-Do

  • OPTIONAL: Update Spec if applicable

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

@codecov-io
Copy link

codecov-io commented Dec 14, 2020

Codecov Report

Merging #163 (2a5648c) into master (00121f5) will decrease coverage by 0.09%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #163      +/-   ##
==========================================
- Coverage   81.39%   81.29%   -0.10%     
==========================================
  Files          44       44              
  Lines        3133     3144      +11     
==========================================
+ Hits         2550     2556       +6     
- Misses        583      588       +5     
Impacted Files Coverage Δ
semantics/src/errors.rs 5.26% <0.00%> (-0.30%) ⬇️
semantics/src/traversal/expressions.rs 75.97% <71.42%> (-0.19%) ⬇️
compiler/src/abi/elements.rs 73.23% <0.00%> (-0.68%) ⬇️

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 00121f5...2a5648c. Read the comment docs.

@cburgdorf cburgdorf force-pushed the christoph/feat/stricter-type-contstructors branch from 37992e5 to a5cfb32 Compare December 14, 2020 18:11
@cburgdorf cburgdorf changed the title Christoph/feat/stricter type contstructors Ensure integer type constructor reject anything but numeric literals Dec 14, 2020
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.

Makes sense. We can perform compile time checks on literals like this and introduce methods like my_num.as_i128() for runtime casting.

@cburgdorf cburgdorf force-pushed the christoph/feat/stricter-type-contstructors branch from a5cfb32 to 2a5648c Compare December 15, 2020 10:14
@cburgdorf
Copy link
Collaborator Author

introduce methods like my_num.as_i128() for runtime casting.

Good idea. I'm writing up an issue for this. Eventually we might open up something like u32(x) in scenarios where x is <= 32 bit.

@cburgdorf
Copy link
Collaborator Author

I created an issue for the runtime conversions #165

@cburgdorf cburgdorf merged commit 220185f into ethereum:master Dec 15, 2020
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