cue-lang / cue

The home of the CUE language! Validate and define text-based and dynamic configuration
https://cuelang.org
Apache License 2.0
5.01k stars 283 forks source link

evaluator: unexpected behavior of `list.Sum` #3310

Open slewiskelly opened 1 month ago

slewiskelly commented 1 month ago

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

$ cue version
cue version v0.9.2

go version go1.22.4
      -buildmode exe
       -compiler gc
       -trimpath true
  DefaultGODEBUG httplaxcontentlength=1,httpmuxgo121=1,tls10server=1,tlsrsakex=1,tlsunsafeekm=1
     CGO_ENABLED 1
          GOARCH arm64
            GOOS darwin
cue.lang.version v0.9.2

Does this issue reproduce with the latest stable release?

Yes.

What did you do?

import "list"

receipt: {
        // Thanks for shopping at Big Kahuna Burger!

        cheeseburger: 3.00
        fries:        2.00
        sprite:       1.00

        total: list.Sum([ for _, v in receipt {v}])
}

What did you expect to see?

Would probably expect this to fail due to a cycle.

What did you see instead?

The value for total is 18, instead of 6.

$ cue export receipt.cue
{
    "receipt": {
        "cheeseburger": 3.00,
        "fries": 2.00,
        "sprite": 1.00,
        "total": 18
    }
}
slewiskelly commented 1 month ago

The following yields the correct result, though I would expect a conflict:

import "list"

receipt: {
        // Thanks for shopping at Big Kahuna Burger!

        cheeseburger: 3.00
        fries:        2.00
        sprite:       1.00

        total: list.Sum([for k, v in receipt {v}])
        total: list.Sum([for k, v in receipt if k != "total" {v}])
}

Finally, the following yields a cycle error, as expected:

import "list"

receipt: {
        // Thanks for shopping at Big Kahuna Burger.

        cheeseburger: 3.00
        fries:        2.00
        sprite:       1.00

        total: list.Sum([for k, v in receipt {v / 2}])
}
myitcv commented 1 month ago

Thanks very much for the report, @slewiskelly.

The new evaluator does see this as a cycle (the following test passes, as of f5b905cec6d6):

env CUE_EXPERIMENT=evalv3
! exec cue export x.cue
cmp stderr stderr.golden

-- x.cue --
import "list"

receipt: {
    // Thanks for shopping at Big Kahuna Burger!

    cheeseburger: 3.00
    fries:        2.00
    sprite:       1.00

    total: list.Sum([for _, v in receipt {v}])
}
-- stderr.golden --
receipt.3: structural cycle:
    ./x.cue:10:18
slewiskelly commented 1 month ago

Sorry, I completely forgot to try with CUE_EXPERIMENT=evalv3. As mentioned, this is identified as a cycle with that enabled:

$ CUE_EXPERIMENT=evalv3 cue export receipt.cue
receipt.3: structural cycle:
    ./receipt.cue:10:25
mvdan commented 1 month ago

@slewiskelly would you be happy with this issue being marked as resolved by the new evaluator, then? A number of issues around disjunctions, cycle detection, and performance are resolved thanks to the new evaluator's design, and our focus is on polishing up the new evaluator so that we may enable it by default for all users as soon as possible. Any time spent trying to backport any fixes to the old evaluator would slow down that transition, as you can imagine.

Just as with previous cases like https://github.com/cue-lang/cue/issues/3149, I will still add a test case to the repository with your example, to make sure that it keeps working with the new evaluator.