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

cmd/cue: various errors related to conflicting defaults #1123

Open
dermetfan opened this issue Jul 21, 2021 · 3 comments
Open

cmd/cue: various errors related to conflicting defaults #1123

dermetfan opened this issue Jul 21, 2021 · 3 comments
Labels
NeedsFix vet candidate for a vet rule to detect suspect usage

Comments

@dermetfan
Copy link

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

$ cue version
cue version 0.4.0 linux/amd64

(Git tag v0.4.0)

Does this issue reproduce with the latest release?

Yes. Tested on master as well (38c8f7d).

What did you do?

It is known that a field cannot have multiple different default values.

However, a user of a field with a default may not always know that it has one. The error when they try to assign a default is not very helpful.

tmpdir="$(mktemp -d)"
cd "$tmpdir"
trap 'rm -r "$tmpdir"' EXIT

alias cat=$(which bat >/dev/null && echo bat || echo cat)

cat > issue.cue <<EOF
#Foo: {
	target: int | *100
}

#Bar: #Foo & {
	target: int | *200
}

baz: #Bar & {
	// no value for target
}
EOF
cat issue.cue

echo
echo "Undescriptive error message:"
echo

echo \$ cue export issue.cue
cue export issue.cue

echo
echo "We don't know which field causes the failure unless we manually check:"
echo

echo \$ cue eval issue.cue
cue eval issue.cue

echo
echo "Workaround if you want to 'override' the default:"
echo

cat > issue-ok.cue <<EOF
#Foo: {
	target: uint | *100
}

#Bar: #Foo & {
	_target: uint | *null

	if _target == null {
		target: 200
	}
	if _target != null {
		target: _target
	}
}

baz: #Bar & {
	// toggle this
	// _target: 300
}
EOF
cat issue-ok.cue

What did you expect to see?

Something more descriptive that tells me that the target field fails to evaluate.

It would be best if the conflicting default values could be listed just like with other concrete values:

#Bar.target: conflicting values 200 and 100:
    ./issue.cue:2:10
    ./issue.cue:5:7
    ./issue.cue:6:10
baz.target: conflicting values 200 and 100:
    ./issue.cue:2:10
    ./issue.cue:5:7
    ./issue.cue:6:10
    ./issue.cue:9:6

What did you see instead?

Only the containing struct is listed:

baz:

This becomes much harder to track down in bigger structs.

@dermetfan dermetfan added NeedsInvestigation Triage Requires triage/attention labels Jul 21, 2021
@verdverm
Copy link

verdverm commented Jul 21, 2021

https://cuelang.org/play/?id=tfXXJjk3Ibt#cue@export@cue

@myitcv this output does not look correct for an export. Should I open an issue on a different repository?

@dermetfan generally, changing defaults is (was?) against the ethos of CUE. I think there may be a way, but having some trouble finding where that was written. Maybe the spec could be useful in figuring it out: https://cuelang.org/docs/references/spec/#default-values

@dermetfan
Copy link
Author

this output does not look correct for an export.

Looks like in the playground "export to CUE" corresponds to cue eval and only when JSON is chosen as output format cue export runs.

@myitcv
Copy link
Member

myitcv commented Jul 28, 2021

@dermetfan - welcome to the CUE project!

Firstly, thank you for the comprehensive repro. In case you are interested, the wiki gives some ideas on how you can use the txtar format to create clean repros for multi-file examples like yours. Here is my encoding of your repro:

# Undescriptive error message:
exec cue export issue.cue

# We don't know which field causes the failure unless we manually check:
exec cue eval issue.cue

-- issue-ok.cue --
#Foo: {
 target: uint | *100
}

#Bar: #Foo & {
 _target: uint | *null

 if _target == null {
  target: 200
 }
 if _target != null {
  target: _target
 }
}

baz: #Bar & {
 // toggle this
 // _target: 300
}
-- issue.cue --
#Foo: {
 target: int | *100
}

#Bar: #Foo & {
 target: int | *200
}

baz: #Bar & {
 // no value for target
}

As you can see from the wiki, it's possible to assert exit codes, stdout, stderr etc.

@verdverm

this output does not look correct for an export.

@dermetfan is correct in saying the following:

Looks like in the playground "export to CUE" corresponds to cue eval and only when JSON is chosen as output format cue export runs.

#1150 will address this.

@dermetfan

It is known that a field cannot have multiple different default values.

There is actually nothing to prevent there being multiple defaults. Consider the following:

x: int | *2 | *3

Evaluated as-is this results in x: 2 | 3. But if a more specific instance specified:

x: !=2

then x: 3 would be the result. This is a specific case of default values being eliminated by virtue of them evaluating to _|_.

So are there any issues in your case? I would say yes.

  1. because cue export issue.cue should not exit non-zero with no errors;
  2. AFAICT the output of cue eval is wrong for the following reason:

In effect #Bar is:

#Bar: {
    target: (int | *100) & (int | *200)
}

which according to the rules of default values simplifies to:

#Bar: {
    target: (int & int) | *(100 & 200) // by U2
}

which is equivalent to:

#Bar: {
    target: int | _|_ 
}

which reduces to:

#Bar: {
    target: int
}

Not only that, int | 100 | 200 (the output of cue eval issue.cue) is equivalent to int according to the rules of disjunctions.

It would be best if the conflicting default values could be listed just like with other concrete values:

When/where would you expect to see this conflict? I can imagine it perhaps being useful for cue vet rule of some sort, but that's just a quick thought.

For thoughts on whether you should override defaults in this way, see #989. As an alternative you can do this sort of thing:

package a

_#Common: {
	name: string
	age:  int
}

_#MoreCommon: {
	_#Common
	address: string
}

#A: {
	_#MoreCommon
	name:    string | *"Paul"
	address: string | *"space"
}

#B: {
	_#Common
	name: string | *"gopher"
}

#C: {
	_#MoreCommon
	age: int | *42
}

a: #A & {
	age: 100
}

And semi-related, defaults can be "cascaded": see #889 (comment) for an example.

@myitcv myitcv removed NeedsInvestigation Triage Requires triage/attention labels Jul 28, 2021
@myitcv myitcv changed the title undescriptive error on conflicting defaults cmd/cue: various errors related to conflicting defaults Jul 28, 2021
@myitcv myitcv removed the S label Oct 26, 2021
@myitcv myitcv added the vet candidate for a vet rule to detect suspect usage label May 24, 2022
@myitcv myitcv added the zGarden label Jun 15, 2023
@mvdan mvdan removed the zGarden label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix vet candidate for a vet rule to detect suspect usage
Projects
None yet
Development

No branches or pull requests

4 participants