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: constant type propagation to avoid type shadowing #3213

Closed

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Jan 4, 2023

normally, type checking is idempotent, but in the case of for loop type checking, correct typechecking depends on the "type" field being fresh. this moves the type annotation to a different field which does not shadow "type".

fixes #3212

What I did

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

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

normally, type checking is idempotent, but in the case of for loop type
checking, correct typechecking depends on the "type" field being fresh.
this moves the type annotation to a different field which does not
shadow "type".
@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2023

Codecov Report

Merging #3213 (c0822d1) into master (8ebabc5) will decrease coverage by 33.29%.
The diff coverage is 10.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           master    #3213       +/-   ##
===========================================
- Coverage   88.89%   55.61%   -33.29%     
===========================================
  Files          84       84               
  Lines       10600    10602        +2     
  Branches     2213     2148       -65     
===========================================
- Hits         9423     5896     -3527     
- Misses        769     4162     +3393     
- Partials      408      544      +136     
Impacted Files Coverage Δ
vyper/ast/folding.py 51.26% <0.00%> (-42.02%) ⬇️
vyper/builtins/functions.py 46.79% <0.00%> (-43.69%) ⬇️
vyper/semantics/analysis/utils.py 48.99% <100.00%> (-42.98%) ⬇️

... and 65 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@trocher
Copy link
Contributor

trocher commented Feb 16, 2023

Here is a minimal repro of a contract that now fails to compile with this PR:

@external
def test() -> String[12]:
    a:uint8 = 1
    return uint2str(a)

I believe the issue is that before, the annotation visitor would annotate a's type in uint2str(a) with uint8, and later, during codegen, when building the IR for uint2str(a), it was retrieved using the "shortcut" in get_possible_types_from_node but now as this shortcut as been removed, it must "recompute" the type using types_from_Name but this fails as the namespace is no longer available at this point.

@charles-cooper
Copy link
Member Author

superseded by #3375

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.

incorrect type checking of loop variables
3 participants