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 incorrect resolution of outer variable when method exists with the same name #25878

Merged
merged 16 commits into from
Sep 5, 2024

Conversation

mppf
Copy link
Member

@mppf mppf commented Sep 4, 2024

Resolves #25551
Resolves #22918

This PR adjusts the way the new scope resolver handles looking for matches for methods in order to resolve problems with incorrect matches to methods interfering with finding an outer variable, which is the problem that occurs in #25551.

What is going wrong in #25551 is that the new scope resolver is 1) looking for the innermost match (for an identifier) and 2) searching first for methods from the definition point of myComparator and its parents. The result is to find the unrelated text method instead of the outer variable.

This PR resolves the problem by adjusting the scope resolution process to only consider methods that could be called when considering method receiver scopes.

Instead of pre-computing the receiver scopes and then checking these for methods, the lookup process now works with helper objects to call back into the scope resolution or full resolution process. There are two different helper objects for different purposes:

  1. a MethodLookupHelper provides a way to access the scopes that should be checked for methods/fields and also provides a way to check if the receiver is applicable
  2. a ReceiverScopeHelper provides a way to get a MethodLookupHelper that corresponds to the receiver type for a particular method ID (and at that point, the scopes from the MethodLookupHelper should be checked for applicable methods)

There are two implementations of each of these -- one that uses simplified rules and works without types (for use with the new scope resolution, which is currently enabled in production) -- and one that uses the full type system.

Reviewed by @benharsh - thanks!

  • full comm=none testing

@mppf mppf changed the title Issue 25551 2 Fix incorrect resolution of outer variable when method exists with the same name Sep 4, 2024
mppf added 9 commits September 5, 2024 10:38
---
Signed-off-by: Michael Ferguson <[email protected]>
---
Signed-off-by: Michael Ferguson <[email protected]>
---
Signed-off-by: Michael Ferguson <[email protected]>
---
Signed-off-by: Michael Ferguson <[email protected]>
---
Signed-off-by: Michael Ferguson <[email protected]>
---
Signed-off-by: Michael Ferguson <[email protected]>
mppf added 2 commits September 5, 2024 11:38
Field declarations *can* refer to fields from the receiver
scope (including parent class fields)

---
Signed-off-by: Michael Ferguson <[email protected]>
---
Signed-off-by: Michael Ferguson <[email protected]>
@mppf mppf marked this pull request as ready for review September 5, 2024 16:06
frontend/include/chpl/resolution/scope-types.h Outdated Show resolved Hide resolved
frontend/lib/resolution/resolution-queries.cpp Outdated Show resolved Hide resolved
mppf added 2 commits September 5, 2024 15:23
---
Signed-off-by: Michael Ferguson <[email protected]>
---
Signed-off-by: Michael Ferguson <[email protected]>
@mppf mppf merged commit 8b9cfac into chapel-lang:main Sep 5, 2024
7 checks passed
@mppf mppf deleted the issue-25551-2 branch September 5, 2024 20:03
mppf added a commit that referenced this pull request Sep 16, 2024
This PR updates the language specification to describe the implicit
`this.` that the compiler adds within methods. It also discusses how
shadowing works in such cases.

Follow-up to PR #25878.

Reviewed by @benharsh - thanks!
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.

[Bug]: odd scope resolution failure bug: compiler prefers parenless non-method to method
2 participants