Give a warning for declarations of the form a: a?

[cueckoo]

Originally opened by @ofw in cuelang/cue#466

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

$ cue version
cue version v0.2.2 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you do?

I tried to merge values from several struct into a new one:

#Error: {
	_codes: [...string]
}

#MultiError: {
	_errors: [string]: #Error
	oneOf: [
		for name, _ in _errors {
			{
				$ref: "#/components/schemas/" + name
			}
		}
	]
	discriminator: {
		propertyName: "code"
		mapping: {
			for name, err in _errors {
				for code in err._codes {
					code: "#/components/schemas/" + name
				}
			}
		}
	}
}

components: schemas: {
    ErrorA: #Error & {
		_codes: ["a"]
	}
    ErrorB: #Error & {
		_codes: ["b"]
	}
	ABError: #MultiError & {
		_errors: {
			ErrorA:  ErrorA
			ErrorB: ErrorB
		}
	}
}

What did you expect to see?

I expected that ABError will look like this:

        ABError: {
            oneOf: [{
                $ref: "#/components/schemas/ErrorA"
            }, {
                $ref: "#/components/schemas/ErrorB"
            }]
            discriminator: {
                propertyName: "code"
                mapping: {
                     "a": "#/components/schemas/ErrorA"
                     "b": "#/components/schemas/ErrorB"
                }
            }
        }

What did you see instead?

Eval result:

› cue eval test.cue -c          
components: {
    schemas: {
        ErrorA: {
            type: "object"
            properties: {
                code: {
                    type: "string"
                    enum: ["a"]
                }
            }
        }
        ErrorB: {
            type: "object"
            properties: {
                code: {
                    type: "string"
                    enum: ["b"]
                }
            }
        }
        ABError: {
            oneOf: [{
                $ref: "#/components/schemas/ErrorA"
            }, {
                $ref: "#/components/schemas/ErrorB"
            }]
            discriminator: {
                propertyName: "code"
                mapping: {}
            }
        }
    }
}

So the mapping field is empty.

[cueckoo]

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

The problem is here:

ErrorA:  ErrorA

Here the ErrorA references refers to itself on the LHS.

This is logically should: it is a tautology akin to "a == a", which is why this isn't an error.
In practice this isn't very useful and there should probably be at least be some kind of "vet" warning for this.

You can fix this here by writing:

"ErrorA":  ErrorA

which unshadows the original value.

Once you've fixed this, you will also want to write

"\(code)": "#/components/schemas/" + name

[cueckoo]

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

For reference, the discussion with Ian Cottrell on Slack:

[cueckoo]

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

For reference here, the discussion with Ian Cottrell on Slack:

Ian Cottrell Jan 29th at 8:08 PM
I think there may be issues around self referential fields, but I am also not clear what should happen so it might just be me…
For instance a: 1, a: a | 2 => a:1 whereas a: int, a: a | 2 => a: int | 2 which seems weird
This came up because I keep (indirectly, with more layers, by accident) hitting a:1, b: { a:a } which I have to fix using a:1, b: { "a":a } (quoting the field name). I also fine it really hard to detect/diagnose when this happens.
14 replies

Marcel van Lohuizen 23 days ago
Yes that is right. a: a logically means a==a. So for the int|2 case int unifies with both and both disjuncts remain, whereas for 1 1&2 fails so only 1&int remains.

That said, there is an issue out to treat a: a as an error. That’s hard to justify by itself, but the idea is to make it a vet error, similar to checking fmt parameters in Go: although it is correct, it is almost always erroneous.

Ian Cottrell 23 days ago
I guess my question really is what a means on the right hand side, I guess to be order agnostic then the only thing you can reasonably put in the right hand side is _, which make the results make sense, but also makes the entire construct nonsensical. I also don’t understand why quoting the field name cause it to allow the use of the shadowed value on the right hand side.

Marcel van Lohuizen 23 days ago
Cue is Lexically scoped and a quoted field is not part of the scope.

Marcel van Lohuizen 23 days ago
I’m not quite sure if I understand what you mean with using a _ on the RHS and what is nonsensical about it. Do you have an example?

Ian Cottrell 23 days ago
as in it has no use, it is easier and clearer in every case to use _ instead of the field name on the right hand side, I cannot think of any situation where being allowed to use the field name on the right hand side is helpful and plenty of cases where you are doing so in error, basically I am agreeing that it should be a vet failure

Marcel van Lohuizen 23 days ago
Ah, I see where you getting at. You mean it is never useful to allow a: a so you might as well forbid it. That is true. The main issue is with disallowing it is that it is inconsistent. For instance a: { b: int, c: a } is meaningful, for instance. So why allow that and not a: a. But maybe that is being too unpragmatic. Perhaps a message suggesting it is probably an error and suggesting to replace it with _ makes sense.

Ian Cottrell 23 days ago
I think I finally understand the scope issue, I was fundamentally assuming the parent struct was in scope, I think the documentation just says “enclosing scope”, but that does explain some things that confused me

Marcel van Lohuizen 23 days ago
So the main question is really should it be a vet failure or outright failure. Including a suggestion to replace it with _ (which is essentially what happens during evaluation) is a neat way of making it a compile error, in case users don't use cue vet or a future cue test.

Marcel van Lohuizen 23 days ago
A bit of an uncharacteristically long error message, but: "expression of field foo cannot reference itself: use an alias or quote foo to reference an enclosing foo, or change the reference to _ if you did mean to refer to the field itself". Something like that could work. (edited)

Ian Cottrell 23 days ago
yeah, that would cover it. If it is too long, just make sure it includes a unique enough phrase that a web search always turns up the complete explanation as the first result 🙂

Kevin Gillette 23 days ago
That said, there is an issue out to treat a: a as an error. That’s hard to justify by itself, but the idea is to make it a vet error
Is there the concept of a "complete tautology" (as distinct from a "partial tautology")?
a: a adds nothing new, unlike:
a: b & {x: 1}
b: a & {y: 2}
in which a and b both contribute something new despite it being a recursive/circular definition.
Could cases that add nothing new in the context of a single declaration be rejected as incorrect at the language level?

Kevin Gillette 23 days ago
// not directly tautological, so okay
a: b
b: a | 5
// these are not okay
c: c // directly self referential
d: 5 | int // 5 does not constrain or expand d further
// this is okay:
e: *5 | int
(edited)

Marcel van Lohuizen 22 days ago
Is there the concept of a "complete tautology" (as distinct from a "partial tautology")?
CUE doesn't recognize that at the moment, indeed. But IIUC a reference to self can be replaced with _ if and only if it is a top-level conjunct, which is not hard to detect.
d: 5 | int // 5 does not constrain or expand d further
Funnily I wrote down that this should be forbidden somewhere, and the old implementation simplified result disjunctions along these lines. Currently CUE allows this as there actually is a problem disallowing this: subsumption of non-concrete values (especially taking pattern constraints in account) is non trivial in CUE and NP-complete. Also, subsumption is not currently needed for evaluation, so this doesn't pose a problem. Introducing guaranteed simplification rules does pose a problem here. All of this disappears, though, when "concretizing" values. I'm coincidentally working on a more exact rules along these lines specifically for this phase. v0.2 did a best-effort attempt at minimizing. For v0.3/4 we're thinking more along the lines of precise minimization when making things concrete. This also leaves intermittent simplifications to be an implementation detail.
Here disallowing could be done on a best-effort basis as a vet rule.

Marcel van Lohuizen 22 days ago
BTW, there is at least one case where a field referring to itself makes sense:
The proposed must builtin would allow making custom constraints. For instance, with this builtin
a: <5
could be written as
a: must(a < 5)
So a mechanism that detects senseless self-references must be aware of the semantics of builtins.
See cuelang/cue#575. (edited)