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

evaluator: unexpected behavior of list.Sum #3310

Closed
slewiskelly opened this issue Jul 21, 2024 · 4 comments
Closed

evaluator: unexpected behavior of list.Sum #3310

slewiskelly opened this issue Jul 21, 2024 · 4 comments
Assignees
Labels
evaluator evalv3-win Issues resolved by switching from evalv2 to evalv3 NeedsInvestigation

Comments

@slewiskelly
Copy link
Contributor

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 slewiskelly added NeedsInvestigation Triage Requires triage/attention labels Jul 21, 2024
@slewiskelly
Copy link
Contributor Author

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 myitcv changed the title Unexpected behavior of list.Sum evaluator: unexpected behavior of list.Sum Jul 22, 2024
@myitcv myitcv added evaluator and removed Triage Requires triage/attention labels Jul 22, 2024
@myitcv
Copy link
Member

myitcv commented Jul 22, 2024

Thanks very much for the report, @slewiskelly.

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

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
Copy link
Contributor Author

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
Copy link
Member

mvdan commented Jul 23, 2024

@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 #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.

@mvdan mvdan self-assigned this Jul 23, 2024
@mvdan mvdan added the evalv3-win Issues resolved by switching from evalv2 to evalv3 label Aug 13, 2024
@cueckoo cueckoo closed this as completed in e29900a Sep 9, 2024
cueckoo pushed a commit that referenced this issue Oct 14, 2024
The sum in the changed test was summing itself, which
is a structural cycle. The old behavior was to give
a garbage result. We now correctly report the structural
cycle.

Issue #3310

Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Change-Id: Ia37a3a0adf373e9455dc921916a1352ebd580b44
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202444
Reviewed-by: Matthew Sackman <matthew@cue.works>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
vanhtuan0409 pushed a commit to anduintransaction/cue that referenced this issue Oct 15, 2024
The sum in the changed test was summing itself, which
is a structural cycle. The old behavior was to give
a garbage result. We now correctly report the structural
cycle.

Issue cue-lang#3310

Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Change-Id: Ia37a3a0adf373e9455dc921916a1352ebd580b44
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1202444
Reviewed-by: Matthew Sackman <matthew@cue.works>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluator evalv3-win Issues resolved by switching from evalv2 to evalv3 NeedsInvestigation
Projects
None yet
Development

No branches or pull requests

3 participants