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

fix: prevent range over decimal #3798

Merged

Conversation

trocher
Copy link
Contributor

@trocher trocher commented Feb 21, 2024

What I did

looping over a range was allowed if the iterator type was decimal, this PR prevents this unintended behaviour.

How I did it

  • Ensure the type of the iterator is an IntegerT.
  • Ensure the literals in the range() expression are not only Num but Int.

How to verify it

See tests

Commit message

fix: prevent range over decimal

Description for the changelog

prevent range over decimal

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@codecov-commenter
Copy link

codecov-commenter commented Feb 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (015cf81) 85.00% compared to head (68b79bb) 84.99%.
Report is 2 commits behind head on master.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3798      +/-   ##
==========================================
- Coverage   85.00%   84.99%   -0.01%     
==========================================
  Files          92       92              
  Lines       13765    13771       +6     
  Branches     3083     3084       +1     
==========================================
+ Hits        11701    11705       +4     
- Misses       1575     1576       +1     
- Partials      489      490       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -512,6 +513,10 @@ def visit_For(self, node):

iter_var = None
if isinstance(node.iter, vy_ast.Call):
if not isinstance(target_type, IntegerT):
raise TypeCheckFailure(
Copy link
Member

Choose a reason for hiding this comment

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

i'd prefer to reuse the exception handling / error message from validate_expected_type

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 also raise TypeMismatch (a regular exception) rather than a TypeCheckFailure (which is a panic)

hm yea the naming is confusing, we should really rename TypeCheckFailure to TypeCheckPanic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced TypeCheckFailure by TypeMismatch and now use validate_expected_type

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

thanks!

@charles-cooper charles-cooper enabled auto-merge (squash) February 22, 2024 02:18
@charles-cooper charles-cooper merged commit 9a31ee6 into vyperlang:master Feb 22, 2024
84 checks passed
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.

4 participants