This is a really obvious problem in retrospect, which makes me feel rather
silly.
The output was useful, but I don't have time to deal with this any further
right now. The comments in the commit explain the problem---that the output
ends up being interpolated as part of the fixpoint test, in an incorrect
context, and so the code that we generate is invalid. Also goes to show why
the fixpoint tests are important.
(Yes, they're still disabled for meta-interp, I'm trying to get them
enabled.)
DEV-13163
The provided documentation provides rationale, and the use case is the
ontree change. I was uncomfortable without the exhaustive match, and I was
further annoyed by the lack of easy `ObjectIndex` narrowing.
DEV-13163
This introduces the ability to specify an edge ordering for the ontological
tree traversal. `tree_reconstruction` will now use a
`SourceCompatibleTreeEdgeOrder`, which will traverse the graph in an order
that will result in a properly ordered source reconstruction. This is
needed for template headers, because interpolation causes
metavariables (exposed as template params) to be mixed into the body.
There's a lot of information here, including some TODOs on possible
improvements. I used the unstable `is_sorted` to output how many template
were already sorted, based on one of our very large packages internally that
uses templates extensively, and found that none of the desugared shorthand
template expansions were already ordered. If I tweak that a bit, then
nearly all templates will already be ordered, reducing the work that needs
to be done, leaving only template definitions with interpolation to be
concerned about, which is infrequent relative to everything else.
DEV-13163
Well, this is both good news and bad news.
The good news is that this finally produces the expected output and
reconstructs sources from interpolated values on the ASG. Yay!
...the bad news is that it's wrong. Notice how the fixpoint test is
disabled.
So, my plan was originally to commit it like this first and see if I was
comfortable relaxing the convention that `<param>` nodes had to appear in
the header. That's nice to do, that's cleaner to do, but would the
XSLT-based compiler really care? I had to investigate.
Well, turns out that TAMER does care. Because, well over a decade ago, I
re-used `<param>`, which could represent not only a template param, but also
a global param, or a function param.
So, XML->NIR considers all `<param>` nodes at the head of a template to be
template parameters. But after the first non-header element, we transition
to another state that allows it to be pretty much anything.
And so, I can't relax that restriction.
And because of that, I can't just stream the tree to the xmli generator,
I'll have to queue up nodes and order them.
Oh well, I tried.
DEV-13163
I'm not sure how I overlooked this previously, and I didn't notice until
trying to generate xmli output. I think I distracted myself with the
use of dangling status, which was not appropriate, and that has since
changed so that we have a dedicated concept.
This introduces the term "instantiation", or more specifically "lexical
instantiation". This is more specific and meaningful than simply
"expansion", which is what occurs during instantiation. I'll try to adjust
terminology and make things more consistent as I go.
DEV-13163
This logic ought to live alongside other definition logic...which in turn
needs its own extraction, but that's a separate concern.
This makes the definition of abstract identifiers very similar to
concrete. But, treating these as dangling, even if that's technically true,
has to change---we still want an edge drawn to the abstract identifier via
e.g. a template since we want the graph to mirror the structure of what it
will expand into concretely. I didn't notice this problem until trying to
generate the xmli for it.
So, see the commit to follow.
DEV-13163
This handles the common cases for meta, which includes what interpolation
desugars into. Most of this work was in testing and reasoning about the
issue; `asg::graph::visit:ontree::test` has a good summary of the structure
of the graph that results.
The last remaining steps to make this work end-to-end is for NIR->AIR to
lower `Nir::Ref` into `Air::BindIdent`, and then for `asg::graph::xmli` to
reconstruct concatenation lists. I'll then be able to commit the xmli test
case I've been sitting on, whose errors have been guiding my development.
DEV-13163
The term "metasyntactic" made sense literally---it's a variable in a
metalanguage that expands into a context that is able to contribute to the
language's syntax. But, the term has a different conventional use in
programming that is misleading.
The term "metalinguistic" is used in mathematics, to describe a metalanguage
or schema atop of a language. This is more fitting.
DEV-13163
This makes `SPair` construction more concise, getting rid of the `into`
invocations. For now I have only made this change in AIR's tests, since
that's what I'm working on and I want to observe how this convention
evolves. This may also encourage other changes, e.g. placing spans within
the `toks` array, rather than having to jump around the test for them.
The comment for `spair` mentions why this is a test-only function. But it
also shows how dangerous `impl Into<SymbolId> for &str` can be, since it
seems so innocuous---it uses a global interner. I'll be interested to see a
year from now if I decided to forego that impl in favor of explicit
internment, since I'm not sure it's worth the convenience anymore.
DEV-13163
This has been bothering me for quite a long time, and is just more test
cleanup before I introduce more. I suspect this came from habit with the
previous Rust edition where `into_iter()` on arrays was a much more verbose
operation.
To be clear: this change isn't for performance. It's about not doing
something silly when it's unnecessary, which also sets a bad example for
others.
There are many other tests in other modules that will need updating at some
point.
DEV-13163
This produces a representation of abstract identifiers on the graph, for
`Expr`s at least. The next step will probably be to get this working
end-to-end in the xmli output before extending it to the other remaining
bindable contexts.
DEV-13163
This enforces the naming convention that is utilized to infer whether an
identifier binding must be translated to an abstract binding.
This does not yet place any restrictions on other characters in identifier
names; both the placement of and flexibility of that has yet to be
decided. This change is sufficient enough to make abstract binding
translation reliable.
DEV-13163
The previous commit made me uncomfortable; we're already parsing with great
precision (and effort!) the grammar of NIR, and know for certain whether
we're in a metavariable binding context, so it makes no sense to have to try
to guess at another point in the lowering pipeline.
This introduces a new token to retain that information from XIR->NIR
lowering and then re-simplifies the lowering operation that was just
introduced in the previous commit (`AbstractBindTranslate`).
DEV-13163
This builds upon the concepts of the previous commit to translate identifier
binding into an abstract binding if it utilizes a symbol that follows a
metavariable naming convention.
See the provided documentation for more information.
This commit _does not_ integrate this into the lowering pipeline yet, since
the abstract identifiers are still rejected (as TODOs) by AIR.
DEV-13163
This introduces the notion of an abstract identifier, where the previous
identifiers are concrete. This serves as a compromise to either introducing
a new object type (another `Ident`), or having every `Ident` name be defined
by a `Meta` edge, which would bloat the graph significantly.
This change causes interpolation within a bind context to desugar into a new
`BindIdentAbstract` token, but AIR will throw an error if it encounters it
for now; that implementation will come soon.
This does not yet handle non-interpolation cases,
e.g. `<classify as="@foo@">`. This is a well-established shorthand for
`as="{@foo@}"`, but is unfortunately ambiguous in the context of
metavariable definitions (template parameters). This language ambiguity
will have to be handled here, and will have to fall back to today's behavior
of assuming concrete in that `param/@name` context but abstract every else,
unless of course interpolation is triggered using `{}` to disambiguate (as
in `<param name="{@foo@}"`).
I was going to handle the short-hand meta binding case as part of
interpolation, but I decided it may be appropriate for its own lowering
operation, since it is intended to work regardless of whether interpolation
takes place; it's a _translation_ of a binding into an abstract one, and it
can clearly delineate the awkward syntactic rules that we have to inherit,
as mentioned above.
DEV-13163
This prepares to make the name of an `Ident` optional to support abstract
identifiers derived from metavariables.
This is an unfortunate change to have to prepare for, since it complicates
how Idents are interpreted, but the alternative (a new object type) is not
good either. We'll see how this evolves.
DEV-13163
This is intended to support NIR's lexical interpolation, which expands in
place into metavariables.
This commit does not yet contain the NIR portion (or xmli system test)
because Meta needs to be able to handle concatenation first; that's next.
DEV-13163
This introduces template/param and regenerates it in the xmli output. Note
that this does not check that applications reference known params; that's a
later phase.
DEV-13163
The previous commit introduced support for a `.experimental` file to tigger
`xmlo-experimental`. This modifies the error message for unsupported
features to make mention of it to help to the user track down the problem.
DEV-13162
If a source file is paired with a `.experimental` file (for example,
`foo.xml` has a silbing `foo.experimental` file), then it will be
precompiled using `--emit xmlo-experimental` instead of `--emit
xmlo`. Further, the contents of the experimental file may contain
`#`-prefixed comments describing why it exists, as well as additional
options to pass to `tamec`.
For example, if this is an experimental file:
```
--foo
--bar=baz
```
Then the tamec invocation will contain:
tamec [...] --emit xmlo-experimental --foo --bar=baz -o foo.xmli
This allows for package-level conditional compilation with new features so
that I am able to focus on packages that will provide the most meaningful
benefits to our team, whether they be performance or features.
DEV-13162
Now that the feature flag for the parser is a command line option, it is
useful to be able to run it on any package and see what errors arise, to use
as a guide for development with the goal of getting a particular package to
compile.
This converts the TODO panic into a recoverable error so that the parser can
spit out as many errors as it can.
DEV-13162
This provides some initial information to help guide a user to discover how
TAMER works, though either the source code or the generated
documentation. This will improve over time, since all of the high-level
abstractions are still under development.
DEV-13162
This introduces `xmlo-experimental` for `--emit`, allowing the new parser to
be toggled selectively for individual packages. This has a few notable
benefits:
1. We'll be able to conditionally compile packages as they are
supported (TAMER will target specific packages in our system to try to
achieve certain results more quickly);
2. This cleans up the code a bit by removing awkward gated logic, allowing
natural abstractions to form; and
3. Removing the compile-time feature flag ensures that the new features
are always built and tested; there are fewer configuration combinations
to test.
DEV-13162
This flag should have never been sprinkled here; it makes the system much
harder to understand.
But, this is working toward a command-line tamec option to toggle NIR
lowering on/off for various packages.
DEV-13162
This was a significant undertaking, with a few thrown-out approaches. The
documentation describes what approach was taken, but I'd also like to
provide some insight into the approaches that I rejected for various
reasons, or because they simply didn't work.
The problem that this commit tries to solve is encapsulation of error
types.
Prior to the introduction of the lowering pipeline macro
`lower_pipeline!`, all pipelines were written by hand using `Lower` and
specifying the applicable types. This included creating sum types to
accommodate each of the errors so that `Lower` could widen automatically.
The introduction of the `lower_pipeline!` macro resolved the boilerplate and
type complexity concerns for the parsers by allowing the pipeline to be
concisely declared. However, it still accepted an error sum type `ER` for
recoverable errors, which meant that we had to break a level of
encapsulation, peering into the pipeline to know both what parsers were in
play and what their error types were.
These error sum types were also the source of a lot of tedious boilerplate
that made adding new parsers to the pipeline unnecessarily unpleasant;
the purpose of the macro is to make composition both easy and clear, and
error types were undermining it.
Another benefit of sum types per pipeline is that callers need only
aggregate those pipeline types, if they care about them, rather than every
error type used as a component of the pipeline.
So, this commit generates the error types. Doing so was non-trivial.
Associated Types and Lifetimes
------------------------------
Error types are associated with their `ParseState` as
`ParseState::Error`. As described in this commit, TAMER's approach to
errors is that they never contain non-static lifetimes; interning and
copying are used to that effect. And, indeed, no errors in TAMER have
lifetimes.
But, some `ParseState`s may. In this case, `AsgTreeToXirf`:
```
impl<'a> ParseState for AsgTreeToXirf<'a> {
// [...]
type Error = AsgTreeToXirfError;
// [...]
}
```
Even though `AsgTreeToXirfError` does not have a lifetime, the `ParseState`
it is associated with _does_`. So to reference that type, we must use
`<AsgTreeToXirf<'a> as ParseState>::Error`. So if we have a sum type:
```
enum Sum<'a> {
// ^^ oh no! vv
AsgTreeToXirfError(<AsgTreeToXirf<'a> as ParseState>::Error),
}
```
There's no way to elide or make anonymous that lifetime, since it's not
used, at the time of writing. `for<'a>` also cannot be used in this
context.
The solution in this commit is to use a macro (`lower_error_sum`) to rewrite
lifetimes: to `'static`:
```
enum Sum {
AsgTreeToXirfError(<AsgTreeToXirf<'static> as ParseState>::Error),
}
```
The `Error` associated type will resolve to `AsgTreeToXirfError` all the
same either way, since it has no lifetimes of its own, letalone any
referencing trait bounds.
That's not to say that we _couldn't_ support lifetimes as long as they're
attached to context, but we have no need to at the moment, and it adds
significant cognitive overhead. Further, the diagnostic system doesn't deal
in lifetimes, and so would need reworking as well. Not worth it.
An alternative solution to this that was rejected is an explicitly `Error`
type in the macro application:
```
// in the lowering pipeline
|> AsgTreeToXirf<'a> { // lifetime
type Error = AsgTreeToXirfError; // no lifetime
}
```
But this requires peeling back the `ParseState` to see what its error is and
_duplicate_ it here. Silly, and it breaks encapsulation, since the lowering
pipeline is supposed to return its own error type.
Yet another option considered was to standardize a submodule convention
whereby each `ParseState` would have a module exporting `Error`, among other
types. This would decouple it from the parent type. However, we still have
the duplication between that and an associated type. Further, there's no
way to enforce this convention (effectively a module API)---the macro would
just fail in obscure ways, at least with `macro_rules!`. It would have been
an ugly kluge.
Overlapping Error Types
-----------------------
Another concern with generating the sum type, resolved in a previous commit,
was overlapping error types, which prohibited `impl From<E> for ER`
generation.
The problem with that a number of `ParseState`s used `Infallible` as their
`Error` type. This was resolved in a previous commit by creating
Infallible-like newtypes (variantless enums).
This was not the only option. `From` fits naturally into how TAMER handles
sum types, and fits naturally into `Lower`'s `WidenedError`. The
alternative is generating explicit `map_err`s in `lower_pipeline!`. This
would have allowed for overlapping error types because the _caller_ knows
what the correct target variant is in the sum type.
The problem with an explicit `map_err` is that it places more power in
`lower_pipeline!`, which is _supposed_ to be a macro that simply removes
boilerplate; it's not supposed to increase expressiveness. It's also not
fun dealing with complexity in macros; they're much more confusing that
normal code.
With the decided-upon approach (newtypes + `From`), hand-written `Lower`
pipelines are just as expressive---just more verbose---as `lower_pipeline!`,
and handles widening for you. Rust's type system will also handle the
complexity of widening automatically for us without us having to reason
about it in the macro. This is not always desirable, but in this case, I
feel that it is.
This configures the pipeline and returns a closure that can then be provided
with the source and sink.
The next obvious step would be to curry the source and sink.
But I wanted to commit this before I take a different (but equivalent)
approach that makes the pipeline operations more explicit and helps to guide
the user (developer) in developing and composing them. The FP approach is
less boilerplate, but is also more general and provides less
guidance. Given that composition at the topmost levels of the system,
especially with all the types involved, is one of the most confusing aspects
of the system---and one of the most important to get right and make clear,
since it's intended to elucidate the entire system at a high level, and
guide the reader. Well, it does a poor job at that now, but that's the
ultimate goal.
In essence---brutally general abstractions make sense at lower levels, but
the complexity at higher levels benefits from rigid guardrails, even though
it does not necessitate it.
DEV-13162
This cleanup is an interesting one, because I think the present me may
disagree with the past me.
The use of generics here to compose the parser from smaller parsers was due
to how I wrote my object-oriented code in other languages: where a class was
an independently tested unit. I was trying to reproduce the same here,
utilizing generics in the same way that one would use compoisition via
object constructors in other languages.
But it's been a long time since then, and I've come to settle on different
standards in Rust. The components of `XmloReader` really are just
implementation details. As I find myself about to want to modify its
behavior, I don't _want_ to compose `XmloReader` from _different_ parsers;
that may result in an invalid parse. There's one correct way to parse an
xmlo file.
If I want to parse the file differently, then `XmloReader` ought to expose
a way of doing so. This is more rigid, but that rigidity buys us confidence
that the system has been explicitly designed to support those
operations. And that confidence gives us peace of mind knowing that the
system won't compose in ways that we don't intend for it to.
Of course, I _could_ design the system to compose in generic ways. But
that's an over-generalization that I don't think will be helpful; it's not
only a greater cognitive burden, but it's also a lot more work to ensure
that invariants are properly upheld and to design an API that will ensure
that parsing is always correct. It's simply not worth it.
So, this makes `XmloReader` consistent with other parsers now, like
`AirAggregate` and nir::parse (ele_parse). This prepares for a change to
make `XmloReader` configurable to avoid loading fragments from object files,
since that's very wasteful for `tamec`.
DEV-13162
More information will be presented in the commit that follows to generalize
these, but this sets the stage.
The recently-introduced pipeline macro takes care of most of the job of a
declarative pipeline, but it's still leaky, since it requires that the
_caller_ create error sum types. This not only exposes implementation
details and so undermines the goal of making pipelines easy to declare and
compose, but it's also one of the last major components of boilerplate for
the lowering pipeline.
My previous attempts at generating error sum types automatically for
pipelines ran into a problem because of overlapping `impl`s for the various
`<S as ParseState>::Error` types; this resolves that issue via
newtypes. I had considered other approaches, including explicitly
generating code to `map_err` as part of the lowering pipeline, but in the
end this is the easier way to reason about things that also keeps manual
`Lower` pipelines on the same level of expressiveness as the pipeline macro;
I want to restrict its unique capabilities as much as possible to
elimination of boilerplate and nothing more.
DEV-13162
At or around 00492ace01, I modified packages
to output canonical `@name`s, which contains a leading forward
slash. Previously, names omitted that slash. I did not believe that this
caused any problems.
It seems that the XSLT-based `standalones` system utilizes this package name
to derive a supplier name, which is supposed to be the filename of the
package without any path. Since the package name changed from
`suppliers/foo` to `/suppliers/foo`, for example, this was now producing
"suppliers/name" instead of "name".
Of course, it was never a good idea to strip off only the first path
component. But, this is how it has been since TAME was originally created
well over a decade ago.
I did not catch this since I was diff'ing the output of the xmle files, not
the final JS files. I had thought that was sufficient, given what I was
changing, but I was wrong.
DEV-14502
I had never intended to avoid pinning nightly. This is an unfortunate thing
to have to do---require a _specific_ version of a compiler to build your
software; it's madness. But the unstable features utilized by TAMER (as
rationalized in `src/lib.rs`) are still worth the effort.
It's not _actually_ that case that we need a specific version of the
compiler, granted; this is outlined in `rust-toolchain.toml`'s
rationale. You should look there for more information; my approach still
utilizes explicit channels via cargo. Unfortunately, I had hard-coded it
previously, putting me in a bit of a bind an unable to override the behavior
without modifying the software.
The reason for this change is that `adt_const_params` has a BC break
involving the introduction of `ConstParamTy`. This is only the second time
I've been bitten by a nightly BC break; the other was the renaming of
`int_log`'s API, as mentioned in
709291b107. This pinning will in fact
mitigate those future issues---TAMER will be able to resolve the issue at
its leisure, and will further be able to continue to build earlier commits
in the future by simply re-bootstrapping with the committed nightly
version.
If you're curious of my rationale for wanting to inhibit toolchain
downloading during build, or use system libraries, have a look at GNU Guix's
approach to building software safely and reproducibly. In particular,
dependencies are also built from source (rather than downloading binaries
from external sources), and builds take place in network-isolated
containers. The `TAMER_RUST_TOOLCHAIN` configure parameter is meant to
facilitate these situations by giving more flexibility to packagers.
DEV-14476
The code utilizing this is flagged, and so the build would output warnings
saying that it was not used. This resolves that (I've been aware of it for
far too long; I'm developing behind the `wip-asg-derived-xmli` flag where I
don't usually see it).
DEV-13162
This generates some documentation helping to describe the lowering pipeline,
since the function type signature can be daunting to those unfamiliar with
it (and I'm sure to the future me too).
DEV-13162
Like the previous commit's removal of the error type, this eliminates the
explicit source token type since we're able to infer it from the pipeline
definition.
DEV-13162
It does not matter what the error of the source is as long as the caller is
able to deal with it, especially given that the particular error is a
property of the source, which is under control of the caller.
DEV-13162
The macro is off-putting and more complicated than the pipeline definitions
themselves (of course), so this tucks it away so that readers are able to
more easily observe the definitions that they're probably looking for
without feeling compelled to try to understand the macro definition.
DEV-13162
All lowering pipelines are now using `lower_pipeline!`. Finally.
The macro does require some refactoring and documentation, but it's working,
and we now have three pipelines whose definitions are smaller than a single
one was previously. I've been hoping to do this for many months, so it's
nice to finally see this come to fruition.
I had been putting it off, but doing so has made it difficult to compose
other parts of the system, not knowing what abstractions I'll have at my
disposal.
DEV-13162
This makes the sink similar to other pipelines without creating a new
ParseState, and so will allow for integrating into the `lower_pipeline!`
abstraction.
DEV-13162
This has been the ultimate goal for the pipeline for some time---the ability
to declaratively define the lowering pipeline in a way that is clear,
concise, and is correct by definition.
The reason that the lowering pipeline required so much boilerplate was
because of the robust types involved, which ensures that everything in the
pipeline is compatible with one-another---it's not possible to construct a
pipeline that will not work.
Of course, there is nuance involved in some cases---I didn't want to include
the `until` clause, which makes it fail the "obviously correct" criterion,
but that can be improved over time.
This only abstracts away `load_xmlo` and `parse_package_xml`; next I'll have
to evolve the abstraction to support lifetimes for `lower_xmli`'s
`AsgTreeToXirf`. That pipeline also ends with a custom sink that really
ought to become its own parser, but I don't want to jump down that rabbit
hole right now, so we may just support custom sinks for now with the intent
of removing it in the future.
This has been a long time coming. The ultimate goal is that you should be
able to look at the parser pipelines to have a clear, high-level overview of
how everything fits together. I'm not generating documentation yet, but
that'll help serve as a guide as well.
DEV-13162
The report acts as the sink for `load_xmlo` and `parse_package_xml`. At the
moment, the type is `()`, and so there's nothing to report on but the
error. But the idea is to add logging via `AirAggregate::Object`, which is
currently just `()`.
This change therefore is only a refactoring---it changes no functionality
but sets up for future changes.
This also introduces consistency with `lower_xmli` in use of `terminal` for
the final operation.
DEV-13162
Diagnostic events need not be errors. While that was the original intent,
it'd also be nice to be able to use the diagnostic system for any type of
logging, where the verbosity level would determine the type of report that
is output (whether source information should be provided).
Then we could have e.g. AirAggregate produce events describing what actions
are occurring, which could be much more useful than a trace in many
contexts, and would be able to operate via a runtime toggle/filter without
having an adverse effect on performance (since the diagnostic rendering
itself is the hit; the underlying data are cheap).
Anyway---I'm addressing this now to generalize the reporter in the lowering
pipeline, so that it can report on not just errors but anything.
DEV-13162
This formats the pipeline to mirror the style of
`parse_package_xml`. Based on the previous commits, the end goal (though
not necessarily now) will be to derive a concise abstraction for all the
lowering pipelines, which means first factoring them into a common form.
DEV-13162
This makes the API of `load_xmlo` much closer to `parse_package_xml`, both
accepting a reporter and distinguishing between recoverable and
unrecoverable errors.
The linker still does not use a reporter and still fails on the first
error, as before; I wanted to keep this change small.
DEV-13162
This allows us to drop `AirIdent::IdentRef`, which in turn allows dropping
`AirIdent` entirely from `AirPkgAggregate`.
This is also a more appropriate abstraction; having to track all the ways in
which `IdentRef` was used can be confusing. This means that `AirIdent` is
true to its name---used only for identifiers. The new token type makes it
very clear where package imports are recognized, and it's also easier to
search for.
DEV-13162
This is the same idea as the previous two commits: get all the lowering
pipelines into the same place so that we can observe commonalities and
attempt to derive an appropriate abstraction.
`lower_xmli` could have invoked `tree_reconstruction` itself, since it has
all the information that it needs to do so, but the idea is that these will
accept sources from the caller. This also demonstrates that sinks need to
be flexible. In an ideal abstraction, perhaps this would be able to produce
an iterator that accepts the first token type and yields the last, which can
then be directed to a sink, but that's not compatible with how the lowering
operations currently work, which requires a single value to be
returned. But if it did work that way, then they'd be able to compose just
as any other parser.
Maybe for the future.
DEV-13162
The previous commit extracted xmlo loading, because that will be a common
operation between `tamec` and `tameld`. This extracts parsing, which will
only used by `tamec` for now, though components of the pipeline are similar
to xmlo loading.
Not only does it need to be removed from `tamec` and better abstracted, but
the intent now is to get all of these things into one place so that the
patterns are obviated and a better abstraction can be created to remove all
of this boilerplate and type complexity.
Furthermore, xmlo loading needs to use reporting and recovery, so having
`parse_package_xml` here will help show how to make that happen easily. I'm
pleased that it ended up being trivial to extract error reporting from the
lowering pipeline as a simple (mutable) callback. I'm not pleased about
the side-effects, but, this works well for now given how the system works
today.
DEV-13162
Mike Gerwitz