cue: use _# instead of #_ for hidden definition

This slighly complicates the spec (though minimal).

The main benefit, though, is to allow for more straighforward
roundtrippability between languages. For instance, a language
may define an identifier starting with `_` that needs to be
mapped to a definition. Now, to avoid the definition from becoming
hidden, this `_` needs to be removed and bookkeeping in the form
of attributes need to be added to preserve the name in a round trip.

With the rewording, the `#` marks the start of the identifier and
an optionally preceding `_` indicates hidden independent of this.
This allows the Go mapping to preserve naming without any
rewrites, for instance.

The spec now also more clearly distinguishes between
hidden and definition fields.

Change-Id: Ic7bdcc3527d0639b0943246ef0af629c6a477493
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/5980
Reviewed-by: Marcel van Lohuizen <mpvl@google.com>

cue/ast.go

@@ -195,21 +195,6 @@ func (v *astVisitor) ident(n *ast.Ident) string {
 	return str
 }
 
-func isDef(f *ast.Field) bool {
-	if f.Token == token.ISA {
-		return true
-	}
-	switch x := f.Label.(type) {
-	case *ast.Alias:
-		if ident, ok := x.Expr.(*ast.Ident); ok {
-			return strings.HasPrefix(ident.Name, "#")
-		}
-	case *ast.Ident:
-		return strings.HasPrefix(x.Name, "#")
-	}
-	return false
-}
-
 // We probably don't need to call Walk.s
 func (v *astVisitor) walk(astNode ast.Node) (ret value) {
 	switch n := astNode.(type) {
@@ -396,7 +381,7 @@ func (v *astVisitor) walk(astNode ast.Node) (ret value) {
 
 	case *ast.Field:
 		opt := n.Optional != token.NoPos
-		isDef := isDef(n)
+		isDef := internal.IsDefinition(n.Label) || n.Token == token.ISA
 		if isDef {
 			ctx := v.ctx()
 			ctx.inDefinition++

cue/ast/ident.go

@@ -21,6 +21,7 @@ import (
 
 	"cuelang.org/go/cue/errors"
 	"cuelang.org/go/cue/token"
+	"cuelang.org/go/pkg/strings"
 )
 
 func isLetter(ch rune) bool {
@@ -38,12 +39,26 @@ func IsValidIdent(ident string) bool {
 		return false
 	}
 
-	r, sz := utf8.DecodeRuneInString(ident)
-	switch {
-	case isDigit(r):
-		return false
-	case r == '#':
-		ident = ident[sz:]
+	consumed := false
+	if strings.HasPrefix(ident, "_") {
+		ident = ident[1:]
+		consumed = true
+		if len(ident) == 0 {
+			return true
+		}
+	}
+	if strings.HasPrefix(ident, "#") {
+		ident = ident[1:]
+		consumed = true
+		if len(ident) == 0 {
+			return false
+		}
+	}
+
+	if !consumed {
+		if r, _ := utf8.DecodeRuneInString(ident); isDigit(r) {
+			return false
+		}
 	}
 
 	for _, r := range ident {
@@ -109,16 +124,27 @@ func parseIdent(pos token.Pos, ident string) (string, error) {
 		quoted = true
 	}
 
-	r, sz := utf8.DecodeRuneInString(ident)
-	switch {
-	case isDigit(r):
-		return "", errors.Newf(pos, "invalid character '%s' in identifier", string(r))
-	case r == '#':
-	default:
-		sz = 0
+	p := 0
+	if strings.HasPrefix(ident, "_") {
+		p++
+		if len(ident) == 1 {
+			return ident, nil
+		}
+	}
+	if strings.HasPrefix(ident[p:], "#") {
+		p++
+		if len(ident) == p {
+			return "", errors.Newf(pos, "invalid identifier '_#'")
+		}
+	}
+
+	if p == 0 {
+		if r, _ := utf8.DecodeRuneInString(ident); isDigit(r) {
+			return "", errors.Newf(pos, "invalid character '%s' in identifier", string(r))
+		}
 	}
 
-	for _, r := range ident[sz:] {
+	for _, r := range ident[p:] {
 		if isLetter(r) || isDigit(r) || r == '_' || r == '$' {
 			continue
 		}

cue/ast/ident_test.go

@@ -15,6 +15,7 @@
 package ast_test
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -48,6 +49,25 @@ func TestLabelName(t *testing.T) {
 		in:      &ast.Ident{Name: "`foo`"},
 		out:     "foo",
 		isIdent: true,
+	}, {
+		in:      &ast.Ident{Name: ""},
+		out:     "",
+		isIdent: false,
+		err:     true,
+	}, {
+		in:      &ast.Ident{Name: "#"},
+		out:     "",
+		isIdent: false,
+		err:     true,
+	}, {
+		in:      &ast.Ident{Name: "_#"},
+		out:     "",
+		isIdent: false,
+		err:     true,
+	}, {
+		in:      &ast.Ident{Name: "_"},
+		out:     "_",
+		isIdent: true,
 	}, {
 		in:      &ast.Ident{Name: "8ball"},
 		out:     "",
@@ -57,10 +77,22 @@ func TestLabelName(t *testing.T) {
 		in:      &ast.Ident{Name: "_hidden"},
 		out:     "_hidden",
 		isIdent: true,
+	}, {
+		in:      &ast.Ident{Name: "#A"},
+		out:     "#A",
+		isIdent: true,
 	}, {
 		in:      &ast.Ident{Name: "#Def"},
 		out:     "#Def",
 		isIdent: true,
+	}, {
+		in:      &ast.Ident{Name: "_#Def"},
+		out:     "_#Def",
+		isIdent: true,
+	}, {
+		in:      &ast.Ident{Name: "#_Def"},
+		out:     "#_Def",
+		isIdent: true,
 	}, {
 		in:      &ast.Ident{Name: "`foo-bar`"},
 		out:     "foo-bar",
@@ -94,6 +126,10 @@ func TestLabelName(t *testing.T) {
 	for _, tc := range testCases {
 		b, _ := format.Node(tc.in)
 		t.Run(string(b), func(t *testing.T) {
+			if id, ok := tc.in.(*ast.Ident); ok && !strings.HasPrefix(id.Name, "`") {
+				assert.Equal(t, tc.isIdent, ast.IsValidIdent(id.Name))
+			}
+
 			str, isIdent, err := ast.LabelName(tc.in)
 			assert.Equal(t, tc.out, str, "value")
 			assert.Equal(t, tc.isIdent, isIdent, "isIdent")

cue/build.go

@@ -17,7 +17,6 @@ package cue
 import (
 	"path"
 	"strconv"
-	"strings"
 	"sync"
 
 	"cuelang.org/go/cue/ast"
@@ -298,10 +297,10 @@ func (idx *index) label(s string, isIdent bool) label {
 	}
 	f <<= labelShift
 	if isIdent {
-		if strings.HasPrefix(s, "#") {
+		if internal.IsDef(s) {
 			f |= definition
 		}
-		if strings.HasPrefix(s, "_") || strings.HasPrefix(s, "#_") {
+		if internal.IsHidden(s) {
 			f |= hidden
 		}
 	}

cue/format/simplify.go

@@ -16,10 +16,10 @@ package format
 
 import (
 	"strconv"
-	"strings"
 
 	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/cue/ast/astutil"
+	"cuelang.org/go/internal"
 )
 
 // labelSimplifier rewrites string labels to identifiers if
@@ -87,10 +87,7 @@ func (s *labelSimplifier) markStrings(n ast.Node) bool {
 	switch x := n.(type) {
 	case *ast.BasicLit:
 		str, err := strconv.Unquote(x.Value)
-		if err != nil ||
-			!ast.IsValidIdent(str) ||
-			strings.HasPrefix(str, "_") ||
-			strings.HasPrefix(str, "#") {
+		if err != nil || !ast.IsValidIdent(str) || internal.IsDefOrHidden(str) {
 			return false
 		}
 		s.scope[str] = true
@@ -108,10 +105,7 @@ func (s *labelSimplifier) replace(c astutil.Cursor) bool {
 	switch x := c.Node().(type) {
 	case *ast.BasicLit:
 		str, err := strconv.Unquote(x.Value)
-		if err == nil &&
-			s.scope[str] &&
-			!strings.HasPrefix(str, "_") &&
-			!strings.HasPrefix(str, "#") {
+		if err == nil && s.scope[str] && !internal.IsDefOrHidden(str) {
 			c.Replace(ast.NewIdent(str))
 		}
 	}

cue/scanner/scanner.go

@@ -268,6 +268,9 @@ func isDigit(ch rune) bool {
 
 func (s *Scanner) scanFieldIdentifier() string {
 	offs := s.offset
+	if s.ch == '_' {
+		s.next()
+	}
 	if s.ch == '#' {
 		s.next()
 	}
@@ -810,7 +813,7 @@ scanAgain:
 				lit = "_|_"
 			} else {
 				tok = token.IDENT
-				lit = "_" + s.scanIdentifier()
+				lit = "_" + s.scanFieldIdentifier()
 			}
 			insertEOL = true
 		case '`':

cue/scanner/scanner_test.go

@@ -77,7 +77,12 @@ var testTokens = [...]elt{
 	{token.IDENT, "foobar", literal},
 	{token.IDENT, "$foobar", literal},
 	{token.IDENT, "#foobar", literal},
+	{token.IDENT, "#0", literal},
 	{token.IDENT, "_foobar", literal},
+	{token.IDENT, "__foobar", literal},
+	{token.IDENT, "#_foobar", literal},
+	{token.IDENT, "_#foobar", literal},
+	{token.IDENT, "__#foobar", literal},
 	{token.IDENT, "`foobar`", literal},
 	{token.IDENT, "a۰۱۸", literal},
 	{token.IDENT, "foo६४", literal},

cue/types_test.go

@@ -859,9 +859,9 @@ func TestValue_LookupDef(t *testing.T) {
 		def: "_foo",
 		out: `_|_(defintion "_foo" not found)`,
 	}, {
-		in:  `#_foo: 3`,
-		def: "#_foo",
-		out: `_|_(defintion "#_foo" not found)`,
+		in:  `_#foo: 3`,
+		def: "_#foo",
+		out: `_|_(defintion "_#foo" not found)`,
 	}}
 
 	for _, tc := range testCases {

doc/ref/spec.md

@@ -178,10 +178,11 @@ these rules.
 
 Identifiers name entities such as fields and aliases.
 An identifier is a sequence of one or more letters (which includes `_` and `$`)
-and digits, optionally preceded by a `#`.
-It may not be `_`, `$`, or `#`.
+and digits, optionally preceded by `#` or `_#`.
+It may not be `_`, `$`, `#`, or `_#`.
 The first character in an identifier must be a letter or `#`.
-Identifiers starting with a `#` or `_` are reserved for definitions.
+Identifiers starting with a `#` or `_` are reserved for definitions and hidden
+fields.
 
 <!--
 TODO: allow identifiers as defined in Unicode UAX #31
@@ -191,7 +192,7 @@ Identifiers are normalized using the NFC normal form.
 -->
 
 ```
-identifier  = [ "#" ] letter { letter | unicode_digit } .
+identifier  = [ "#" | "_#" ] letter { letter | unicode_digit } .
 ```
 
 ```
@@ -1292,12 +1293,13 @@ S3: {
 ```
 
 
-#### Definitions
+#### Definitions and hidden fields
 
-A field is a _definition_ if its identifier starts with `#` or `_`.
-Definitions are not emitted as part of the model and are never required
-to be concrete when emitting data.
-For definitions with identifiers starting with `#`,
+A field is a _definition_ if its identifier starts with `#` or `_#`.
+A field is _hidden_ if its starts with a `_`.
+Definitions and hidden fields are not emitted when converting a CUE program
+to data and are never required to be concrete.
+For definitions
 literal structs that are part of a definition's value are implicitly closed,
 but may unify unrestricted with other structs within the field's declaration.
 This excludes literals structs in embeddings and aliases.
@@ -1699,10 +1701,9 @@ float64   >=-1.797693134862315708145274237317043567981e+308 &
 
 An identifier of a package may be exported to permit access to it
 from another package.
-All identifiers of regular fields (those not starting with a `#` or `_`)
-are exported.
-A definition identifier is exported if it does not start with `_` or `#_`.
-Any other defintion is not visible outside the package and resides
+All identifiers not starting with `_` (so all regular fields and definitions
+starting with `#`) are exported.
+Any identifier starting with `_` is not visible outside the package and resides
 in a separate namespace than namesake identifiers of other packages.
 
 ```
@@ -1718,7 +1719,7 @@ foo:   string  // visible outside mypackage
     #C: {    // visible outside mypackage
         d: 4 // visible outside mypackage
     }
-    #_E: foo // not visible outside mypackage
+    _#E: foo // not visible outside mypackage
 }
 ```
 

encoding/jsonschema/ref.go

@@ -22,6 +22,7 @@ import (
 	"cuelang.org/go/cue/ast"
 	"cuelang.org/go/cue/errors"
 	"cuelang.org/go/cue/token"
+	"cuelang.org/go/internal"
 )
 
 func (d *decoder) parseRef(p token.Pos, str string) []string {
@@ -99,8 +100,7 @@ func jsonSchemaRef(p token.Pos, a []string) ([]ast.Label, error) {
 	name := a[1]
 	if ast.IsValidIdent(name) &&
 		name != rootDefs[1:] &&
-		!strings.HasPrefix(name, "#") &&
-		!strings.HasPrefix(name, "_") {
+		!internal.IsDefOrHidden(name) {
 		return []ast.Label{ast.NewIdent("#" + name)}, nil
 	}
 	return []ast.Label{ast.NewIdent(rootDefs), ast.NewString(name)}, nil

encoding/openapi/decode.go

@@ -135,8 +135,7 @@ func openAPIMapping(pos token.Pos, a []string) ([]ast.Label, error) {
 	name := a[2]
 	if ast.IsValidIdent(name) &&
 		name != rootDefs[1:] &&
-		!strings.HasPrefix(name, "#") &&
-		!strings.HasPrefix(name, "_") {
+		!internal.IsDefOrHidden(name) {
 		return []ast.Label{ast.NewIdent("#" + name)}, nil
 	}
 	return []ast.Label{ast.NewIdent(rootDefs), ast.NewString(name)}, nil