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

"cycle error" regression with let fields between 0.4.3 and 0.5.0 #2355

Closed
mvdan opened this issue Apr 20, 2023 · 7 comments
Closed

"cycle error" regression with let fields between 0.4.3 and 0.5.0 #2355

mvdan opened this issue Apr 20, 2023 · 7 comments

Comments

@mvdan
Copy link
Member

mvdan commented Apr 20, 2023

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

$ cue version
cue version v0.6.0-alpha.1.0.20230418084629-59080b65a5ba

go version devel go1.21-9d35ebba06 Thu Apr 20 01:07:29 2023 +0000
      -buildmode exe
       -compiler gc
        -ldflags -w -s
  DefaultGODEBUG panicnil=1
     CGO_ENABLED 1
          GOARCH amd64
            GOOS linux
         GOAMD64 v3

Does this issue reproduce with the latest stable release?

Yes; tested with v0.5.0 and master per the version output above.

What did you do?

$ cue export -
#GetF1: {
	in:  _
	out: in.f1
}

#PutOut1: {
	in: _
	out: out1: "\(in)"
}

export: {
	let data = {f1: "foo"}
	(#PutOut1 & {
		in: (#GetF1 & {
			in: data
		}).out
	}).out
}

What did you expect to see?

Same output as v0.4.3:

{
    "export": {
        "out1": "foo"
    }
}

What did you see instead?

invalid interpolation: cycle error:
    -:8:13

This bug is very similar to #2351; it's entirely possible that both will be fixed with the same commit. However, that other bug was replacing one error with a slightly less helpful error, whereas this bug is turning a successful export into an error. I also think the cycle isn't valid; just like with #2351, embedding a struct shouldn't break let definitions.

Thanks to @massive for reporting this bug and helping investigate it.

@mvdan
Copy link
Member Author

mvdan commented Apr 28, 2023

Bisected to https://review.gerrithub.io/c/cue-lang/cue/+/543362, although that commit first broke this code with a slightly different error:

in: invalid interpolation: undefined field: data:
    ../../../../../tmp/f.cue:8:13
    ../../../../../tmp/f.cue:15:8

@morlay
Copy link

morlay commented May 5, 2023

Not just let fields

#Build: {
	steps: [#Step, ...#Step]
	output: #Image

	_dag: {
		for idx, step in steps if idx == 0 {
			"\(idx)": step
		}

		for idx, step in steps if idx > 0 {
			"\(idx)": {
                                 // here throw cyclic task dependency in cueflow
                                 // it works well with v0.4.3 and v0.5.0-beta.1
				_prev: _dag["\(idx-1)"].output

				step & {
					input: _prev
				}
			}
		}
	}

	if len(_dag) > 0 {
		output: _dag["\(len(_dag)-1)"].output
	}
}

#Step: {
	input?: #Image
	output: #Image
	...
}

@mvdan
Copy link
Member Author

mvdan commented Jun 16, 2023

@morlay could you please raise a new bug following the bug report template? That will include enough information for us to reproduce your problem and be able investigate it. What you shared here isn't enough, and on a first look, the lack of let fields means that it's a different issue.

@mvdan mvdan removed their assignment Jun 16, 2023
@mvdan mvdan removed the Triage Requires triage/attention label Jun 16, 2023
@morlay
Copy link

morlay commented Jun 25, 2023

@myitcv Sorry.
My case may a issue of tools/flow

I have to add new config DisableDynamicTasks to disable initTasks when after task.Fill() for a quick fix.
morlay@b9dd73e

Since v0.5.0-beta.2 (v0.5.0-beta.1 works well) the task deps will be changed after task.Fill() in complex workflow, even all tasks are static.
I will try to create another issue to post this, when I have enough time to make a simplier test case.

cueckoo pushed a commit that referenced this issue Jul 24, 2023
Before it was assumed that addList would always be
called in a final stage. However, this is not true if
it is called as a composite literal in an expression,
for intsance `[ expr ][0]`.

Passing this will allow the computation of embeddings
to be defered in a future CL.

Issue #2244
Issue #2351
Issue #2355

Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Change-Id: I6d27495d8a9413033ad202fc995e297da66e9309
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/556544
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
cueckoo pushed a commit that referenced this issue Jul 26, 2023
Issue #2351
Issue #2355

Signed-off-by: Marcel van Lohuizen <mpvl@gmail.com>
Change-Id: I82afbcc4ebba79e3931fc9dde53c231d77264dd1
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/556919
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
@morlay
Copy link

morlay commented Aug 2, 2023

@myitcv just let you know.

Since I upgrade to v0.6.0-rc.1, my issue is gone without any patch.

@myitcv
Copy link
Member

myitcv commented Aug 2, 2023

@myitcv just let you know.

Since I upgrade to v0.6.0-rc.1, my issue is gone without any patch.

Thanks very much for confirming, @morlay

@morlay
Copy link

morlay commented Aug 2, 2023

Thanks very much for confirming, @morlay

cycle error is gone, but deps issue coming 🤣

#2516 #2517

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

No branches or pull requests

4 participants