Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
dyno: Add resolution of nested functions with outer variables (#25273)
This PR adds support for resolving calls to nested functions that
reference outer variables.
It adds a new type called `ResolutionContext*` that is passed instead of
the `Context*` to resolution functions that require it. The
`ResolutionContext*` is used to power a new type of query, the
`CHPL_RESOLUTION_QUERY...`. These queries enable stack frames for parent
functions to be consulted while maintaining a strong _invariant_: any
query computation that references state from a mutable parent frame will
not store its results in the global `Context*` query cache.
With stack frames for functions present, outer variables can be typed in
programs like the following:
```chapel
proc foo() {
var x = 0;
proc bar() { writeln(x); }
// Notice that the outer variable usage actually occurs in the sibling 'bar'.
// The current implementation will handle this because it occurs during
// resolution when the call to 'bar()' can be resolved.
proc baz() { bar(); }
baz();
}
foo();
```
## Interior calls to nested functions cause a problem for the query
framework
As Chapel exists today, only _interior_ calls to nested functions can be
written - calls such as `bar()` or `baz()` in the example above. An
_interior_ call is a call to a nested function that is issued within its
parent or a sibling function.
Interior calls present a unique problem. When the call `bar()` is being
resolved, the procedures `baz()` _and_ `foo()` are also in the process
of being resolved! While a function is being resolved it is a big blob
of mutable state - and many details of resolution are not finalized
until several AST walks have been completed.
One of the requirements of the query framework is that all the inputs to
a query function are _immutable_: they must be values, or pointers to
constants. When the procedure `bar()` is resolved, it will ask `foo()`
for the type of `x`. Since `foo()` is still being resolved at that time,
the answer that `foo()` produces (though correct) is taken from its
mutable `ResolutionResultsByPostorderID`.
On its own, this would not be a problem, as the type of `x` is still
just a value. However, the _stack frames_ used to retrieve `x` are also
a critical part of computing it, and they cannot easily be captured by
the query framework. If not all of the inputs required to perform a
computation can be stored by the query framework, then the computation
can't be a query!
(This is not to say that we cannot represent the stack frames as a query
input in some other way. E.g., one of the followup ideas thrown around
is to create some sort of trace, similar to what is done for POIs, used
to indicate where all the outer variables came from. But it is not 100%
clear what needs to be in such a type.)
## The new `ResolutionContext` type
This PR adds a new type called `ResolutionContext*` that is passed
instead of the `Context*` to resolution functions that require it. The
`ResolutionContext*` is used to power a new type of query, the
`CHPL_RESOLUTION_QUERY...`.
These new "resolution context queries" are written _exactly_ the same as
regular `QUERY_...` queries, except the first argument must be of type
`ResolutionContext*`:
```cpp
const TypedFnSignature* typedSignatureInitial(ResolutionContext* rc, UntypedFnSignature* ufs) {
CHPL_RESOLUTION_QUERY_BEGIN(typedSignatureInitial, rc, ufs);
auto ret = ...; // Some computation...
return CHPL_RESOLUTION_QUERY_END(ret);
}
```
The `ResolutionContext` (abbreviated as `RC`) contains zero or more
stack frames of the type `ResolutionContext::Frame`. Right now, frames
are pushed when a `Resolver` for a function is created, and are popped
when that `Resolver` is destroyed.
A "resolution context query" will not always cache its computations in
the "global query cache" that is maintained in the `Context*`. At any
point in time, the `RC` is either _stable_ or _unstable_. When the `RC`
is unstable, that means it contains one or more
`ResolutionContext::Frames` that encapsulate a `Resolver`. This
`Resolver` is mutable and is being mutated during an AST walk for some
computation - usually a call to `resolveFunction` for a parent
function(s).
When the resolution context query begins, the `RC` and the query inputs
are consulted. If the `RC` is _stable_, then that means no mutable state
is present, so the global query cache can always be used. If the `RC` is
_unstable_, then the query inputs determine if the global query cache
can be consulted. Right now, if any input ID is for a nested function,
the global query cache cannot be used. This is a coarse filter and can
be adjusted (along with many other aspects of a resolution context
query) by specializing query components on a per-query basis.
When the global query cache cannot be used, in most cases this means the
query's computation will be performed every single time (as though it is
uncached). However, it is possible for queries to specialize the
"unstable cache" behavior and to fetch/store results within mutable
resolver frames. This is done for `resolveFunction` to prevent interior
calls to nested functions from resulting in the nested function's body
being resolved repeatedly.
## History of this effort
Originally (any commit before 07ed1d5), this PR adopted a strategy of
passing along an extra argument (called `CallerDetails`) to resolution
functions which required it. This type modeled stack frames. When a
signature or function required an outer variable, the resolver would
walk up stack frames to find the variable.
Because the global context query cache requires immutable inputs, any
time stack frames were consulted to compute a result, the result could
not be stored in the context query cache. This **invariant** was
maintained in an impromptu manner, usually by adding the following
branch to queries that required it:
```cpp
if (parsing::idIsNestedFunction(context, id)) {
ret = theComputationImpl(context, ..., stackFrames);
} else {
ret = theComputationQuery(context, ...);
}
```
Not only does this make the code harder to digest, it makes it hard to
maintain the invariant (and thus the _correctness_ of the query
framework) due to the possibility of developer error.
After discussions with @benharsh and @mppf we came up with the following
sort of ideas which could make this effort more maintainable going
forward:
- The idea of a `ResolutionContext` type which could be passed instead
of the `Context*`, removing the extra `CallerDetails&` argument
- The idea of a new type of query which can be used to ensure the
invariant and avoid caching computations when the context is in an
_unstable_ state
Some things, like the `ResolutionContext` and the
`ResolutionContext::Frame`, were very easy to add, as they were just
adaptations of existing code. However, I really struggled with
implementing the idea of a new query that contained and conditionally
ran a traditional query.
At first, I tried to embed the `QUERY_BEGIN` macros inside of new
macros, but this very quickly became impossible to edit, understand, or
maintain. However, I also ran into a more fundamental problem.
The entire context query framework is implemented around the idea of a
singular "query function", that must have a certain shape:
- The function must return by `const&`.
- The function must take the `Context*` as the first argument.
- The remaining arguments must always be captured by value as if by
`std::decay`.
But these new "resolution query" functions would have a new type
`ResolutionContext*` as the first argument, so they violated the second
requirement.
My first strategy to solve this problem was to embed a secondary, hidden
function inside of the query function which has a different signature:
```cpp
void someNewResolutionQuery(ResolutionContext* rc, ArgType arg) {
// Declared deep within a macro invocation:
struct hidden__ {
static auto theGlobalQuery(Context* context, ArgPackTuple ap) {
}
};
// User code...
}
```
The function had to be embedded within a struct so that it could have a
user-writeable name (lambdas are anonymous). There was a kernel of my
final code present in this attempt, but it had another problem: the
function `hidden__::theGlobalQuery` was not visible outside of
`someNewResolutionQuery`! It could not be used in inactive stores, that
is: `QUERY_STORE_RESULT` and friends.
Along with this embedded struct, I also implemented the "global portion"
of the query end using what were effectively `STORE_RESULT` calls. This
would come back to bite me later: inactive stores are NOT the same as
`QUERY_END`, and encapsulating the user's computation within some form
of `QUERY_BEGIN` and `QUERY_END` would prove essential to tracking query
dependencies!
---
Eventually, I got fed up with writing code embedded in giant macros, and
started to think about a way around that. The problem with the
`QUERY...` macros is that they are not extensible, because all names
must be at the outermost scope in order to be referenced in both
`QUERY_BEGIN` and `QUERY_END`.
I eventually realized that a good workaround is to move all the names
that were at the top level, into a templated `struct` instead. This has
the added advantage of allowing me to write 99% of the code as code that
is not embedded in macros. I am very happy with how readable the code
ultimately ended up looking. A new C++17 feature, "`auto` value template
parameters", enabled me to capture the resolution query functions as a
template value in a very succinct manner.
As iterations over this idea continued, I eventually realized that I had
to insert `QUERY_BEGIN` and `QUERY_END` calls into my new query guards
in order to ensure that global dependencies were tracked. But I could
not embed them in the `ResolutionContext::Query` struct methods because
they are implemented by declaring hidden variables at the top level of a
function.
Ultimately, I had to reach into the body of the `QUERY...` macros and
reimplement them as calls within my new struct methods. As I was working
on this, I realized that I had implemented two types:
- A `GlobalQuery`, which essentially reimplements the body of the
`QUERY...` functions in a more readable and maintainable fashion.
- A `ResolutionContext::Query`, which is a wrapper around a
`GlobalQuery` that is required to uphold the _invariant_.
Future work should take this new implementation and utilize it to
implement the `QUERY...` macro functions.
TESTING
- [x] `linux64`, `standard`
Thanks to @mppf for a thorough review and pointers on future work.
Thanks to @mppf and @benharsh for brainstorming the `ResolutionContext`
and the `CHPL_RESOLUTION_QUERY...`. And thanks to the dyno team for
their patience!
FUTURE WORK
See: Cray/chapel-private#6721- Loading branch information
