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

Merge scope hierarchy for evaluation #2151

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

Conversation

wildum
Copy link
Contributor

@wildum wildum commented Nov 25, 2024

Description of the problem

The evaluateExpr func in the vm pkg recursively evaluate expressions. If you want to evaluate this:

a = [loki.write.endpoint.receiver]

the [loki.write.endpoint.receiver] will be represented by nested structs:

  • ArrayExpr
    • AccessExpr (receiver)
      • AccessExpr (endpoint)
        • AccessExpr (write)
          • IdentifierExpr (loki)

The evaluate will go through the nesting recursively and when it arrives at the IdentifierExpr, it will use the scope to look it up.
The scope looks like this:

  • Parent (the parent scope)
  • Variables (nested maps)
    • "loki"
      • "write"
        • "endpoint"

The Lookup will check whether the identifier is in the variables. Here it matches "loki" so it return the associated map:

  • "write"
    • "endpoint"

Then the recursion unfolds because "write" will match and it will return "endpoint" which will also match and return the receiver object.
During the lookup, if "loki" would not have been found in the scope, it would have looked up at the parent scope recursively till there is a match. If there is no match and no more parent then it's an error.
The problem with this process is that if you have the following:
Scope:

  • Parent
    • Parent
    • Variables
      • "loki"
        • "write"
          • "endpoint2"
  • Variables
    • "loki"
      • "write"
        • "endpoint"

And you want to evaluate the expression:

a = [loki.write.endpoint2.receiver]

The algorithm will match with the wrong path because it will match "loki" and only at the last AccessExpr it will realise that "endpoint" does not match with "endpoint2" and it will throw an error.
This was not a problem so far because the ParentScope is only used for the ModulePath AFAIK. But now I'm using it for the foreach because unlike modules that uses "arguments" I think that it's nicer if the components in the foreach have access to the outside:

  • The scope contains the info in the foreach
  • The parent scope contains the info from outside of the foreach

Solution

This solution gets rid of the parent scope aed merges the current scope with the one built for the component. I could not find a way to change it easily in syntax pkg. Solving it in the value_cache seemed easier and safer (thanks @rfratto for the hint).

@wildum wildum requested a review from a team as a code owner November 25, 2024 11:27
ptodev
ptodev previously approved these changes Nov 25, 2024
internal/runtime/internal/controller/value_cache.go Outdated Show resolved Hide resolved
internal/runtime/internal/controller/value_cache.go Outdated Show resolved Hide resolved
@wildum wildum marked this pull request as draft November 27, 2024 14:25
@ptodev ptodev dismissed their stale review November 28, 2024 13:04

approved by accident

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants