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 resolving 'this' call on shadowing variable #25954

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

riftEmber
Copy link
Member

@riftEmber riftEmber commented Sep 17, 2024

Fix a dyno bug preventing resolution of 'this' calls on a variable that shadows another.

The problem stemmed from looking up the identifier we call 'this' on as though it were a function. In function resolution, we look for all visible identifiers rather than just the innermost. However, for an implicit 'this' call on a var, we want to look up the (innermost) var, then call 'this' on it. Fixed by just resolving to the innermost result between an ambiguous set of variables.

Also adds testing for the fixed case, as well as that a similar case (correct ambiguity between methods in analogous scopes) isn't broken.

Resolves https://github.com/Cray/chapel-private/issues/6706.

Note to reviewer: Went though several iterations, so changes are probably easiest to review as a whole rather than by commit.

[reviewer info placeholder]

Testing:

  • dyno tests
  • paratest

@riftEmber riftEmber force-pushed the dyno-shadowed-this branch 2 times, most recently from b23a3c8 to c39fd0f Compare September 17, 2024 19:37
@riftEmber riftEmber marked this pull request as ready for review September 17, 2024 20:00
@riftEmber riftEmber force-pushed the dyno-shadowed-this branch 5 times, most recently from a33b27b to 9262dfd Compare September 18, 2024 19:58
@riftEmber riftEmber requested a review from mppf September 18, 2024 20:05
frontend/lib/resolution/Resolver.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants