tools/flow: bad unification when flow value is not a root value

[eonpatapon]

What version of CUE are you using (cue version)?

0.4.1

Does this issue reproduce with the latest release?

Yes

What did you do?

Using a non root value in flow.Run leads to unexpected task fill results.

To visualize the issue it is handy to print the final value of the flow:

diff --git a/tools/flow/flow.go b/tools/flow/flow.go
index 17eabe4e..86657b1e 100644
--- a/tools/flow/flow.go
+++ b/tools/flow/flow.go
@@ -68,6 +68,7 @@ package flow

 import (
        "context"
+       "fmt"

        "cuelang.org/go/cue"
        "cuelang.org/go/cue/errors"
@@ -224,6 +225,7 @@ func (c *Controller) Run(ctx context.Context) error {
        defer c.cancelFunc()

        c.runLoop()
+       fmt.Println(c.inst)
        return c.errs
 }

Now using the following cue file:

package x

// #T: {
//  t1: {...}
// }

a: {
    t1: {
        op:     "print"
        val:    "foo"
        output: string
    }
}

With the following go code:

package main

import (
    "context"
    "fmt"
    "log"

    "cuelang.org/go/cue"
    "cuelang.org/go/cue/cuecontext"
    "cuelang.org/go/cue/load"
    "cuelang.org/go/tools/flow"
)

func main() {
    ctx := cuecontext.New()

    i := load.Instances([]string{}, nil)
    v := ctx.BuildInstance(i[0])

    x := v.LookupPath(cue.ParsePath("a"))

    c := flow.New(nil, x, func(v cue.Value) (flow.Runner, error) {
        op := v.LookupPath(cue.ParsePath("op"))
        if !op.Exists() {
            return nil, nil
        }

        t, _ := op.String()

        switch t {
        case "print":
            r := printRunner{}
            return r, nil
        }

        return nil, nil
    })
    err := c.Run(context.TODO())
    if err != nil {
        log.Fatal(err)
    }
}

type printRunner struct{}

func (p printRunner) Run(t *flow.Task, terr error) error {
    v, _ := t.Value().LookupPath(cue.ParsePath("val")).String()
    fmt.Println(t.Value().Path(), v)
    t.Fill(map[string]interface{}{"output": "hello"})
    return nil
}

Note we are passing v.LookupPath(cue.ParsePath("a")) to flow.Run, which is not the root value.

Will result in:

 go run main.go 
a.t1 foo
t1 foo
{
    t1: {
        op:     "print"
        val:    "foo"
        output: "hello"
    }
    a: {
        t1: {
            output: "hello"
        }
    }
}

Note that the runner is run twice as it reports the full path a.t1 then the relative path t1.

Therefore flow has updated a.t1 but has also added a.a.t1. (we are in the context of a here)

Of course, if we close a, flow will report an error:

package x

#T: {
    t1: {...}
}

a: #T & {
    t1: {
        op:     "print"
        val:    "foo"
        output: string
    }
}

 go run main.go 
a.t1 foo
_|_ // field not allowed: a
main.go:40: field not allowed: a
exit status 1

The following quick'n dirty patch seems to resolve the issue:

diff --git a/tools/flow/tasks.go b/tools/flow/tasks.go
index 519a3865..7e0507ef 100644
--- a/tools/flow/tasks.go
+++ b/tools/flow/tasks.go
@@ -88,7 +88,9 @@ func (c *Controller) getTask(scope *Task, v cue.Value) *Task {
        }

        // Look up cached task from previous evaluation.
-       p := v.Path()
+       // p := v.Path()
+       sels := v.Path().Selectors()
+       p := cue.MakePath(sels[len(c.inst.Path().Selectors()):len(sels)]...)
        key := p.String()

        t := c.keys[key]
@@ -110,11 +112,12 @@ func (c *Controller) getTask(scope *Task, v cue.Value) *Task {
                if r != nil {
                        index := len(c.tasks)
                        t = &Task{
-                               v:      v,
-                               c:      c,
-                               r:      r,
-                               path:   p,
-                               labels: w.Path(),
+                               v:    v,
+                               c:    c,
+                               r:    r,
+                               path: p,
+                               //labels: w.Path(),
+                               labels: w.Path()[len(c.inst.Path().Selectors()):len(sels)],
                                key:    key,
                                index:  index,
                                err:    errs,

Basically the path of the task should be relative to the flow value because it is used by some LookupPath which must be relative to the flow value (https://github.com/cue-lang/cue/blob/master/tools/flow/run.go#L178)

Next the labels are used to create the expr that will be unified with the original task value, theses need to be relative as well, it seems.

I have no idea why originally flow was running twice the task though.

 go run main.go 
a.t1 foo
{
    t1: {
        op:     "print"
        val:    "foo"
        output: "hello"
    }
}

[verdverm]

Related https://github.com/cue-lang/cue/discussions/1364

[rogpeppe]

It seems like the problem, or at least part of the problem, looks to me like it could be related to the place in Controller.updateValue where it's updating the value of c.inst. The original value of c.inst had the whole path in; the updated path does not, which means that the Controller.keys map indexes will change, which is probably why the task is being run twice.