Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

idl: record document positions on constant nodes #503

Merged
merged 4 commits into from
Jun 25, 2021
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions idl/config.go
Original file line number Diff line number Diff line change
@@ -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)
}
46 changes: 46 additions & 0 deletions idl/config_test.go
Original file line number Diff line number Diff line change
@@ -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]))
}
}
40 changes: 40 additions & 0 deletions idl/info.go
Original file line number Diff line number Diff line change
@@ -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}
}
Comment on lines +34 to +40
Copy link
Contributor Author

@jparise jparise Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@abhinav I've just realized this will panic if passed an unhashable node that doesn't support ast.LineNumber (or ast.LineNumber() returns 0). Can you think of a pattern to avoid that here aside from using reflection or recover()?

These cases are more relevant to tests which rely more heavily on zero values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yeah, that's a problem. I think we should not ignore the != 0 case here: if the type records its own line number, we should return that as-is. That would narrow down the possibility of panic to only those types that are unhashable, but I don't think we have anything that is unhashable that doesn't record its own line number.

To respect the line number of AST nodes that know their own line number, we have two options: Info moves to the AST package (which I suspect is not possible without exposing Info.nodePositions), or we add an ast.LookupLineNumber(ast.Node) (n int, ok bool) which reports the line number only if available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this is an opportunity to move Position to the ast package with an accessor that returns pos, ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable!

Copy link
Contributor Author

@jparise jparise Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in #504

59 changes: 59 additions & 0 deletions idl/info_test.go
Original file line number Diff line number Diff line change
@@ -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))
}
}
16 changes: 11 additions & 5 deletions idl/internal/lex.go
Original file line number Diff line number Diff line change
@@ -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.
6 changes: 6 additions & 0 deletions idl/internal/lex.rl
Original file line number Diff line number Diff line change
@@ -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.
15 changes: 12 additions & 3 deletions idl/internal/parser.go
Original file line number Diff line number Diff line change
@@ -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
5 changes: 5 additions & 0 deletions idl/internal/position.go
Original file line number Diff line number Diff line change
@@ -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
11 changes: 5 additions & 6 deletions idl/internal/thrift.y
Original file line number Diff line number Diff line change
@@ -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($$) }
Comment on lines +390 to +394
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this will record the line number right after the token.

This is probably not an issue for constant primitives because they'll never be split across multiple lines, but if we want accurate column numbers later, the position would have to be recorded before the value (like with lineno IDENTIFIER below). I'm okay doing that when we add support for column numbers, though.

| lineno IDENTIFIER
{ $$ = ast.ConstantReference{Name: $2, Line: $1} }

5 changes: 5 additions & 0 deletions idl/internal/y.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions idl/parser.go
Original file line number Diff line number Diff line change
@@ -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)
}