cmd/cue: unclear how to validate selector and qualified identifier errors

[cueckoo]

Originally opened by @myitcv in cuelang/cue#629

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

$ cue version
cue version +40d9da31-dirty linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

Consider the following:

exec cue eval x.cue

-- cue.mod/module.cue --
module: "mod.com"
-- fn/fn.cue --
package fn
-- x.cue --
package x

import "mod.com/fn"

s: {}

a: y    // ERROR: reference "y" not found
b: s.y  // not an error
c: fn.y // not an error

gives the following:

a: reference "y" not found:
    ./x.cue:7:8

This error above is as expected.

According to my reading of the spec, both s.y and fn.y evaluate to _|_. Hence, strictly speaking, the output above is correct.

But the lack of errors for lines 8 and 9 are tricky as the author of this code. I have no way of knowing that I have referenced non-existent fields until evaluation time. My error has been masked by the use of a disjunction.

Note: this repro is derived from a real-world example where this typo was made across tens of files.

What did you expect to see?

Unclear, because it's conceivable that a different user might have intended the above behaviour, and so not expect to see errors for s.y or fn.y

cc @jlongtine

2022-05-03 edit: removed use of disjunctions, because this issue is primarily being referenced because of the lack of errors/UX issues that come with incomplete references. The disjunction case is an interesting extension of that same problem, but is one we necessarily need to solve second.

[cueckoo]

Original reply by @mpvl in cuelang/cue#629 (comment)

Like with Go, one should distinguish a reference from a selector.

A missing reference, the first y in this case, is a unrecoverable compile-time error. It is illegal CUE. CUE cannot evaluate meaningfully without resolving a reference.

This is the only reference of y. The other y's are selectors in the resolved references fn and s. These are not compile time errors. As per the CUE spec, these are just errors that get eliminated as disjuncts as part of CUE evaluation and thus will not generate user-facing errors. In fact, these errors are even resolvable (it is an "incomplete" error), as a user may resolve the error by making a configuration more concrete.

In the case of the package one could argue the user has no control over adding more values and thus the error should be terminal, but also terminal evaluation errors simply get eliminated as a disjunct.

I'm not sure what errors you would want to present to the user in this case, as these really aren't errors. A linter could maybe complain about the package reference, assuming that packages are static. But the s.y expression is completely valid and could be as intended. It would be incorrect for a linter to complain about this.

A different story would be if s were closed. In that case s.y could never resolve to a valid value, which would indeed be suspicious. So in that case it makes sense for a linter to complain, even though it is still valid CUE. We could also consider making this not valid CUE, although the definition of that would be somewhat tricky.

[cueckoo]

Original reply by @myitcv in cuelang/cue#629 (comment)

Like with Go, one should distinguish a reference from a selector.

Indeed, which is why I included all three, as a compare and contrast. The case of a bad reference is I agree clear.

In the case of the package one could argue the user has no control over adding more values and thus the error should be terminal, but also terminal evaluation errors simply get eliminated as a disjunct.

Yes, the case of a package selector is what gave rise to this case.

A different story would be if s were closed. In that case s.y could never resolve to a valid value, which would indeed be suspicious. So in that case it makes sense for a linter to complain, even though it is still valid CUE. We could also consider making this not valid CUE, although the definition of that would be somewhat tricky.

You're quite right, I intended for my example using s to either be a definition or close()-d. Because as written it it not contentious at all. Here's an updated version:

exec cue eval
-- cue.mod/module.cue --
module: "mod.com"
-- fn/fn.cue --
package fn
-- x.cue --
package x

import "mod.com/fn"

s: {}
#def: {}

a: 5 | y        // ERROR: reference "y" not found
b: 5 | s.y      // not an error - OK
c: 5 | fn.y     // not an error - ??
d: 5 | #def.y   // not an error - ??

So in that case it makes sense for a linter to complain, even though it is still valid CUE. We could also consider making this not valid CUE, although the definition of that would be somewhat tricky.

In the case of the package and definition selectors (cases c and d above), I can see how it being something a linter would catch might indeed make sense (given as you say it is valid CUE).

[cueckoo]

Original reply by @mpvl in cuelang/cue#629 (comment)

I can see how it being something a linter would catch might indeed make sense (given as you say it is valid CUE).

It probably makes sense to do something very similar to what Go does, and automatically trigger such errors for cue test and cue vet, for instance.

[cueckoo]

Original reply by @mpvl in cuelang/cue#629 (comment)

FTR: internally, CUE marks s.y as an incomplete error (could be resolved by making a configuration more concrete) and #def.y as an eval error (cannot be fixed by making something more concrete). So there is a mechanism in place to distinguish these cases already.

For fn.y we would need to decide. Technically, as the top-level of the package isn't closed, fn.y could still be resolved by making the package more concrete. One could argue that this is not going to happen. It is very hard to figure out what would be the right semantics in the general case: as values of packages and non-packages get mixed during computation, things get complicated. So I would argue this still should be marked as an incomplete error during evaluation. However, a linter could still recognize this as a special case (incomplete error of an unknown selector into a package reference) and complain.

[cueckoo]

Original reply by @myitcv in cuelang/cue#629 (comment)

@mpvl I suggest we at least consider this for v0.5.0 - thoughts?

[eonpatapon]

Using the go API, I'd love to have an option I can pass to Value.Validate() that will check that selectors are valid and point to some value (which may be concrete or not). Maybe using the existing cue.Final() option ?

So that cases like s.y, fn.y can be detected as I know that my configuration is final and nothing can be added in it.

In my case I can't use cue.Concrete(true) which would detect theses because some parts of my config are inherently not concrete by the usage of cue flows.

[helderco]

I just referenced a few issues from dagger.

Users are getting confused because typos are leading to unexpected errors elsewhere and it's hard to understand why.

At least now I understand it a bit more to be able to help them, but should we be doing something differently?

I follow what @eonpatapon is saying, it's the same here.

[myitcv]

@helderco please can you provide examples of the issues people are seeing in Dagger? We should categorise them to ensure they are either covered by the cases described in the description of this issue, or to add to that list. Thanks

[myitcv]

@eonpatapon there is precedent for your suggestion; it aligns with the use of cue.Final() when it comes to cue.Value.Fields():

cue/cue/types_test.go

Lines 747 to 752 in c942d0a

	opts: []Option{Final()},
	value: `
	step1: {}
	if step1.value > 100 {
	}`,
	err: "undefined field: value",

But clearly Validate() is what we would reach for here and not Fields().

So the suggestion of having cue.Value.Validate() support cue.Final() appears to be a reasonable way forward, not least because it allows us to punt on the question of whether package values are closed or not.

cc @mpvl

[helderco]

@helderco please can you provide examples of the issues people are seeing in Dagger? We should categorise them to ensure they are either covered by the cases described in the description of this issue, or to add to that list. Thanks

They're essentially about those two situations:

b: 5 | s.y
c: 5 | fn.y

Except that instead of referencing from a disjunction they're being referenced from a cue/flow task which is making the task not run, and thus leading to all sorts of unexpected behavior elsewhere (missing dependency values).

[myitcv]

@helderco please can you provide complete examples so that we can validate the solution suggested by @eonpatapon (and indeed any others)?

[helderco]

Yes I planned on doing that today but I just haven’t been able to get to it yet.

[helderco]

There's an example from:

• tools/flow: No error when using an undefined reference to a sub-field, in a dependency #1611
• Reference to a non-existing client filesystem doesn't break, and makes the action disappear/unusable dagger/dagger#1885 (comment)

exec cue cmd actions

-- main_tool.cue --
package main

import "tool/cli"

command: {
	actions: {
		a: cli.Print & {
			$before: [actions.b]
			text: "foobar"
		}
	}
}

It doesn't fail as we'd expect, but we're also seeing that the task isn't running (not sure how to reproduce here).

We're seeing this because of typos.

[aluzzardi]

I believe the solution suggested by @eonpatapon should work for us. We'll try and provide some additional examples.

We did manage to implement a workaround for now, which is to walk through the values and check Value.Error() for undefined fields.

On a general note, I've re-read through the comments and still cannot understand why referencing something that doesn't exist doesn't throw an error. The evaluator is clearly aware of that since there's something in .Err().

[myitcv]

@aluzzardi @helderco

We'll try and provide some additional examples.

Thanks. Seeing real Dagger examples will help to understand the context here. The example that @helderco links to above is a cue cmd example, but one of the issues linked from that comment is a Dagger issue. The reason this is significant is that the former does not use definitions (which is something we plan to fix, incidentally) but the latter does. Seeing the context in which the "errors" are not appearing is critical to understanding the UX you expect and how that might reasonably be enforced given the context.

[myitcv]

Except that instead of referencing from a disjunction

Edit comment: I have just updated the title and description of this issue to remove the reference to "disjunctions". This issue is primarily being referenced because of the UX issues caused by a lack of errors in selector and qualified identifier references. The case of disjunctions is also interesting/important, but we should focus on that non-disjunction case first.

[helderco]

Here's an incomplete cause disjunction we're seeing.

exec cue eval x.cue -c

-- cue.mod/module.cue --
module: "example.com"

-- fn/fn.cue --
package fn

#M: {
	t: "s"
	c: string
} | {
	t: "i"
	c: int
}

-- x.cue --
package x

import "example.com/fn"

m: fn.#M

// typo
m: fn.#MM & {
	c: 5
}

> exec cue eval x.cue -c
[stderr]
m: incomplete cause disjunction

Just another side effect of 5 | fn.y above, I guess.

Expected to see m: undefined field: #MM. Without that failure, we get unexpected errors elsewhere, because they're not the root cause.

Related:

• 🐞 panic: not properly initialized state dagger/dagger#2326

[myitcv]

@helderco thanks.

Please can you share Dagger examples for these "problem" scenarios? Reason being, I think what we're discussing here is an (important) issue of UX, and whether/when certain state is surfaced as an "error" or not. That means we really need to see situations where Dagger users expect to see an "error" but have not.

(Note I am using "error" here to indicate the user's expectation of "I expected to see an error reported by Dagger/my editor because what I have written is wrong/doesn't work" rather than any specific type of error as defined by CUE).

[helderco]

They're always about typos that users make. Instead of the plan failing, actions just got "dropped" (not executed), which leads to unexpected errors elsewhere because values aren't being filled by their dependencies. The problem is the error message doesn't point to the origin of the issue, but to a side-effect of it, which leads to confusion.

The issues have more context. They're now closed because we've added validation, but I'm compiling a couple examples here so you get an ideia.

Typos in imports:

• Some cue errors are not caught before running dagger/dagger#493
• Referencing an inexistent action in a package makes the action be ignored from the #Plan dagger/dagger#1635
• 🐞 Dagger: Migrating from the dagger library to core dagger/dagger#1990

So a user may use core.#WriteFiles when the correct import is core.#WriteFile. Here's an example:

package main

import (
	"dagger.io/dagger"
	"dagger.io/dagger/core"
	"universe.dagger.io/python"
)

dagger.#Plan & {
	actions: {
		_fs: core.#WriteFiles & { // typo, doesn't fail here
			input: dagger.#Scratch
			path: "/test.py"
			contents: "print('foobar')"
		}
		test: python.#Run & {
			script: {
				directory: _fs.output // fails on an internal field that depends on this, since it's empty/inconcrete
				filename: "test.py"
			}
		}
	}
}

// OUTPUT:
// failed to load plan: actions.test.mounts."Python script": 1 errors in empty disjunction:

Expected:

failed to load plan: actions._fs: undefined field: #WriteFiles:

Typos in references to a non-existing field in an open struct

• Incomplete action is silently ignored dagger/dagger#1572
• Changing client filesystem read path from "." to "./" triggers cyclic task dependency error dagger/dagger#1857
• Reference to a non-existing client filesystem doesn't break, and makes the action disappear/unusable dagger/dagger#1885

These also lead to actions being dropped and producing errors relating to side-effects. Here's a simple example:

package main

import (
	"dagger.io/dagger"
	"universe.dagger.io/python"
)

dagger.#Plan & {
	client: filesystem: ".": read: contents: dagger.#FS
	actions: {
		test: python.#Run & {
			script: {
				directory: client.filesystem."./".read.contents // typo in the path, doesn't match to any action
				filename: "test.py"
			}
		}
	}
}

Conclusion

Here's what @aluzzardi concluded:

I discovered this affects fields of "open" structures (e.g. _, maps, ...):

• actions.nonexistent: since actions is open (e.g. actions: _)
• dagger.#Foo: apparently imported modules are open as well
• client.filesystem."/non/existent": open as wellfilesystem: [path=string]

I made another discovery: CUE fails right away when referencing undefined fields in closed structures. It's not the case for open structures. HOWEVER, cue.Value.Err() does return an error

I implemented a POC in #2334 that was able to catch all of that by recursively checking cue errors in the entire tree. It is highly hackish as we expect errors in dagger plans by definition (e.g. because of non-concrete fields that will be filled by the runtime later on):

dagger.#Plan & {
	client: {}

	actions: test: {
		undefinedAction: core.#Nop & {
			input: actions.nonexistent
		}

		undefinedDef: core.#NonExistent & {
			input: dagger.#Scratch
		}

		filesystem: core.#Nop & {
			input: client.filesystem."/non/existent".read.contents
		}
	}
}

$ dagger do test
failed to load plan: actions.test.undefinedAction.input: undefined field: nonexistent:
    ./undefined.cue:13:19
actions.test.undefinedAction.output: undefined field: nonexistent:
    ./undefined.cue:13:19
actions.test.undefinedDef: undefined field: #NonExistent:
    ./undefined.cue:16:22
actions.test.filesystem.input: undefined field: "/non/existent":
    ./undefined.cue:21:29
actions.test.filesystem.output: undefined field: "/non/existent":
    ./undefined.cue:21:29

Originally posted in dagger/dagger#493 (comment)

I hope this helps.

[myitcv]

Typos in references to a non-existing field in an open struct

These also lead to actions being dropped and producing errors relating to side-effects. Here's a simple example:

Please can you confirm the expected behaviour in this case?

dagger.#Plan & {
	client: {}

	actions: test: {
		undefinedAction: core.#Nop & {
			input: actions.nonexistent
		}

		undefinedDef: core.#NonExistent & {
			input: dagger.#Scratch
		}

		filesystem: core.#Nop & {
			input: client.filesystem."/non/existent".read.contents
		}
	}
}

Please can you or @aluzzardi confirm the expected output in this situation? i.e. ignoring the POC/solution you have now, which "errors" would you expect to be flagged in an ideal world?

[helderco]

We expect the undefined fields to fail during load, before anything else. Because these lead to other types of errors.

• actions.nonexistent
• core.#NonExistent
• client.filesystem."/non/existent"

[helderco]

The client.filesystem one is a bit tricky, because even though we don't support dynamic tasks now, I think we would like to in the future:

client: {
    env: KUBECONFIG: string
    filesystem: (env.KUBECONFIG): read: contents: string
}

So env and filesystem: [string]: read are different tasks, and this would require that the CUE path to the filesystem task be dependent on the output of another task (which we don't support yet).

In that case we wouldn't want to fail on load with client.filesystem."/non/existent", I think, because we wouldn't know during load.

[helderco]

I guess it's the same with actions.nonexistent actually, if we do support dynamic actions:

actions: {
    foo: core.#Nop & {
        input: actions.bar
    }
    if foo.output == "success" {
        bar: core.#Nop & {
            input: "foobar"
        }
    }
}

[helderco]

Disregard my last example, it's not a good one. Not sure how you'd depend on a field that might not exist.

[eonpatapon]

On a general note, I've re-read through the comments and still cannot understand why referencing something that doesn't exist doesn't throw an error. The evaluator is clearly aware of that since there's something in .Err().

I think it depends how you evaluate cue.

If using the go api after the first evaluation even if there is some missing references you can still use FillPath() to define them and make the errors go way. It really depends how the application manipulate the cue instance. That's why I was suggesting to have that option in Validate(), so that one could decide when such validation has to occur.

In the case of cue eval I don't really see why it couldn't be done by default (except it would be a breaking change) as the evaluation is hermetic and done in one go. Also I see there is a existing global --strict option. Maybe that could enable such checks when used ?

For cue cmd it's not very clear when such validation could be done because as tasks are being run they can resolve references used by downstream tasks. For example:

command: x: {
  a: http.Get & {
    url: "..."
    response: body: string
    decoded: json.Unmarshal(response.body)
  }
  b: cli.Print & {
   text: a.decoded.foo.bar
  }
}

Technically before the flow is run a.decoded.foo.bar doesn't exists. So references validation would fail, unless the user specify what is expected explicitly:

decoded: json.Unmarshal(response.body)
decoded: foo: bar: string

[hectorj2f]

In relation to this issue, I have been using struct.MinFields(X) function expecting to throw errors when the condition was not satisfied. However It didn't happen in contrast to struct.MaxFields(X) that actually reported errors. I'd expect the same behavior for struct.MinFields. It looks like the return arguments are even different for both functions which looks weird.

[myitcv]

@hectorj2f yes, a similar "issue" exists for struct.MinFields(). Because it's possible to supply further CUE that causes that constraint to be satisfied, much like it's possible for an undefined reference to be resolved in the presence of further CUE. The same cannot be said for struct.MaxFields() hence the latter can more aggressively be reported as an error. But as you've seen from the discussion above, from a UX perspective this approach doesn't necessarily align with DX.