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: bounded recursion regression #2052

Closed
caleblloyd opened this issue Nov 1, 2022 · 3 comments
Closed

evaluator: bounded recursion regression #2052

caleblloyd opened this issue Nov 1, 2022 · 3 comments

Comments

@caleblloyd
Copy link

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

v0.5.0-alpha.1.0.20221101144719-b3bcc7aaf3d0 built from commit b3bcc7a

Does this issue reproduce with the latest release?

Reproduces starting in v0.5.0-alpha.1

Works as expected in v0.4.3

What did you do?

Following example at https://cuetorials.com/deep-dives/recursion/#bounded-recursion

Package r

recurse.cue:

package r

import "list"

#RecurseN: {
	// this is the bound on our recursion
	#maxiter: uint | *4

	// This is the function list element
	// we generate this to simulate recursion
	#funcFactory: {
		#next: _
		#func: _
	}

	// this is our "recursion unrolling"
	for k, v in list.Range(0, #maxiter, 1) {
		// this is where we build up our indexed functions and the references between them
		#funcs: "\(k)": (#funcFactory & {#next: #funcs["\(k+1)"]}).#func
	}

	// our final value needs to be null
	#funcs: "\(#maxiter)": null

	// we embed the head of the list so that the values
	// we write this into can be used like other CUE "functions"
	#funcs["0"]
}

depth.cue:

package r

import "list"

#DepthF: {
	#next: _
	#func: {
		#in:    _
		#basic: int | number | string | bytes | null
		out: {
			// if we have a basic type, we are at a leaf, depth is 1
			if (#in & #basic) != _|_ {1}

			// if we are not a basic type, then we are 1 + the max of children
			if (#in & #basic) == _|_ {
				// this is our "recursion" for each child
				let depths = [ for k, v in #in {(#next & {#in: v}).out}]
				list.Max(depths) + 1
			}
		}
	}
}

#Depth: #RecurseN & {#maxiter: 11, #funcFactory: #DepthF}

tree: {
	a: {
		foo: "bar"
		a: b: c: "d"
	}
	cow: "moo"
}

d: #Depth & {#in: tree}

Then run the command cue export --out cue ./r

What did you expect to see?

tree: {
        a: {
                foo: "bar"
                a: b: c: "d"
        }
        cow: "moo"
}
d: out: 5

What did you see instead?

d: undefined field: #maxiter:
    ./r/recurse.cue:17:28
d: cannot add field #funcs: was already used:
    ./r/recurse.cue:23:2
@caleblloyd caleblloyd added NeedsInvestigation Triage Requires triage/attention labels Nov 1, 2022
@myitcv myitcv added this to the v0.5.0 comprehension rework milestone Nov 2, 2022
@myitcv
Copy link
Member

myitcv commented Nov 2, 2022

Thanks for raising this, @caleblloyd. Confirmed and bisected to 748a685.

What did you do?

go mod tidy
go run cuelang.org/go/cmd/cue export
cmp stdout stdout.golden

-- go.mod --
module mod.com

go 1.18

require cuelang.org/go v0.4.4-0.20220622094641-748a68589747

-- deps.go --
//go:build tools
// +build tools

package tools

import (
	_ "cuelang.org/go/cmd/cue"
)
-- depth.cue --
package r

import "list"

#DepthF: {
	#next: _
	#func: {
		#in:    _
		#basic: int | number | string | bytes | null
		out: {
			// if we have a basic type, we are at a leaf, depth is 1
			if (#in & #basic) != _|_ {1}

			// if we are not a basic type, then we are 1 + the max of children
			if (#in & #basic) == _|_ {
				// this is our "recursion" for each child
				let depths = [ for k, v in #in {(#next & {#in: v}).out}]
				list.Max(depths) + 1
			}
		}
	}
}

#Depth: #RecurseN & {#maxiter: 11, #funcFactory: #DepthF}

tree: {
	a: {
		foo: "bar"
		a: b: c: "d"
	}
	cow: "moo"
}

d: #Depth & {#in: tree}
-- recurse.cue --
package r

import "list"

#RecurseN: {
	// this is the bound on our recursion
	#maxiter: uint | *4

	// This is the function list element
	// we generate this to simulate recursion
	#funcFactory: {
		#next: _
		#func: _
	}

	// this is our "recursion unrolling"
	for k, v in list.Range(0, #maxiter, 1) {
		// this is where we build up our indexed functions and the references between them
		#funcs: "\(k)": (#funcFactory & {#next: #funcs["\(k+1)"]}).#func
	}

	// our final value needs to be null
	#funcs: "\(#maxiter)": null

	// we embed the head of the list so that the values
	// we write this into can be used like other CUE "functions"
	#funcs["0"]
}
-- stdout.golden --
{
    "tree": {
        "a": {
            "foo": "bar",
            "a": {
                "b": {
                    "c": "d"
                }
            }
        },
        "cow": "moo"
    },
    "d": {
        "out": 5
    }
}

What did you expect to see?

Passing test.

What did you see instead?

> go run cuelang.org/go/cmd/cue export
[stderr]
d: undefined field: #maxiter:
    ./recurse.cue:17:28
d: cannot add field #funcs: was already used:
    ./recurse.cue:23:2
exit status 1

@myitcv myitcv removed the Triage Requires triage/attention label Nov 2, 2022
@myitcv myitcv changed the title Bounded Recursion Example regression in v0.5.0-alpha.1 evaluator: bounded recursion regression Nov 2, 2022
@mpvl
Copy link
Member

mpvl commented Nov 14, 2022

The underlying problem is a spurious structural cycle. The below is a simplification of what triggers it.

#maxiter: 2

// Trigger lookup of a.x0 before the "regular" evaluation of a.
x: a.x0

a: {
	for k, _ in [0, 1] {
		"x\(k)": (#DepthF & {#in: a["x\(k+1)"]}).#out
	}
}

a: x2: null // no cycle with {}!!!

#DepthF: {
	#in: _
	#out: {}
}

@mpvl
Copy link
Member

mpvl commented Nov 15, 2022

Another issue that is exposed by this once the cycle issue is fixed:

X: 1

for v in [0, X] + [] {
	x: "x\(v)": {}
}

x: _

x.x0

The issue is that X's evaluation is triggered by the evaluation of x.x0 before X has been added to the root. Normally these cases are handled fine, but the addition of + [] prevents its correct handling.

cueckoo pushed a commit that referenced this issue Nov 17, 2022
Without this fix errors could be doubled for each argument, causing
a rapid growth of errors lists. These errors were deduplicated down the
line, causing this to go unnoticed.

Issue #2052

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: Ia26a8b3a3ccde1d01ce28fdbc247e6643d1eafa3
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/546219
Reviewed-by: Roger Peppe <[email protected]>
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
cueckoo pushed a commit that referenced this issue Nov 17, 2022
They all fail and will be made to pass in subsequent CLs.

Issue #2052

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: Ib3f6941734042dd701bcf45cb91db93b657e214b
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/546102
Reviewed-by: Roger Peppe <[email protected]>
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
cueckoo pushed a commit that referenced this issue Nov 17, 2022
Cycle tracking is more strict in expressions. However,
when an inlined expression refers to "regular" fields
(for instance a["x\(k+1)"]), the same expression may
occur within a recursion referring to different, existing
fields. Not taking into account that the same expression
may refer to different existing fields may result in
in spurious cycles. This relaxes this to avoid such issues.

Issue #2052

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: I054730cb2ccdf7288f0d068b898defcf54b2788d
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/546220
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Reviewed-by: Roger Peppe <[email protected]>
cueckoo pushed a commit that referenced this issue Nov 17, 2022
When not finalizing a node, i.e. if state <= Partial.

Don't recursively evaluate constructed lists
  (i.e. don't use Finalize).

Don't propagate error up in evalCached

Incomplete comprehension errors are reported at the origin as well as
"void" arcs. This should not be done, however, when the error is not
yet reported, but only consulted for control flow

IndexExpr and SelectorExpr should not unconditionally set
a node to an error.

Issue #2052

Signed-off-by: Marcel van Lohuizen <[email protected]>
Change-Id: If5e7b6921f61dd16397c4b50e22decc43ad3f07d
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/546221
Reviewed-by: Roger Peppe <[email protected]>
Unity-Result: CUEcueckoo <[email protected]>
TryBot-Result: CUEcueckoo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants