From 069f279aa682f3b0c16f65c930462044d599f4ac Mon Sep 17 00:00:00 2001 From: Roger Peppe Date: Fri, 10 Apr 2026 17:14:46 +0100 Subject: [PATCH] internal: replace ParseAttrBody with ParseAttr Currently ParseAttrBody is the only way of creating an internal.Attr, but it doesn't populate the Name field because it doesn't have access to it. It turns out that everywhere that is currently using ParseAttrBody can also use slightly higher level function that takes an ast.Attribute instead. This can usefully populate the Name and Pos fields from the AST node, which removes the need for second-guessing of the position logic (as currently done in the cue package). There are some places which don't have an ast.Attribute already, but they're only in tests and it's easy to construct one for the purpose. We make ParseAttr return a pointer because it's conventional in Go that the constructor for a type with all pointer methods returns a pointer. There's a small potential performance overhead to this change but this should be negligible compared to what's going on inside the ParseAttr logic itself. The only wrinkle is that most of the places that currently call ParseAttrBody invoke Attribute.Split first to find the attribute name. We add an ast.Attribute.Name method to make this inspection trivial. This also implies a small amount of additional runtime overhead, but it should be lost in the noise: scanning a few bytes is cheap and even cheaper when we've done exactly the same scan a few instructions previously. I think that the additional simplicity makes this worthwhile. Signed-off-by: Roger Peppe Change-Id: I23eb0e3f997ef520a9a8e3d5abe8b9afde3121dc Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1235347 TryBot-Result: CUEcueckoo Unity-Result: CUE porcuepine Reviewed-by: Matthew Sackman --- cue/ast/ast.go | 7 ++++- cue/attribute.go | 9 +----- cue/load/tags.go | 15 +++++----- internal/attrs.go | 38 ++++++++++++++++--------- internal/attrs_test.go | 25 +++++++---------- internal/core/runtime/extern.go | 48 +++++++++++++++----------------- internal/cuetxtar/inline.go | 29 ++++++++++--------- internal/cuetxtar/inline_err.go | 7 +++-- internal/encoding/yaml/encode.go | 5 ++-- 9 files changed, 92 insertions(+), 91 deletions(-) diff --git a/cue/ast/ast.go b/cue/ast/ast.go index 25745e4f6..d24b83550 100644 --- a/cue/ast/ast.go +++ b/cue/ast/ast.go @@ -303,7 +303,12 @@ func (a *Attribute) Pos() token.Pos { return a.At } func (a *Attribute) pos() *token.Pos { return &a.At } func (a *Attribute) End() token.Pos { return a.At.Add(len(a.Text)) } -func (a *Attribute) Split() (key, body string) { +func (a *Attribute) Name() string { + name, _ := a.Split() + return name +} + +func (a *Attribute) Split() (name, body string) { s := a.Text p := strings.IndexByte(s, '(') if p < 0 || !strings.HasPrefix(s, "@") || !strings.HasSuffix(s, ")") { diff --git a/cue/attribute.go b/cue/attribute.go index d570281df..97c718281 100644 --- a/cue/attribute.go +++ b/cue/attribute.go @@ -44,14 +44,7 @@ func (v Value) Attribute(key string) Attribute { } func newAttr(k internal.AttrKind, a *ast.Attribute) Attribute { - key, body := a.Split() - // Note: the body is always positioned just after - // the opening ( after the key. - x := internal.ParseAttrBody(a.Pos().Add(len(key)+1), body) - x.Name = key - x.Kind = k - x.Pos = a.Pos() - return Attribute{x} + return Attribute{*internal.ParseAttr(a)} } func nonExistAttr(key string) Attribute { diff --git a/cue/load/tags.go b/cue/load/tags.go index e44670b93..c335814a8 100644 --- a/cue/load/tags.go +++ b/cue/load/tags.go @@ -169,15 +169,15 @@ type tag struct { field *ast.Field } -func parseTag(pos token.Pos, body string) (t *tag, err errors.Error) { +func parseTag(astAttr *ast.Attribute) (t *tag, err errors.Error) { t = &tag{} t.kind = cue.StringKind - a := internal.ParseAttrBody(pos, body) + a := internal.ParseAttr(astAttr) t.key, _ = a.String(0) if !ast.IsValidIdent(t.key) { - return t, errors.Newf(pos, "invalid identifier %q", t.key) + return t, errors.Newf(a.Pos, "invalid identifier %q", t.key) } if s, ok, _ := a.Lookup(1, "type"); ok { @@ -190,14 +190,14 @@ func parseTag(pos token.Pos, body string) (t *tag, err errors.Error) { case "bool": t.kind = cue.BoolKind default: - return t, errors.Newf(pos, "invalid type %q", s) + return t, errors.Newf(a.Pos, "invalid type %q", s) } } if s, ok, _ := a.Lookup(1, "short"); ok { for s := range strings.SplitSeq(s, "|") { if !ast.IsValidIdent(t.key) { - return t, errors.Newf(pos, "invalid identifier %q", s) + return t, errors.Newf(a.Pos, "invalid identifier %q", s) } t.shorthands = append(t.shorthands, s) } @@ -265,11 +265,10 @@ func findTags(b *build.Instance) (tags []*tag, errs errors.Error) { } for _, a := range x.Attrs { - key, body := a.Split() - if key != "tag" { + if a.Name() != "tag" { continue } - t, err := parseTag(a.Pos(), body) + t, err := parseTag(a) if err != nil { errs = errors.Append(errs, err) continue diff --git a/internal/attrs.go b/internal/attrs.go index 836099715..e8a24fda9 100644 --- a/internal/attrs.go +++ b/internal/attrs.go @@ -18,6 +18,7 @@ import ( "fmt" "strings" + "cuelang.org/go/cue/ast" "cuelang.org/go/cue/errors" "cuelang.org/go/cue/literal" "cuelang.org/go/cue/scanner" @@ -155,18 +156,29 @@ func (a *Attr) Lookup(pos int, key string) (val string, found bool, err error) { return "", false, nil } -func ParseAttrBody(pos token.Pos, s string) (a Attr) { +// ParseAttr parses the given attribute. It always returns a non-nil +// [Attr], which will have a non-nil Err field if there's an error. +func ParseAttr(astAttr *ast.Attribute) *Attr { + key, body := astAttr.Split() + + // Note: the body is always positioned just after the opening ( + // after the key + pos := astAttr.Pos().Add(len(key) + 1) + // Create temporary token.File so that scanner has something // to work with. // TODO it's probably possible to do this without allocations. - tmpFile := token.NewFile("", -1, len(s)) - if len(s) > 0 { - tmpFile.AddLine(len(s) - 1) + tmpFile := token.NewFile("", -1, len(body)) + if len(body) > 0 { + tmpFile.AddLine(len(body) - 1) + } + a := &Attr{ + Pos: astAttr.Pos(), + Name: key, + Body: body, } - a.Body = s - a.Pos = pos var scan scanner.Scanner - scan.Init(tmpFile, []byte(s), nil, scanner.DontInsertCommas) + scan.Init(tmpFile, []byte(body), nil, scanner.DontInsertCommas) for { start := scan.Offset() tok, err := scanAttributeTokens(&scan, pos, 1< 0 && !hasPkg: for _, a := range fileAttrs { - err = errors.Append(err, errors.Newf(a.Pos(), + err = errors.Append(err, errors.Newf(a, "extern attribute without package clause")) } return nil, nil, err @@ -252,17 +250,16 @@ func ExtractAttrsByKind(file *ast.File, kind string) (attrsByNode map[ast.Node][ nodeStack = append(nodeStack, n.Value) case *ast.Attribute: - k, body := n.Split() - if k != kind { + if n.Name() != kind { break } - attrParsed := internal.ParseAttrBody(n.Pos(), body) + attrParsed := internal.ParseAttr(n) parent := nodeStack[len(nodeStack)-1] if attrsByNode == nil { attrsByNode = make(map[ast.Node][]*internal.Attr) } - attrsByNode[parent] = append(attrsByNode[parent], &attrParsed) + attrsByNode[parent] = append(attrsByNode[parent], attrParsed) return false } @@ -348,26 +345,25 @@ func (d *externDecorator) processNode(n adt.Node, scope *adt.Vertex) { } } -func (d *externDecorator) externValue(attr *ast.Attribute, name string, kinds map[string]bool, scope *adt.Vertex) adt.Expr { - kind, body := attr.Split() - if !kinds[kind] { +func (d *externDecorator) externValue(astAttr *ast.Attribute, name string, kinds map[string]bool, scope *adt.Vertex) adt.Expr { + if !kinds[astAttr.Name()] { return nil } - parsed := internal.ParseAttrBody(attr.Pos(), body) - if parsed.Err != nil { - d.errs = errors.Append(d.errs, parsed.Err) + attr := internal.ParseAttr(astAttr) + if attr.Err != nil { + d.errs = errors.Append(d.errs, attr.Err) return nil } - c := d.compilers[kind] + c := d.compilers[attr.Name] if c == nil { return nil } - if a, ok, _ := parsed.Lookup(1, "name"); ok { + if a, ok, _ := attr.Lookup(1, "name"); ok { name = a } - b, err := c.Compile(name, scope, &parsed) + b, err := c.Compile(name, scope, attr) if err != nil { - d.errs = errors.Append(d.errs, errors.Wrap(errors.Newf(attr.Pos(), "@%s", kind), err)) + d.errs = errors.Append(d.errs, errors.Wrap(errors.Newf(attr.Pos, "@%s", attr.Name), err)) return nil } return b diff --git a/internal/cuetxtar/inline.go b/internal/cuetxtar/inline.go index 56b509f04..793a35b9b 100644 --- a/internal/cuetxtar/inline.go +++ b/internal/cuetxtar/inline.go @@ -151,7 +151,7 @@ type parsedTestAttr struct { version string // raw is the parsed internal.Attr for accessing remaining arguments. - raw internal.Attr + raw *internal.Attr // For "err" directives, parsed sub-options are stored here. errArgs *errArgs @@ -197,23 +197,22 @@ type parsedTestAttr struct { // It returns a parsedTestAttr for each logical directive in the attribute. // A single @test(...) contains exactly one directive (the first positional // argument or the key of the first key=value pair). -func parseTestAttr(a *ast.Attribute) (parsedTestAttr, error) { - key, body := a.Split() - if key != "test" { - return parsedTestAttr{}, fmt.Errorf("not a @test attribute: @%s", key) +func parseTestAttr(astAttr *ast.Attribute) (parsedTestAttr, error) { + if name := astAttr.Name(); name != "test" { + return parsedTestAttr{}, fmt.Errorf("not a @test attribute: @%s", name) } - parsed := internal.ParseAttrBody(a.Pos(), body) - if parsed.Err != nil { - return parsedTestAttr{}, parsed.Err + attr := internal.ParseAttr(astAttr) + if attr.Err != nil { + return parsedTestAttr{}, attr.Err } result := parsedTestAttr{ - raw: parsed, - srcAttr: a, + raw: attr, + srcAttr: astAttr, } - if len(parsed.Fields) == 0 || (len(parsed.Fields) == 1 && parsed.Fields[0] == internal.KeyValue{}) { + if len(attr.Fields) == 0 || (len(attr.Fields) == 1 && attr.Fields[0] == internal.KeyValue{}) { // @test() — empty placeholder or bare marker. result.directive = "" return result, nil @@ -222,7 +221,7 @@ func parseTestAttr(a *ast.Attribute) (parsedTestAttr, error) { // The first field determines the directive. // Case 1: key=value form like desc="hello", shareID=name — directive is the key. // Case 2: positional form like eq, err, kind — directive (with optional :vN suffix) is the value. - f0 := parsed.Fields[0] + f0 := attr.Fields[0] if f0.Key() != "" { dir := f0.Key() // Key-based directives may carry a version suffix: "shareID:v3" → directive="shareID", version="v3". @@ -246,7 +245,7 @@ func parseTestAttr(a *ast.Attribute) (parsedTestAttr, error) { // Parse directive-specific sub-options. switch result.directive { case "err": - ea, err := parseErrArgs(parsed) + ea, err := parseErrArgs(attr) if err != nil { return result, err } @@ -257,7 +256,7 @@ func parseTestAttr(a *ast.Attribute) (parsedTestAttr, error) { // Positional args (kv.Key() == "") are accepted by directives as needed. // Directives with their own flag parsers (err, todo, skip, shareID) are // responsible for validating their own flags. - for _, kv := range parsed.Fields[1:] { + for _, kv := range attr.Fields[1:] { switch kv.Key() { case "hint": result.hint = kv.Value() @@ -1449,7 +1448,7 @@ func normalizeLines(s string) string { // attrHasSkip reports whether the raw attribute body contains a skip: arg // at position 2 or later. Returns the version string (e.g. "v3") and true // when a skip arg is found; returns "", false otherwise. -func attrHasSkip(raw internal.Attr) (ver string, ok bool) { +func attrHasSkip(raw *internal.Attr) (ver string, ok bool) { for i := 2; i < len(raw.Fields); i++ { text := raw.Fields[i].Text() if text == "skip" { diff --git a/internal/cuetxtar/inline_err.go b/internal/cuetxtar/inline_err.go index ff0144d3d..a54805739 100644 --- a/internal/cuetxtar/inline_err.go +++ b/internal/cuetxtar/inline_err.go @@ -30,6 +30,7 @@ import ( "golang.org/x/tools/txtar" "cuelang.org/go/cue" + "cuelang.org/go/cue/ast" cueerrors "cuelang.org/go/cue/errors" "cuelang.org/go/cue/token" "cuelang.org/go/internal" @@ -111,7 +112,7 @@ type posWrite struct { // parseErrArgs extracts err sub-options from an already-parsed Attr. // The attribute body is expected to start with "err" as the first positional arg. -func parseErrArgs(a internal.Attr) (errArgs, error) { +func parseErrArgs(a *internal.Attr) (errArgs, error) { var ea errArgs // Start from index 1 (index 0 is "err"). for _, kv := range a.Fields[1:] { @@ -142,7 +143,9 @@ func parseErrArgs(a internal.Attr) (errArgs, error) { return ea, fmt.Errorf("@test(err, suberr=...): %w", err) } // Reuse parseErrArgs by building a synthetic "err, " attr body. - syntheticAttr := internal.ParseAttrBody(token.NoPos, "err, "+inner) + syntheticAttr := internal.ParseAttr(&ast.Attribute{ + Text: fmt.Sprintf("@test(err, %s)", inner), + }) subEA, err := parseErrArgs(syntheticAttr) if err != nil { return ea, fmt.Errorf("@test(err, suberr=...): %w", err) diff --git a/internal/encoding/yaml/encode.go b/internal/encoding/yaml/encode.go index 4e76ddb66..516b446f6 100644 --- a/internal/encoding/yaml/encode.go +++ b/internal/encoding/yaml/encode.go @@ -239,11 +239,10 @@ func encodeExprs(exprs []ast.Expr) (n *yaml.Node, err error) { // Returns an error if the attribute is malformed. func extractYAMLTag(attrs []*ast.Attribute) (string, error) { for _, attr := range attrs { - key, body := attr.Split() - if key != "yaml" { + if attr.Name() != "yaml" { continue } - parsed := internal.ParseAttrBody(attr.Pos(), body) + parsed := internal.ParseAttr(attr) if parsed.Err != nil { return "", parsed.Err } -- 2.51.2