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

handle variable evaluation in one location #30312

Closed
wants to merge 1 commit into from
Closed

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jan 7, 2022

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

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.
@jbardin jbardin requested a review from a team January 7, 2022 15:01
@jbardin jbardin self-assigned this Jan 7, 2022
@alisdair
Copy link
Member

alisdair commented Jan 7, 2022

At first glance this looks like it overlaps with #29959, which is on my shame list for re-reviewing. Does that seem right to you?

@jbardin
Copy link
Member Author

jbardin commented Jan 7, 2022

Oh, I thought that was only addressing the root variable problems 😞
At least Martin an I came to mostly the same conclusions about the sprawl of variable handling. I'll close this for now and join the other conversation in #29959.

@github-actions
Copy link

github-actions bot commented Feb 7, 2022

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 7, 2022
@jbardin jbardin deleted the jbardin/variable-eval branch November 30, 2022 19:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not nullable module input variable validates null before default value
2 participants