cmd/cue: vet's -c flag may not be working as documented

[mvdan]

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

$ cue version
cue version v0.5.0-beta.1.0.20221119112418-32bfffd0aa53

Does this issue reproduce with the latest release?

Yes; v0.4.3 has the same output.

What did you do?

exec cue vet schema.cue data.json
! exec cue vet -c schema.cue data.json

exec cue vet schema.cue data.cue
! exec cue vet -c schema.cue data.cue
-- schema.cue --
foo: string
bar: string
-- data.cue --
foo: "x"
-- data.json --
{
	"foo": "x"
}

What did you expect to see?

From cue vet -h, I see:

vet validates CUE and other data files

By default it will only validate if there are no errors.
The -c validates that all regular fields are concrete.
[...]

So I would assume that cue vet would only check errors, and cue vet -c would check for errors and require the resulting values to be concrete.

So, cue vet schema.cue data.json should succeed, but cue vet -c schema.cue data.json should fail as the value is not concrete.

I would expect the same if the data is in CUE format rather than JSON. Any arguments being non-CUE files does make vet behave in a different mode, but as far as I can tell, that shouldn't affect whether we require concreteness (or completeness?).

What did you see instead?

All four commands fail:

$ testscript -v repro-cmd.txtar 
[...]
> exec cue vet schema.cue data.json
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]
> ! exec cue vet -c schema.cue data.json
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]
> exec cue vet schema.cue data.cue
[stderr]
some instances are incomplete; use the -c flag to show errors or suppress this message
[exit status 1]
> ! exec cue vet -c schema.cue data.cue
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]

cc @rogpeppe

[myitcv]

So, cue vet schema.cue data.json should succeed

I've updated the repro to reflect this expectation, and for the data.cue case too.

Indeed, just to confirm my understanding, does this reflect the expectation?

What did you do?

# With -c flag
! exec cue vet -c schema.cue data.json
cmp stderr stderr.golden
! exec cue vet -c schema.cue data.cue
cmp stderr stderr.golden

# No -c flag
exec cue vet schema.cue data.json
! stdout .+
exec cue vet schema.cue data.cue
! stdout .+

-- schema.cue --
foo: string
bar: string
-- data.cue --
foo: "x"
-- data.json --
{
	"foo": "x"
}
-- stderr.golden --
bar: incomplete value string:
    ./schema.cue:2:6

What did you expect to see?

A passing test.

What did you see instead?

$ ts repro.txtar
# With -c flag (0.027s)
# No -c flag (0.015s)
> exec cue vet schema.cue data.json
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]
FAIL: /tmp/testscript2980766130/repro.txtar/script.txtar:8: unexpected command failure
> exec cue vet schema.cue data.cue
[stderr]
some instances are incomplete; use the -c flag to show errors or suppress this message
[exit status 1]
FAIL: /tmp/testscript4206477420/repro.txtar/script.txtar:10: unexpected command failure

(notice I have hand-crafted the output, because testscript ordinarily fails on the first failure)

i.e. the bug here is not with -c: that appears to be working as expected.

Rather that without -c, we still get errors when we should not, correct?

[mvdan]

Rather that without -c, we still get errors when we should not, correct?

Correct.

(notice I have hand-crafted the output, because testscript ordinarily fails on the first failure)

I can see how it is better from a clarity perspective to represent the desired behavior in the testscript, not the current behavior. It's just hard to do that currently, as testscript stops at the first unexpected command result. That's why I tend to write a testscript which passes, even if that reflects the current bad behavior.

Perhaps it would be enough for testscript to support ? - then our reproducer could use that in the last two commands, allowing them to show either success or failure, but never stop the execution.

Either way, it would be nice to clarify this in https://github.com/cue-lang/cue/wiki/Creating-test-or-performance-reproducers. That is, when filing a reproducer, should the testscript represent the desired or the current behavior?

[mvdan]

It's perhaps also worth noting that the thread is a bit confusing now that we have two slightly different test scripts and manually crafted outputs :) It's fine by me for you to continue editing my original post - then there's no need to have a sibling in a comment.

[myitcv]

It's just hard to do that currently, as testscript stops at the first unexpected command result.

Which is why I was pondering a mode for cmd/testscript that allows execution to continue in the presence of errors, just logging the result. But that's very much unrelated to this issue.

That's why I tend to write a testscript which passes, even if that reflects the current bad behavior.

I find this counterintuitive given that the heading is "What did you do?", not "What is the current behaviour?". As a reader, my expectation is therefore that I will be looking at a reproducer that shows me what the user tried to do. The heading that follows, "What did you expect to see?" then reflects their expectation given the reproducer. "What did you see instead?" represents the reality.

Perhaps it would be enough for testscript to support ? - then our reproducer could use that in the last two commands, allowing them to show either success or failure, but never stop the execution.

I'm not clear that this helps, because ? doesn't say anything about the expectation of whether the command should pass or not. And that's the whole point in this case (along with the output to stderr). It might help for the construction of the repro, but I think it confuses the actual bug report.

That is, when filing a reproducer, should the testscript represent the desired or the current behavior?

Is that not clear from the questions in the bug report, per my comments above? I don't think it's specific to the wiki, rather the bug template itself. So if that can be improved we should do it there.

[mvdan]

allows execution to continue in the presence of errors

I guess its usefulness would be rather limited, because some errors really are fatal and will break the following commands in basic ways. That's why I was thinking of ? - it doesn't set an expectation either way, but it does signal "this result is not what I would expect".

It might help for the construction of the repro, but I think it confuses the actual bug report.

Sure, it's a bit of a tradeoff and I agree that clarity probably wins here. Though another aspect of manually constructing the testscript is that I can't run the whole thing to see what it does, as it just stops :) but perhaps that's solved by the same change we're talking about above.

Is that not clear from the questions in the bug report, per my comments above?

Well, "what did you do" says nothing about whether the reproducer testscript should "PASS" or "FAIL". I default to "PASS to reproduce the bug", and you default to "PASS once the bug is fixed per my expectation". I don't think the template nor the wiki page point towards one method or the other. I did follow "what did you expect to see" (I expected to see "FAIL") and "what did you see instead" (I saw a "PASS", with the provided output).

I don't have a particularly good idea about what the template could look like, yet. But where we explain how to write testscripts is the wiki page, so my intuition is that the expected result of running the testscript should be defined there too.

[myitcv]

FYI for anyone following along. Much of the exchange above will be dramatically improved by rogpeppe/go-internal#189.

[myitcv]

rogpeppe/go-internal#189 has landed, which makes it easier to express what's going on here in the form of test that is expected to pass.

What did you do?

Ran:

testscript -continue <<EOD
# With -c flag
! exec cue vet -c schema.cue data.json
! exec cue vet -c schema.cue data.cue

# No -c flag
exec cue vet schema.cue data.json
exec cue vet schema.cue data.cue

-- schema.cue --
foo: string
bar: string
-- data.cue --
foo: "x"
-- data.json --
{
	"foo": "x"
}
EOD

(notice use of the new -continue flag)

What did you expect to see?

Passing test.

What did you see instead?

# With -c flag (0.027s)
# No -c flag (0.025s)
> exec cue vet schema.cue data.json
[stderr]
bar: incomplete value string:
    ./schema.cue:2:6
[exit status 1]
FAIL: /tmp/testscript1217859574/-/script.txtar:6: unexpected command failure
> exec cue vet schema.cue data.cue
[stderr]
some instances are incomplete; use the -c flag to show errors or suppress this message
[exit status 1]
FAIL: /tmp/testscript1217859574/-/script.txtar:7: unexpected command failure
error running <stdin> in /tmp/testscript1217859574/-

[jpluscplusm]

Rather that without -c, we still get errors when we should not, correct?

I've just been bitten by exactly this.

I've tended to use cue vet as a signal for "have I done anything, thus far, that CUE can tell me is /definitively/ wrong?", often as a mid-implementation-journey lightweight check. I use cue vet -c in a distinctly different scenario, to ask "have I done /enough/ that I'll be able to emit structures into non-CUE-aware tooling?". I've also used cue vet as a check for unit-testing-CUE-"functions" ("Functional CUE", not actual functions) - a setup which doesn't work so long as cue vet, without -c, has exit-code-affecting opinions about concreteness.

The stderr warning given for non-concrete values when -c isn't used is fine; but the resulting error code being non-zero solely because of the non-concreteness -- when I feel I didn't ask for that check to be performed -- is quite problematic. It effectively stops cue vet being used in various testing scenarios.

The discussion, above, of using a new testscript feature is (IMHO!) onerous. Requiring the consumer to have testscript also installed to work round the problem isn't a great fix; especially when these kinds of tests are being run in Enterprise-level CI/CD pipelines, where the process of on-boarding new tooling is often /highly/ non-trivial! I /may/ have misread the testscript feature's discussion (perhaps it's a meta discussion, not affecting the cue-vet-c problem?), but I thought it worth explicitly making the point that the problem should -- IMHO! -- most properly be addressed in the cue CLI :-)

[mvdan]

The discussion about testscript was purely about providing a useful and self-describing reproduction script :) it has nothing to do with the fix itself.

[mvdan]

@rogpeppe pointed out that the flag currently has three states:

cue/cmd/cue/cmd/vet.go

Lines 113 to 118 in 48207fb

	if flag := cmd.Flag(string(flagConcrete)); flag != nil {
	hasFlag = flag.Changed
	if hasFlag {
	concrete = flagConcrete.Bool(cmd)
	}
	}

That is, the presence of the flag changes behavior, it's not just its true/false value.

So we then have three modes currently:

• Without any -c flag, we get the default behavior, currently documented as "only validate if there are no errors". This mode can trigger some instances are incomplete; use the -c flag to show errors or suppress this message.
• With -c or -c=true, we require the evaluation to be concrete, showing all the non-concrete errors.
• With -c=false, we don't require the evaluation to be concrete at all.

I wasn't aware of the default mode being neither true or false when I raised this issue; I naively understood that the boolean flag would work like most others, where there are just two modes and the default is false.

I would still leave this issue open. If the current behavior is desirable, it should be better documented, because I completely misunderstood it until Roger pointed me at the code. I would argue that the default behavior should be tweaked as well, though, per the discussion above; cue vet shouldn't fail on incomplete errors at all.

[mvdan]

My last comment here is correct - I still think the three modes are slightly confusing and could be better documented - but we should focus on the bug at hand with cue vet (without a flag) first. That is what Paul showed in #2120 (comment): cue vet appears to show either errors or warnings relating to incomplete values, and return an exit code of 1, even though we didn't specify the -c flag.

[myitcv]

Working on https://review.gerrithub.io/c/cue-lang/cuelang.org/+/1203092 with @jpluscplusm, again highlights we should fix things here.

[myitcv]

FWIW, I'm still of the opinion here that the default of cue vet should simply be "tell me if there are any errors". Note that this definition does not include "tell me if my CUE is noncrete" (internally we refer to incomplete errors as helping to identify this state). Because CUE is quite comfortable with incomplete and noncrete values, values that can later be "fixed" or improved by unifying with further CUE. It is not an error to have CUE that, absent any other errors, is simply incomplete and noncrete.

Hence:

• cue vet: fail if any errors, printing details of those errors
• cue vet -c: fail if any errors, or if the resulting value is noncrete, printing relevant details

We can therefore drop the tri-state nature of this flag, and lose -c=false, because the current default mode (fail if any errors with details, or if resulting value is noncrete but don't show details) doesn't appear to be useful: I would just use -c (in the new world I'm proposing) and check the exit code for being non-zero, ignoring the details of what is noncrete.

Edit: updated to use incomplete/noncrete properly. I was sloppy given my focus on https://review.gerrithub.io/c/cue-lang/cuelang.org/+/1203092

[mpvl]

Changing the behavior is a breaking change for people that include cue vet in scripts. This should be gauged on a tooling version, experiment, or such.

But I think we are better served with coming up with a redesign on cue vet based on first principles and then come up with a transition plan for that.

[mvdan]

@myitcv and I are in agreement that, at minimum, for v0.12 we need to better document the existing behavior in cue vet -h and cuelang.org content.

[jpluscplusm]

@myitcv and I are in agreement that, at minimum, for v0.12 we need to better document the existing behavior in cue vet -h and cuelang.org content.

On cuelang.org/docs/reference/command/ we have both the full set of auto-generated cue help ... texts, and also the beginnings of a set of reference guides, each focused on a specific command.

At the top of that page you'll see the link to the single guide we have thus far, for cue export. The purpose of that guide (at least in my head) is to nudge readers on to the more in depth concept guide for each command - which has the dual advantage of being able to be written more quickly as it's in a somewhat less reference-y style; and is also able to be more approachable than a formal reference guide, hence can cater to a wider audience. You can be the judge of how true you feel those points are: Using the cue export command.

I propose we publish a parallel pair of cue vet reference and concept guides, targeting v0.12's release window; alongside improvements to cue help vet.

[myitcv]

I propose we publish a parallel pair of cue vet reference and concept guides, targeting v0.12's release window; alongside improvements to cue help vet.

Agreed.

[jpluscplusm]

a parallel pair of cue vet reference and concept guides

Tracked as cue-lang/docs-and-content#192

[jpluscplusm]

better document the existing behavior in cue help vet

I've attempted to do this in https://cuelang.org/cl/1207991.

and cuelang.org content

This is underway in https://cuelang.org/cl/1207010.

[jpluscplusm]

How would folks feel about a CL that proposes a change in the general cue vet error message from

some instances are incomplete; use the -c flag to show errors or suppress this message

... to ...

some instances are incomplete; use the -c flag to show errors or -c=false to suppress this message

This small update would give the user of the current cue vet command a reasonably accurate and contextually relevant hint at their possible next steps, without taking up too much additional horizontal space on stderr.

[myitcv]

-c=false to suppress this message

I think this might be a useful short term fix, however, I'm not clear that this is the best choice of wording. Because it seems to suggest we will suppress this message - what about the exit code?

Instead, I would positively specify:

some instances are incomplete; use the -c flag to show errors or -c=false to allow incomplete instances

(granted the current error message is not good, but that's what we're trying to improve)

[jpluscplusm]

Instead, I would positively specify:

Proposed in https://cuelang.org/cl/1208694.