From 9fba4fa553dd681a9e766678488f832d2ea32cf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Mart=C3=AD?= Date: Thu, 4 Apr 2024 14:58:12 +0900 Subject: [PATCH] internal/encoding/yaml: add the new Decoder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This implementation builds directly on top of gopkg.in/yaml.v3's Decoder as opposed to internal/third_party/yaml.Decoder, which forked go-yaml. There are a number of differences between the old and new decoders. Starting with the disadvantages or minor regressions per decode_test.go: * yaml.v3's Node only has each node's start position, lacking any end or closing token position. As such, empty lists or objects are always decoded into a single line even when the YAML was multi-line. * yaml.v3's Node does not record precise comment positions, so we must assume that any HeadComment lines immediately precede the YAML node. In some cases this isn't true, and we remove an empty line. * yaml.v3's errors do not always contain line information, such as the "unknown anchor" errors already covered in our test suite. There are a number of advantages or improvements, though: * We no longer lose inline or foot/trailing comments in some cases, although we continue to lose comments inside empty objects and lists. * Floating point scalars which are valid CUE integral literals are now decoded as `number & N` rather than `float & N`, as the latter simply fails, e.g. `float & 123` is an error. * The test case using simply `v: ! test` works as expected now. It should also be noted that an empty YAML input or document decodes as a nil ast.Expr in both the old and new decoder, even though the old decoder seemingly intended to decode empty input as a CUE null literal. It returned that null literal alongside an io.EOF error, which consumers such as internal/encoding and pkg/encoding/yaml ignored as they stopped looking at ast.Expr result values at the first error. Add a TODO to consider switching the behavior for empty inputs to produce a single "null" value instead as a separate change. And for now, continue with the simple design of never returning io.EOF alongside any non-nil valid result, to not make changes elsewhere. The new decoder is disabled by default and can be enabled via CUE_EXPERIMENT=yamlv3decoder - a follow-up CL will enable it by default. Updates #3027. Signed-off-by: Daniel Martí Change-Id: I538725c5157924639d7084228d59e3f4e58c6d23 Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1191897 Reviewed-by: Roger Peppe TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine --- cmd/cue/cmd/import.go | 4 +- encoding/yaml/yaml.go | 7 +- internal/cueexperiment/exp.go | 4 + internal/cueexperiment/exp_test.go | 3 +- internal/encoding/encoding.go | 6 +- internal/encoding/yaml/decode.go | 675 ++++++++++++++++++++++ internal/encoding/yaml/decode_test.go | 100 ++-- internal/encoding/yaml/testdata/merge.out | 39 +- pkg/encoding/yaml/manual.go | 19 +- 9 files changed, 764 insertions(+), 93 deletions(-) create mode 100644 internal/encoding/yaml/decode.go diff --git a/cmd/cue/cmd/import.go b/cmd/cue/cmd/import.go index 80a669af4..dfd947dff 100644 --- a/cmd/cue/cmd/import.go +++ b/cmd/cue/cmd/import.go @@ -36,7 +36,7 @@ import ( "cuelang.org/go/encoding/json" "cuelang.org/go/encoding/protobuf" "cuelang.org/go/internal" - "cuelang.org/go/internal/third_party/yaml" + pkgyaml "cuelang.org/go/pkg/encoding/yaml" ) func newImportCmd(c *Command) *cobra.Command { @@ -590,7 +590,7 @@ func tryParse(str string) (s ast.Expr, pkg string) { return nil, "" } - if expr, err := yaml.Unmarshal("", b); err == nil { + if expr, err := pkgyaml.Unmarshal(b); err == nil { switch expr.(type) { case *ast.StructLit, *ast.ListLit: default: diff --git a/encoding/yaml/yaml.go b/encoding/yaml/yaml.go index d24872e76..118f086b8 100644 --- a/encoding/yaml/yaml.go +++ b/encoding/yaml/yaml.go @@ -23,7 +23,7 @@ import ( "cuelang.org/go/cue" "cuelang.org/go/cue/ast" cueyaml "cuelang.org/go/internal/encoding/yaml" - "cuelang.org/go/internal/third_party/yaml" + "cuelang.org/go/internal/source" pkgyaml "cuelang.org/go/pkg/encoding/yaml" ) @@ -33,11 +33,12 @@ import ( // src is nil, the result of reading the file specified by filename will // be used. func Extract(filename string, src interface{}) (*ast.File, error) { - a := []ast.Expr{} - d, err := yaml.NewDecoder(filename, src) + data, err := source.ReadAll(filename, src) if err != nil { return nil, err } + a := []ast.Expr{} + d := cueyaml.NewDecoder(filename, data) for { expr, err := d.Decode() if err != nil { diff --git a/internal/cueexperiment/exp.go b/internal/cueexperiment/exp.go index 302b6ac20..3ee658afe 100644 --- a/internal/cueexperiment/exp.go +++ b/internal/cueexperiment/exp.go @@ -10,6 +10,10 @@ import ( // by Init. var Flags struct { Modules bool + + // YAMLV3Decoder swaps the old internal/third_party/yaml decoder with the new + // decoder implemented in internal/encoding/yaml on top of yaml.v3. + YAMLV3Decoder bool } // Init initializes Flags. Note: this isn't named "init" because we diff --git a/internal/cueexperiment/exp_test.go b/internal/cueexperiment/exp_test.go index 760eede6a..e75a6e847 100644 --- a/internal/cueexperiment/exp_test.go +++ b/internal/cueexperiment/exp_test.go @@ -8,8 +8,9 @@ import ( func TestInit(t *testing.T) { // This is just a smoke test to make sure it's all wired up OK. - t.Setenv("CUE_EXPERIMENT", "modules") + t.Setenv("CUE_EXPERIMENT", "modules,yamlv3decoder") err := Init() qt.Assert(t, qt.IsNil(err)) qt.Assert(t, qt.IsTrue(Flags.Modules)) + qt.Assert(t, qt.IsTrue(Flags.YAMLV3Decoder)) } diff --git a/internal/encoding/encoding.go b/internal/encoding/encoding.go index 1f451e7d9..84ac9f52a 100644 --- a/internal/encoding/encoding.go +++ b/internal/encoding/encoding.go @@ -38,9 +38,9 @@ import ( "cuelang.org/go/encoding/protobuf/jsonpb" "cuelang.org/go/encoding/protobuf/textproto" "cuelang.org/go/internal" + "cuelang.org/go/internal/encoding/yaml" "cuelang.org/go/internal/filetypes" "cuelang.org/go/internal/source" - "cuelang.org/go/internal/third_party/yaml" "golang.org/x/text/encoding/unicode" "golang.org/x/text/transform" ) @@ -251,9 +251,9 @@ func NewDecoder(f *build.File, cfg *Config) *Decoder { i.next = json.NewDecoder(nil, path, r).Extract i.Next() case build.YAML: - d, err := yaml.NewDecoder(path, r) + b, err := io.ReadAll(r) i.err = err - i.next = d.Decode + i.next = yaml.NewDecoder(path, b).Decode i.Next() case build.Text: b, err := io.ReadAll(r) diff --git a/internal/encoding/yaml/decode.go b/internal/encoding/yaml/decode.go new file mode 100644 index 000000000..5c6889255 --- /dev/null +++ b/internal/encoding/yaml/decode.go @@ -0,0 +1,675 @@ +package yaml + +import ( + "bytes" + "encoding/base64" + "errors" + "fmt" + "io" + "strconv" + "strings" + + "gopkg.in/yaml.v3" + + "cuelang.org/go/cue/ast" + "cuelang.org/go/cue/literal" + "cuelang.org/go/cue/token" + "cuelang.org/go/internal" + "cuelang.org/go/internal/cueexperiment" + tpyaml "cuelang.org/go/internal/third_party/yaml" +) + +// TODO(mvdan): we should sanity check that the decoder always produces valid CUE, +// as it is possible to construct a cue/ast syntax tree with invalid literals +// or with expressions that will always error, such as `float & 123`. +// +// One option would be to do this as part of the tests; a more general approach +// may be fuzzing, which would find more bugs and work for any decoder, +// although it may be slow as we need to involve the evaluator. + +// Decoder is a temporary interface compatible with both the old and new yaml decoders. +type Decoder interface { + // Decode consumes a YAML value and returns it in CUE syntax tree node. + Decode() (ast.Expr, error) +} + +// NewDecoder is a temporary constructor compatible with both the old and new yaml decoders. +// Note that the signature matches the new yaml decoder, as the old signature can only error +// when reading a source that isn't []byte. +func NewDecoder(filename string, b []byte) Decoder { + if cueexperiment.Flags.YAMLV3Decoder { + return newDecoder(filename, b) + } + dec, err := tpyaml.NewDecoder(filename, b) + if err != nil { + panic(err) // should never happen as we give it []byte + } + return dec +} + +// decoder wraps a [yaml.Decoder] to extract CUE syntax tree nodes. +type decoder struct { + yamlDecoder yaml.Decoder + + // yamlNonEmpty is true once yamlDecoder tells us the input YAML wasn't empty. + // Useful so that we can extract "null" when the input is empty. + yamlNonEmpty bool + + // decodeErr is returned by any further calls to Decode when not nil. + decodeErr error + + tokFile *token.File + tokLines []int + + // pendingHeadComments collects the head (preceding) comments + // from the YAML nodes we are extracting. + // We can't add comments to a CUE syntax tree node until we've created it, + // but we need to extract these comments first since they have earlier positions. + pendingHeadComments []*ast.Comment + + // extractingAliases ensures we don't loop forever when expanding YAML anchors. + extractingAliases map[*yaml.Node]bool + + // lastPos is the last YAML node position that we decoded, + // used for working out relative positions such as token.NewSection. + // This position can only increase, moving forward in the file. + lastPos token.Position + + // forceNewline ensures that the next position will be on a new line. + forceNewline bool +} + +// TODO(mvdan): this can be io.Reader really, except that token.Pos is offset-based, +// so the only way to really have true Offset+Line+Col numbers is to know +// the size of the entire YAML node upfront. +// With json we can use RawMessage to know the size of the input +// before we extract into ast.Expr, but unfortunately, yaml.Node has no size. + +// newDecoder creates a decoder for YAML values to extract CUE syntax tree nodes. +// +// The filename is used for position information in CUE syntax tree nodes +// as well as any errors encountered while decoding YAML. +func newDecoder(filename string, b []byte) *decoder { + // Note that yaml.v3 can insert a null node just past the end of the input + // in some edge cases, so we pretend that there's an extra newline + // so that we don't panic when handling such a position. + tokFile := token.NewFile(filename, 0, len(b)+1) + tokFile.SetLinesForContent(b) + return &decoder{ + tokFile: tokFile, + tokLines: append(tokFile.Lines(), len(b)), + yamlDecoder: *yaml.NewDecoder(bytes.NewReader(b)), + } +} + +// Decode consumes a YAML value and returns it in CUE syntax tree node. +// +// A nil node with an io.EOF error is returned once no more YAML values +// are available for decoding. +func (d *decoder) Decode() (ast.Expr, error) { + if err := d.decodeErr; err != nil { + return nil, err + } + var yn yaml.Node + if err := d.yamlDecoder.Decode(&yn); err != nil { + if err == io.EOF { + // Any further Decode calls must return EOF to avoid an endless loop. + d.decodeErr = io.EOF + + // If the input is empty, we produce a single null literal with EOF. + // Note that when the input contains "---", we get an empty document + // with a null scalar value inside instead. + // + // TODO(mvdan): the old decoder seemingly intended to do this, + // but returned a "null" literal with io.EOF, which consumers ignored. + if false && !d.yamlNonEmpty { + return &ast.BasicLit{ + Kind: token.NULL, + Value: "null", + }, nil + } + // If the input wasn't empty, we already decoded some CUE syntax nodes, + // so here we should just return io.EOF to stop. + return nil, io.EOF + } + // Unfortunately, yaml.v3's syntax errors are opaque strings, + // and they only include line numbers in some but not all cases. + // TODO(mvdan): improve upstream's errors so they are structured + // and always contain some position information. + e := err.Error() + if s, ok := strings.CutPrefix(e, "yaml: line "); ok { + // From "yaml: line 3: some issue" to "foo.yaml:3: some issue". + e = d.tokFile.Name() + ":" + s + } else if s, ok := strings.CutPrefix(e, "yaml:"); ok { + // From "yaml: some issue" to "foo.yaml: some issue". + e = d.tokFile.Name() + ":" + s + } else { + return nil, err + } + err = errors.New(e) + // Any further Decode calls repeat this error. + d.decodeErr = err + return nil, err + } + d.yamlNonEmpty = true + return d.extract(&yn) +} + +// Unmarshal parses a single YAML value to a CUE expression. +func Unmarshal(filename string, data []byte) (ast.Expr, error) { + d := NewDecoder(filename, data) + x, err := d.Decode() + if err != nil { + if err == io.EOF { + return nil, nil // empty input + } + return nil, err + } + // TODO(mvdan): fail if there are more documents or garbage in the input + return x, nil +} + +func (d *decoder) extract(yn *yaml.Node) (ast.Expr, error) { + d.addHeadCommentsToPending(yn) + var expr ast.Expr + var err error + switch yn.Kind { + case yaml.DocumentNode: + expr, err = d.document(yn) + case yaml.SequenceNode: + expr, err = d.sequence(yn) + case yaml.MappingNode: + expr, err = d.mapping(yn) + case yaml.ScalarNode: + expr, err = d.scalar(yn) + case yaml.AliasNode: + expr, err = d.alias(yn) + default: + return nil, d.posErrorf(yn, "unknown yaml node kind: %d", yn.Kind) + } + if err != nil { + return nil, err + } + d.addCommentsToNode(expr, yn, 1) + return expr, nil +} + +// comments parses a newline-delimited list of YAML "#" comments +// and turns them into a list of cue/ast comments. +func (d *decoder) comments(src string) []*ast.Comment { + if src == "" { + return nil + } + var comments []*ast.Comment + for _, line := range strings.Split(src, "\n") { + if line == "" { + continue // yaml.v3 comments have a trailing newline at times + } + comments = append(comments, &ast.Comment{ + // Trim the leading "#". + // Note that yaml.v3 does not give us comment positions. + Text: "//" + line[1:], + }) + } + return comments +} + +// addHeadCommentsToPending parses a node's head comments and adds them to a pending list, +// to be used later by addComments once a cue/ast node is constructed. +func (d *decoder) addHeadCommentsToPending(yn *yaml.Node) { + comments := d.comments(yn.HeadComment) + // TODO(mvdan): once yaml.v3 records comment positions, + // we can better ensure that sections separated by empty lines are kept that way. + // For now, all we can do is approximate by counting lines, + // and assuming that head comments are not separated from their node. + // This will be wrong in some cases, moving empty lines, but is better than nothing. + if len(d.pendingHeadComments) == 0 && len(comments) > 0 { + c := comments[0] + if d.lastPos.IsValid() && (yn.Line-len(comments))-d.lastPos.Line >= 2 { + c.Slash = c.Slash.WithRel(token.NewSection) + } + } + d.pendingHeadComments = append(d.pendingHeadComments, comments...) +} + +// addCommentsToNode adds any pending head comments, plus a YAML node's line +// and foot comments, to a cue/ast node. +func (d *decoder) addCommentsToNode(n ast.Node, yn *yaml.Node, linePos int8) { + // cue/ast and cue/format are not able to attach a comment to a node + // when the comment immediately follows the node. + // For some nodes like fields, the best we can do is move the comments up. + // For the root-level struct, we do want to leave comments + // at the end of the document to be left at the very end. + // + // TODO(mvdan): can we do better? for example, support attaching trailing comments to a cue/ast.Node? + footComments := d.comments(yn.FootComment) + if _, ok := n.(*ast.StructLit); !ok { + d.pendingHeadComments = append(d.pendingHeadComments, footComments...) + footComments = nil + } + if comments := d.pendingHeadComments; len(comments) > 0 { + ast.AddComment(n, &ast.CommentGroup{ + Doc: true, + Position: 0, + List: comments, + }) + } + if comments := d.comments(yn.LineComment); len(comments) > 0 { + ast.AddComment(n, &ast.CommentGroup{ + Line: true, + Position: linePos, + List: comments, + }) + } + if comments := footComments; len(comments) > 0 { + ast.AddComment(n, &ast.CommentGroup{ + // After 100 tokens, so that the comment goes after the entire node. + // TODO(mvdan): this is hacky, can the cue/ast API support trailing comments better? + Position: 100, + List: comments, + }) + } + d.pendingHeadComments = nil +} + +func (d *decoder) posErrorf(yn *yaml.Node, format string, args ...any) error { + // TODO(mvdan): use columns as well; for now they are left out to avoid test churn + // return fmt.Errorf(d.pos(n).String()+" "+format, args...) + return fmt.Errorf(d.tokFile.Name()+":"+strconv.Itoa(yn.Line)+": "+format, args...) +} + +// pos converts a YAML node position to a cue/ast position. +// Note that this method uses and updates the last position in lastPos, +// so it should be called on YAML nodes in increasing position order. +func (d *decoder) pos(yn *yaml.Node) token.Pos { + // Calculate the position's offset via the line and column numbers. + offset := d.tokLines[yn.Line-1] + (yn.Column - 1) + pos := d.tokFile.Pos(offset, token.NoRelPos) + + if d.forceNewline { + d.forceNewline = false + pos = pos.WithRel(token.Newline) + } else if d.lastPos.IsValid() { + switch { + case yn.Line-d.lastPos.Line >= 2: + pos = pos.WithRel(token.NewSection) + case yn.Line-d.lastPos.Line == 1: + pos = pos.WithRel(token.Newline) + case yn.Column-d.lastPos.Column > 0: + pos = pos.WithRel(token.Blank) + default: + pos = pos.WithRel(token.NoSpace) + } + // If for any reason the node's position is before the last position, + // give up and return an empty position. Akin to: yn.Pos().Before(d.lastPos) + // + // TODO(mvdan): Brought over from the old decoder; when does this happen? + // Can we get rid of those edge cases and this bit of logic? + if yn.Line < d.lastPos.Line || (yn.Line == d.lastPos.Line && yn.Column < d.lastPos.Column) { + return token.NoPos + } + } + d.lastPos = token.Position{Line: yn.Line, Column: yn.Column} + return pos +} + +func (d *decoder) document(yn *yaml.Node) (ast.Expr, error) { + if n := len(yn.Content); n != 1 { + return nil, d.posErrorf(yn, "yaml document nodes are meant to have one content node but have %d", n) + } + return d.extract(yn.Content[0]) +} + +func (d *decoder) sequence(yn *yaml.Node) (ast.Expr, error) { + list := &ast.ListLit{ + Lbrack: d.pos(yn).WithRel(token.Blank), + } + multiline := false + if len(yn.Content) > 0 { + multiline = yn.Line < yn.Content[len(yn.Content)-1].Line + } + + // If a list is empty, or ends with a struct, the closing `]` is on the same line. + closeSameLine := true + for _, c := range yn.Content { + d.forceNewline = multiline + elem, err := d.extract(c) + if err != nil { + return nil, err + } + list.Elts = append(list.Elts, elem) + // A list of structs begins with `[{`, so let it end with `}]`. + _, closeSameLine = elem.(*ast.StructLit) + } + if multiline && !closeSameLine { + list.Rbrack = list.Rbrack.WithRel(token.Newline) + } + return list, nil +} + +func (d *decoder) mapping(yn *yaml.Node) (ast.Expr, error) { + strct := &ast.StructLit{} + multiline := false + if len(yn.Content) > 0 { + multiline = yn.Line < yn.Content[len(yn.Content)-1].Line + } + + if err := d.insertMap(yn, strct, multiline, false); err != nil { + return nil, err + } + // TODO(mvdan): moving these positions above insertMap breaks a few tests, why? + strct.Lbrace = d.pos(yn).WithRel(token.Blank) + if multiline { + strct.Rbrace = strct.Lbrace.WithRel(token.Newline) + } else { + strct.Rbrace = strct.Lbrace + } + return strct, nil +} + +func (d *decoder) insertMap(yn *yaml.Node, m *ast.StructLit, multiline, mergeValues bool) error { + l := len(yn.Content) +outer: + for i := 0; i < l; i += 2 { + if multiline { + d.forceNewline = true + } + yk, yv := yn.Content[i], yn.Content[i+1] + d.addHeadCommentsToPending(yk) + if isMerge(yk) { + mergeValues = true + if err := d.merge(yv, m, multiline); err != nil { + return err + } + continue + } + if yk.Kind != yaml.ScalarNode { + return d.posErrorf(yn, "invalid map key: %v", yk.ShortTag()) + } + + field := &ast.Field{} + label, err := d.label(yk) + if err != nil { + return err + } + d.addCommentsToNode(label, yk, 1) + field.Label = label + + if mergeValues { + key := labelStr(label) + for _, decl := range m.Elts { + f := decl.(*ast.Field) + name, _, err := ast.LabelName(f.Label) + if err == nil && name == key { + f.Value, err = d.extract(yv) + if err != nil { + return err + } + continue outer + } + } + } + + value, err := d.extract(yv) + if err != nil { + return err + } + field.Value = value + + m.Elts = append(m.Elts, field) + } + return nil +} + +func (d *decoder) merge(yn *yaml.Node, m *ast.StructLit, multiline bool) error { + switch yn.Kind { + case yaml.MappingNode: + return d.insertMap(yn, m, multiline, true) + case yaml.AliasNode: + return d.insertMap(yn.Alias, m, multiline, true) + case yaml.SequenceNode: + // Step backwards as earlier nodes take precedence. + for i := len(yn.Content) - 1; i >= 0; i-- { + if err := d.merge(yn.Content[i], m, multiline); err != nil { + return err + } + } + return nil + default: + return d.posErrorf(yn, "map merge requires map or sequence of maps as the value") + } +} + +func (d *decoder) label(yn *yaml.Node) (ast.Label, error) { + pos := d.pos(yn) + + expr, err := d.scalar(yn) + if err != nil { + return nil, err + } + switch expr := expr.(type) { + case *ast.BasicLit: + if expr.Kind == token.STRING { + if ast.IsValidIdent(yn.Value) && !internal.IsDefOrHidden(yn.Value) { + return &ast.Ident{ + NamePos: pos, + Name: yn.Value, + }, nil + } + ast.SetPos(expr, pos) + return expr, nil + } + + return &ast.BasicLit{ + ValuePos: pos, + Kind: token.STRING, + Value: literal.Label.Quote(expr.Value), + }, nil + + default: + return nil, d.posErrorf(yn, "invalid label "+yn.Value) + } +} + +const ( + // TODO(mvdan): The strings below are from yaml.v3; should we be relying on upstream somehow? + nullTag = "!!null" + boolTag = "!!bool" + strTag = "!!str" + intTag = "!!int" + floatTag = "!!float" + timestampTag = "!!timestamp" + seqTag = "!!seq" + mapTag = "!!map" + binaryTag = "!!binary" + mergeTag = "!!merge" +) + +func (d *decoder) scalar(yn *yaml.Node) (ast.Expr, error) { + switch tag := yn.ShortTag(); tag { + // TODO: use parse literal or parse expression instead. + case timestampTag: + return &ast.BasicLit{ + ValuePos: d.pos(yn), + Kind: token.STRING, + Value: literal.String.Quote(yn.Value), + }, nil + case strTag: + return &ast.BasicLit{ + ValuePos: d.pos(yn), + Kind: token.STRING, + Value: quoteString(yn.Value), + }, nil + + case binaryTag: + data, err := base64.StdEncoding.DecodeString(yn.Value) + if err != nil { + return nil, d.posErrorf(yn, "!!binary value contains invalid base64 data") + } + return &ast.BasicLit{ + ValuePos: d.pos(yn), + Kind: token.STRING, + Value: literal.Bytes.Quote(string(data)), + }, nil + + case boolTag: + t := false + switch yn.Value { + // TODO(mvdan): The strings below are from yaml.v3; should we be relying on upstream somehow? + case "true", "True", "TRUE": + t = true + } + lit := ast.NewBool(t) + lit.ValuePos = d.pos(yn) + return lit, nil + + case intTag: + // Convert YAML octal to CUE octal. If YAML accepted an invalid + // integer, just convert it as well to ensure CUE will fail. + value := yn.Value + if len(value) > 1 && value[0] == '0' && value[1] <= '9' { + value = "0o" + value[1:] + } + var info literal.NumInfo + // We make the assumption that any valid YAML integer literal will be a valid + // CUE integer literal as well, with the only exception of octal numbers above. + // Note that `!!int 123.456` is not allowed. + if err := literal.ParseNum(value, &info); err != nil || !info.IsInt() { + return nil, d.posErrorf(yn, "cannot decode %q as %s", value, yn.ShortTag()) + } + return d.makeNum(yn, value, token.INT), nil + + case floatTag: + value := yn.Value + // TODO(mvdan): The strings below are from yaml.v3; should we be relying on upstream somehow? + switch value { + case ".inf", ".Inf", ".INF", "+.inf", "+.Inf", "+.INF": + value = "+Inf" + case "-.inf", "-.Inf", "-.INF": + value = "-Inf" + case ".nan", ".NaN", ".NAN": + value = "NaN" + default: + var info literal.NumInfo + // We make the assumption that any valid YAML float literal will be a valid + // CUE float literal as well, with the only exception of Inf/NaN above. + // Note that `!!float 123` is allowed. + if err := literal.ParseNum(value, &info); err != nil { + return nil, d.posErrorf(yn, "cannot decode %q as %s", value, yn.ShortTag()) + } + // If the decoded YAML scalar was explicitly or implicitly a float, + // and the scalar literal looks like an integer, + // unify it with "number" to record the fact that it was represented as a float. + // Don't unify with float, as `float & 123` is invalid, and there's no need + // to forbid representing the number as an integer either. + if yn.Tag != "" { + if p := strings.IndexAny(value, ".eEiInN"); p == -1 { + // TODO: number(v) when we have conversions + // TODO(mvdan): don't shove the unification inside a BasicLit.Value string + // + // TODO(mvdan): would it be better to do turn `!!float 123` into `123.0` + // rather than `number & 123`? Note that `float & 123` is an error. + value = fmt.Sprintf("number & %s", value) + } + } + } + return d.makeNum(yn, value, token.FLOAT), nil + + case nullTag: + return &ast.BasicLit{ + ValuePos: d.pos(yn).WithRel(token.Blank), + Kind: token.NULL, + Value: "null", + }, nil + default: + return nil, d.posErrorf(yn, "cannot unmarshal tag %q", tag) + } +} + +func (d *decoder) makeNum(yn *yaml.Node, val string, kind token.Token) (expr ast.Expr) { + val, negative := strings.CutPrefix(val, "-") + expr = &ast.BasicLit{ + ValuePos: d.pos(yn), + Kind: kind, + Value: val, + } + if negative { + expr = &ast.UnaryExpr{ + OpPos: d.pos(yn), + Op: token.SUB, + X: expr, + } + } + return expr +} + +func (d *decoder) alias(yn *yaml.Node) (ast.Expr, error) { + if d.extractingAliases[yn] { + // TODO this could actually be allowed in some circumstances. + return nil, d.posErrorf(yn, "anchor %q value contains itself", yn.Value) + } + if d.extractingAliases == nil { + d.extractingAliases = make(map[*yaml.Node]bool) + } + d.extractingAliases[yn] = true + var node ast.Expr + node, err := d.extract(yn.Alias) + delete(d.extractingAliases, yn) + return node, err +} + +// quoteString converts a string to a CUE multiline string if needed. +// TODO(mvdan): this is brought over from the old decoder; we should consider +// polishing this API and moving it someplace better like cue/literal. +func quoteString(s string) string { + lines := []string{} + last := 0 + for i, c := range s { + if c == '\n' { + lines = append(lines, s[last:i]) + last = i + 1 + } + if c == '\r' { + goto quoted + } + } + lines = append(lines, s[last:]) + if len(lines) >= 2 { + buf := []byte{} + buf = append(buf, `"""`+"\n"...) + for _, l := range lines { + if l == "" { + // no indentation for empty lines + buf = append(buf, '\n') + continue + } + buf = append(buf, '\t') + p := len(buf) + // TODO(mvdan): do not use Go's strconv for CUE syntax. + buf = strconv.AppendQuote(buf, l) + // remove quotes + buf[p] = '\t' + buf[len(buf)-1] = '\n' + } + buf = append(buf, "\t\t"+`"""`...) + return string(buf) + } +quoted: + return literal.String.Quote(s) +} + +func labelStr(l ast.Label) string { + switch l := l.(type) { + case *ast.Ident: + return l.Name + case *ast.BasicLit: + s, _ := literal.Unquote(l.Value) + return s + } + return "" +} + +func isMerge(yn *yaml.Node) bool { + // TODO(mvdan): The boolean logic below is from yaml.v3; should we be relying on upstream somehow? + return yn.Kind == yaml.ScalarNode && yn.Value == "<<" && (yn.Tag == "" || yn.Tag == "!" || yn.ShortTag() == mergeTag) +} diff --git a/internal/encoding/yaml/decode_test.go b/internal/encoding/yaml/decode_test.go index 6e69f38fe..768f0b851 100644 --- a/internal/encoding/yaml/decode_test.go +++ b/internal/encoding/yaml/decode_test.go @@ -28,10 +28,17 @@ import ( "cuelang.org/go/cue/ast" "cuelang.org/go/cue/format" + "cuelang.org/go/internal" + "cuelang.org/go/internal/cueexperiment" "cuelang.org/go/internal/cuetest" - "cuelang.org/go/internal/third_party/yaml" + "cuelang.org/go/internal/encoding/yaml" + "github.com/google/go-cmp/cmp" ) +// These tests are only for the new YAML decoder. +// The old YAML decoder has its own tests in internal/third_party/yaml. +func init() { cueexperiment.Flags.YAMLV3Decoder = true } + var unmarshalTests = []struct { data string want string @@ -234,7 +241,7 @@ apple: "newline"`, \ttext - """`, + """ // Comment`, }, // Folded block scalar @@ -249,7 +256,7 @@ apple: "newline"`, last line - """`, + """ // Comment`, }, // Structs @@ -375,7 +382,7 @@ Null: 1 }, { "float32_maxuint64+1: 18446744073709551616", - `"float32_maxuint64+1": 18446744073709551616`, + `"float32_maxuint64+1": number & 18446744073709551616`, }, // float64 @@ -391,9 +398,13 @@ Null: 1 "float64_maxuint64: 18446744073709551615", "float64_maxuint64: 18446744073709551615", }, + // TODO(mvdan): yaml.v3 uses strconv APIs like ParseUint to try to detect + // whether a scalar should be considered a YAML integer or a float. + // Integers in CUE aren't limited to 64 bits, so we should arguably not decode + // large integers that don't fit in 64 bits as floats via `number &`. { "float64_maxuint64+1: 18446744073709551616", - `"float64_maxuint64+1": 18446744073709551616`, + `"float64_maxuint64+1": number & 18446744073709551616`, }, // Overflow cases. @@ -429,10 +440,10 @@ Null: 1 "v: 1.1", }, { "v: !!float 0", - "v: float & 0", // Should this be 0.0? + "v: number & 0", }, { "v: !!float -1", - "v: float & -1", // Should this be -1.0? + "v: number & -1", }, { "v: !!null ''", "v: null", @@ -444,8 +455,7 @@ Null: 1 // Non-specific tag (Issue #75) { `v: ! test`, - // TODO this should work and produce a string. - "", + `v: "test"`, }, // Anchors and aliases. @@ -567,12 +577,11 @@ d: [ ] e: [] `, + // TODO(mvdan): keep the separated opening/closing tokens once yaml.v3 exposes end positions. `a: {} b: {} - c: 1 -d: [ -] +d: [] e: []`, }, @@ -675,14 +684,15 @@ a: }, // Floating comments. - // TODO: avoid losing some of these. + // TODO(mvdan): all empty lines separating comments should stay in place. + // TODO(mvdan): avoid losing comments in empty lists and objects. { "# Start\n\na: 123\n\n# Middle\n\nb: 456\n\n# End", - "// Start\n\na: 123\n\n// Middle\n\nb: 456", + "// Start\na: 123\n\n// Middle\nb: 456\n// End", }, { "a: [\n\t# Comment\n]", - "a: [\n\n]", + "a: []", }, { "a: {\n\t# Comment\n}", @@ -692,7 +702,7 @@ a: // Attached comments. { "start: 100\n\n# Before\na: 123 # Inline\n# After\n\nend: 200", - "start: 100\n\n// Before\na: 123 // Inline\n// After\n\nend: 200", + "start: 100\n\n// Before\n// After\na: 123 // Inline\nend: 200", }, { "# One\none: null\n\n# Two\ntwo: [2, 2]\n\n# Three\nthree: {val: 3}", @@ -755,6 +765,8 @@ a: "Reuse anchor": "Bar"`, }, // Single document with garbage following it. + // TODO(mvdan): This should work for a single Decoder.Decode call, + // but it should be an error with Unmarshal. { "---\nhello\n...\n}not yaml", `"hello"`, @@ -764,24 +776,22 @@ a: type M map[interface{}]interface{} func cueStr(node ast.Node) string { - if s, ok := node.(*ast.StructLit); ok { - node = &ast.File{ - Decls: s.Elts, - } + if node == nil { + return "" } - b, _ := format.Node(node) + b, _ := format.Node(internal.ToFile(node)) return strings.TrimSpace(string(b)) } -func newDecoder(t *testing.T, data string) *yaml.Decoder { - dec, err := yaml.NewDecoder("test.yaml", strings.NewReader(data)) - if err != nil { - t.Fatal(err) - } - return dec +func newDecoder(t *testing.T, data string) yaml.Decoder { + t.Helper() + t.Logf("input yaml:\n%s", data) + return yaml.NewDecoder("test.yaml", []byte(data)) } func callUnmarshal(t *testing.T, data string) (ast.Expr, error) { + t.Helper() + t.Logf(" yaml:\n%s", data) return yaml.Unmarshal("test.yaml", []byte(data)) } @@ -790,11 +800,11 @@ func TestUnmarshal(t *testing.T) { t.Run(strconv.Itoa(i), func(t *testing.T) { t.Logf("test %d: %q", i, item.data) expr, err := callUnmarshal(t, item.data) - if _, ok := err.(*yaml.TypeError); !ok && err != nil { - t.Fatal("expected error to be nil") + if err != nil { + t.Fatalf("expected error to be nil: %v", err) } if got := cueStr(expr); got != item.want { - t.Errorf("\n got:\n%v\nwant:\n%v", got, item.want) + t.Errorf("\n got:\n%v\n want:\n%v", got, item.want) } }) } @@ -810,7 +820,7 @@ func TestX(t *testing.T) { } expr, err := callUnmarshal(t, y) - if _, ok := err.(*yaml.TypeError); !ok && err != nil { + if err != nil { t.Fatal(err) } t.Error(cueStr(expr)) @@ -826,11 +836,11 @@ func TestDecoderSingleDocument(t *testing.T) { return } expr, err := newDecoder(t, item.data).Decode() - if _, ok := err.(*yaml.TypeError); !ok && err != nil { + if err != nil { t.Errorf("err should be nil, was %v", err) } if got := cueStr(expr); got != item.want { - t.Errorf("\n got: %v;\nwant: %v", got, item.want) + t.Errorf("\n got:\n%v\n want:\n%v", got, item.want) } }) } @@ -876,7 +886,7 @@ func TestDecoder(t *testing.T) { } got := strings.Join(values, "\n") if got != item.want { - t.Errorf("\n got: %v;\nwant: %v", got, item.want) + t.Errorf("\n got:\n%v\n want:\n%v", got, item.want) } }) } @@ -897,20 +907,20 @@ func TestUnmarshalNaN(t *testing.T) { var unmarshalErrorTests = []struct { data, error string }{ - {"\nv: !!float 'error'", "test.yaml:2: cannot decode !!str `error` as a !!float"}, - {"\nv: !!int 'error'", "test.yaml:2: cannot decode !!str `error` as a !!int"}, - {"\nv: !!int 123.456", "test.yaml:2: cannot decode !!float `123.456` as a !!int"}, + {"\nv: !!float 'error'", `test.yaml:2: cannot decode "error" as !!float`}, + {"\nv: !!int 'error'", `test.yaml:2: cannot decode "error" as !!int`}, + {"\nv: !!int 123.456", `test.yaml:2: cannot decode "123.456" as !!int`}, {"v: [A,", "test.yaml:1: did not find expected node content"}, {"v:\n- [A,", "test.yaml:2: did not find expected node content"}, {"a:\n- b: *,", "test.yaml:2: did not find expected alphabetic or numeric character"}, - {"a: *b\n", "test.yaml:1: unknown anchor 'b' referenced"}, - {"a: &a\n b: *a\n", "test.yaml:2: anchor 'a' value contains itself"}, - {"value: -", "test.yaml:1: block sequence entries are not allowed in this context"}, + {"a: *b\n", "test.yaml: unknown anchor 'b' referenced"}, + {"a: &a\n b: *a\n", `test.yaml:2: anchor "a" value contains itself`}, + {"value: -", "test.yaml: block sequence entries are not allowed in this context"}, {"a: !!binary ==", "test.yaml:1: !!binary value contains invalid base64 data"}, - {"{[.]}", `test.yaml:1: invalid map key: sequence`}, - {"{{.}}", `test.yaml:1: invalid map key: map`}, - {"b: *a\na: &a {c: 1}", `test.yaml:1: unknown anchor 'a' referenced`}, - {"%TAG !%79! tag:yaml.org,2002:\n---\nv: !%79!int '1'", "test.yaml:1: did not find expected whitespace"}, + {"{[.]}", `test.yaml:1: invalid map key: !!seq`}, + {"{{.}}", `test.yaml:1: invalid map key: !!map`}, + {"b: *a\na: &a {c: 1}", `test.yaml: unknown anchor 'a' referenced`}, + {"%TAG !%79! tag:yaml.org,2002:\n---\nv: !%79!int '1'", "test.yaml: did not find expected whitespace"}, } func TestUnmarshalErrors(t *testing.T) { @@ -963,7 +973,7 @@ func TestFiles(t *testing.T) { t.Fatal(err) } if want := string(b); got != want { - t.Errorf("\n got: %v;\nwant: %v", got, want) + t.Error(cmp.Diff(want, got)) } }) } diff --git a/internal/encoding/yaml/testdata/merge.out b/internal/encoding/yaml/testdata/merge.out index ee6e6bbe0..bc55dde7a 100644 --- a/internal/encoding/yaml/testdata/merge.out +++ b/internal/encoding/yaml/testdata/merge.out @@ -13,7 +13,6 @@ anchors: { } // All the following maps are equal: - plain: { // Explicit keys x: 1 @@ -21,56 +20,48 @@ plain: { r: 10 label: "center/big" } - mergeOne: { - x: 1 - y: 2 // Merge one map - + x: 1 + y: 2 r: 10 label: "center/big" } - mergeMultiple: { - r: 10 - x: 1 - y: 2 // Merge multiple maps - + r: 10 + x: 1 + y: 2 label: "center/big" } - override: { + // Override r: 10 x: 1 y: 2 label: "center/big" } - shortTag: { - r: 10 - x: 1 - y: 2 // Explicit short merge tag - + r: 10 + x: 1 + y: 2 label: "center/big" } - longTag: { - r: 10 - x: 1 - y: 2 // Explicit merge long tag - + r: 10 + x: 1 + y: 2 label: "center/big" } - inlineMap: { // Inlined map - x: 1, y: 2, r: 10 + x: 1 + y: 2 + r: 10 label: "center/big" } - inlineSequenceMap: { // Inlined map in sequence r: 10 diff --git a/pkg/encoding/yaml/manual.go b/pkg/encoding/yaml/manual.go index 5912c48dc..c65c3621e 100644 --- a/pkg/encoding/yaml/manual.go +++ b/pkg/encoding/yaml/manual.go @@ -23,7 +23,6 @@ import ( "cuelang.org/go/cue/errors" cueyaml "cuelang.org/go/internal/encoding/yaml" "cuelang.org/go/internal/pkg" - "cuelang.org/go/internal/third_party/yaml" ) // Marshal returns the YAML encoding of v. @@ -64,16 +63,12 @@ func MarshalStream(v cue.Value) (string, error) { // Unmarshal parses the YAML to a CUE expression. func Unmarshal(data []byte) (ast.Expr, error) { - return yaml.Unmarshal("", data) + return cueyaml.Unmarshal("", data) } // UnmarshalStream parses the YAML to a CUE list expression on success. func UnmarshalStream(data []byte) (ast.Expr, error) { - d, err := yaml.NewDecoder("", data) - if err != nil { - return nil, err - } - + d := cueyaml.NewDecoder("", data) a := []ast.Expr{} for { x, err := d.Decode() @@ -92,10 +87,7 @@ func UnmarshalStream(data []byte) (ast.Expr, error) { // Validate validates YAML and confirms it is an instance of the schema // specified by v. If the YAML source is a stream, every object must match v. func Validate(b []byte, v cue.Value) (bool, error) { - d, err := yaml.NewDecoder("yaml.Validate", b) - if err != nil { - return false, err - } + d := cueyaml.NewDecoder("yaml.Validate", b) r := v.Context() for { expr, err := d.Decode() @@ -141,10 +133,7 @@ func Validate(b []byte, v cue.Value) (bool, error) { // but does not have to be an instance of v. If the YAML source is a stream, // every object must match v. func ValidatePartial(b []byte, v cue.Value) (bool, error) { - d, err := yaml.NewDecoder("yaml.ValidatePartial", b) - if err != nil { - return false, err - } + d := cueyaml.NewDecoder("yaml.ValidatePartial", b) r := v.Context() for { expr, err := d.Decode() -- 2.51.2