From 6ba9e6ea8400caba7532e4add2ded3197c009579 Mon Sep 17 00:00:00 2001 From: Patrick Stephen Date: Sat, 4 Dec 2021 13:33:21 -0600 Subject: [PATCH 1/5] Format kinds when expectAnyOfNext hits EOF --- parse.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/parse.go b/parse.go index 6c4fbc3..9a6fd57 100644 --- a/parse.go +++ b/parse.go @@ -128,7 +128,11 @@ func expectAnyOfNext(tr *tokenReader, kinds ...tokenKind) error { return tr.Err() } if !next { - return readError(tr.nextToken, "expected (%v), got no token", kinds) + kindsStrs := make([]string, len(kinds)) + for i, k := range kinds { + kindsStrs[i] = k.String() + } + return readError(tr.nextToken, "expected (%v), got no token", kindsStr(kinds)) } tk := tr.Token() found := false From d1690459a875e7b1de945f057d80d62f530bac4a Mon Sep 17 00:00:00 2001 From: Patrick Stephen Date: Sat, 4 Dec 2021 13:46:58 -0600 Subject: [PATCH 2/5] Fix bug in union and message parsing 1. Do not arbitrarily skip tokens after reading a union field, only skip one '}' and optional comments and newlines 2. Enforce that messages do not skip invalid tokens in the middle of their structure Addresses #18 --- parse.go | 20 ++- parse_test.go | 67 +++++++- testdata/base/union_2.bop | 14 ++ .../generated/import_separate_a.go | 157 ++++++++++++++++++ testdata/incompatible/import_separate_a.bop | 2 +- .../invalid/invalid_union_no_message_int.bop | 8 + 6 files changed, 261 insertions(+), 7 deletions(-) create mode 100644 testdata/base/union_2.bop create mode 100644 testdata/invalid/invalid_union_no_message_int.bop diff --git a/parse.go b/parse.go index 9a6fd57..61c9994 100644 --- a/parse.go +++ b/parse.go @@ -424,11 +424,19 @@ func readMessage(tr *tokenReader) (Message, error) { nextDeprecatedMessage := "" nextIsDeprecated := false for tr.Token().kind != tokenKindCloseCurly { - if !tr.Next() { - return msg, readError(tr.nextToken, "message definition ended early") + if err := expectAnyOfNext(tr, + tokenKindNewline, + tokenKindIntegerLiteral, + tokenKindOpenSquare, + tokenKindBlockComment, + tokenKindLineComment, + tokenKindCloseCurly); err != nil { + return msg, err } tk := tr.Token() switch tk.kind { + case tokenKindCloseCurly: + // break case tokenKindNewline: nextCommentLines = []string{} case tokenKindIntegerLiteral: @@ -557,9 +565,11 @@ func readUnion(tr *tokenReader) (Union, error) { nextCommentLines = []string{} nextCommentTags = []Tag{} - skipEndOfLineComments(tr) - tr.Next() + // This is a close curly-- we must advance past it or the union + // will read it and believe it is complete tr.Next() + skipEndOfLineComments(tr) + optNewline(tr) case tokenKindOpenSquare: if nextIsDeprecated { @@ -743,7 +753,7 @@ func parseCommentTag(s string) (Tag, bool) { // Not OK // [tag(db:unquotedstring)] // [tag()] - + if !strings.HasPrefix(s, "[tag(") || !strings.HasSuffix(s, ")]") { return Tag{}, false } diff --git a/parse_test.go b/parse_test.go index 4b3725f..f38a008 100644 --- a/parse_test.go +++ b/parse_test.go @@ -12,6 +12,70 @@ func TestReadFile(t *testing.T) { expected File } tcs := []testCase{ + { + file: "union_2", + expected: File{ + Unions: []Union{ + { + Name: "U2", + Fields: map[uint8]UnionField{ + 1: { + Struct: &Struct{ + Name: "U3", + Fields: []Field{ + { + Name: "hello", + FieldType: FieldType{ + Simple: "uint32", + }, + }, + }, + }, + }, + 2: { + Message: &Message{ + Name: "U4", + Fields: map[uint8]Field{ + 1: { + Name: "goodbye", + FieldType: FieldType{ + Simple: "uint32", + }, + }, + }, + }, + }, + 3: { + Message: &Message{ + Name: "U5", + Fields: map[uint8]Field{ + 1: { + Name: "goodbye", + FieldType: FieldType{ + Simple: "uint32", + }, + }, + }, + }, + }, + 4: { + Struct: &Struct{ + Name: "U6", + Fields: []Field{ + { + Name: "hello", + FieldType: FieldType{ + Simple: "uint32", + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, { file: "tags", expected: File{ @@ -1437,7 +1501,7 @@ func TestReadFileError(t *testing.T) { {file: "invalid_message_double_deprecated", errMessage: "[2:5] expected field following deprecated annotation"}, {file: "invalid_message_hex_int", errMessage: "[1:7] strconv.ParseUint: parsing \"0x1\": invalid syntax"}, {file: "invalid_message_no_arrow", errMessage: "[1:11] expected (Arrow) got Ident"}, - {file: "invalid_message_no_close", errMessage: "[1:19] message definition ended early"}, + {file: "invalid_message_no_close", errMessage: "[1:19] expected (Newline, Integer Literal, Open Square, Block Comment, Line Comment, Close Curly), got no token"}, {file: "invalid_message_no_curly", errMessage: "[1:0] expected (Open Curly) got Newline"}, {file: "invalid_message_no_field_name", errMessage: "[1:15] expected (Ident) got Semicolon"}, {file: "invalid_message_no_name", errMessage: "[0:9] expected (Ident) got Open Curly"}, @@ -1481,6 +1545,7 @@ func TestReadFileError(t *testing.T) { {file: "invalid_array_bad_key", errMessage: "[1:15] expected (Ident, Array, Map) got Close Square"}, {file: "invalid_array_no_close_square", errMessage: "[1:19] expected (Close Square) got Ident"}, {file: "invalid_array_suffix__no_close_square", errMessage: "[1:13] expected (Close Square) got Ident"}, + {file: "invalid_union_no_message_int", errMessage: "[5:14] expected (Newline, Integer Literal, Open Square, Block Comment, Line Comment, Close Curly) got Ident"}, } for _, tc := range tcs { tc := tc diff --git a/testdata/base/union_2.bop b/testdata/base/union_2.bop new file mode 100644 index 0000000..77b751b --- /dev/null +++ b/testdata/base/union_2.bop @@ -0,0 +1,14 @@ +union U2 { + 1 -> struct U3{ + uint32 hello; + } + 2 -> message U4{ + 1 -> uint32 goodbye; + } + 3 -> message U5{ + 1 -> uint32 goodbye; + } + 4 -> struct U6{ + uint32 hello; + } +} \ No newline at end of file diff --git a/testdata/incompatible/generated/import_separate_a.go b/testdata/incompatible/generated/import_separate_a.go index 88fdbf7..6b391b8 100644 --- a/testdata/incompatible/generated/import_separate_a.go +++ b/testdata/incompatible/generated/import_separate_a.go @@ -280,10 +280,124 @@ func MustMakeUnionStructFromBytes(buf []byte) UnionStruct { return v } +var _ bebop.Record = &UnionMessage{} + +type UnionMessage struct { + Goodbye *generatedtwo.ImportedEnum +} + +func (bbp UnionMessage) MarshalBebopTo(buf []byte) int { + at := 0 + iohelp.WriteUint32Bytes(buf[at:], uint32(bbp.Size()-4)) + at += 4 + if bbp.Goodbye != nil { + buf[at] = 1 + at++ + iohelp.WriteUint32Bytes(buf[at:], uint32(*bbp.Goodbye)) + at += 4 + } + return at +} + +func (bbp *UnionMessage) UnmarshalBebop(buf []byte) (err error) { + at := 0 + _ = iohelp.ReadUint32Bytes(buf[at:]) + buf = buf[4:] + for { + switch buf[at] { + case 1: + at += 1 + bbp.Goodbye = new(generatedtwo.ImportedEnum) + (*bbp.Goodbye) = generatedtwo.ImportedEnum(iohelp.ReadUint32Bytes(buf[at:])) + at += 4 + default: + return nil + } + } +} + +func (bbp *UnionMessage) MustUnmarshalBebop(buf []byte) { + at := 0 + _ = iohelp.ReadUint32Bytes(buf[at:]) + buf = buf[4:] + for { + switch buf[at] { + case 1: + at += 1 + bbp.Goodbye = new(generatedtwo.ImportedEnum) + (*bbp.Goodbye) = generatedtwo.ImportedEnum(iohelp.ReadUint32Bytes(buf[at:])) + at += 4 + default: + return + } + } +} + +func (bbp UnionMessage) EncodeBebop(iow io.Writer) (err error) { + w := iohelp.NewErrorWriter(iow) + iohelp.WriteUint32(w, uint32(bbp.Size()-4)) + if bbp.Goodbye != nil { + w.Write([]byte{1}) + iohelp.WriteUint32(w, uint32(*bbp.Goodbye)) + } + w.Write([]byte{0}) + return w.Err +} + +func (bbp *UnionMessage) DecodeBebop(ior io.Reader) (err error) { + r := iohelp.NewErrorReader(ior) + bodyLen := iohelp.ReadUint32(r) + r.Reader = &io.LimitedReader{R:r.Reader, N:int64(bodyLen)} + for { + switch iohelp.ReadByte(r) { + case 1: + bbp.Goodbye = new(generatedtwo.ImportedEnum) + *bbp.Goodbye = generatedtwo.ImportedEnum(iohelp.ReadUint32(r)) + default: + io.ReadAll(r) + return r.Err + } + } +} + +func (bbp UnionMessage) Size() int { + bodyLen := 5 + if bbp.Goodbye != nil { + bodyLen += 1 + bodyLen += 4 + } + return bodyLen +} + +func (bbp UnionMessage) MarshalBebop() []byte { + buf := make([]byte, bbp.Size()) + bbp.MarshalBebopTo(buf) + return buf +} + +func MakeUnionMessage(r iohelp.ErrorReader) (UnionMessage, error) { + v := UnionMessage{} + err := v.DecodeBebop(r) + return v, err +} + +func MakeUnionMessageFromBytes(buf []byte) (UnionMessage, error) { + v := UnionMessage{} + err := v.UnmarshalBebop(buf) + return v, err +} + +func MustMakeUnionMessageFromBytes(buf []byte) UnionMessage { + v := UnionMessage{} + v.MustUnmarshalBebop(buf) + return v +} + var _ bebop.Record = &UsesImportUnion{} type UsesImportUnion struct { UnionStruct *UnionStruct + UnionMessage *UnionMessage } func (bbp UsesImportUnion) MarshalBebopTo(buf []byte) int { @@ -297,6 +411,13 @@ func (bbp UsesImportUnion) MarshalBebopTo(buf []byte) int { at += (*bbp.UnionStruct).Size() return at } + if bbp.UnionMessage != nil { + buf[at] = 2 + at++ + (*bbp.UnionMessage).MarshalBebopTo(buf[at:]) + at += (*bbp.UnionMessage).Size() + return at + } return at } @@ -318,6 +439,15 @@ func (bbp *UsesImportUnion) UnmarshalBebop(buf []byte) (err error) { } at += ((*bbp.UnionStruct)).Size() return nil + case 2: + at += 1 + bbp.UnionMessage = new(UnionMessage) + (*bbp.UnionMessage), err = MakeUnionMessageFromBytes(buf[at:]) + if err != nil{ + return err + } + at += ((*bbp.UnionMessage)).Size() + return nil default: return nil } @@ -336,6 +466,12 @@ func (bbp *UsesImportUnion) MustUnmarshalBebop(buf []byte) { (*bbp.UnionStruct) = MustMakeUnionStructFromBytes(buf[at:]) at += ((*bbp.UnionStruct)).Size() return + case 2: + at += 1 + bbp.UnionMessage = new(UnionMessage) + (*bbp.UnionMessage) = MustMakeUnionMessageFromBytes(buf[at:]) + at += ((*bbp.UnionMessage)).Size() + return default: return } @@ -353,6 +489,14 @@ func (bbp UsesImportUnion) EncodeBebop(iow io.Writer) (err error) { } return w.Err } + if bbp.UnionMessage != nil { + w.Write([]byte{2}) + err = (*bbp.UnionMessage).EncodeBebop(w) + if err != nil{ + return err + } + return w.Err + } return w.Err } @@ -370,6 +514,14 @@ func (bbp *UsesImportUnion) DecodeBebop(ior io.Reader) (err error) { } io.ReadAll(r) return r.Err + case 2: + bbp.UnionMessage = new(UnionMessage) + (*bbp.UnionMessage), err = MakeUnionMessage(r) + if err != nil{ + return err + } + io.ReadAll(r) + return r.Err default: io.ReadAll(r) return r.Err @@ -384,6 +536,11 @@ func (bbp UsesImportUnion) Size() int { bodyLen += (*bbp.UnionStruct).Size() return bodyLen } + if bbp.UnionMessage != nil { + bodyLen += 1 + bodyLen += (*bbp.UnionMessage).Size() + return bodyLen + } return bodyLen } diff --git a/testdata/incompatible/import_separate_a.bop b/testdata/incompatible/import_separate_a.bop index a7b2609..2e57f75 100644 --- a/testdata/incompatible/import_separate_a.bop +++ b/testdata/incompatible/import_separate_a.bop @@ -16,6 +16,6 @@ union UsesImportUnion { ImportedEnum hello; } 2 -> message UnionMessage{ - ImportedEnum goodbye; + 1 -> ImportedEnum goodbye; } } \ No newline at end of file diff --git a/testdata/invalid/invalid_union_no_message_int.bop b/testdata/invalid/invalid_union_no_message_int.bop new file mode 100644 index 0000000..787ec78 --- /dev/null +++ b/testdata/invalid/invalid_union_no_message_int.bop @@ -0,0 +1,8 @@ +union UsesImportUnion { + 1 -> struct UnionStruct{ + uint32 hello; + } + 2 -> message UnionMessage{ + uint32 goodbye; + } +} \ No newline at end of file From 9aca713c9b4a24df84b29b3154f1aeca74779986 Mon Sep 17 00:00:00 2001 From: Patrick Stephen Date: Sat, 4 Dec 2021 13:48:00 -0600 Subject: [PATCH 3/5] Document which incompatible files are incompatible for which compilers --- compatibility_test.go | 98 +++++++++++++++++++++++++ testdata/incompatible/README.md | 4 +- testdata/incompatible/quoted_string.bop | 2 +- 3 files changed, 102 insertions(+), 2 deletions(-) diff --git a/compatibility_test.go b/compatibility_test.go index c1396d7..9526ff4 100644 --- a/compatibility_test.go +++ b/compatibility_test.go @@ -1,10 +1,12 @@ package bebop import ( + "bytes" "fmt" "os" "os/exec" "path/filepath" + "strings" "testing" ) @@ -64,3 +66,99 @@ func TestUpstreamCompatiblityFailures(t *testing.T) { }) } } + +func TestIncompatibilityExpectations_200sc(t *testing.T) { + files, err := os.ReadDir(filepath.Join(".", "testdata", "incompatible")) + if err != nil { + t.Fatalf("failed to list incompatible files: %v", err) + } + + failures := map[string]struct{}{ + "import_loop_a.bop": {}, + "import_loop_b.bop": {}, + "import_loop_c.bop": {}, + "invalid_enum_primitive.bop": {}, + "invalid_import_no_const.bop": {}, + "invalid_message_primitive.bop": {}, + "invalid_struct_primitive.bop": {}, + "recursive_struct.bop": {}, + } + + for _, f := range files { + if f.IsDir() { + continue + } + filename := f.Name() + if !strings.HasSuffix(filename, ".bop") { + continue + } + t.Run(filename, func(t *testing.T) { + f, err := os.Open(filepath.Join("testdata", "incompatible", filename)) + if err != nil { + t.Fatalf("failed to open test file %s: %v", filename, err) + } + defer f.Close() + bopf, _, err := ReadFile(f) + if err != nil { + t.Fatalf("failed to read file %s: %v", filename, err) + } + err = bopf.Generate(bytes.NewBuffer([]byte{}), GenerateSettings{ + PackageName: "generated", + }) + _, shouldFail := failures[filename] + if shouldFail && err == nil { + t.Fatal("expected generation failure") + } + if !shouldFail && err != nil { + t.Fatalf("expected generation success: %v", err) + } + }) + } +} + +func TestIncompatibilityExpectations_Rainway(t *testing.T) { + if testing.Short() { + t.Skip("upstream tests skipped by --short") + } + skipIfUpstreamMissing(t) + + files, err := os.ReadDir(filepath.Join(".", "testdata", "incompatible")) + if err != nil { + t.Fatalf("failed to list incompatible files: %v", err) + } + + failures := map[string]struct{}{ + "import_loop_a.bop": {}, + "import_loop_b.bop": {}, + "import_loop_c.bop": {}, + "import_separate_a.bop": {}, + "invalid_import_no_const.bop": {}, + "quoted_string.bop": {}, + "union.bop": {}, + } + + for _, f := range files { + if f.IsDir() { + continue + } + filename := f.Name() + if !strings.HasSuffix(filename, ".bop") { + continue + } + t.Run(filename, func(t *testing.T) { + cmd := exec.Command(upsteamCompilerName, "--ts", "./out.ts", "--files", filepath.Join(".", "testdata", "incompatible", filename)) + out, err := cmd.CombinedOutput() + bytes.TrimSuffix(out, []byte("\n")) + + _, shouldFail := failures[filename] + if shouldFail && err == nil { + t.Fatal("expected generation failure") + } + if !shouldFail && err != nil { + t.Fatalf("expected generation success: %v", err) + } + + fmt.Printf("filename: %v err: %v out:%v\n", filename, err, string(out)) + }) + } +} diff --git a/testdata/incompatible/README.md b/testdata/incompatible/README.md index b412a3e..b85579c 100644 --- a/testdata/incompatible/README.md +++ b/testdata/incompatible/README.md @@ -2,10 +2,12 @@ Files in this directory should reasonably be supported by rainway bebop but are not, and we support, or vice versa. -## causes +## Sample causes "uint8" is a supported type, but "int8" is not. Rainway allows overwriting primitive types with message, struct, or enum definitions. We do not. We do not allow recursive struct definitions, as they will never terminate when encoded or parsed. Rainway does not have a check for this. + +See `compatability_test.go` for which files specifically fail for which compilers. \ No newline at end of file diff --git a/testdata/incompatible/quoted_string.bop b/testdata/incompatible/quoted_string.bop index 9f00bbe..d59e0bd 100644 --- a/testdata/incompatible/quoted_string.bop +++ b/testdata/incompatible/quoted_string.bop @@ -1,4 +1,4 @@ -// we support escaped quotes inside of deprecated stings. rainway does not as of 2.2.6 +// we support escaped quotes inside of deprecated stings. rainway does not as of 2.3.1 struct QuotedString { [deprecated("\"deprecated\"")] From fc99cc9cd08345c94750db7e3e6d8d5995d804e8 Mon Sep 17 00:00:00 2001 From: Patrick Stephen Date: Sat, 4 Dec 2021 13:48:27 -0600 Subject: [PATCH 4/5] Bump version to 0.2.4 --- bebop.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bebop.go b/bebop.go index ce5a6df..7b77661 100644 --- a/bebop.go +++ b/bebop.go @@ -2,7 +2,7 @@ package bebop // Version is the library version. Should be used by CLI tools when passed a '--version' flag. -const Version = "v0.2.3" +const Version = "v0.2.4" // A File is a structured representation of a .bop file. type File struct { From 4f4cba3b9429380961ef3b4d9a73e9955bb09e6f Mon Sep 17 00:00:00 2001 From: Patrick Stephen Date: Sat, 4 Dec 2021 14:02:01 -0600 Subject: [PATCH 5/5] Linting, typos --- gen_test.go | 10 ++++++++-- main/bebopfmt/main.go | 7 +++++-- testdata/formatted/quoted_string_formatted.bop | 2 +- testdata/incompatible/README.md | 2 +- tokenize.go | 8 ++++++-- 5 files changed, 21 insertions(+), 8 deletions(-) diff --git a/gen_test.go b/gen_test.go index ad5ba58..faee7de 100644 --- a/gen_test.go +++ b/gen_test.go @@ -183,7 +183,10 @@ func TestGenerateToFile_SeperateImports(t *testing.T) { } // use a separate directory to ensure duplicate definitions in combined mode // do not cause compilation failures - os.MkdirAll(filepath.Join("testdata", "incompatible", filepath.Dir(filedef.outfile)), 0777) + err = os.MkdirAll(filepath.Join("testdata", "incompatible", filepath.Dir(filedef.outfile)), 0777) + if err != nil { + t.Fatalf("failed to mkdir: %v", err) + } outFile := filepath.Join("testdata", "incompatible", filedef.outfile) out, err := os.Create(outFile) if err != nil { @@ -345,7 +348,10 @@ func TestGenerateToFile_Imports(t *testing.T) { } // use a separate directory to ensure duplicate definitions in combined mode // do not cause compilation failures - os.MkdirAll(filepath.Join("testdata", "generated", filename), 0777) + err = os.MkdirAll(filepath.Join("testdata", "generated", filename), 0777) + if err != nil { + t.Fatalf("failed to mkdir: %v", err) + } outFile := filepath.Join("testdata", "generated", filename, filename+".go") out, err := os.Create(outFile) if err != nil { diff --git a/main/bebopfmt/main.go b/main/bebopfmt/main.go index c00bd7d..e220f7e 100644 --- a/main/bebopfmt/main.go +++ b/main/bebopfmt/main.go @@ -107,10 +107,13 @@ func formatFile(path string) error { if err != nil { return fmt.Errorf("Failed to open path to rewrite: %w", err) } - f.Write(out.Bytes()) + _, err = f.Write(out.Bytes()) + if err != nil { + return fmt.Errorf("Failed to write to output: %w", err) + } f.Close() } else { - fmt.Println(string(out.Bytes())) + fmt.Println(out.String()) } return nil } diff --git a/testdata/formatted/quoted_string_formatted.bop b/testdata/formatted/quoted_string_formatted.bop index 8eddb2c..f9cb998 100644 --- a/testdata/formatted/quoted_string_formatted.bop +++ b/testdata/formatted/quoted_string_formatted.bop @@ -1,4 +1,4 @@ -// we support escaped quotes inside of deprecated stings. rainway does not as of 2.2.6 +// we support escaped quotes inside of deprecated stings. rainway does not as of 2.3.1 struct QuotedString { [deprecated("\"deprecated\"")] int32 X; diff --git a/testdata/incompatible/README.md b/testdata/incompatible/README.md index b85579c..e197682 100644 --- a/testdata/incompatible/README.md +++ b/testdata/incompatible/README.md @@ -10,4 +10,4 @@ Rainway allows overwriting primitive types with message, struct, or enum definit We do not allow recursive struct definitions, as they will never terminate when encoded or parsed. Rainway does not have a check for this. -See `compatability_test.go` for which files specifically fail for which compilers. \ No newline at end of file +See `compatibility_test.go` for which files specifically fail for which compilers. \ No newline at end of file diff --git a/tokenize.go b/tokenize.go index 4c104d5..87447b1 100644 --- a/tokenize.go +++ b/tokenize.go @@ -71,7 +71,10 @@ func (tr *tokenReader) readByte() (byte, error) { } func (tr *tokenReader) unreadByte() { - tr.r.UnreadByte() + err := tr.r.UnreadByte() + if err != nil { + panic(fmt.Errorf("unreadByte failed: %w", err)) + } tr.loc.inc(-1) } @@ -316,7 +319,8 @@ func (tr *tokenReader) nextIdent(firstRune rune) bool { case unicode.IsDigit(rn): case rn == '_': default: - tr.r.UnreadRune() + // ignore this error, we just called ReadRune + _ = tr.r.UnreadRune() tr.loc.inc(-sz) if keywordKind, ok := keywords[string(tk.concrete)]; ok { tk.kind = keywordKind