cmd/cue: hard-to-understand error message when attribute is malformed

[rogpeppe]

trafficstars

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

f1f0963cb0f978020fc895202c50f40452da31b9

Does this issue reproduce with the latest stable release?

N/A

What did you do?

env CUE_EXPERIMENT=embed
exec cue eval .

-- x.cue --
@extern(embed)

package x

x: _	@embed(file="foo" type=json)

-- cue.mod/module.cue --
module: "foo.test"
language: version: "v0.9.2"

-- foo.json --
{"foo": true}

What did you expect to see?

An error saying that the @embed attribute is syntactically malformed (I've used space as a separator rather than comma).

What did you see instead?

> env CUE_EXPERIMENT=embed
> exec cue eval .
[stderr]
@embed: no encoding specified for file "\"foo\" type=json":
    ./x.cue:5:6
[exit status 1]
FAIL: /tmp/x.txtar:2: unexpected command failure

I have specified the encoding - just not in the correct attribute syntax (which isn't documented anywhere AFAIK). I found the error message confusing and it took me a while to work out what was going on.

[rogpeppe]

To update on this, it seems to me like that attribute parser in internal/attrs.go is somewhat too lenient.

For example, consider this attribute:

@foo(a=one b=two, c="three" four)

That's considered entirely legitimate, and corresponds to the following key-value pairs:

{
    "a": #"one b=two"#
    "c": #""three" four"#
}

I'm wondering if we should tighten this up a bit, so the above yields an error instead.

To be specific, I think we should accept exactly one identifier for a key, and either a single token or a bracketed expression for a value.

So, the above example would be invalid because one b=two is not a single token or bracketed expression. @foo("foo" x=value) would be invalid because the key is not a single identifier, as would @foo("foo"=bar) ("foo" is a string not an identifier).

Note that when the key is a string, it does not currently get unquoted as a value does.

With these restrictions in place, we could even make the commas separating fields optional.

[mvdan]

This was added to the 0.10 milestone but I don't see it as a serious issue or regression, and we're only two days until the final release, so I'm removing it for now.