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.

Sign up for GitHub

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

Jump to bottom

feat: new conversion rules #2694

Merged
merged 47 commits into from
Mar 18, 2022
Merged

feat: new conversion rules #2694

merged 47 commits into from
Mar 18, 2022

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Mar 7, 2022 •
edited
Loading

What I did

fix #2507

Commit message

This commit refactors and changes conversion semantics to be in line
with VIP 2507. The main changes from existing behavior are:

- fewer allowed conversions with address (eg. decimal/address unallowed)
- decimals are bitcasted with bytes
- fixes involving sign extension

By generalizing the conversion rules to not use hardcoded integer/bytes
types, this commit lays some groundwork for more integer/bytes types
since we will not have to add a case for every single type.

This commit is a first pass; the tests should be consolidated and
refactored before release, and some cases involving conversion to bytes
have not been added yet. Also, the logic should probably be cleaned up
so that it is easier to validate/reason about.

Description for the changelog

Cute Animal Picture

image

@charles-cooper
simplify decimal codegen
67a7b57 
a lot of the work is already done in the typechecker
@charles-cooper
wip refactor convert
50af15e
@charles-cooper charles-cooper changed the title Refactor convert feat: new conversion rules Mar 7, 2022
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 7, 2022

This pull request introduces 3 alerts when merging 50af15e into aa8affb - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Syntax error

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 7, 2022

This pull request introduces 11 alerts when merging 7e73219 into aa8affb - view on LGTM.com

new alerts:

  • 7 for Unused import
  • 4 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 8, 2022

This pull request introduces 2 alerts when merging a9f8d99 into aa8affb - view on LGTM.com

new alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Wrong number of arguments in a call

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 9, 2022

This pull request introduces 4 alerts when merging 238949a into aa8affb - view on LGTM.com

new alerts:

  • 2 for Unused import
  • 1 for Conflicting attributes in base classes
  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 10, 2022

This pull request introduces 2 alerts when merging e3a729f into aa8affb - view on LGTM.com

new alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 10, 2022

This pull request introduces 2 alerts when merging fd6f333 into aa8affb - view on LGTM.com

new alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Unused local variable

electriclilies
electriclilies reviewed Mar 11, 2022
View reviewed changes
vyper/builtin_functions/convert.py Outdated
def _input_types(*allowed_types):
def decorator(f):
@functools.wraps(f)
def g(expr, arg, out_typ):
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest better names, expr->convert_expr, arg -> to_convert or something

Copy link
Contributor

Choose a reason for hiding this comment

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

out_type -> cast_type or convert_type

Copy link
Member

Choose a reason for hiding this comment

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

I agree, g is not a great name

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2022 •
edited
Loading

Codecov Report

Merging #2694 (876b822) into master (aa8affb) will increase coverage by 0.55%.
The diff coverage is 88.07%.

❗ Current head 876b822 differs from pull request most recent head c9ab8aa. Consider uploading reports for the commit c9ab8aa to get more accurate results

@@            Coverage Diff             @@
##           master    #2694      +/-   ##
==========================================
+ Coverage   86.43%   86.98%   +0.55%     
==========================================
  Files          91       91              
  Lines        9883     9846      -37     
  Branches     2492     2469      -23     
==========================================
+ Hits         8542     8565      +23     
+ Misses        839      798      -41     
+ Partials      502      483      -19
Impacted Files Coverage Δ
vyper/builtin_functions/signatures.py 71.73% <ø> (+4.23%) ⬆️
vyper/utils.py 85.62% <66.66%> (-0.10%) ⬇️
vyper/codegen/core.py 83.58% <68.18%> (-0.81%) ⬇️
vyper/builtin_functions/convert.py 86.84% <87.33%> (+10.84%) ⬆️
vyper/builtin_functions/functions.py 89.25% <100.00%> (+0.88%) ⬆️
vyper/codegen/expr.py 80.19% <100.00%> (+0.68%) ⬆️
vyper/codegen/types/types.py 86.73% <100.00%> (+1.81%) ⬆️
vyper/lll/optimizer.py 86.59% <100.00%> (+0.30%) ⬆️
vyper/semantics/types/abstract.py 100.00% <100.00%> (ø)
vyper/semantics/types/value/bytes_fixed.py 100.00% <100.00%> (ø)
... and 12 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 aa8affb...c9ab8aa. Read the comment docs.

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 12, 2022

This pull request introduces 2 alerts when merging d4f1797 into cfe26e1 - view on LGTM.com

new alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 14, 2022

This pull request introduces 2 alerts when merging c916f58 into cfe26e1 - view on LGTM.com

new alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 16, 2022

This pull request introduces 2 alerts when merging 4ed8435 into cfe26e1 - view on LGTM.com

new alerts:

  • 1 for Conflicting attributes in base classes
  • 1 for Unused local variable

@lgtm-com
Copy link
Contributor

lgtm-com bot commented Mar 16, 2022

This pull request introduces 1 alert when merging 6838dd0 into cfe26e1 - view on LGTM.com

new alerts:

  • 1 for Unused local variable

@charles-cooper
fix bytesM hierarchy
7401b8f
@charles-cooper
add some notes
0ba250e
@charles-cooper
feat: generalize conversion rules
876b822 
This commit refactors and changes conversion semantics to be in line
with VIP 2507. The main changes from existing behavior are:

- fewer allowed conversions with address (eg. decimal/address unallowed)
- decimals are bitcasted with bytes
- fixes involving sign extension

By generalizing the conversion rules to not use hardcoded integer/bytes
types, this commit lays some groundwork for more integer/bytes types
since we will not have to add a case for every single type.

This commit is a first pass; the tests should be consolidated and
refactored before release, and some cases involving conversion to bytes
have not been added yet. Also, the logic should probably be cleaned up
so that it is easier to validate/reason about.
@charles-cooper charles-cooper marked this pull request as ready for review March 18, 2022 17:29
fubuloubu
fubuloubu reviewed Mar 18, 2022
View reviewed changes
tests/parser/functions/test_convert_to_int128.py Show resolved Hide resolved
vyper/builtin_functions/functions.py Outdated Show resolved Hide resolved
vyper/codegen/types/types.py Show resolved Hide resolved
vyper/builtin_functions/convert.py Outdated
def _input_types(*allowed_types):
def decorator(f):
@functools.wraps(f)
def g(expr, arg, out_typ):
Copy link
Member

Choose a reason for hiding this comment

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

I agree, g is not a great name

@charles-cooper charles-cooper enabled auto-merge (squash) March 18, 2022 18:32
@charles-cooper charles-cooper merged commit a125a88 into vyperlang:master Mar 18, 2022
@charles-cooper charles-cooper deleted the refactor_convert branch March 18, 2022 23:05
@tserg tserg mentioned this pull request Mar 21, 2022
Merged
This was referenced Mar 22, 2022
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electriclilies electriclilies electriclilies left review comments

@tserg tserg tserg left review comments

@fubuloubu fubuloubu fubuloubu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

VIP: Simplify+generalize conversion rules
5 participants
@charles-cooper @codecov-commenter @fubuloubu @tserg @electriclilies