From 604826125f871b71bbacbbe56ed7778be7055e60 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Wed, 19 May 2021 18:53:26 -0700 Subject: [PATCH 01/16] Back to development --- CHANGELOG.md | 4 ++++ gen/internal/tests/collision/collision.go | 2 +- gen/internal/tests/constants/constants.go | 2 +- gen/internal/tests/containers/containers.go | 2 +- gen/internal/tests/enum_conflict/enum_conflict.go | 2 +- gen/internal/tests/enums/enums.go | 2 +- gen/internal/tests/exceptions/exceptions.go | 2 +- gen/internal/tests/hyphenated-file/hyphenated-file.go | 2 +- gen/internal/tests/hyphenated_file/hyphenated_file.go | 2 +- gen/internal/tests/non_hyphenated/non_hyphenated.go | 2 +- gen/internal/tests/nozap/nozap.go | 2 +- gen/internal/tests/other_constants/other_constants.go | 2 +- gen/internal/tests/services/services.go | 2 +- gen/internal/tests/set_to_slice/set_to_slice.go | 2 +- gen/internal/tests/structs/structs.go | 2 +- gen/internal/tests/typedefs/typedefs.go | 2 +- gen/internal/tests/unions/unions.go | 2 +- gen/internal/tests/uuid_conflict/uuid_conflict.go | 2 +- internal/envelope/exception/exception.go | 2 +- plugin/api/api.go | 2 +- version/version.go | 2 +- 21 files changed, 24 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dc0da0c..6ffbeb17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,9 @@ 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). +## [Unreleased] +- No changes yet. + ## [1.27.0] - 2021-05-20 ### Added - ThriftRW is now able to parse Thrift files with `cpp_include` statements. @@ -365,6 +368,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added - Initial release. +[Unreleased]: https://github.com/thriftrw/thriftrw-go/compare/v1.27.0...HEAD [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 diff --git a/gen/internal/tests/collision/collision.go b/gen/internal/tests/collision/collision.go index f0bf03f8..277acaf1 100644 --- a/gen/internal/tests/collision/collision.go +++ b/gen/internal/tests/collision/collision.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package collision diff --git a/gen/internal/tests/constants/constants.go b/gen/internal/tests/constants/constants.go index 0bf18264..e0bd09d6 100644 --- a/gen/internal/tests/constants/constants.go +++ b/gen/internal/tests/constants/constants.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package constants diff --git a/gen/internal/tests/containers/containers.go b/gen/internal/tests/containers/containers.go index 0f1a4342..c43c54db 100644 --- a/gen/internal/tests/containers/containers.go +++ b/gen/internal/tests/containers/containers.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package containers diff --git a/gen/internal/tests/enum_conflict/enum_conflict.go b/gen/internal/tests/enum_conflict/enum_conflict.go index 54aa806a..e8cdc302 100644 --- a/gen/internal/tests/enum_conflict/enum_conflict.go +++ b/gen/internal/tests/enum_conflict/enum_conflict.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package enum_conflict diff --git a/gen/internal/tests/enums/enums.go b/gen/internal/tests/enums/enums.go index 2ffd969d..b6cbb189 100644 --- a/gen/internal/tests/enums/enums.go +++ b/gen/internal/tests/enums/enums.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package enums diff --git a/gen/internal/tests/exceptions/exceptions.go b/gen/internal/tests/exceptions/exceptions.go index fb64427d..c02c9f99 100644 --- a/gen/internal/tests/exceptions/exceptions.go +++ b/gen/internal/tests/exceptions/exceptions.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package exceptions diff --git a/gen/internal/tests/hyphenated-file/hyphenated-file.go b/gen/internal/tests/hyphenated-file/hyphenated-file.go index 2c3ebe7e..521b4abc 100644 --- a/gen/internal/tests/hyphenated-file/hyphenated-file.go +++ b/gen/internal/tests/hyphenated-file/hyphenated-file.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package hyphenated_file diff --git a/gen/internal/tests/hyphenated_file/hyphenated_file.go b/gen/internal/tests/hyphenated_file/hyphenated_file.go index 751f3cb4..a9255c44 100644 --- a/gen/internal/tests/hyphenated_file/hyphenated_file.go +++ b/gen/internal/tests/hyphenated_file/hyphenated_file.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package hyphenated_file diff --git a/gen/internal/tests/non_hyphenated/non_hyphenated.go b/gen/internal/tests/non_hyphenated/non_hyphenated.go index 81ab67dc..b1f02e6f 100644 --- a/gen/internal/tests/non_hyphenated/non_hyphenated.go +++ b/gen/internal/tests/non_hyphenated/non_hyphenated.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package non_hyphenated diff --git a/gen/internal/tests/nozap/nozap.go b/gen/internal/tests/nozap/nozap.go index 32da2494..9dbb633a 100644 --- a/gen/internal/tests/nozap/nozap.go +++ b/gen/internal/tests/nozap/nozap.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package nozap diff --git a/gen/internal/tests/other_constants/other_constants.go b/gen/internal/tests/other_constants/other_constants.go index c4feead9..e0129740 100644 --- a/gen/internal/tests/other_constants/other_constants.go +++ b/gen/internal/tests/other_constants/other_constants.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package other_constants diff --git a/gen/internal/tests/services/services.go b/gen/internal/tests/services/services.go index 78a464fa..3c43f5da 100644 --- a/gen/internal/tests/services/services.go +++ b/gen/internal/tests/services/services.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package services diff --git a/gen/internal/tests/set_to_slice/set_to_slice.go b/gen/internal/tests/set_to_slice/set_to_slice.go index cf36de64..6b4b7c16 100644 --- a/gen/internal/tests/set_to_slice/set_to_slice.go +++ b/gen/internal/tests/set_to_slice/set_to_slice.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package set_to_slice diff --git a/gen/internal/tests/structs/structs.go b/gen/internal/tests/structs/structs.go index 0e862022..7f2aeecc 100644 --- a/gen/internal/tests/structs/structs.go +++ b/gen/internal/tests/structs/structs.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package structs diff --git a/gen/internal/tests/typedefs/typedefs.go b/gen/internal/tests/typedefs/typedefs.go index edaeed0e..e5f10de1 100644 --- a/gen/internal/tests/typedefs/typedefs.go +++ b/gen/internal/tests/typedefs/typedefs.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package typedefs diff --git a/gen/internal/tests/unions/unions.go b/gen/internal/tests/unions/unions.go index 02759729..0f17e9bd 100644 --- a/gen/internal/tests/unions/unions.go +++ b/gen/internal/tests/unions/unions.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package unions diff --git a/gen/internal/tests/uuid_conflict/uuid_conflict.go b/gen/internal/tests/uuid_conflict/uuid_conflict.go index 1431bd57..3c3ddd26 100644 --- a/gen/internal/tests/uuid_conflict/uuid_conflict.go +++ b/gen/internal/tests/uuid_conflict/uuid_conflict.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated package uuid_conflict diff --git a/internal/envelope/exception/exception.go b/internal/envelope/exception/exception.go index e080e0c8..cf408297 100644 --- a/internal/envelope/exception/exception.go +++ b/internal/envelope/exception/exception.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated // Copyright (c) 2021 Uber Technologies, Inc. diff --git a/plugin/api/api.go b/plugin/api/api.go index 3b79f657..57812320 100644 --- a/plugin/api/api.go +++ b/plugin/api/api.go @@ -1,4 +1,4 @@ -// Code generated by thriftrw v1.27.0. DO NOT EDIT. +// Code generated by thriftrw v1.28.0. DO NOT EDIT. // @generated // Copyright (c) 2021 Uber Technologies, Inc. diff --git a/version/version.go b/version/version.go index 722d5406..89b3e938 100644 --- a/version/version.go +++ b/version/version.go @@ -21,4 +21,4 @@ package version // Version is the current ThriftRW version. -const Version = "1.27.0" +const Version = "1.28.0" From 52b616151f844549a1fe4898aeee1ee8ed40d229 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 25 May 2021 10:04:56 -0700 Subject: [PATCH 02/16] Integrate FOSSA (#482) Add a FOSSA check to the build steps for ThriftRW Go. Refs GO-468 --- .github/workflows/go.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 56b48c16..7d72676e 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -27,6 +27,11 @@ jobs: - name: Checkout code uses: actions/checkout@v2 + - name: FOSSA analysis + uses: fossas/fossa-action@v1 + with: + api-key: ${{ secrets.FOSSA_API_KEY }} + - name: Load cached dependencies uses: actions/cache@v1 with: From e06da9071826676ae027bcd741ba22d8ff13d9c1 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 25 May 2021 15:18:34 -0700 Subject: [PATCH 03/16] fossa: Run separately, only on push (#483) Currently, the FOSSA analysis is set to run as part of CI. This makes it impossible to run builds for incoming pull requests because PRs don't have access to the secrets needed for the FOSSA analysis. Resolve this by running the FOSSA analysis only when we push to a branch of the repository. --- .github/workflows/fossa.yaml | 17 +++++++++++++++++ .github/workflows/go.yml | 5 ----- 2 files changed, 17 insertions(+), 5 deletions(-) create mode 100644 .github/workflows/fossa.yaml diff --git a/.github/workflows/fossa.yaml b/.github/workflows/fossa.yaml new file mode 100644 index 00000000..5556fdcf --- /dev/null +++ b/.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 }} + diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 7d72676e..56b48c16 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -27,11 +27,6 @@ jobs: - name: Checkout code uses: actions/checkout@v2 - - name: FOSSA analysis - uses: fossas/fossa-action@v1 - with: - api-key: ${{ secrets.FOSSA_API_KEY }} - - name: Load cached dependencies uses: actions/cache@v1 with: From cf9f7a69dcdcaca207e2118f43db5ebcb90c14c7 Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Tue, 25 May 2021 17:25:21 -0700 Subject: [PATCH 04/16] idl: support for auto-assigned field identifiers (#481) Thrift allows implicit assignment of field identifiers. This is legacy syntax, but it's useful to support it for compatibility with old files. Auto-assigned fields start at -1 and get increasingly negative. If a negative field identifier is explicitly given, it will be used as the new basis for any additional auto-assigned fields. This behavior is consistent with the Apache Thrift implementation. --- ast/definition.go | 4 +- compile/compiler.go | 4 +- compile/field.go | 16 ++- compile/struct.go | 7 +- compile/struct_test.go | 58 +++++++- idl/internal/field.go | 26 ++++ idl/internal/thrift.y | 39 +++--- idl/internal/y.go | 308 +++++++++++++++++++++-------------------- idl/parser_test.go | 66 +++++++++ 9 files changed, 356 insertions(+), 172 deletions(-) create mode 100644 idl/internal/field.go diff --git a/ast/definition.go b/ast/definition.go index 35525f8f..de940826 100644 --- a/ast/definition.go +++ b/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 diff --git a/compile/compiler.go b/compile/compiler.go index 29e5aadf..00760490 100644 --- a/compile/compiler.go +++ b/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} } diff --git a/compile/field.go b/compile/field.go index 1c7268b9..0f433620 100644 --- a/compile/field.go +++ b/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{ diff --git a/compile/struct.go b/compile/struct.go index 5537b646..11b2514a 100644 --- a/compile/struct.go +++ b/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 diff --git a/compile/struct_test.go b/compile/struct_test.go index f2b974cc..7054a3a8 100644 --- a/compile/struct_test.go +++ b/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) { diff --git a/idl/internal/field.go b/idl/internal/field.go new file mode 100644 index 00000000..1e3edc05 --- /dev/null +++ b/idl/internal/field.go @@ -0,0 +1,26 @@ +// 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 internal + +type fieldIdentifier struct { + ID int + Unset bool +} diff --git a/idl/internal/thrift.y b/idl/internal/thrift.y index ea744233..015d1428 100644 --- a/idl/internal/thrift.y +++ b/idl/internal/thrift.y @@ -24,6 +24,7 @@ import "go.uber.org/thriftrw/ast" fieldType ast.Type structType ast.StructureType baseTypeID ast.BaseTypeID + fieldIdentifier fieldIdentifier fieldRequired ast.Requiredness field *ast.Field @@ -64,6 +65,7 @@ import "go.uber.org/thriftrw/ast" %type program %type type %type base_type_name +%type field_identifier %type field_required %type struct_type @@ -268,40 +270,45 @@ enum_item ; fields - : /* nothing */ { $$ = nil } + : /* nothing */ { $$ = nil } | fields field optional_sep { $$ = append($1, $2) } ; - field - : lineno docstring INTCONSTANT ':' field_required type IDENTIFIER type_annotations + : lineno docstring field_identifier field_required type IDENTIFIER type_annotations { $$ = &ast.Field{ - ID: int($3), - Name: $7, - Type: $6, - Requiredness: $5, - Annotations: $8, + ID: $3.ID, + IDUnset: $3.Unset, + Name: $6, + Type: $5, + Requiredness: $4, + Annotations: $7, Line: $1, Doc: ParseDocstring($2), } } - | lineno docstring INTCONSTANT ':' field_required type IDENTIFIER '=' const_value - type_annotations + | lineno docstring field_identifier field_required type IDENTIFIER '=' const_value type_annotations { $$ = &ast.Field{ - ID: int($3), - Name: $7, - Type: $6, - Requiredness: $5, - Default: $9, - Annotations: $10, + ID: $3.ID, + IDUnset: $3.Unset, + Name: $6, + Type: $5, + Requiredness: $4, + Default: $8, + Annotations: $9, Line: $1, Doc: ParseDocstring($2), } } ; +field_identifier + : INTCONSTANT ':' { $$ = fieldIdentifier{ID: int($1)} } + | /* na */ { $$ = fieldIdentifier{Unset: true} } + ; + field_required : REQUIRED { $$ = ast.Required } | OPTIONAL { $$ = ast.Optional } diff --git a/idl/internal/y.go b/idl/internal/y.go index d0d1c95f..a8f90b19 100644 --- a/idl/internal/y.go +++ b/idl/internal/y.go @@ -45,10 +45,11 @@ type yySymType struct { i64 int64 dub float64 - fieldType ast.Type - structType ast.StructureType - baseTypeID ast.BaseTypeID - fieldRequired ast.Requiredness + fieldType ast.Type + structType ast.StructureType + baseTypeID ast.BaseTypeID + fieldIdentifier fieldIdentifier + fieldRequired ast.Requiredness field *ast.Field fields []*ast.Field @@ -170,128 +171,127 @@ var yyExca = [...]int{ 1, -1, -2, 0, -1, 2, - 8, 71, - 9, 71, - 10, 71, + 8, 73, + 9, 73, + 10, 73, -2, 9, -1, 3, 1, 1, - -2, 71, + -2, 73, } const yyPrivate = 57344 -const yyLast = 190 +const yyLast = 180 var yyAct = [...]int{ - 32, 11, 68, 5, 7, 65, 66, 58, 124, 87, - 31, 73, 69, 70, 12, 12, 94, 14, 13, 13, - 89, 126, 96, 95, 62, 61, 60, 161, 153, 152, - 128, 92, 33, 59, 59, 159, 59, 146, 142, 129, - 131, 122, 71, 72, 85, 82, 73, 69, 70, 79, - 56, 107, 54, 53, 57, 120, 91, 67, 74, 156, - 63, 55, 19, 106, 90, 81, 84, 138, 139, 136, - 117, 76, 77, 78, 115, 134, 93, 71, 72, 10, - 8, 9, 98, 16, 15, 101, 28, 97, 104, 17, - 100, 99, 148, 103, 102, 140, 18, 114, 110, 88, - 52, 37, 36, 112, 113, 111, 35, 34, 30, 74, - 123, 121, 125, 29, 119, 155, 118, 130, 116, 105, - 75, 109, 127, 132, 74, 108, 133, 3, 6, 64, - 80, 86, 2, 4, 135, 83, 143, 23, 137, 141, - 38, 1, 0, 74, 144, 147, 0, 0, 145, 150, - 84, 0, 149, 74, 0, 154, 151, 0, 0, 0, - 0, 84, 157, 158, 0, 160, 21, 25, 26, 27, - 42, 0, 24, 22, 20, 0, 0, 0, 43, 44, + 32, 58, 65, 5, 7, 68, 87, 31, 11, 66, + 125, 73, 69, 70, 14, 12, 89, 12, 94, 13, + 127, 13, 96, 95, 62, 61, 60, 162, 155, 33, + 151, 129, 92, 160, 59, 59, 59, 148, 144, 133, + 135, 123, 71, 72, 85, 82, 73, 69, 70, 79, + 56, 107, 91, 54, 63, 121, 53, 67, 74, 57, + 90, 55, 19, 131, 132, 81, 84, 158, 76, 77, + 78, 106, 10, 8, 9, 140, 93, 71, 72, 118, + 116, 97, 16, 15, 100, 138, 28, 103, 99, 98, + 17, 102, 101, 147, 142, 104, 18, 112, 113, 157, + 114, 110, 111, 88, 52, 37, 36, 35, 34, 74, + 124, 30, 29, 119, 122, 117, 128, 120, 134, 126, + 21, 25, 26, 27, 105, 74, 24, 22, 20, 139, + 137, 136, 75, 109, 108, 3, 6, 143, 141, 64, + 80, 86, 146, 2, 4, 74, 83, 145, 23, 150, + 149, 152, 74, 84, 130, 115, 156, 154, 153, 159, + 42, 38, 84, 161, 1, 0, 0, 0, 43, 44, 45, 46, 47, 48, 49, 50, 51, 39, 40, 41, } var yyPact = [...]int{ - -1000, -1000, -1000, -1000, -1000, 71, -32, -1000, 79, 84, - 58, -1000, -1000, -1000, 141, -1000, 81, -1000, 109, 104, - -1000, -1000, 103, 102, 98, -1000, -1000, -1000, -1000, -1000, - -1000, 97, 166, 96, 13, 12, 21, 15, -7, -19, + -1000, -1000, -1000, -1000, -1000, 64, -31, -1000, 78, 85, + 58, -1000, -1000, -1000, 95, -1000, 81, -1000, 108, 107, + -1000, -1000, 104, 103, 102, -1000, -1000, -1000, -1000, -1000, + -1000, 101, 156, 100, 16, 13, 21, 20, -7, -19, -20, -21, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -1000, -7, -1000, -1000, -1000, -1000, 41, -1000, -1000, - -1000, -1000, -1000, -1000, 8, 4, 3, 95, -1000, -1000, - -1000, -1000, -1000, -1000, 16, -13, -30, -24, -25, -7, - -32, -1000, -7, -32, -1000, -7, -32, 39, 11, -1000, - -1000, -1000, -1000, 94, -1000, -7, -7, -1000, -1000, 93, - -1000, -1000, 68, -1000, -1000, 59, -1000, -1000, 6, 0, - -31, -26, -1000, -1000, -9, -3, -1000, -1000, -1000, -1, - -1000, -32, -1000, 41, 70, -1000, -7, -1000, 63, 33, - 91, -7, -1000, -4, -32, -1000, -7, -1000, -1000, -1000, - -6, -1000, 41, -1000, -1000, 88, -1000, -32, -10, -16, - -1000, -1000, 41, 29, -7, -7, -8, -1000, -1000, -1000, - -17, -1000, + -1000, -1000, -1000, -1000, 8, 4, 3, 99, -1000, -1000, + -1000, -1000, -1000, -1000, 12, -12, -28, -24, -25, -7, + -31, -1000, -7, -31, -1000, -7, -31, 47, 11, -1000, + -1000, -1000, -1000, 97, -1000, -7, -7, -1000, -1000, 96, + -1000, -1000, 74, -1000, -1000, 68, -1000, -1000, 6, 0, + -29, -27, -1000, -1000, -8, 29, -3, -1000, -1000, -1000, + -1, -1000, -31, -1000, 41, 80, -1000, -7, -1000, 69, + -1000, -1000, -1000, -1000, 90, -7, -1000, -4, -31, -1000, + -7, 89, -6, -1000, 41, -1000, -1000, -9, -1000, -31, + -1000, 41, -16, -1000, -7, 37, -1000, -7, -10, -1000, + -1000, -17, -1000, } var yyPgo = [...]int{ - 0, 0, 9, 141, 10, 140, 138, 137, 135, 5, - 133, 132, 131, 6, 130, 129, 128, 127, 2, 125, - 121, 120, 7, 1, 119, 118, 115, + 0, 0, 6, 164, 7, 161, 155, 154, 148, 146, + 2, 144, 143, 141, 9, 140, 139, 136, 135, 5, + 134, 133, 132, 1, 8, 124, 115, 99, } var yyR1 = [...]int{ - 0, 3, 11, 11, 10, 10, 10, 10, 10, 17, - 17, 16, 16, 16, 16, 16, 16, 7, 7, 7, - 15, 15, 14, 14, 9, 9, 8, 8, 6, 6, - 6, 13, 13, 12, 24, 24, 25, 25, 26, 26, - 4, 4, 4, 4, 4, 5, 5, 5, 5, 5, - 5, 5, 5, 5, 18, 18, 18, 18, 18, 18, - 18, 18, 19, 19, 20, 20, 22, 22, 21, 21, - 21, 1, 2, 23, 23, 23, + 0, 3, 12, 12, 11, 11, 11, 11, 11, 18, + 18, 17, 17, 17, 17, 17, 17, 8, 8, 8, + 16, 16, 15, 15, 10, 10, 9, 9, 6, 6, + 7, 7, 7, 14, 14, 13, 25, 25, 26, 26, + 27, 27, 4, 4, 4, 4, 4, 5, 5, 5, + 5, 5, 5, 5, 5, 5, 19, 19, 19, 19, + 19, 19, 19, 19, 20, 20, 21, 21, 23, 23, + 22, 22, 22, 1, 2, 24, 24, 24, } var yyR2 = [...]int{ 0, 2, 0, 2, 3, 4, 3, 4, 4, 0, 3, 7, 6, 8, 8, 8, 11, 1, 1, 1, - 0, 3, 4, 6, 0, 3, 8, 10, 1, 1, - 0, 0, 3, 10, 1, 0, 1, 1, 0, 4, - 3, 8, 6, 6, 2, 1, 1, 1, 1, 1, - 1, 1, 1, 1, 1, 1, 1, 1, 1, 2, - 4, 4, 0, 3, 0, 6, 0, 3, 0, 6, - 4, 0, 0, 1, 1, 0, + 0, 3, 4, 6, 0, 3, 7, 9, 2, 0, + 1, 1, 0, 0, 3, 10, 1, 0, 1, 1, + 0, 4, 3, 8, 6, 6, 2, 1, 1, 1, + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, + 1, 2, 4, 4, 0, 3, 0, 6, 0, 3, + 0, 6, 4, 0, 0, 1, 1, 0, } var yyChk = [...]int{ - -1000, -3, -11, -17, -10, -1, -16, -1, 9, 10, - 8, -23, 46, 50, -2, 5, 4, 5, 38, 4, - 33, 25, 32, -7, 31, 26, 27, 28, 5, 4, + -1000, -3, -12, -18, -11, -1, -17, -1, 9, 10, + 8, -24, 46, 50, -2, 5, 4, 5, 38, 4, + 33, 25, 32, -8, 31, 26, 27, 28, 5, 4, 4, -4, -1, -4, 4, 4, 4, 4, -5, 21, 22, 23, 4, 12, 13, 14, 15, 16, 17, 18, - 19, 20, 4, 40, 40, 40, 29, 39, -22, 43, - 45, 45, 45, -22, -15, -9, -13, -1, -18, 6, - 7, 36, 37, 5, -1, -21, -4, -4, -4, 41, - -14, -1, 41, -8, -1, 41, -12, -2, 4, 4, - 48, 40, 44, -1, 46, 47, 47, -22, -23, -2, - -22, -23, -2, -22, -23, -24, 24, 40, -19, -20, - 4, -4, -22, -22, 4, 6, -25, 11, -4, -13, - 49, -18, 41, -1, 39, -23, 47, -22, 39, 42, - -1, 41, -23, -18, 5, -22, 6, -6, 34, 35, - 4, -22, 42, -23, -22, -4, 43, -18, 4, -9, - -23, -22, 39, 44, -18, -26, 30, -22, -22, 43, - -9, 44, + 19, 20, 4, 40, 40, 40, 29, 39, -23, 43, + 45, 45, 45, -23, -16, -10, -14, -1, -19, 6, + 7, 36, 37, 5, -1, -22, -4, -4, -4, 41, + -15, -1, 41, -9, -1, 41, -13, -2, 4, 4, + 48, 40, 44, -1, 46, 47, 47, -23, -24, -2, + -23, -24, -2, -23, -24, -25, 24, 40, -20, -21, + 4, -4, -23, -23, 4, -6, 6, -26, 11, -4, + -14, 49, -19, 41, -1, 39, -24, 47, -23, 39, + -7, 34, 35, 42, -1, 41, -24, -19, 5, -23, + 6, -4, 4, -23, 42, -24, -23, 4, 43, -19, + -23, 39, -10, -24, -19, 44, -23, -27, 30, -23, + 43, -10, 44, } var yyDef = [...]int{ - 2, -2, -2, -2, 3, 0, 75, 72, 0, 0, - 0, 10, 73, 74, 0, 4, 0, 6, 0, 0, - 71, 71, 0, 0, 0, 17, 18, 19, 5, 7, - 8, 0, 0, 0, 0, 0, 0, 0, 66, 0, - 0, 0, 44, 45, 46, 47, 48, 49, 50, 51, - 52, 53, 66, 20, 24, 31, 71, 71, 40, 68, - 71, 71, 71, 12, 71, 71, 72, 0, 11, 54, - 55, 56, 57, 58, 0, 71, 0, 0, 0, 66, - 75, 72, 66, 75, 72, 66, 75, 35, 0, 59, - 62, 64, 67, 0, 71, 66, 66, 13, 21, 0, - 14, 25, 0, 15, 32, 71, 34, 31, 71, 71, - 75, 0, 42, 43, 66, 0, 71, 36, 37, 72, - 60, 75, 61, 71, 0, 70, 66, 22, 0, 30, - 0, 66, 63, 0, 75, 41, 66, 71, 28, 29, - 0, 16, 71, 69, 23, 0, 24, 75, 66, 71, - 65, 26, 71, 38, 66, 66, 0, 27, 33, 24, - 71, 39, + 2, -2, -2, -2, 3, 0, 77, 74, 0, 0, + 0, 10, 75, 76, 0, 4, 0, 6, 0, 0, + 73, 73, 0, 0, 0, 17, 18, 19, 5, 7, + 8, 0, 0, 0, 0, 0, 0, 0, 68, 0, + 0, 0, 46, 47, 48, 49, 50, 51, 52, 53, + 54, 55, 68, 20, 24, 33, 73, 73, 42, 70, + 73, 73, 73, 12, 73, 73, 74, 0, 11, 56, + 57, 58, 59, 60, 0, 73, 0, 0, 0, 68, + 77, 74, 68, 77, 74, 68, 77, 37, 0, 61, + 64, 66, 69, 0, 73, 68, 68, 13, 21, 0, + 14, 25, 29, 15, 34, 73, 36, 33, 73, 73, + 77, 0, 44, 45, 68, 32, 0, 73, 38, 39, + 74, 62, 77, 63, 73, 0, 72, 68, 22, 0, + 73, 30, 31, 28, 0, 68, 65, 0, 77, 43, + 68, 0, 0, 16, 73, 71, 23, 68, 24, 77, + 26, 73, 73, 67, 68, 40, 27, 68, 0, 35, + 24, 73, 41, } var yyTok1 = [...]int{ @@ -856,58 +856,70 @@ yydefault: yyVAL.fields = append(yyDollar[1].fields, yyDollar[2].field) } case 26: - yyDollar = yyS[yypt-8 : yypt+1] + yyDollar = yyS[yypt-7 : yypt+1] { yyVAL.field = &ast.Field{ - ID: int(yyDollar[3].i64), - Name: yyDollar[7].str, - Type: yyDollar[6].fieldType, - Requiredness: yyDollar[5].fieldRequired, - Annotations: yyDollar[8].typeAnnotations, + ID: yyDollar[3].fieldIdentifier.ID, + IDUnset: yyDollar[3].fieldIdentifier.Unset, + Name: yyDollar[6].str, + Type: yyDollar[5].fieldType, + Requiredness: yyDollar[4].fieldRequired, + Annotations: yyDollar[7].typeAnnotations, Line: yyDollar[1].line, Doc: ParseDocstring(yyDollar[2].docstring), } } case 27: - yyDollar = yyS[yypt-10 : yypt+1] + yyDollar = yyS[yypt-9 : yypt+1] { yyVAL.field = &ast.Field{ - ID: int(yyDollar[3].i64), - Name: yyDollar[7].str, - Type: yyDollar[6].fieldType, - Requiredness: yyDollar[5].fieldRequired, - Default: yyDollar[9].constantValue, - Annotations: yyDollar[10].typeAnnotations, + ID: yyDollar[3].fieldIdentifier.ID, + IDUnset: yyDollar[3].fieldIdentifier.Unset, + Name: yyDollar[6].str, + Type: yyDollar[5].fieldType, + Requiredness: yyDollar[4].fieldRequired, + Default: yyDollar[8].constantValue, + Annotations: yyDollar[9].typeAnnotations, Line: yyDollar[1].line, Doc: ParseDocstring(yyDollar[2].docstring), } } case 28: + yyDollar = yyS[yypt-2 : yypt+1] + { + yyVAL.fieldIdentifier = fieldIdentifier{ID: int(yyDollar[1].i64)} + } + case 29: + yyDollar = yyS[yypt-0 : yypt+1] + { + yyVAL.fieldIdentifier = fieldIdentifier{Unset: true} + } + case 30: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.fieldRequired = ast.Required } - case 29: + case 31: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.fieldRequired = ast.Optional } - case 30: + case 32: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.fieldRequired = ast.Unspecified } - case 31: + case 33: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.functions = nil } - case 32: + case 34: yyDollar = yyS[yypt-3 : yypt+1] { yyVAL.functions = append(yyDollar[1].functions, yyDollar[2].function) } - case 33: + case 35: yyDollar = yyS[yypt-10 : yypt+1] { yyVAL.function = &ast.Function{ @@ -921,197 +933,197 @@ yydefault: Doc: ParseDocstring(yyDollar[1].docstring), } } - case 34: + case 36: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.bul = true } - case 35: + case 37: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.bul = false } - case 36: + case 38: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.fieldType = nil } - case 37: + case 39: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.fieldType = yyDollar[1].fieldType } - case 38: + case 40: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.fields = nil } - case 39: + case 41: yyDollar = yyS[yypt-4 : yypt+1] { yyVAL.fields = yyDollar[3].fields } - case 40: + case 42: yyDollar = yyS[yypt-3 : yypt+1] { yyVAL.fieldType = ast.BaseType{ID: yyDollar[2].baseTypeID, Annotations: yyDollar[3].typeAnnotations, Line: yyDollar[1].line} } - case 41: + case 43: yyDollar = yyS[yypt-8 : yypt+1] { yyVAL.fieldType = ast.MapType{KeyType: yyDollar[4].fieldType, ValueType: yyDollar[6].fieldType, Annotations: yyDollar[8].typeAnnotations, Line: yyDollar[1].line} } - case 42: + case 44: yyDollar = yyS[yypt-6 : yypt+1] { yyVAL.fieldType = ast.ListType{ValueType: yyDollar[4].fieldType, Annotations: yyDollar[6].typeAnnotations, Line: yyDollar[1].line} } - case 43: + case 45: yyDollar = yyS[yypt-6 : yypt+1] { yyVAL.fieldType = ast.SetType{ValueType: yyDollar[4].fieldType, Annotations: yyDollar[6].typeAnnotations, Line: yyDollar[1].line} } - case 44: + case 46: yyDollar = yyS[yypt-2 : yypt+1] { yyVAL.fieldType = ast.TypeReference{Name: yyDollar[2].str, Line: yyDollar[1].line} } - case 45: + case 47: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.BoolTypeID } - case 46: + case 48: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.I8TypeID } - case 47: + case 49: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.I8TypeID } - case 48: + case 50: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.I16TypeID } - case 49: + case 51: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.I32TypeID } - case 50: + case 52: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.I64TypeID } - case 51: + case 53: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.DoubleTypeID } - case 52: + case 54: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.StringTypeID } - case 53: + case 55: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.baseTypeID = ast.BinaryTypeID } - case 54: + case 56: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantInteger(yyDollar[1].i64) } - case 55: + case 57: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantDouble(yyDollar[1].dub) } - case 56: + case 58: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantBoolean(true) } - case 57: + case 59: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantBoolean(false) } - case 58: + case 60: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantString(yyDollar[1].str) } - case 59: + case 61: yyDollar = yyS[yypt-2 : yypt+1] { yyVAL.constantValue = ast.ConstantReference{Name: yyDollar[2].str, Line: yyDollar[1].line} } - case 60: + case 62: yyDollar = yyS[yypt-4 : yypt+1] { yyVAL.constantValue = ast.ConstantList{Items: yyDollar[3].constantValues, Line: yyDollar[1].line} } - case 61: + case 63: yyDollar = yyS[yypt-4 : yypt+1] { yyVAL.constantValue = ast.ConstantMap{Items: yyDollar[3].constantMapItems, Line: yyDollar[1].line} } - case 62: + case 64: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.constantValues = nil } - case 63: + case 65: yyDollar = yyS[yypt-3 : yypt+1] { yyVAL.constantValues = append(yyDollar[1].constantValues, yyDollar[2].constantValue) } - case 64: + case 66: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.constantMapItems = nil } - case 65: + case 67: yyDollar = yyS[yypt-6 : yypt+1] { yyVAL.constantMapItems = append(yyDollar[1].constantMapItems, ast.ConstantMapItem{Key: yyDollar[3].constantValue, Value: yyDollar[5].constantValue, Line: yyDollar[2].line}) } - case 66: + case 68: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.typeAnnotations = nil } - case 67: + case 69: yyDollar = yyS[yypt-3 : yypt+1] { yyVAL.typeAnnotations = yyDollar[2].typeAnnotations } - case 68: + case 70: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.typeAnnotations = nil } - case 69: + case 71: yyDollar = yyS[yypt-6 : yypt+1] { yyVAL.typeAnnotations = append(yyDollar[1].typeAnnotations, &ast.Annotation{Name: yyDollar[3].str, Value: yyDollar[5].str, Line: yyDollar[2].line}) } - case 70: + case 72: yyDollar = yyS[yypt-4 : yypt+1] { yyVAL.typeAnnotations = append(yyDollar[1].typeAnnotations, &ast.Annotation{Name: yyDollar[3].str, Line: yyDollar[2].line}) } - case 71: + case 73: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.line = yylex.(*lexer).line } - case 72: + case 74: yyDollar = yyS[yypt-0 : yypt+1] { yyVAL.docstring = yylex.(*lexer).LastDocstring() diff --git a/idl/parser_test.go b/idl/parser_test.go index c00e26b5..846a8ab1 100644 --- a/idl/parser_test.go +++ b/idl/parser_test.go @@ -906,6 +906,38 @@ func TestParseStruct(t *testing.T) { }, }}, }, + { + ` + struct LegacyStruct { + optional string a + optional string b = "string" + } + `, + &Program{Definitions: []Definition{ + &Struct{ + Name: "LegacyStruct", + Line: 2, + Type: StructType, + Fields: []*Field{ + { + IDUnset: true, + Name: "a", + Requiredness: Optional, + Type: BaseType{ID: StringTypeID, Line: 3}, + Line: 3, + }, + { + IDUnset: true, + Name: "b", + Requiredness: Optional, + Default: ConstantString("string"), + Type: BaseType{ID: StringTypeID, Line: 4}, + Line: 4, + }, + }, + }, + }}, + }, } assertParseCases(t, tests) @@ -1087,6 +1119,40 @@ func TestParseServices(t *testing.T) { }, }}, }, + { + ` + service LegacyService { + string legacyFunc(string a, string b) + } + `, + &Program{Definitions: []Definition{ + &Service{ + Name: "LegacyService", + Line: 2, + Functions: []*Function{ + { + Name: "legacyFunc", + Line: 3, + ReturnType: BaseType{ID: StringTypeID, Line: 3}, + Parameters: []*Field{ + { + IDUnset: true, + Name: "a", + Type: BaseType{ID: StringTypeID, Line: 3}, + Line: 3, + }, + { + IDUnset: true, + Name: "b", + Type: BaseType{ID: StringTypeID, Line: 3}, + Line: 3, + }, + }, + }, + }, + }, + }}, + }, } assertParseCases(t, tests) From 4b3602d7ee9189572b4868d2ff614acb15465b76 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 25 May 2021 17:37:27 -0700 Subject: [PATCH 05/16] CHANGELOG: Add entry for (#481) (#484) Add a changelog entry for #481. --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6ffbeb17..5dd38b60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,8 @@ 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). ## [Unreleased] -- No changes yet. +### Changed +- Support parsing struct fields without identifiers. ## [1.27.0] - 2021-05-20 ### Added From c4365248f08f0714821ed0c27d91ed984bc4dd0f Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Wed, 9 Jun 2021 13:51:17 -0700 Subject: [PATCH 06/16] idl: Return structured ParseError from idl.Parse() (#492) Previously, parse errors (`idl.internal.parseError`) weren't visible to `idl.Parse()`'s callers. This was inconvenient because it's useful to have access to (a) the full list of parse errors and (b) the line number that caused each parse error. This change introduces `idl.ParseError` which provides a list of errors and their lines. Fixes #489 --- idl/error.go | 58 ++++++++++++++++++++++++++++++++++++++++++ idl/internal/error.go | 32 +++-------------------- idl/internal/lex.go | 28 +++++++++++--------- idl/internal/lex.rl | 22 +++++++++------- idl/internal/parser.go | 4 +-- idl/parser.go | 12 ++++++--- idl/parser_test.go | 1 + 7 files changed, 102 insertions(+), 55 deletions(-) create mode 100644 idl/error.go diff --git a/idl/error.go b/idl/error.go new file mode 100644 index 00000000..7adc9d01 --- /dev/null +++ b/idl/error.go @@ -0,0 +1,58 @@ +// 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 idl + +import ( + "bytes" + "fmt" + + "go.uber.org/thriftrw/idl/internal" +) + +// ParseError is an error type listing parse errors and the positions +// that caused them. +type ParseError struct{ Lines []LineError } + +// LineError holds an error and the line number that caused it. +type LineError struct { + Line int + Err error +} + +func newParseError(errors []internal.LineError) error { + if len(errors) == 0 { + return nil + } + lines := make([]LineError, len(errors)) + for i, err := range errors { + lines[i] = LineError{Line: err.Line, Err: err.Err} + } + return &ParseError{Lines: lines} +} + +func (pe *ParseError) Error() string { + var buffer bytes.Buffer + buffer.WriteString("parse error\n") + for _, line := range pe.Lines { + buffer.WriteString(fmt.Sprintf(" line %d: %s\n", line.Line, line.Err)) + } + return buffer.String() +} diff --git a/idl/internal/error.go b/idl/internal/error.go index 6e8d4b75..81d4298f 100644 --- a/idl/internal/error.go +++ b/idl/internal/error.go @@ -20,32 +20,8 @@ package internal -import ( - "bytes" - "fmt" -) - -// parseError is an error type to keep track of any parse errors and the -// positions they occur at. -type parseError struct { - Errors map[int][]string -} - -func newParseError() parseError { - return parseError{Errors: make(map[int][]string)} -} - -func (pe parseError) add(line int, msg string) { - pe.Errors[line] = append(pe.Errors[line], msg) -} - -func (pe parseError) Error() string { - var buffer bytes.Buffer - buffer.WriteString("parse error\n") - for line, msgs := range pe.Errors { - for _, msg := range msgs { - buffer.WriteString(fmt.Sprintf(" line %d: %s\n", line, msg)) - } - } - return buffer.String() +// LineError holds a parse error and the line number that caused it. +type LineError struct { + Line int + Err error } diff --git a/idl/internal/lex.go b/idl/internal/lex.go index 6fcd4055..f5ec60b1 100644 --- a/idl/internal/lex.go +++ b/idl/internal/lex.go @@ -26,6 +26,7 @@ package internal import ( + "errors" "fmt" "strconv" @@ -47,7 +48,7 @@ type lexer struct { lastDocstring string linesSinceDocstring int - err parseError + errors []LineError parseFailed bool // Ragel: @@ -58,7 +59,6 @@ type lexer struct { func newLexer(data []byte) *lexer { lex := &lexer{ line: 1, - err: newParseError(), parseFailed: false, data: data, p: 0, @@ -936,7 +936,7 @@ func (lex *lexer) Lex(out *yySymType) int { } if err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.str = str tok = LITERAL @@ -1263,7 +1263,7 @@ func (lex *lexer) Lex(out *yySymType) int { } if i64, err := strconv.ParseInt(str, base, 64); err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.i64 = i64 tok = INTCONSTANT @@ -1280,7 +1280,7 @@ func (lex *lexer) Lex(out *yySymType) int { str := string(lex.data[lex.ts:lex.te]) if dub, err := strconv.ParseFloat(str, 64); err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.dub = dub tok = DUBCONSTANT @@ -1295,7 +1295,7 @@ func (lex *lexer) Lex(out *yySymType) int { { (lex.p) = (lex.te) - 1 - lex.Error(fmt.Sprintf("%q is a reserved keyword", reservedKeyword)) + lex.AppendError(fmt.Errorf("%q is a reserved keyword", reservedKeyword)) { (lex.p)++ lex.cs = 19 @@ -1344,7 +1344,7 @@ func (lex *lexer) Lex(out *yySymType) int { } if i64, err := strconv.ParseInt(str, base, 64); err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.i64 = i64 tok = INTCONSTANT @@ -1396,7 +1396,7 @@ func (lex *lexer) Lex(out *yySymType) int { } if i64, err := strconv.ParseInt(str, base, 64); err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.i64 = i64 tok = INTCONSTANT @@ -1414,7 +1414,7 @@ func (lex *lexer) Lex(out *yySymType) int { { str := string(lex.data[lex.ts:lex.te]) if dub, err := strconv.ParseFloat(str, 64); err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.dub = dub tok = DUBCONSTANT @@ -1448,7 +1448,7 @@ func (lex *lexer) Lex(out *yySymType) int { lex.te = (lex.p) (lex.p)-- { - lex.Error(fmt.Sprintf("%q is a reserved keyword", reservedKeyword)) + lex.AppendError(fmt.Errorf("%q is a reserved keyword", reservedKeyword)) { (lex.p)++ lex.cs = 19 @@ -16594,14 +16594,18 @@ func (lex *lexer) Lex(out *yySymType) int { if lex.cs == thrift_error { - lex.Error(fmt.Sprintf("unknown token at index %d", lex.p)) + lex.AppendError(fmt.Errorf("unknown token at index %d", lex.p)) } return tok } func (lex *lexer) Error(e string) { + lex.AppendError(errors.New(e)) +} + +func (lex *lexer) AppendError(err error) { lex.parseFailed = true - lex.err.add(lex.line, e) + lex.errors = append(lex.errors, LineError{Line: lex.line, Err: err}) } func (lex *lexer) LastDocstring() string { diff --git a/idl/internal/lex.rl b/idl/internal/lex.rl index e78011e1..6c07f631 100644 --- a/idl/internal/lex.rl +++ b/idl/internal/lex.rl @@ -3,6 +3,7 @@ package internal import ( + "errors" "fmt" "strconv" @@ -27,7 +28,7 @@ type lexer struct { lastDocstring string linesSinceDocstring int - err parseError + errors []LineError parseFailed bool // Ragel: @@ -39,7 +40,6 @@ type lexer struct { func newLexer(data []byte) *lexer { lex := &lexer{ line: 1, - err: newParseError(), parseFailed: false, data: data, p: 0, @@ -281,7 +281,7 @@ func (lex *lexer) Lex(out *yySymType) int { } if i64, err := strconv.ParseInt(str, base, 64); err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.i64 = i64 tok = INTCONSTANT @@ -292,7 +292,7 @@ func (lex *lexer) Lex(out *yySymType) int { double => { str := string(lex.data[lex.ts:lex.te]) if dub, err := strconv.ParseFloat(str, 64); err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.dub = dub tok = DUBCONSTANT @@ -312,7 +312,7 @@ func (lex *lexer) Lex(out *yySymType) int { } if err != nil { - lex.Error(err.Error()) + lex.AppendError(err) } else { out.str = str tok = LITERAL @@ -322,7 +322,7 @@ func (lex *lexer) Lex(out *yySymType) int { }; reservedKeyword __ => { - lex.Error(fmt.Sprintf("%q is a reserved keyword", reservedKeyword)) + lex.AppendError(fmt.Errorf("%q is a reserved keyword", reservedKeyword)) fbreak; }; @@ -338,14 +338,18 @@ func (lex *lexer) Lex(out *yySymType) int { }%% if lex.cs == thrift_error { - lex.Error(fmt.Sprintf("unknown token at index %d", lex.p)) + lex.AppendError(fmt.Errorf("unknown token at index %d", lex.p)) } return tok } func (lex *lexer) Error(e string) { - lex.parseFailed = true - lex.err.add(lex.line, e) + lex.AppendError(errors.New(e)) +} + +func (lex *lexer) AppendError(err error) { + lex.parseFailed = true + lex.errors = append(lex.errors, LineError{Line: lex.line, Err: err}) } func (lex *lexer) LastDocstring() string { diff --git a/idl/internal/parser.go b/idl/internal/parser.go index 931c2a50..70a79f3c 100644 --- a/idl/internal/parser.go +++ b/idl/internal/parser.go @@ -27,13 +27,13 @@ func init() { } // Parse parses the given Thrift document. -func Parse(s []byte) (*ast.Program, error) { +func Parse(s []byte) (*ast.Program, []LineError) { lex := newLexer(s) e := yyParse(lex) if e == 0 && !lex.parseFailed { return lex.program, nil } - return nil, lex.err + return nil, lex.errors } //go:generate ragel -Z -G2 -o lex.go lex.rl diff --git a/idl/parser.go b/idl/parser.go index e3a520b9..a8bd6afe 100644 --- a/idl/parser.go +++ b/idl/parser.go @@ -21,10 +21,14 @@ // Package idl provides a parser for Thrift IDL files. package idl -import "go.uber.org/thriftrw/ast" -import "go.uber.org/thriftrw/idl/internal" +import ( + "go.uber.org/thriftrw/ast" + "go.uber.org/thriftrw/idl/internal" +) -// Parse parses a Thrift document. +// Parse parses a Thrift document. If there is an error, it will be of type +// *ParseError. func Parse(s []byte) (*ast.Program, error) { - return internal.Parse(s) + prog, errors := internal.Parse(s) + return prog, newParseError(errors) } diff --git a/idl/parser_test.go b/idl/parser_test.go index 846a8ab1..b70efd00 100644 --- a/idl/parser_test.go +++ b/idl/parser_test.go @@ -153,6 +153,7 @@ func TestParseErrors(t *testing.T) { for _, tt := range tests { _, err := Parse([]byte(tt.give)) if assert.Error(t, err, "expected error while parsing:\n%s", tt.give) { + assert.IsType(t, &ParseError{}, err) for _, msg := range tt.wantErrors { assert.Contains(t, err.Error(), msg, "error for %q must contain %q", tt.give, err.Error(), msg) } From 056435c24a97d7886b0321e04515cb21f1981ca8 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Thu, 10 Jun 2021 15:42:00 -0700 Subject: [PATCH 07/16] Add CHANGELOG entry for #492 (#494) --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dd38b60..b1cf1f6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ 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). ## [Unreleased] +### Added +- idl.Parse() now returns structured ParseError upon error. + ### Changed - Support parsing struct fields without identifiers. From 077b7b59eb37ffafb50032e0c6c0b7eda89717eb Mon Sep 17 00:00:00 2001 From: Wit Riewrangboonya Date: Wed, 16 Jun 2021 16:35:23 -0500 Subject: [PATCH 08/16] Support "<" in the templating language (#499) The templating language used for code generation uses `<` and `>` as its delimiters. This results in syntax errors when trying to use the beginning delimiter as an actual character, not as invoking a template. Add an explicit command (``) to inject `<` as needed. --- gen/generator.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gen/generator.go b/gen/generator.go index ea02b9dc..ccae5fbc 100644 --- a/gen/generator.go +++ b/gen/generator.go @@ -215,6 +215,7 @@ func (g *generator) LookupConstantName(c *compile.Constant) (string, error) { // TextTemplate renders the given template with the given template context. func (g *generator) TextTemplate(s string, data interface{}, opts ...TemplateOption) (string, error) { templateFuncs := template.FuncMap{ + "lessthan": lessThanSymbol, "enumItemName": enumItemName, "formatDoc": formatDoc, "goCase": goCase, @@ -539,3 +540,7 @@ func formatDoc(s string) string { } return strings.Join(lines, "\n") + "\n" } + +func lessThanSymbol() string { + return "<" +} From 060bb0f70163ce048417d2f963d9a4e6daee65cd Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Tue, 22 Jun 2021 10:07:42 -0700 Subject: [PATCH 09/16] idl: add a Position struct to wrap reported lines (#497) Line-based data types are too limiting should we add support for other positional information, such as offsets/columns. --- idl/error.go | 25 ++++++++++++++----------- idl/internal/error.go | 8 ++++---- idl/internal/lex.go | 4 ++-- idl/internal/lex.rl | 4 ++-- idl/internal/parser.go | 2 +- idl/internal/position.go | 26 ++++++++++++++++++++++++++ idl/position.go | 26 ++++++++++++++++++++++++++ 7 files changed, 75 insertions(+), 20 deletions(-) create mode 100644 idl/internal/position.go create mode 100644 idl/position.go diff --git a/idl/error.go b/idl/error.go index 7adc9d01..7c1a083e 100644 --- a/idl/error.go +++ b/idl/error.go @@ -29,30 +29,33 @@ import ( // ParseError is an error type listing parse errors and the positions // that caused them. -type ParseError struct{ Lines []LineError } +type ParseError struct{ Errors []Error } -// LineError holds an error and the line number that caused it. -type LineError struct { - Line int - Err error +// Error holds an error and the position that caused it. +type Error struct { + Pos Position + Err error } -func newParseError(errors []internal.LineError) error { +func newParseError(errors []internal.ParseError) error { if len(errors) == 0 { return nil } - lines := make([]LineError, len(errors)) + errs := make([]Error, len(errors)) for i, err := range errors { - lines[i] = LineError{Line: err.Line, Err: err.Err} + errs[i] = Error{ + Pos: Position{Line: err.Pos.Line}, + Err: err.Err, + } } - return &ParseError{Lines: lines} + return &ParseError{Errors: errs} } func (pe *ParseError) Error() string { var buffer bytes.Buffer buffer.WriteString("parse error\n") - for _, line := range pe.Lines { - buffer.WriteString(fmt.Sprintf(" line %d: %s\n", line.Line, line.Err)) + for _, pe := range pe.Errors { + buffer.WriteString(fmt.Sprintf(" line %d: %s\n", pe.Pos.Line, pe.Err)) } return buffer.String() } diff --git a/idl/internal/error.go b/idl/internal/error.go index 81d4298f..76301fcf 100644 --- a/idl/internal/error.go +++ b/idl/internal/error.go @@ -20,8 +20,8 @@ package internal -// LineError holds a parse error and the line number that caused it. -type LineError struct { - Line int - Err error +// ParseError holds a parse error and the position that caused it. +type ParseError struct { + Pos Position + Err error } diff --git a/idl/internal/lex.go b/idl/internal/lex.go index f5ec60b1..a3809a9d 100644 --- a/idl/internal/lex.go +++ b/idl/internal/lex.go @@ -48,7 +48,7 @@ type lexer struct { lastDocstring string linesSinceDocstring int - errors []LineError + errors []ParseError parseFailed bool // Ragel: @@ -16605,7 +16605,7 @@ func (lex *lexer) Error(e string) { func (lex *lexer) AppendError(err error) { lex.parseFailed = true - lex.errors = append(lex.errors, LineError{Line: lex.line, Err: err}) + lex.errors = append(lex.errors, ParseError{Pos: Position{Line: lex.line}, Err: err}) } func (lex *lexer) LastDocstring() string { diff --git a/idl/internal/lex.rl b/idl/internal/lex.rl index 6c07f631..4d91f357 100644 --- a/idl/internal/lex.rl +++ b/idl/internal/lex.rl @@ -28,7 +28,7 @@ type lexer struct { lastDocstring string linesSinceDocstring int - errors []LineError + errors []ParseError parseFailed bool // Ragel: @@ -349,7 +349,7 @@ func (lex *lexer) Error(e string) { func (lex *lexer) AppendError(err error) { lex.parseFailed = true - lex.errors = append(lex.errors, LineError{Line: lex.line, Err: err}) + lex.errors = append(lex.errors, ParseError{Pos: Position{Line: lex.line}, Err: err}) } func (lex *lexer) LastDocstring() string { diff --git a/idl/internal/parser.go b/idl/internal/parser.go index 70a79f3c..6fd7e3e2 100644 --- a/idl/internal/parser.go +++ b/idl/internal/parser.go @@ -27,7 +27,7 @@ func init() { } // Parse parses the given Thrift document. -func Parse(s []byte) (*ast.Program, []LineError) { +func Parse(s []byte) (*ast.Program, []ParseError) { lex := newLexer(s) e := yyParse(lex) if e == 0 && !lex.parseFailed { diff --git a/idl/internal/position.go b/idl/internal/position.go new file mode 100644 index 00000000..dc285714 --- /dev/null +++ b/idl/internal/position.go @@ -0,0 +1,26 @@ +// 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 internal + +// Position represents a position in the parsed document. +type Position struct { + Line int +} diff --git a/idl/position.go b/idl/position.go new file mode 100644 index 00000000..6d246680 --- /dev/null +++ b/idl/position.go @@ -0,0 +1,26 @@ +// 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 idl + +// Position represents a position in the parsed document. +type Position struct { + Line int +} From b59dc507759258b1dfa4112c5bc9cca8733c0f52 Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Fri, 25 Jun 2021 09:34:06 -0700 Subject: [PATCH 10/16] idl: record document positions on constant nodes (#503) This change does two things: First, it records document positions for parsed nodes that cannot store their own position data. This primarily applies to primitive constants. Second, it makes this information available through a new Config-based parsing interface using an optional idl.Info structure. Node positions are available though idl.Info.Pos(), which first attempts to return a node's "native" position (line) value before falling back to its internal nodePositions map. Closes #493 --- idl/config.go | 42 ++++++++++++++++++++++++++++ idl/config_test.go | 46 +++++++++++++++++++++++++++++++ idl/info.go | 40 +++++++++++++++++++++++++++ idl/info_test.go | 59 ++++++++++++++++++++++++++++++++++++++++ idl/internal/lex.go | 16 +++++++---- idl/internal/lex.rl | 6 ++++ idl/internal/parser.go | 15 ++++++++-- idl/internal/position.go | 5 ++++ idl/internal/thrift.y | 11 ++++---- idl/internal/y.go | 5 ++++ idl/parser.go | 4 +-- 11 files changed, 233 insertions(+), 16 deletions(-) create mode 100644 idl/config.go create mode 100644 idl/config_test.go create mode 100644 idl/info.go create mode 100644 idl/info_test.go diff --git a/idl/config.go b/idl/config.go new file mode 100644 index 00000000..91b9208d --- /dev/null +++ b/idl/config.go @@ -0,0 +1,42 @@ +// 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 idl + +import ( + "go.uber.org/thriftrw/ast" + "go.uber.org/thriftrw/idl/internal" +) + +// Config configures the Thrift IDL parser. +type Config struct { + // If Info is non-nil, it will be populated with information about the + // parsed nodes. + Info *Info +} + +// Parse parses the given Thrift document. +func (c *Config) Parse(s []byte) (*ast.Program, error) { + result, errors := internal.Parse(s) + if c.Info != nil { + c.Info.nodePositions = result.NodePositions + } + return result.Program, newParseError(errors) +} diff --git a/idl/config_test.go b/idl/config_test.go new file mode 100644 index 00000000..2b3e1720 --- /dev/null +++ b/idl/config_test.go @@ -0,0 +1,46 @@ +// 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 idl + +import ( + "testing" + + "go.uber.org/thriftrw/ast" + + "github.com/stretchr/testify/assert" +) + +func TestParse(t *testing.T) { + c := &Config{} + prog, err := c.Parse([]byte{}) + if assert.NoError(t, err) { + assert.Equal(t, &ast.Program{}, prog) + } +} + +func TestInfoPos(t *testing.T) { + c := &Config{Info: &Info{}} + prog, err := c.Parse([]byte(`const string a = 'a';`)) + if assert.NoError(t, err, "%v", err) { + assert.Equal(t, Position{Line: 0}, c.Info.Pos(prog)) + assert.Equal(t, Position{Line: 1}, c.Info.Pos(prog.Definitions[0])) + } +} diff --git a/idl/info.go b/idl/info.go new file mode 100644 index 00000000..97cc7917 --- /dev/null +++ b/idl/info.go @@ -0,0 +1,40 @@ +// 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 idl + +import ( + "go.uber.org/thriftrw/ast" + "go.uber.org/thriftrw/idl/internal" +) + +// Info contains additional information about the parsed document. +type Info struct { + nodePositions internal.NodePositions +} + +// Pos returns a node's position in the parsed document. +func (i *Info) Pos(n ast.Node) Position { + if line := ast.LineNumber(n); line != 0 { + return Position{Line: line} + } + pos := i.nodePositions[n] + return Position{Line: pos.Line} +} diff --git a/idl/info_test.go b/idl/info_test.go new file mode 100644 index 00000000..f915de59 --- /dev/null +++ b/idl/info_test.go @@ -0,0 +1,59 @@ +// 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 idl + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "go.uber.org/thriftrw/ast" + "go.uber.org/thriftrw/idl/internal" +) + +func TestPos(t *testing.T) { + tests := []struct { + node ast.Node + pos *internal.Position + want Position + }{ + { + node: &ast.Struct{Line: 10}, + want: Position{Line: 10}, + }, + { + node: ast.ConstantString("s"), + want: Position{Line: 0}, + }, + { + node: ast.ConstantString("s"), + pos: &internal.Position{Line: 1}, + want: Position{Line: 1}, + }, + } + + for _, tt := range tests { + i := &Info{} + if tt.pos != nil { + i.nodePositions = internal.NodePositions{tt.node: *tt.pos} + } + assert.Equal(t, tt.want, i.Pos(tt.node)) + } +} diff --git a/idl/internal/lex.go b/idl/internal/lex.go index a3809a9d..f3e5e0ff 100644 --- a/idl/internal/lex.go +++ b/idl/internal/lex.go @@ -47,6 +47,7 @@ type lexer struct { docstringStart int lastDocstring string linesSinceDocstring int + nodePositions NodePositions errors []ParseError parseFailed bool @@ -58,11 +59,12 @@ type lexer struct { func newLexer(data []byte) *lexer { lex := &lexer{ - line: 1, - parseFailed: false, - data: data, - p: 0, - pe: len(data), + line: 1, + nodePositions: make(NodePositions, 0), + parseFailed: false, + data: data, + p: 0, + pe: len(data), } { @@ -16608,6 +16610,10 @@ func (lex *lexer) AppendError(err error) { lex.errors = append(lex.errors, ParseError{Pos: Position{Line: lex.line}, Err: err}) } +func (lex *lexer) RecordPosition(n ast.Node) { + lex.nodePositions[n] = Position{Line: lex.line} +} + func (lex *lexer) LastDocstring() string { // If we've had more than one line since we recorded // the docstring, ignore it. diff --git a/idl/internal/lex.rl b/idl/internal/lex.rl index 4d91f357..62b09389 100644 --- a/idl/internal/lex.rl +++ b/idl/internal/lex.rl @@ -27,6 +27,7 @@ type lexer struct { docstringStart int lastDocstring string linesSinceDocstring int + nodePositions NodePositions errors []ParseError parseFailed bool @@ -40,6 +41,7 @@ type lexer struct { func newLexer(data []byte) *lexer { lex := &lexer{ line: 1, + nodePositions: make(NodePositions, 0), parseFailed: false, data: data, p: 0, @@ -352,6 +354,10 @@ func (lex *lexer) AppendError(err error) { lex.errors = append(lex.errors, ParseError{Pos: Position{Line: lex.line}, Err: err}) } +func (lex* lexer) RecordPosition(n ast.Node) { + lex.nodePositions[n] = Position{Line: lex.line} +} + func (lex *lexer) LastDocstring() string { // If we've had more than one line since we recorded // the docstring, ignore it. diff --git a/idl/internal/parser.go b/idl/internal/parser.go index 6fd7e3e2..3ca156e0 100644 --- a/idl/internal/parser.go +++ b/idl/internal/parser.go @@ -26,14 +26,23 @@ func init() { yyErrorVerbose = true } +// ParseResult holds the result of a successful Parse. +type ParseResult struct { + Program *ast.Program + NodePositions NodePositions +} + // Parse parses the given Thrift document. -func Parse(s []byte) (*ast.Program, []ParseError) { +func Parse(s []byte) (ParseResult, []ParseError) { lex := newLexer(s) e := yyParse(lex) if e == 0 && !lex.parseFailed { - return lex.program, nil + return ParseResult{ + Program: lex.program, + NodePositions: lex.nodePositions, + }, nil } - return nil, lex.errors + return ParseResult{}, lex.errors } //go:generate ragel -Z -G2 -o lex.go lex.rl diff --git a/idl/internal/position.go b/idl/internal/position.go index dc285714..4dac7afb 100644 --- a/idl/internal/position.go +++ b/idl/internal/position.go @@ -20,7 +20,12 @@ package internal +import "go.uber.org/thriftrw/ast" + // Position represents a position in the parsed document. type Position struct { Line int } + +// NodePositions maps (hashable) nodes to their document positions. +type NodePositions map[ast.Node]Position diff --git a/idl/internal/thrift.y b/idl/internal/thrift.y index 015d1428..931b0d4a 100644 --- a/idl/internal/thrift.y +++ b/idl/internal/thrift.y @@ -386,13 +386,12 @@ base_type_name /*************************************************************************** Constant values ***************************************************************************/ - const_value - : INTCONSTANT { $$ = ast.ConstantInteger($1) } - | DUBCONSTANT { $$ = ast.ConstantDouble($1) } - | TRUE { $$ = ast.ConstantBoolean(true) } - | FALSE { $$ = ast.ConstantBoolean(false) } - | LITERAL { $$ = ast.ConstantString($1) } + : INTCONSTANT { $$ = ast.ConstantInteger($1); yylex.(*lexer).RecordPosition($$) } + | DUBCONSTANT { $$ = ast.ConstantDouble($1); yylex.(*lexer).RecordPosition($$) } + | TRUE { $$ = ast.ConstantBoolean(true); yylex.(*lexer).RecordPosition($$) } + | FALSE { $$ = ast.ConstantBoolean(false); yylex.(*lexer).RecordPosition($$) } + | LITERAL { $$ = ast.ConstantString($1); yylex.(*lexer).RecordPosition($$) } | lineno IDENTIFIER { $$ = ast.ConstantReference{Name: $2, Line: $1} } diff --git a/idl/internal/y.go b/idl/internal/y.go index a8f90b19..7bfedf37 100644 --- a/idl/internal/y.go +++ b/idl/internal/y.go @@ -1037,26 +1037,31 @@ yydefault: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantInteger(yyDollar[1].i64) + yylex.(*lexer).RecordPosition(yyVAL.constantValue) } case 57: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantDouble(yyDollar[1].dub) + yylex.(*lexer).RecordPosition(yyVAL.constantValue) } case 58: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantBoolean(true) + yylex.(*lexer).RecordPosition(yyVAL.constantValue) } case 59: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantBoolean(false) + yylex.(*lexer).RecordPosition(yyVAL.constantValue) } case 60: yyDollar = yyS[yypt-1 : yypt+1] { yyVAL.constantValue = ast.ConstantString(yyDollar[1].str) + yylex.(*lexer).RecordPosition(yyVAL.constantValue) } case 61: yyDollar = yyS[yypt-2 : yypt+1] diff --git a/idl/parser.go b/idl/parser.go index a8bd6afe..cddbbeb8 100644 --- a/idl/parser.go +++ b/idl/parser.go @@ -29,6 +29,6 @@ import ( // Parse parses a Thrift document. If there is an error, it will be of type // *ParseError. func Parse(s []byte) (*ast.Program, error) { - prog, errors := internal.Parse(s) - return prog, newParseError(errors) + result, errors := internal.Parse(s) + return result.Program, newParseError(errors) } From 6ccdbde1bb6accb6eb5d23369397cd96a29c6cdb Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Fri, 25 Jun 2021 12:30:54 -0700 Subject: [PATCH 11/16] ast: move idl.Position to the ast package (#504) --- {idl => ast}/position.go | 11 ++++++++++- idl/config_test.go | 6 +++--- idl/error.go | 5 +++-- idl/info.go | 10 +++++----- idl/info_test.go | 8 ++++---- 5 files changed, 25 insertions(+), 15 deletions(-) rename {idl => ast}/position.go (79%) diff --git a/idl/position.go b/ast/position.go similarity index 79% rename from idl/position.go rename to ast/position.go index 6d246680..40b90f64 100644 --- a/idl/position.go +++ b/ast/position.go @@ -18,9 +18,18 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. -package idl +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 +} diff --git a/idl/config_test.go b/idl/config_test.go index 2b3e1720..9b0ddf43 100644 --- a/idl/config_test.go +++ b/idl/config_test.go @@ -39,8 +39,8 @@ func TestParse(t *testing.T) { func TestInfoPos(t *testing.T) { c := &Config{Info: &Info{}} prog, err := c.Parse([]byte(`const string a = 'a';`)) - if assert.NoError(t, err, "%v", err) { - assert.Equal(t, Position{Line: 0}, c.Info.Pos(prog)) - assert.Equal(t, Position{Line: 1}, c.Info.Pos(prog.Definitions[0])) + if assert.NoError(t, err) { + assert.Equal(t, ast.Position{Line: 0}, c.Info.Pos(prog)) + assert.Equal(t, ast.Position{Line: 1}, c.Info.Pos(prog.Definitions[0])) } } diff --git a/idl/error.go b/idl/error.go index 7c1a083e..59ec1823 100644 --- a/idl/error.go +++ b/idl/error.go @@ -24,6 +24,7 @@ import ( "bytes" "fmt" + "go.uber.org/thriftrw/ast" "go.uber.org/thriftrw/idl/internal" ) @@ -33,7 +34,7 @@ type ParseError struct{ Errors []Error } // Error holds an error and the position that caused it. type Error struct { - Pos Position + Pos ast.Position Err error } @@ -44,7 +45,7 @@ func newParseError(errors []internal.ParseError) error { errs := make([]Error, len(errors)) for i, err := range errors { errs[i] = Error{ - Pos: Position{Line: err.Pos.Line}, + Pos: ast.Position{Line: err.Pos.Line}, Err: err.Err, } } diff --git a/idl/info.go b/idl/info.go index 97cc7917..57fe9799 100644 --- a/idl/info.go +++ b/idl/info.go @@ -30,11 +30,11 @@ type Info struct { nodePositions internal.NodePositions } -// Pos returns a node's position in the parsed document. -func (i *Info) Pos(n ast.Node) Position { - if line := ast.LineNumber(n); line != 0 { - return Position{Line: line} +// Pos returns a Node's position in the parsed document. +func (i *Info) Pos(n ast.Node) ast.Position { + if pos, ok := ast.Pos(n); ok { + return pos } pos := i.nodePositions[n] - return Position{Line: pos.Line} + return ast.Position{Line: pos.Line} } diff --git a/idl/info_test.go b/idl/info_test.go index f915de59..164256ed 100644 --- a/idl/info_test.go +++ b/idl/info_test.go @@ -32,20 +32,20 @@ func TestPos(t *testing.T) { tests := []struct { node ast.Node pos *internal.Position - want Position + want ast.Position }{ { node: &ast.Struct{Line: 10}, - want: Position{Line: 10}, + want: ast.Position{Line: 10}, }, { node: ast.ConstantString("s"), - want: Position{Line: 0}, + want: ast.Position{Line: 0}, }, { node: ast.ConstantString("s"), pos: &internal.Position{Line: 1}, - want: Position{Line: 1}, + want: ast.Position{Line: 1}, }, } From dcb28e24fe674771ffe05c37f612291355202800 Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Fri, 25 Jun 2021 14:50:37 -0700 Subject: [PATCH 12/16] idl: replace internal.Position with ast.Position (#505) With idl.Position moved to ast.Position, there's no need for a separate internal.Position type. --- idl/error.go | 2 +- idl/info_test.go | 4 ++-- idl/internal/error.go | 4 +++- idl/internal/lex.go | 4 ++-- idl/internal/lex.rl | 4 ++-- idl/internal/parser.go | 3 +++ idl/internal/position.go | 31 ------------------------------- 7 files changed, 13 insertions(+), 39 deletions(-) delete mode 100644 idl/internal/position.go diff --git a/idl/error.go b/idl/error.go index 59ec1823..994bc956 100644 --- a/idl/error.go +++ b/idl/error.go @@ -45,7 +45,7 @@ func newParseError(errors []internal.ParseError) error { errs := make([]Error, len(errors)) for i, err := range errors { errs[i] = Error{ - Pos: ast.Position{Line: err.Pos.Line}, + Pos: err.Pos, Err: err.Err, } } diff --git a/idl/info_test.go b/idl/info_test.go index 164256ed..7ea0516c 100644 --- a/idl/info_test.go +++ b/idl/info_test.go @@ -31,7 +31,7 @@ import ( func TestPos(t *testing.T) { tests := []struct { node ast.Node - pos *internal.Position + pos *ast.Position want ast.Position }{ { @@ -44,7 +44,7 @@ func TestPos(t *testing.T) { }, { node: ast.ConstantString("s"), - pos: &internal.Position{Line: 1}, + pos: &ast.Position{Line: 1}, want: ast.Position{Line: 1}, }, } diff --git a/idl/internal/error.go b/idl/internal/error.go index 76301fcf..56cb79dc 100644 --- a/idl/internal/error.go +++ b/idl/internal/error.go @@ -20,8 +20,10 @@ package internal +import "go.uber.org/thriftrw/ast" + // ParseError holds a parse error and the position that caused it. type ParseError struct { - Pos Position + Pos ast.Position Err error } diff --git a/idl/internal/lex.go b/idl/internal/lex.go index f3e5e0ff..c8fdc326 100644 --- a/idl/internal/lex.go +++ b/idl/internal/lex.go @@ -16607,11 +16607,11 @@ func (lex *lexer) Error(e string) { func (lex *lexer) AppendError(err error) { lex.parseFailed = true - lex.errors = append(lex.errors, ParseError{Pos: Position{Line: lex.line}, Err: err}) + lex.errors = append(lex.errors, ParseError{Pos: ast.Position{Line: lex.line}, Err: err}) } func (lex *lexer) RecordPosition(n ast.Node) { - lex.nodePositions[n] = Position{Line: lex.line} + lex.nodePositions[n] = ast.Position{Line: lex.line} } func (lex *lexer) LastDocstring() string { diff --git a/idl/internal/lex.rl b/idl/internal/lex.rl index 62b09389..35fa1624 100644 --- a/idl/internal/lex.rl +++ b/idl/internal/lex.rl @@ -351,11 +351,11 @@ func (lex *lexer) Error(e string) { func (lex *lexer) AppendError(err error) { lex.parseFailed = true - lex.errors = append(lex.errors, ParseError{Pos: Position{Line: lex.line}, Err: err}) + lex.errors = append(lex.errors, ParseError{Pos: ast.Position{Line: lex.line}, Err: err}) } func (lex* lexer) RecordPosition(n ast.Node) { - lex.nodePositions[n] = Position{Line: lex.line} + lex.nodePositions[n] = ast.Position{Line: lex.line} } func (lex *lexer) LastDocstring() string { diff --git a/idl/internal/parser.go b/idl/internal/parser.go index 3ca156e0..52400e6d 100644 --- a/idl/internal/parser.go +++ b/idl/internal/parser.go @@ -26,6 +26,9 @@ func init() { yyErrorVerbose = true } +// NodePositions maps (hashable) nodes to their document positions. +type NodePositions map[ast.Node]ast.Position + // ParseResult holds the result of a successful Parse. type ParseResult struct { Program *ast.Program diff --git a/idl/internal/position.go b/idl/internal/position.go deleted file mode 100644 index 4dac7afb..00000000 --- a/idl/internal/position.go +++ /dev/null @@ -1,31 +0,0 @@ -// 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 internal - -import "go.uber.org/thriftrw/ast" - -// Position represents a position in the parsed document. -type Position struct { - Line int -} - -// NodePositions maps (hashable) nodes to their document positions. -type NodePositions map[ast.Node]Position From c4ea837a99c3feb28a2ef0324c114c9792c46da8 Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Wed, 30 Jun 2021 11:03:41 -0700 Subject: [PATCH 13/16] Mark assertParseCases() as a test helper (#509) This improves stack-level reports for test failures more accurate. --- idl/parser_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/idl/parser_test.go b/idl/parser_test.go index b70efd00..1839bfb9 100644 --- a/idl/parser_test.go +++ b/idl/parser_test.go @@ -36,6 +36,7 @@ type parseCase struct { } func assertParseCases(t *testing.T, tests []parseCase) { + t.Helper() for _, tt := range tests { program, err := Parse([]byte(tt.document)) if assert.NoError(t, err, "Parsing failed:\n%s", tt.document) { From 5d168080c7c6068c354e90228d7763506f006c30 Mon Sep 17 00:00:00 2001 From: Jon Parise Date: Mon, 12 Jul 2021 13:50:39 -0400 Subject: [PATCH 14/16] Upgrade to Ragel version 6.10 (from 6.9) (#523) This doesn't change anything in current practice aside from moving to the latest stable version of Ragel. From the changelog: Ragel 6.10 - Mar 24, 2017 ========================= -C codegen: test P vs PE in goto/call/ret statements in EOF actions, just before re-entering. If at the end of the input block then the EOF check is jumped to. This change prevents overrunning the buffer if control flow is issued in an EOF action without fixing the input pointer first. If a program properly issues an fhold before the control flow the program won't be affected. -Updated action label generation. The previous set of conditions for generating the label didn't cover actions coming from the eofAction pointer (eof trans covered since it points into the set of transitions). -Use separate signed/unsigned values for host type min/max. Using separate values avoids the need to type cast before the data goes into FsmCtx structs. Keep it in native types until it is used. -Optionally do not generate entry point variables. Adds noentry write option for data. -Various warning elimination and build updates. --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 900950b3..3da99c19 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,5 @@ BUILD_DIR = $(shell pwd)/build -RAGEL_VERSION = 6.9 +RAGEL_VERSION = 6.10 export GOBIN = $(BUILD_DIR)/bin From ea2a93d839246e9977c8acf4259243f2a1f739f3 Mon Sep 17 00:00:00 2001 From: aloksinghaluber <72428224+aloksinghaluber@users.noreply.github.com> Date: Sat, 24 Jul 2021 02:22:36 +0530 Subject: [PATCH 15/16] enum/json: Support rejecting unknown values (#502) Add a new hidden flag `--enum-text-marshal-strict` that will cause the MarshalText and MarshalJSON methods methods for enums to fail with an error if the enum being serialized is unknown. This change is necessary because some clients crash during serialization of JSON or YAML if they receive an integer instead of a string (for unknown enums). Refs EDGE-577 Co-authored-by: Abhinav Gupta --- gen/enum.go | 32 ++- gen/enum_test.go | 55 ++++- gen/generate.go | 13 +- gen/generate_test.go | 13 +- gen/generator.go | 30 ++- gen/golden_test.go | 16 +- gen/internal/tests/Makefile | 3 + gen/internal/tests/collision/collision.go | 16 +- .../enum-text-marshal-strict.go | 223 ++++++++++++++++++ .../tests/enum_conflict/enum_conflict.go | 8 +- gen/internal/tests/enums/enums.go | 72 +++--- gen/internal/tests/nozap/nozap.go | 8 +- .../thrift/enum-text-marshal-strict.thrift | 3 + gen/quick_test.go | 64 +++-- main.go | 42 ++-- 15 files changed, 471 insertions(+), 127 deletions(-) create mode 100644 gen/internal/tests/enum-text-marshal-strict/enum-text-marshal-strict.go create mode 100644 gen/internal/tests/thrift/enum-text-marshal-strict.thrift diff --git a/gen/enum.go b/gen/enum.go index 1fd10c9f..b229004d 100644 --- a/gen/enum.go +++ b/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 default: - <$val>, err := <$strconv>.ParseInt(<$s>, 10, 32) + <$val>, err := .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. + + // Otherwise, an error is returned. + + // Otherwise, its integer value is returned. + // // This implements the TextMarshaler interface. func (<$v> <$enumName>) MarshalText() ([]byte, error) { @@ -134,7 +137,11 @@ func enum(g Generator, spec *compile.EnumSpec) error { } - return []byte(<$strconv>.FormatInt(int64(<$v>), 10)), nil + + return nil, <$fmt>.Errorf("unknown enum value %q for %q", <$v>, "<$enumName>") + + return []byte(.FormatInt(int64(<$v>), 10)), nil + } @@ -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. + + // Otherwise, an error is returned. + + // Otherwise, its integer value is returned. + // // This implements json.Marshaler. func (<$v> <$enumName>) MarshalJSON() ([]byte, error) { @@ -226,7 +237,11 @@ func enum(g Generator, spec *compile.EnumSpec) error { } - return ([]byte)(<$strconv>.FormatInt(int64(<$v>), 10)), nil + + return nil, <$fmt>.Errorf("unknown enum value %q for %q", <$v>, "<$enumName>") + + return ([]byte)(.FormatInt(int64(<$v>), 10)), nil + } <$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) diff --git a/gen/enum_test.go b/gen/enum_test.go index 93f41b09..b67e331b 100644 --- a/gen/enum_test.go +++ b/gen/enum_test.go @@ -25,13 +25,13 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.uber.org/thriftrw/compile" + tems "go.uber.org/thriftrw/gen/internal/tests/enum-text-marshal-strict" tec "go.uber.org/thriftrw/gen/internal/tests/enum_conflict" te "go.uber.org/thriftrw/gen/internal/tests/enums" "go.uber.org/thriftrw/wire" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func TestValueOfEnumDefault(t *testing.T) { @@ -501,6 +501,55 @@ func TestEnumLabelValid(t *testing.T) { } } +func TestEnumStrictMarshalText(t *testing.T) { + tests := []struct { + give tems.EnumMarshalStrict + wantText string + wantJSON string + wantErr string + }{ + { + give: tems.EnumMarshalStrictFoo, + wantText: "Foo", + wantJSON: `"Foo"`, + }, + { + give: tems.EnumMarshalStrictBar, + wantText: "Bar", + wantJSON: `"Bar"`, + }, + { + give: tems.EnumMarshalStrictBaz, + wantText: "Baz", + wantJSON: `"Baz"`, + }, + { + give: tems.EnumMarshalStrictBat, + wantText: "Bat", + wantJSON: `"Bat"`, + }, + { + give: 5, + wantErr: `unknown enum value "EnumMarshalStrict(5)" for "EnumMarshalStrict"`, + }, + } + for _, tt := range tests { + t.Run(fmt.Sprint(tt.give), func(t *testing.T) { + gotText, err := tt.give.MarshalText() + gotJSON, jsonErr := tt.give.MarshalJSON() + if len(tt.wantErr) > 0 { + require.Error(t, err) + assert.Equal(t, tt.wantErr, err.Error()) + assert.Equal(t, tt.wantErr, jsonErr.Error()) + } else { + require.NoError(t, err) + assert.Equal(t, tt.wantText, string(gotText)) + assert.Equal(t, tt.wantJSON, string(gotJSON)) + } + }) + } +} + func TestEnumLabelInvalidUnmarshal(t *testing.T) { tests := []struct { errVal string diff --git a/gen/generate.go b/gen/generate.go index 65f08dee..dcc72a0c 100644 --- a/gen/generate.go +++ b/gen/generate.go @@ -85,6 +85,10 @@ type Options struct { // Name of the file to be generated by ThriftRW. OutputFile string + + // Generates an error on MarshalText and MarshalJSON if the enum value is + // unrecognized. + EnumTextMarshalStrict bool } // Generate generates code based on the given options. @@ -271,10 +275,11 @@ func generateModule( // converts package name from ab-def to ab_def for golang code generation normalizedPackageName := normalizePackageName(filepath.Base(packageRelPath)) g := NewGenerator(&GeneratorOptions{ - Importer: i, - ImportPath: importPath, - PackageName: normalizedPackageName, - NoZap: o.NoZap, + Importer: i, + ImportPath: importPath, + PackageName: normalizedPackageName, + NoZap: o.NoZap, + EnumTextMarshalStrict: o.EnumTextMarshalStrict, }) if len(m.Constants) > 0 { diff --git a/gen/generate_test.go b/gen/generate_test.go index 84079643..5da6b286 100644 --- a/gen/generate_test.go +++ b/gen/generate_test.go @@ -324,12 +324,13 @@ func TestGenerate(t *testing.T) { } err = Generate(module, &Options{ - OutputDir: outputDir, - PackagePrefix: "go.uber.org/thriftrw/gen/internal/tests", - ThriftRoot: testdata(t, "thrift"), - Plugin: p, - NoRecurse: tt.noRecurse, - OutputFile: tt.outputFile, + OutputDir: outputDir, + PackagePrefix: "go.uber.org/thriftrw/gen/internal/tests", + ThriftRoot: testdata(t, "thrift"), + Plugin: p, + NoRecurse: tt.noRecurse, + OutputFile: tt.outputFile, + EnumTextMarshalStrict: true, }) if tt.wantError != "" { assert.Contains(t, err.Error(), tt.wantError) diff --git a/gen/generator.go b/gen/generator.go index ccae5fbc..08b9c2fc 100644 --- a/gen/generator.go +++ b/gen/generator.go @@ -134,7 +134,8 @@ type generator struct { thriftImporter ThriftPackageImporter mangler *mangler - fset *token.FileSet + fset *token.FileSet + enumTextMarshalStrict bool // TODO use something to group related decls together } @@ -145,7 +146,8 @@ type GeneratorOptions struct { ImportPath string PackageName string - NoZap bool + NoZap bool + EnumTextMarshalStrict bool } // NewGenerator sets up a new generator for Go code. @@ -153,14 +155,15 @@ func NewGenerator(o *GeneratorOptions) Generator { // TODO(abg): Determine package name from `namespace go` directive. namespace := NewNamespace() return &generator{ - PackageName: o.PackageName, - ImportPath: o.ImportPath, - Namespace: namespace, - importer: newImporter(namespace.Child()), - mangler: newMangler(), - thriftImporter: o.Importer, - fset: token.NewFileSet(), - noZap: o.NoZap, + PackageName: o.PackageName, + ImportPath: o.ImportPath, + Namespace: namespace, + importer: newImporter(namespace.Child()), + mangler: newMangler(), + thriftImporter: o.Importer, + fset: token.NewFileSet(), + noZap: o.NoZap, + enumTextMarshalStrict: o.EnumTextMarshalStrict, } } @@ -172,6 +175,13 @@ func checkNoZap(g Generator) bool { return false } +func checkEnumTextMarshalStrict(g Generator) bool { + if gen, ok := g.(*generator); ok { + return gen.enumTextMarshalStrict + } + return false +} + func (g *generator) MangleType(t compile.TypeSpec) string { return g.mangler.MangleType(t) } diff --git a/gen/golden_test.go b/gen/golden_test.go index d1a85c8d..0d644d17 100644 --- a/gen/golden_test.go +++ b/gen/golden_test.go @@ -42,6 +42,10 @@ var noZapFiles = map[string]struct{}{ "nozap": {}, } +var enumTextMarshalStrictFiles = map[string]struct{}{ + "enum-text-marshal-strict": {}, +} + func TestCodeIsUpToDate(t *testing.T) { // This test just verifies that the generated code in internal/tests/ is up to // date. If this test failed, run 'make' in the internal/tests/ directory and @@ -69,12 +73,14 @@ func TestCodeIsUpToDate(t *testing.T) { require.NoError(t, err, "failed to compile %q", thriftFile) _, nozap := noZapFiles[pkgRelPath] + _, enumTextMarshalStrict := enumTextMarshalStrictFiles[pkgRelPath] err = Generate(module, &Options{ - OutputDir: outputDir, - PackagePrefix: "go.uber.org/thriftrw/gen/internal/tests", - ThriftRoot: thriftRoot, - NoRecurse: true, - NoZap: nozap, + OutputDir: outputDir, + PackagePrefix: "go.uber.org/thriftrw/gen/internal/tests", + ThriftRoot: thriftRoot, + NoRecurse: true, + NoZap: nozap, + EnumTextMarshalStrict: enumTextMarshalStrict, }) require.NoError(t, err, "failed to generate code for %q", thriftFile) diff --git a/gen/internal/tests/Makefile b/gen/internal/tests/Makefile index c474856f..f7bf316d 100644 --- a/gen/internal/tests/Makefile +++ b/gen/internal/tests/Makefile @@ -19,5 +19,8 @@ $(THRIFTRW): nozap: thrift/nozap.thrift $(THRIFTRW) $(THRIFTRW) $(THRIFTRW_FLAGS) --no-recurse --no-zap $< +enum-text-marshal-strict: thrift/enum-text-marshal-strict.thrift $(THRIFTRW) + $(THRIFTRW) $(THRIFTRW_FLAGS) --no-recurse --enum-text-marshal-strict $< + %: thrift/%.thrift $(THRIFTRW) $(THRIFTRW) $(THRIFTRW_FLAGS) $< diff --git a/gen/internal/tests/collision/collision.go b/gen/internal/tests/collision/collision.go index 277acaf1..6bff45bf 100644 --- a/gen/internal/tests/collision/collision.go +++ b/gen/internal/tests/collision/collision.go @@ -541,8 +541,8 @@ func (v *MyEnum) UnmarshalText(value []byte) error { // MarshalText encodes MyEnum 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v MyEnum) MarshalText() ([]byte, error) { @@ -640,8 +640,8 @@ func (v MyEnum) Equals(rhs MyEnum) bool { // MarshalJSON serializes MyEnum 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v MyEnum) MarshalJSON() ([]byte, error) { @@ -1761,8 +1761,8 @@ func (v *MyEnum2) UnmarshalText(value []byte) error { // MarshalText encodes MyEnum2 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v MyEnum2) MarshalText() ([]byte, error) { @@ -1848,8 +1848,8 @@ func (v MyEnum2) Equals(rhs MyEnum2) bool { // MarshalJSON serializes MyEnum2 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v MyEnum2) MarshalJSON() ([]byte, error) { diff --git a/gen/internal/tests/enum-text-marshal-strict/enum-text-marshal-strict.go b/gen/internal/tests/enum-text-marshal-strict/enum-text-marshal-strict.go new file mode 100644 index 00000000..66a56c7a --- /dev/null +++ b/gen/internal/tests/enum-text-marshal-strict/enum-text-marshal-strict.go @@ -0,0 +1,223 @@ +// Code generated by thriftrw v1.28.0. DO NOT EDIT. +// @generated + +package enum_text_marshal_strict + +import ( + bytes "bytes" + json "encoding/json" + fmt "fmt" + thriftreflect "go.uber.org/thriftrw/thriftreflect" + wire "go.uber.org/thriftrw/wire" + zapcore "go.uber.org/zap/zapcore" + math "math" + strconv "strconv" +) + +type EnumMarshalStrict int32 + +const ( + EnumMarshalStrictFoo EnumMarshalStrict = 0 + EnumMarshalStrictBar EnumMarshalStrict = 1 + EnumMarshalStrictBaz EnumMarshalStrict = 2 + EnumMarshalStrictBat EnumMarshalStrict = 3 +) + +// EnumMarshalStrict_Values returns all recognized values of EnumMarshalStrict. +func EnumMarshalStrict_Values() []EnumMarshalStrict { + return []EnumMarshalStrict{ + EnumMarshalStrictFoo, + EnumMarshalStrictBar, + EnumMarshalStrictBaz, + EnumMarshalStrictBat, + } +} + +// UnmarshalText tries to decode EnumMarshalStrict from a byte slice +// containing its name. +// +// var v EnumMarshalStrict +// err := v.UnmarshalText([]byte("Foo")) +func (v *EnumMarshalStrict) UnmarshalText(value []byte) error { + switch s := string(value); s { + case "Foo": + *v = EnumMarshalStrictFoo + return nil + case "Bar": + *v = EnumMarshalStrictBar + return nil + case "Baz": + *v = EnumMarshalStrictBaz + return nil + case "Bat": + *v = EnumMarshalStrictBat + return nil + default: + val, err := strconv.ParseInt(s, 10, 32) + if err != nil { + return fmt.Errorf("unknown enum value %q for %q: %v", s, "EnumMarshalStrict", err) + } + *v = EnumMarshalStrict(val) + return nil + } +} + +// MarshalText encodes EnumMarshalStrict to text. +// +// If the enum value is recognized, its name is returned. +// Otherwise, an error is returned. +// +// This implements the TextMarshaler interface. +func (v EnumMarshalStrict) MarshalText() ([]byte, error) { + switch int32(v) { + case 0: + return []byte("Foo"), nil + case 1: + return []byte("Bar"), nil + case 2: + return []byte("Baz"), nil + case 3: + return []byte("Bat"), nil + } + return nil, fmt.Errorf("unknown enum value %q for %q", v, "EnumMarshalStrict") +} + +// MarshalLogObject implements zapcore.ObjectMarshaler, enabling +// fast logging of EnumMarshalStrict. +// Enums are logged as objects, where the value is logged with key "value", and +// if this value's name is known, the name is logged with key "name". +func (v EnumMarshalStrict) MarshalLogObject(enc zapcore.ObjectEncoder) error { + enc.AddInt32("value", int32(v)) + switch int32(v) { + case 0: + enc.AddString("name", "Foo") + case 1: + enc.AddString("name", "Bar") + case 2: + enc.AddString("name", "Baz") + case 3: + enc.AddString("name", "Bat") + } + return nil +} + +// Ptr returns a pointer to this enum value. +func (v EnumMarshalStrict) Ptr() *EnumMarshalStrict { + return &v +} + +// ToWire translates EnumMarshalStrict into a Thrift-level intermediate +// representation. This intermediate representation may be serialized +// into bytes using a ThriftRW protocol implementation. +// +// Enums are represented as 32-bit integers over the wire. +func (v EnumMarshalStrict) ToWire() (wire.Value, error) { + return wire.NewValueI32(int32(v)), nil +} + +// FromWire deserializes EnumMarshalStrict from its Thrift-level +// representation. +// +// x, err := binaryProtocol.Decode(reader, wire.TI32) +// if err != nil { +// return EnumMarshalStrict(0), err +// } +// +// var v EnumMarshalStrict +// if err := v.FromWire(x); err != nil { +// return EnumMarshalStrict(0), err +// } +// return v, nil +func (v *EnumMarshalStrict) FromWire(w wire.Value) error { + *v = (EnumMarshalStrict)(w.GetI32()) + return nil +} + +// String returns a readable string representation of EnumMarshalStrict. +func (v EnumMarshalStrict) String() string { + w := int32(v) + switch w { + case 0: + return "Foo" + case 1: + return "Bar" + case 2: + return "Baz" + case 3: + return "Bat" + } + return fmt.Sprintf("EnumMarshalStrict(%d)", w) +} + +// Equals returns true if this EnumMarshalStrict value matches the provided +// value. +func (v EnumMarshalStrict) Equals(rhs EnumMarshalStrict) bool { + return v == rhs +} + +// MarshalJSON serializes EnumMarshalStrict into JSON. +// +// If the enum value is recognized, its name is returned. +// Otherwise, an error is returned. +// +// This implements json.Marshaler. +func (v EnumMarshalStrict) MarshalJSON() ([]byte, error) { + switch int32(v) { + case 0: + return ([]byte)("\"Foo\""), nil + case 1: + return ([]byte)("\"Bar\""), nil + case 2: + return ([]byte)("\"Baz\""), nil + case 3: + return ([]byte)("\"Bat\""), nil + } + return nil, fmt.Errorf("unknown enum value %q for %q", v, "EnumMarshalStrict") +} + +// UnmarshalJSON attempts to decode EnumMarshalStrict from its JSON +// representation. +// +// This implementation supports both, numeric and string inputs. If a +// string is provided, it must be a known enum name. +// +// This implements json.Unmarshaler. +func (v *EnumMarshalStrict) UnmarshalJSON(text []byte) error { + d := json.NewDecoder(bytes.NewReader(text)) + d.UseNumber() + t, err := d.Token() + if err != nil { + return err + } + + switch w := t.(type) { + case json.Number: + x, err := w.Int64() + if err != nil { + return err + } + if x > math.MaxInt32 { + return fmt.Errorf("enum overflow from JSON %q for %q", text, "EnumMarshalStrict") + } + if x < math.MinInt32 { + return fmt.Errorf("enum underflow from JSON %q for %q", text, "EnumMarshalStrict") + } + *v = (EnumMarshalStrict)(x) + return nil + case string: + return v.UnmarshalText([]byte(w)) + default: + return fmt.Errorf("invalid JSON value %q (%T) to unmarshal into %q", t, t, "EnumMarshalStrict") + } +} + +// ThriftModule represents the IDL file used to generate this package. +var ThriftModule = &thriftreflect.ThriftModule{ + Name: "enum-text-marshal-strict", + Package: "go.uber.org/thriftrw/gen/internal/tests/enum-text-marshal-strict", + FilePath: "enum-text-marshal-strict.thrift", + SHA1: "7d9566a0ff9eccda2ed5be518f321cfcba028dcb", + Raw: rawIDL, +} + +const rawIDL = "enum EnumMarshalStrict {\n Foo, Bar, Baz, Bat\n}" diff --git a/gen/internal/tests/enum_conflict/enum_conflict.go b/gen/internal/tests/enum_conflict/enum_conflict.go index e8cdc302..83917e50 100644 --- a/gen/internal/tests/enum_conflict/enum_conflict.go +++ b/gen/internal/tests/enum_conflict/enum_conflict.go @@ -61,8 +61,8 @@ func (v *RecordType) UnmarshalText(value []byte) error { // MarshalText encodes RecordType 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v RecordType) MarshalText() ([]byte, error) { @@ -142,8 +142,8 @@ func (v RecordType) Equals(rhs RecordType) bool { // MarshalJSON serializes RecordType 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v RecordType) MarshalJSON() ([]byte, error) { diff --git a/gen/internal/tests/enums/enums.go b/gen/internal/tests/enums/enums.go index b6cbb189..58726649 100644 --- a/gen/internal/tests/enums/enums.go +++ b/gen/internal/tests/enums/enums.go @@ -39,8 +39,8 @@ func (v *EmptyEnum) UnmarshalText(value []byte) error { // MarshalText encodes EmptyEnum 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v EmptyEnum) MarshalText() ([]byte, error) { @@ -102,8 +102,8 @@ func (v EmptyEnum) Equals(rhs EmptyEnum) bool { // MarshalJSON serializes EmptyEnum 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v EmptyEnum) MarshalJSON() ([]byte, error) { @@ -191,8 +191,8 @@ func (v *EnumDefault) UnmarshalText(value []byte) error { // MarshalText encodes EnumDefault 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v EnumDefault) MarshalText() ([]byte, error) { @@ -278,8 +278,8 @@ func (v EnumDefault) Equals(rhs EnumDefault) bool { // MarshalJSON serializes EnumDefault 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v EnumDefault) MarshalJSON() ([]byte, error) { @@ -405,8 +405,8 @@ func (v *EnumWithDuplicateName) UnmarshalText(value []byte) error { // MarshalText encodes EnumWithDuplicateName 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v EnumWithDuplicateName) MarshalText() ([]byte, error) { @@ -528,8 +528,8 @@ func (v EnumWithDuplicateName) Equals(rhs EnumWithDuplicateName) bool { // MarshalJSON serializes EnumWithDuplicateName 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v EnumWithDuplicateName) MarshalJSON() ([]byte, error) { @@ -637,8 +637,8 @@ func (v *EnumWithDuplicateValues) UnmarshalText(value []byte) error { // MarshalText encodes EnumWithDuplicateValues 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v EnumWithDuplicateValues) MarshalText() ([]byte, error) { @@ -718,8 +718,8 @@ func (v EnumWithDuplicateValues) Equals(rhs EnumWithDuplicateValues) bool { // MarshalJSON serializes EnumWithDuplicateValues 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v EnumWithDuplicateValues) MarshalJSON() ([]byte, error) { @@ -828,8 +828,8 @@ func (v *EnumWithLabel) UnmarshalText(value []byte) error { // MarshalText encodes EnumWithLabel 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v EnumWithLabel) MarshalText() ([]byte, error) { @@ -933,8 +933,8 @@ func (v EnumWithLabel) Equals(rhs EnumWithLabel) bool { // MarshalJSON serializes EnumWithLabel 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v EnumWithLabel) MarshalJSON() ([]byte, error) { @@ -1036,8 +1036,8 @@ func (v *EnumWithValues) UnmarshalText(value []byte) error { // MarshalText encodes EnumWithValues 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v EnumWithValues) MarshalText() ([]byte, error) { @@ -1123,8 +1123,8 @@ func (v EnumWithValues) Equals(rhs EnumWithValues) bool { // MarshalJSON serializes EnumWithValues 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v EnumWithValues) MarshalJSON() ([]byte, error) { @@ -1228,8 +1228,8 @@ func (v *RecordType) UnmarshalText(value []byte) error { // MarshalText encodes RecordType 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v RecordType) MarshalText() ([]byte, error) { @@ -1315,8 +1315,8 @@ func (v RecordType) Equals(rhs RecordType) bool { // MarshalJSON serializes RecordType 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v RecordType) MarshalJSON() ([]byte, error) { @@ -1407,8 +1407,8 @@ func (v *RecordTypeValues) UnmarshalText(value []byte) error { // MarshalText encodes RecordTypeValues 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v RecordTypeValues) MarshalText() ([]byte, error) { @@ -1488,8 +1488,8 @@ func (v RecordTypeValues) Equals(rhs RecordTypeValues) bool { // MarshalJSON serializes RecordTypeValues 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v RecordTypeValues) MarshalJSON() ([]byte, error) { @@ -1737,8 +1737,8 @@ func (v *LowerCaseEnum) UnmarshalText(value []byte) error { // MarshalText encodes LowerCaseEnum 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v LowerCaseEnum) MarshalText() ([]byte, error) { @@ -1824,8 +1824,8 @@ func (v LowerCaseEnum) Equals(rhs LowerCaseEnum) bool { // MarshalJSON serializes LowerCaseEnum 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v LowerCaseEnum) MarshalJSON() ([]byte, error) { diff --git a/gen/internal/tests/nozap/nozap.go b/gen/internal/tests/nozap/nozap.go index 9dbb633a..61e86113 100644 --- a/gen/internal/tests/nozap/nozap.go +++ b/gen/internal/tests/nozap/nozap.go @@ -60,8 +60,8 @@ func (v *EnumDefault) UnmarshalText(value []byte) error { // MarshalText encodes EnumDefault 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. +// Otherwise, its integer value is returned. // // This implements the TextMarshaler interface. func (v EnumDefault) MarshalText() ([]byte, error) { @@ -130,8 +130,8 @@ func (v EnumDefault) Equals(rhs EnumDefault) bool { // MarshalJSON serializes EnumDefault 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. +// Otherwise, its integer value is returned. // // This implements json.Marshaler. func (v EnumDefault) MarshalJSON() ([]byte, error) { diff --git a/gen/internal/tests/thrift/enum-text-marshal-strict.thrift b/gen/internal/tests/thrift/enum-text-marshal-strict.thrift new file mode 100644 index 00000000..e6cd906c --- /dev/null +++ b/gen/internal/tests/thrift/enum-text-marshal-strict.thrift @@ -0,0 +1,3 @@ +enum EnumMarshalStrict { + Foo, Bar, Baz, Bat +} \ No newline at end of file diff --git a/gen/quick_test.go b/gen/quick_test.go index 8f5b5592..aa89e0cc 100644 --- a/gen/quick_test.go +++ b/gen/quick_test.go @@ -36,6 +36,7 @@ import ( tl "go.uber.org/thriftrw/gen/internal/tests/collision" tc "go.uber.org/thriftrw/gen/internal/tests/containers" + tems "go.uber.org/thriftrw/gen/internal/tests/enum-text-marshal-strict" tle "go.uber.org/thriftrw/gen/internal/tests/enum_conflict" te "go.uber.org/thriftrw/gen/internal/tests/enums" tx "go.uber.org/thriftrw/gen/internal/tests/exceptions" @@ -124,28 +125,49 @@ func unionValueGenerator(sample interface{}) func(*testing.T, *rand.Rand) thrift } } -// enumValueGenerator builds a generator for random enum values given the -// `*_Values` function for that enum. -func enumValueGenerator(valuesFunc interface{}) func(*testing.T, *rand.Rand) thriftType { +type enumValueGen struct { + typ reflect.Type + knownValues reflect.Value // output of Foo_Values() + onlyKnown bool // use only known enum values +} + +func newEnumValueGen(valuesFunc interface{}) *enumValueGen { vfunc := reflect.ValueOf(valuesFunc) typ := vfunc.Type().Out(0).Elem() // Foo_Values() []Foo -> Foo - return func(t *testing.T, rand *rand.Rand) thriftType { - knownValues := vfunc.Call(nil)[0] - - var giveV reflect.Value - // Flip a coin to decide whether we're evaluating a known or - // unknown value. - if rand.Int()%2 == 0 && knownValues.Len() > 0 { - // Pick a known value at random - giveV = knownValues.Index(rand.Intn(knownValues.Len())) - } else { - // give = MyEnum($randomValue) - giveV = reflect.New(typ).Elem() - giveV.Set(reflect.ValueOf(rand.Int31()).Convert(typ)) - } + return &enumValueGen{ + typ: typ, + knownValues: vfunc.Call(nil)[0], + } +} - return giveV.Addr().Interface().(thriftType) +func (g *enumValueGen) Generate(t *testing.T, rand *rand.Rand) thriftType { + var giveV reflect.Value + // Flip a coin to decide whether we're evaluating a known or + // unknown value, unless onlyKnown is set to true. + if (g.onlyKnown || rand.Int()%2 == 0) && g.knownValues.Len() > 0 { + // Pick a known value at random + giveV = g.knownValues.Index(rand.Intn(g.knownValues.Len())) + } else { + // give = MyEnum($randomValue) + giveV = reflect.New(g.typ).Elem() + giveV.Set(reflect.ValueOf(rand.Int31()).Convert(g.typ)) } + + return giveV.Addr().Interface().(thriftType) +} + +// enumValueGenerator builds a generator for random enum values given the +// `*_Values` function for that enum. +func enumValueGenerator(valuesFunc interface{}) func(*testing.T, *rand.Rand) thriftType { + return newEnumValueGen(valuesFunc).Generate +} + +// knownEnumValuesGenerator builds a generator for random enum values that only +// yields known enum values. +func knownEnumValuesGenerator(valuesFunc interface{}) func(*testing.T, *rand.Rand) thriftType { + gen := newEnumValueGen(valuesFunc) + gen.onlyKnown = true + return gen.Generate } // populateDefaults copies the given thrift object to a new object @@ -541,6 +563,11 @@ func TestQuickSuite(t *testing.T) { Kind: thriftEnum, NoLog: true, }, + { + Sample: tems.EnumMarshalStrict(0), + Generator: knownEnumValuesGenerator(tems.EnumMarshalStrict_Values), + Kind: thriftEnum, + }, } // Log the seed so that we can reproduce this if it ever fails. @@ -664,7 +691,6 @@ func TestQuickSuite(t *testing.T) { t.Run("EqualsNil", suite.testEqualsNil) } } - }) } } diff --git a/main.go b/main.go index 07121c3c..be293522 100644 --- a/main.go +++ b/main.go @@ -52,14 +52,15 @@ type genOptions struct { NoRecurse bool `long:"no-recurse" description:"Don't generate code for included Thrift files."` Plugins plugin.Flags `long:"plugin" short:"p" value-name:"PLUGIN" description:"Code generation plugin for ThriftRW. This option may be provided multiple times to apply multiple plugins."` - GeneratePluginAPI bool `long:"generate-plugin-api" hidden:"true" description:"Generates code for the plugin API"` - NoVersionCheck bool `long:"no-version-check" hidden:"true" description:"Does not add library version checks to generated code."` - NoTypes bool `long:"no-types" description:"Do not generate code for types, implies --no-service-helpers."` - NoConstants bool `long:"no-constants" description:"Do not generate code for const declarations."` - NoServiceHelpers bool `long:"no-service-helpers" description:"Do not generate service helpers."` - NoEmbedIDL bool `long:"no-embed-idl" description:"Do not embed IDLs into the generated code."` - NoZap bool `long:"no-zap" description:"Do not generate code for Zap logging."` - OutputFile string `long:"output-file" value-name:"FILENAME" description:"Generates a single .go file as an output. Specifying an OutputFile prevents code generation for included Thrift Files."` + GeneratePluginAPI bool `long:"generate-plugin-api" hidden:"true" description:"Generates code for the plugin API"` + NoVersionCheck bool `long:"no-version-check" hidden:"true" description:"Does not add library version checks to generated code."` + NoTypes bool `long:"no-types" description:"Do not generate code for types, implies --no-service-helpers."` + NoConstants bool `long:"no-constants" description:"Do not generate code for const declarations."` + NoServiceHelpers bool `long:"no-service-helpers" description:"Do not generate service helpers."` + NoEmbedIDL bool `long:"no-embed-idl" description:"Do not embed IDLs into the generated code."` + NoZap bool `long:"no-zap" description:"Do not generate code for Zap logging."` + OutputFile string `long:"output-file" value-name:"FILENAME" description:"Generates a single .go file as an output. Specifying an OutputFile prevents code generation for included Thrift Files."` + EnumTextMarshalStrict bool `long:"enum-text-marshal-strict" hidden:"true" description:"Generate code to throw error on trying to marshal unknown enum"` // TODO(abg): Detailed help with examples of --thrift-root, --pkg-prefix, // and --plugin @@ -177,18 +178,19 @@ func do() (err error) { ServiceGenerator: pluginHandle.ServiceGenerator(), } generatorOptions := gen.Options{ - OutputDir: gopts.OutputDirectory, - PackagePrefix: gopts.PackagePrefix, - ThriftRoot: gopts.ThriftRoot, - NoRecurse: gopts.NoRecurse, - NoVersionCheck: gopts.NoVersionCheck, - Plugin: codeGenerator, - NoTypes: gopts.NoTypes, - NoConstants: gopts.NoConstants, - NoServiceHelpers: gopts.NoServiceHelpers || gopts.NoTypes, - NoEmbedIDL: gopts.NoEmbedIDL, - NoZap: gopts.NoZap, - OutputFile: gopts.OutputFile, + OutputDir: gopts.OutputDirectory, + PackagePrefix: gopts.PackagePrefix, + ThriftRoot: gopts.ThriftRoot, + NoRecurse: gopts.NoRecurse, + NoVersionCheck: gopts.NoVersionCheck, + Plugin: codeGenerator, + NoTypes: gopts.NoTypes, + NoConstants: gopts.NoConstants, + NoServiceHelpers: gopts.NoServiceHelpers || gopts.NoTypes, + NoEmbedIDL: gopts.NoEmbedIDL, + NoZap: gopts.NoZap, + OutputFile: gopts.OutputFile, + EnumTextMarshalStrict: gopts.EnumTextMarshalStrict, } if err := gen.Generate(module, &generatorOptions); err != nil { return fmt.Errorf("Failed to generate code: %+v", err) From a710cb8ad4f0b17bb22f42d441a20d31baa0714f Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Fri, 23 Jul 2021 14:13:50 -0700 Subject: [PATCH 16/16] CHANGELOG: Update based on commits in releaes Update the changelog to reflect the commits going in this release. --- CHANGELOG.md | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1cf1f6f..7764d783 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,12 +4,24 @@ 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). -## [Unreleased] +## [1.28.0] - 2021-07-26 ### Added -- idl.Parse() now returns structured ParseError upon error. +- `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 -- Support parsing struct fields without identifiers. +- 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 @@ -372,7 +384,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Added - Initial release. -[Unreleased]: https://github.com/thriftrw/thriftrw-go/compare/v1.27.0...HEAD +[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