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

Dyno: Fix (callable variable, function) disambiguation #26454

Merged
merged 13 commits into from
Jan 8, 2025

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Jan 2, 2025

This is a precursor to another PR, which enables resolution of more of the domain module code, and finds this curious case that doesn't work in Dyno:

proc foo(x: int) {}
proc bar(foo: (int, int)) {
  var res = foo(0);
}

In production, we expect the call to foo in the body of bar to be a tuple element access, but dyno incorrectly attempts to perform full function resolution. This is difficult since the inner foo is not a function.

In production, we resolve this problem by stopping the search for functions when we encounter a variable. In the case of an innermost variable, this is sound since variables are not overloaded or disambiguated (innermost is preferred). In the case of functions, this is sound because any functions defined after the variable during the search must be more distant in terms of scopes, and thus would be rejected in favor of "nearer" functions.

I chose to implement this behavior in Dyno, particularly because of a TODO by @riftEmber that seemed to be adjusting for this kind of behavior. To do so in the most performant way (i.e., without iterating over the list of found IDs to see if any were variables), I adjusted all the lookup procedures to return a LookupResult instead of a boolean. This type is boolean-like (it supports operator bool and |=), but it contains two booleans, one of which is whether a variable was found. From there, I adjust the early-return logic (previously: innermostOnly && got) to also handle the case of "I found a variable so I should stop looking".

The only tricky aspect is detecting ambiguity, in which we found a function and non-function at the exact same place. We need to report an error when a variable and a non-variable were found since the last "return point" (last check for innermostOnly && got etc.). For this, I did fall back to an iteration: if a variable is detected in a scope, and there are several copies of that variable, it's almost certainly an error.

Reviewed by @riftEmber -- thanks!

Testing

  • dyno tests
  • paratest

@DanilaFe DanilaFe mentioned this pull request Jan 2, 2025
2 tasks
@riftEmber riftEmber self-requested a review January 6, 2025 19:27
frontend/include/chpl/resolution/scope-types.h Outdated Show resolved Hide resolved
frontend/lib/resolution/scope-queries.cpp Outdated Show resolved Hide resolved
frontend/lib/resolution/scope-queries.cpp Outdated Show resolved Hide resolved
frontend/lib/resolution/scope-queries.cpp Outdated Show resolved Hide resolved
frontend/test/resolution/testResolve.cpp Outdated Show resolved Hide resolved
frontend/test/resolution/testResolve.cpp Outdated Show resolved Hide resolved
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
@DanilaFe DanilaFe merged commit 6188df9 into chapel-lang:main Jan 8, 2025
8 checks passed
DanilaFe added a commit that referenced this pull request Jan 8, 2025
Closes Cray/chapel-private#6923.

Depends on #26454,
#26455 and
#26456 because this causes
more Domain methods to be resolved.

The issue was in resolving code like this:

```Chapel
class C {
  var x: int;
  var y: string;

  iter type myIter() {
    yield 3;
    yield 5;
    yield 7;
    yield 11;
  }
}

for i in C.myIter() do
  writeln(i);
```

Previously, dyno incorrectly set `isMethod` to be false, which made the
`this` formal for methods to be incorrectly set. This PR adjusts this
behavior to correctly track method calls when resolving iterator
overloads.

Reviewed by @benharsh -- thanks!

## Testing
- [x] dyno tests
- [x] paratest
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.

2 participants