handle variable evaluation in one location #30312
Closed
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.
Module variable evaluation was spread out over 3 locations, which led to
a skew in what value could be seen within a module, and what value could
be seen within a variable's own validation scope.
First, the graph transformer would create a fake input expression on the
fly for variables with no input, and load it with the default value.
This caused trouble during actual evaluation where the input expression
would be mapped to the wrong source location, and made it difficult to
determine if an input value was really assigned or not.
The variables nodes would then evaluate the expression, but not be able
to handle defaults, because there always appeared to be an input
expression set for the variable. Because defaults could not be handled,
the wrong value could be passed into the validation expressions for the
variable.
GetInputVariable from the evaluator would check again for defaults, but
was usually fooled by the fake expression value. Even when it could
assign a default, as in the case of
nullable=false
with a null input,it was already too late for the variable validation block to handle it.
We can unify all evaluation handling into the nodeModuleVaraible exec
node. This way the context is always loaded with the correct final value
of the variable, so GetInputVariable is only concerned with fetching
that value to evaluate references to it, and the graph transformer is
taken out of the picture entirely.
Due to the possibility of uncovering other bugs while refactoring variable evaluation, this won't be backported to v1.1, however a more localized fix may be possible to mitigate the incorrect validation values.
Fixes #30307