Release v1.28.0 (#528)

This release v1.28.0, which includes a series of changes cherry-picked from
`dev`. It skips the following changes that are currently on dev:

- #485, #486, #488: These are part of the streaming changes that are still in
  progress in the streamdev branch and should not be released without the rest
  of those changes.
- #507, #508: These changes by @jparise record and expose column numbers for
  AST identities. These cannot be released without #522, which is not yet
  ready.

Besides that, all changes from dev are included in this change.

Here's an API comparison between the last release (v1.27.0) and this release,
generated with the help of apidiff.

```
--- go.uber.org/thriftrw/ast ---
Compatible changes:
- Field.IDUnset: added
- Pos: added
- Position: added
--- go.uber.org/thriftrw/gen ---
Compatible changes:
- GeneratorOptions.EnumTextMarshalStrict: added
- Options.EnumTextMarshalStrict: added
--- go.uber.org/thriftrw/gen/internal/tests/enum-text-marshal-strict ---
NEW PACKAGE
--- go.uber.org/thriftrw/idl ---
Compatible changes:
- Config: added
- Error: added
- Info: added
- ParseError: added
--- go.uber.org/thriftrw/idl/internal ---
Incompatible changes:
- Parse: changed from func([]byte) (*go.uber.org/thriftrw/ast.Program, error) to func([]byte) (ParseResult, []ParseError)
Compatible changes:
- NodePositions: added
- ParseError: added
- ParseResult: added
--- go.uber.org/thriftrw/version ---
Incompatible changes:
- Version: value changed from "1.27.0" to "1.28.0"
```

Of the two incompatible changes:

- One is the version number change. This is desirable.
- The other is an internal API. This is safe.

.github/workflows/fossa.yaml

@@ -0,0 +1,17 @@
+name: FOSSA Analysis
+on: push
+
+jobs:
+
+  build:
+    runs-on: ubuntu-latest
+    if: github.repository_owner == 'thriftrw'
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v2
+
+      - name: FOSSA analysis
+        uses: fossas/fossa-action@v1
+        with:
+          api-key: ${{ secrets.FOSSA_API_KEY }}
+

CHANGELOG.md

@@ -4,6 +4,25 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [1.28.0] - 2021-07-26
+### Added
+- `idl.Parse` now returns structured `ParseError` on parse failures.
+- `idl.Config` provides a new means of parsing Thrift IDLs. This is the
+  recommended API going forward.
+
+### Changed
+- ThriftRW now tracks positional information about constant values in the
+  Thrift IDL. These were previously ignored. To access this information, see
+  the documentation for `idl.Config`.
+
+### Fixed
+- Support parsing `struct` fields without field identifiers. This is legacy
+  syntax still supported by Apache Thrift. Note that this is a parser-level
+  change only; ThriftRW will refuse to generate Go code from these.
+
+Thanks to [@jparise](https://github.com/jparise) for their contributions to
+this release.
+
 ## [1.27.0] - 2021-05-20
 ### Added
 - ThriftRW is now able to parse Thrift files with `cpp_include` statements.
@@ -365,6 +384,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ### Added
 - Initial release.
 
+[1.28.0]: https://github.com/thriftrw/thriftrw-go/compare/v1.27.0...v1.28.0
 [1.27.0]: https://github.com/thriftrw/thriftrw-go/compare/v1.26.0...v1.27.0
 [1.26.0]: https://github.com/thriftrw/thriftrw-go/compare/v1.25.1...v1.26.0
 [1.25.1]: https://github.com/thriftrw/thriftrw-go/compare/v1.25.0...v1.25.1

Makefile

@@ -1,5 +1,5 @@
 BUILD_DIR = $(shell pwd)/build
-RAGEL_VERSION = 6.9
+RAGEL_VERSION = 6.10
 
 export GOBIN = $(BUILD_DIR)/bin
 

ast/definition.go

@@ -297,7 +297,9 @@ const (
 // 	3: i64 baz (go.name = "qux")
 //
 type Field struct {
-	ID           int
+	ID int
+	// IDUnset indicates that a field identifier wasn't provided.
+	IDUnset      bool
 	Name         string
 	Type         Type
 	Requiredness Requiredness

ast/position.go

@@ -0,0 +1,35 @@
+// Copyright (c) 2021 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package ast
+
+// Position represents a position in the parsed document.
+type Position struct {
+	Line int
+}
+
+// Pos attempts to return the position of a Node in the parsed document.
+// For most use cases, prefer to use idl.Info to access positional information.
+func Pos(n Node) (Position, bool) {
+	if nl, ok := n.(nodeWithLine); ok {
+		return Position{Line: nl.lineNumber()}, true
+	}
+	return Position{}, false
+}

compile/compiler.go

@@ -220,10 +220,12 @@ func (c compiler) gather(m *Module, prog *ast.Program) error {
 			m.Types[enum.ThriftName()] = enum
 		case *ast.Struct:
 			requiredness := explicitRequiredness
+			allowNegativeIDs := false
 			if c.nonStrict {
 				requiredness = defaultToOptional
+				allowNegativeIDs = true
 			}
-			s, err := compileStruct(m.ThriftPath, definition, requiredness)
+			s, err := compileStruct(m.ThriftPath, definition, requiredness, allowNegativeIDs)
 			if err != nil {
 				return definitionError{Definition: d, Reason: err}
 			}

compile/field.go

@@ -85,9 +85,13 @@ func (r fieldRequiredness) isRequired(src *ast.Field) (bool, error) {
 //
 // disallowDefaultValue specifies whether the field is allowed to have a default
 // value.
+//
+// allowNegativeIDs controls whether negative field identifiers are allowed.
+// This also enables auto-assigning (negative) values for unset field IDs.
 type fieldOptions struct {
 	requiredness         fieldRequiredness
 	disallowDefaultValue bool
+	allowNegativeIDs     bool
 }
 
 // FieldSpec represents a single field of a struct or parameter list.
@@ -103,7 +107,7 @@ type FieldSpec struct {
 
 // compileField compiles the given Field source into a FieldSpec.
 func compileField(src *ast.Field, options fieldOptions) (*FieldSpec, error) {
-	if src.ID < 1 || src.ID > math.MaxInt16 {
+	if (src.ID < 1 && !options.allowNegativeIDs) || src.ID > math.MaxInt16 {
 		return nil, fieldIDOutOfBoundsError{ID: src.ID, Name: src.Name}
 	}
 
@@ -177,6 +181,7 @@ type FieldGroup []*FieldSpec
 func compileFields(src []*ast.Field, options fieldOptions) (FieldGroup, error) {
 	fieldsNS := newNamespace(caseSensitive)
 	usedIDs := make(map[int16]string)
+	nextNegativeID := -1
 
 	fields := make([]*FieldSpec, 0, len(src))
 	for _, astField := range src {
@@ -188,6 +193,15 @@ func compileFields(src []*ast.Field, options fieldOptions) (FieldGroup, error) {
 			}
 		}
 
+		if options.allowNegativeIDs {
+			if astField.ID < 0 {
+				nextNegativeID = astField.ID - 1
+			} else if astField.IDUnset {
+				astField.ID = nextNegativeID
+				nextNegativeID--
+			}
+		}
+
 		field, err := compileField(astField, options)
 		if err != nil {
 			return nil, compileError{

compile/struct.go

@@ -38,8 +38,11 @@ type StructSpec struct {
 }
 
 // compileStruct compiles a struct AST into a StructSpec.
-func compileStruct(file string, src *ast.Struct, requiredness fieldRequiredness) (*StructSpec, error) {
-	opts := fieldOptions{requiredness: requiredness}
+func compileStruct(file string, src *ast.Struct, requiredness fieldRequiredness, allowNegativeIDs bool) (*StructSpec, error) {
+	opts := fieldOptions{
+		requiredness:     requiredness,
+		allowNegativeIDs: allowNegativeIDs,
+	}
 
 	if src.Type == ast.UnionType {
 		opts.requiredness = noRequiredFields

compile/struct_test.go

@@ -49,12 +49,14 @@ func TestCompileStructSuccess(t *testing.T) {
 		src          string
 		scope        Scope
 		requiredness fieldRequiredness
+		negativeIDs  bool
 		spec         *StructSpec
 	}{
 		{
 			"struct Health { 1: optional bool healthy = true }",
 			nil,
 			explicitRequiredness,
+			false,
 			&StructSpec{
 				Name: "Health",
 				File: "test.thrift",
@@ -74,6 +76,7 @@ func TestCompileStructSuccess(t *testing.T) {
 			"struct Health { 1: bool healthy = true }",
 			nil,
 			defaultToOptional,
+			false,
 			&StructSpec{
 				Name: "Health",
 				File: "test.thrift",
@@ -96,6 +99,7 @@ func TestCompileStructSuccess(t *testing.T) {
 			}`,
 			nil,
 			defaultToOptional,
+			false,
 			&StructSpec{
 				Name: "Foo",
 				File: "test.thrift",
@@ -125,6 +129,7 @@ func TestCompileStructSuccess(t *testing.T) {
 			}`,
 			scope("Key", &TypedefSpec{Name: "Key", Target: &StringSpec{}}),
 			explicitRequiredness,
+			false,
 			&StructSpec{
 				Name: "KeyNotFoundError",
 				File: "test.thrift",
@@ -152,6 +157,7 @@ func TestCompileStructSuccess(t *testing.T) {
 			}`,
 			nil,
 			explicitRequiredness,
+			false,
 			&StructSpec{
 				Name: "Body",
 				File: "test.thrift",
@@ -172,13 +178,59 @@ func TestCompileStructSuccess(t *testing.T) {
 				},
 			},
 		},
+		{
+			`struct AutoAssignedIDs {
+				optional bool a
+				1: optional bool b
+				-3: optional bool c
+				optional bool d
+			}`,
+			nil,
+			explicitRequiredness,
+			true,
+			&StructSpec{
+				Name: "AutoAssignedIDs",
+				File: "test.thrift",
+				Type: ast.StructType,
+				Fields: FieldGroup{
+					{
+						ID:       -1,
+						Name:     "a",
+						Type:     &BoolSpec{},
+						Required: false,
+						Default:  nil,
+					},
+					{
+						ID:       1,
+						Name:     "b",
+						Type:     &BoolSpec{},
+						Required: false,
+						Default:  nil,
+					},
+					{
+						ID:       -3,
+						Name:     "c",
+						Type:     &BoolSpec{},
+						Required: false,
+						Default:  nil,
+					},
+					{
+						ID:       -4,
+						Name:     "d",
+						Type:     &BoolSpec{},
+						Required: false,
+						Default:  nil,
+					},
+				},
+			},
+		},
 	}
 
 	for _, tt := range tests {
 		expected := mustLink(t, tt.spec, defaultScope)
 
 		src := parseStruct(tt.src)
-		structSpec, err := compileStruct("test.thrift", src, tt.requiredness)
+		structSpec, err := compileStruct("test.thrift", src, tt.requiredness, tt.negativeIDs)
 		scope := scopeOrDefault(tt.scope)
 		if assert.NoError(t, err) {
 			spec, err := structSpec.Link(scope)
@@ -268,7 +320,7 @@ func TestCompileStructFailure(t *testing.T) {
 
 	for _, tt := range tests {
 		src := parseStruct(tt.src)
-		_, err := compileStruct("test.thrift", src, explicitRequiredness)
+		_, err := compileStruct("test.thrift", src, explicitRequiredness, false)
 
 		if assert.Error(t, err, tt.desc) {
 			for _, msg := range tt.messages {
@@ -309,7 +361,7 @@ func TestLinkStructFailure(t *testing.T) {
 		src := parseStruct(tt.src)
 		scope := scopeOrDefault(tt.scope)
 
-		spec, err := compileStruct("test.thrift", src, explicitRequiredness)
+		spec, err := compileStruct("test.thrift", src, explicitRequiredness, false)
 		if assert.NoError(t, err, tt.desc) {
 			_, err := spec.Link(scope)
 			if assert.Error(t, err, tt.desc) {

gen/enum.go

@@ -66,7 +66,6 @@ func enum(g Generator, spec *compile.EnumSpec) error {
 		<$fmt := import "fmt">
 		<$json := import "encoding/json">
 		<$math := import "math">
-		<$strconv := import "strconv">
 
 		<$wire := import "go.uber.org/thriftrw/wire">
 
@@ -110,7 +109,7 @@ func enum(g Generator, spec *compile.EnumSpec) error {
 					return nil
 			<end ->
 				default:
-					<$val>, err := <$strconv>.ParseInt(<$s>, 10, 32)
+					<$val>, err := <import "strconv">.ParseInt(<$s>, 10, 32)
 					if err != nil {
 						return <$fmt>.Errorf("unknown enum value %q for %q: %v", <$s>, "<$enumName>", err)
 					}
@@ -121,8 +120,12 @@ func enum(g Generator, spec *compile.EnumSpec) error {
 
 		// MarshalText encodes <$enumName> to text.
 		//
-		// If the enum value is recognized, its name is returned. Otherwise,
-		// its integer value is returned.
+		// If the enum value is recognized, its name is returned.
+		<if checkEnumTextMarshalStrict ->
+		// Otherwise, an error is returned.
+		<else ->
+		// Otherwise, its integer value is returned.
+		<end ->
 		//
 		// This implements the TextMarshaler interface.
 		func (<$v> <$enumName>) MarshalText() ([]byte, error) {
@@ -134,7 +137,11 @@ func enum(g Generator, spec *compile.EnumSpec) error {
 				<end ->
 				}
 			<end ->
-			return []byte(<$strconv>.FormatInt(int64(<$v>), 10)), nil
+			<if checkEnumTextMarshalStrict ->
+				return nil, <$fmt>.Errorf("unknown enum value %q for %q", <$v>, "<$enumName>")
+			<else ->	
+				return []byte(<import "strconv">.FormatInt(int64(<$v>), 10)), nil
+			<end ->
 		}
 
 		<if not (checkNoZap) ->
@@ -213,8 +220,12 @@ func enum(g Generator, spec *compile.EnumSpec) error {
 
 		// MarshalJSON serializes <$enumName> into JSON.
 		//
-		// If the enum value is recognized, its name is returned. Otherwise,
-		// its integer value is returned.
+		// If the enum value is recognized, its name is returned.
+		<if checkEnumTextMarshalStrict ->
+		// Otherwise, an error is returned.
+		<else ->
+		// Otherwise, its integer value is returned.
+		<end ->
 		//
 		// This implements json.Marshaler.
 		func (<$v> <$enumName>) MarshalJSON() ([]byte, error) {
@@ -226,7 +237,11 @@ func enum(g Generator, spec *compile.EnumSpec) error {
 				<end ->
 				}
 			<end ->
-			return ([]byte)(<$strconv>.FormatInt(int64(<$v>), 10)), nil
+			<if checkEnumTextMarshalStrict ->
+				return nil, <$fmt>.Errorf("unknown enum value %q for %q", <$v>, "<$enumName>")
+			<else ->
+				return ([]byte)(<import "strconv">.FormatInt(int64(<$v>), 10)), nil
+			<end ->
 		}
 
 		<$text := newVar "text">
@@ -279,6 +294,7 @@ func enum(g Generator, spec *compile.EnumSpec) error {
 		},
 		TemplateFunc("enumItemLabelName", entityLabel),
 		TemplateFunc("checkNoZap", checkNoZap),
+		TemplateFunc("checkEnumTextMarshalStrict", checkEnumTextMarshalStrict),
 	)
 
 	return wrapGenerateError(spec.Name, err)