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: Support non-local receivers and improve resolution of type queries #26038

Merged

Conversation

dlongnecke-cray
Copy link
Contributor

@dlongnecke-cray dlongnecke-cray commented Oct 3, 2024

This PR adds support for non-local receivers to dyno and fixes a bug affecting the resolution of type queries. It adds support for programs like the following:

    class Foo {
      type T;
      proc get(type t) type do return t;

      proc foo() {
        // Here 'nested' is not a method, but 'this' is still visible as
        // an outer variable. It is a non-local receiver from the
        // parent method 'foo'.
        proc nested(x : get(T)) do return x;
        return nested(0);
      }
    }

Refactor the Resolver::methodReceiverType() method to make use of a new method closestMethodReceiverInfo(). Remove getMethodReceiver and streamline how receiver information is computed. When resolving, try to compute a local method receiver using whatever means are available. If that fails, consult stack frames for the first method that lexically encloses the current symbol and use its receiver instead.

In getMethodReceiverScopeHelper() and friend, flatten out the branches used to make the function idempotent and to handle scopeResolveOnly. This makes the typed path for setting up the helper easier to digest.

Fix a bug in type query resolution that caused every call to be treated as though it were a type constructor for a composite type. To do this, compute if type queries are present in children in Resolver::resolveTypeQueries and do nothing if they are not. Secondly, rely on the Resolver::signatureOnly flag to avoid emitting errors unless we are resolving a signature and have access to the types of sub-expressions.

Reviewed by @mppf. Thanks!

Copy link
Member

@mppf mppf left a comment

Choose a reason for hiding this comment

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

My only concern is that, perhaps, we can get the resolver to work more with ReceiverScopeTypedHelper / TypedMethodLookupHelper. The scope resolution process works with these so that it doesn't even need to think about the "closest" method receiver. That should not be relevant to the process.

That said, I think this PR has some good cleanups and gets some more cases working, so we can proceed with it.

// TODO: Is it even legal to reference fields from two different
// receivers? Shouldn't we run into a 'NestedClassFieldRef' error?
// Also, if they were two different instantiations of the same
// composite then this check would be insufficient.
Copy link
Member

Choose a reason for hiding this comment

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

We can consider making this a full-on error for now. Would that break tests in production?

ReceiverScopeTypedHelper(fn->id(), std::move(receiverType));
receiverScopeHelper = &receiverScopeTypedHelper;
}
// barrier to only compute the receiver scopes once
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, I wouldn't use the term "barrier" here since it means something specific (and different) in parallel computing. IMO the comment could just be "only compute the receiver scopes once".

@@ -775,39 +712,100 @@ const MethodLookupHelper* Resolver::getFieldDeclarationLookupHelper() {
return nullptr;
}

if (!methodHelperComputed) {
methodLookupHelper = nullptr;
// barrier to only compute the lookup helper once
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@dlongnecke-cray
Copy link
Contributor Author

Do you mean that maybe we can have those types themselves compute the closest method receiver? I think something will have to walk the stack in some way at some point to get at the types, but I'm not picky about what does it.

@mppf
Copy link
Member

mppf commented Oct 22, 2024

Do you mean that maybe we can have those types themselves compute the closest method receiver? I think something will have to walk the stack in some way at some point to get at the types, but I'm not picky about what does it.

It's more that even the idea of whether or not the "closest method receiver" is relevant (vs all method receivers) should be up to the scope lookup code (according to LOOKUP_INNERMOST). For example, we might have nested methods (where the inner method is a secondary/tertiary method) and then things like x and y could refer to either object. So I think it would be more consistent to always let the LookupHelper code decide when to stop going up the stack, so to speak.

@dlongnecke-cray dlongnecke-cray merged commit 07d6adc into chapel-lang:main Oct 31, 2024
7 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.

3 participants