stackeval: Improve coverage of namedPromiseReporter impls #35153
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
namedPromiseReporter
interface is a helper we use to collect user-friendly names for promises when we need to report a promise self-reference error to the user.We collect names for promises reactively on error, rather than proactively in the normal course of work, because proactively maintaining a suitable data structure for this cross-cutting concern would be both expensive and complicated.
This compromise therefore allows us to pay the cost of walking over all of the promise-owning objects only in the exceptional case where we need to report a self-reference error, but it does come at the expense of it being possible for us to forget to implement this interface for certain types and thus be unable to name a subset of the affected promises.
(Those who are familiar with the implementation details of Terraform's modules runtime might think of this as being roughly analogous to calling
dag.VertexName
on all of the graph nodes in a dependency cycle, but in this case the detection of the cycle is done bypackage promising
while the reporting is done bypackage stackeval
, so the parts take on a different shape.)This commit implements
reportNamedPromises
for all promise-owning types that are reachable from astackeval.Main
, although there is a notable gap here where we can't actually report promises from the individual instances of a multi-instance object (e.g. a component) because the promising API doesn't let us peek into apromising.Once
to see if it's resolved without blocking until it becomes resolved.There are FIXME comments for that limitation in all of the places where it's relevant, and so hopefully in a future commit we'll devise a suitable way to perform a fallible non-blocking peek of a promise so that we can report promises nested in its result only if they are already present.
There were already implementations in place for the "Config" variants of these types from the original work on implementing the validation phase, so this is just plugging gaps we missed when later implementing plan and apply.
While I was working on this I noticed that we weren't properly treating
Provider
objects as singletons, and thus preventing any promise coalescing for multiple accesses to results derived fromprovider
blocks. The first commit in this PR fixes that.I was intending to split that into a separate PR, but unfortunately these two overlap due to the second commit making use of the new
providers
map inStack
, so to make things less confusing and require less hoop-jumping I just combined the two together.