idl: record document positions on constant nodes #503

jparise · 2021-06-25T00:39:14Z

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

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 thriftrw#493

codecov · 2021-06-25T00:41:42Z

Codecov Report

Merging #503 (2b229c1) into dev (369aed6) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #503      +/-   ##
==========================================
+ Coverage   78.99%   79.01%   +0.01%     
==========================================
  Files         126      128       +2     
  Lines       16075    16086      +11     
==========================================
+ Hits        12699    12710      +11     
  Misses       2066     2066              
  Partials     1310     1310

Impacted Files	Coverage Δ
idl/config.go	`100.00% <100.00%> (ø)`
idl/info.go	`100.00% <100.00%> (ø)`
idl/internal/parser.go	`100.00% <100.00%> (ø)`
idl/parser.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 369aed6...2b229c1. Read the comment docs.

idl/internal/parser.go

This gives us room to grow the set of returned data without expanding the number of return values.

abhinav

Pretty straightforward. Thanks!

idl/config_test.go

abhinav · 2021-06-25T16:30:10Z

idl/internal/thrift.y

+    : 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($$) }


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.

jparise · 2021-06-25T17:00:33Z

idl/info.go

+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}
+}


@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.

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.

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

That sounds reasonable!

Done in #504

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

# Commits The following comments are included in this release. Some of these cherry-picked and released in v1.28.0, but they appear again in the list above. - protocol: Add streaming interfaces (#485) - Move Stream-based interfaces into their own package - Make Streaming interfaces private to allow for safe experimentation (#488) - idl: Return structured ParseError from idl.Parse() (#492) - Add CHANGELOG entry for #492 (#494) - Support "<" in the templating language (#499) - idl: add a Position struct to wrap reported lines (#497) - Add streamwriter implementation (#490) - Add a "StreamReader" which implements "stream.Reader" - Use the "stream.Reader" in the "binary.Reader" - Add code generation for all wire types for stream encoding (#500) - Generate "Decode" for "enums" that will directly decode (#495) - Provide "decode" code generation for the streaming variants for all other types (#496) - idl: record document positions on constant nodes (#503) - ast: move idl.Position to the ast package (#504) - idl: replace internal.Position with ast.Position (#505) - Expose stream protocol method to close Writer (#506) - idl: add column numbers to parse error positions (#507) - idl: record full positions for constants (#508) - Mark assertParseCases() as a test helper (#509) - protocol/stream: Define enveloping interfaces (#511) - protocol/stream: Declare interface for encoding envelopes (#513) - binary/StreamWriter: Borrow => New; unexport Return (#515) - stream: add Close method, pool binary reader (#514) - binary/reader: Return to pool after ReadValue (#517) - binary/reader: Skip fixed width collections faster (#518) - binary/stream/reader: Fast-path offsetReader skips (#519) - binary: Move Responders and Protocol into package (#516) - benchmark: Refactor into a suite (#520) - Upgrade to Ragel version 6.10 (from 6.9) (#523) - Responder: Deduplicate interface (#524) - gen/quick_test: Add missing types (#525) - enum/json: Support rejecting unknown values (#502) - Back to development - Upgrade to golang.org/x/tools version 0.1.5 (#529) - ast: add column values to the AST nodes (#522) - stream: Implement Request and Response handling with Enveloping (#526) - offsetReader: Implement io.Seeker - binary/ReadRequest: Use io.Seeker if available - StreamReader: Use Seeker instead of offsetReader - protocol/stream: Unembed stream.Protocol from stream.RequestReader (#532) - thrifttest: Add mocks for streaming interfaces (#527) - streaming: Unembed iface.Private in streaming-based interfaces (#533) - Regenerate files for tests after merging `streamdev` - ast: formally declare CppInclude as a Node (#536) - ast: add Annotations(Node) []*Annotations (#537) - Preparing release v1.29.0 # API changes I ran apidiff on all packages in v1.28.0 and compared it with this release. Removing changes to gen/internal/tests, the result is: ``` --- go.uber.org/thriftrw/ast --- Compatible changes: - Annotation.Column: added - Annotations: added - BaseType.Column: added - Constant.Column: added - ConstantList.Column: added - ConstantMap.Column: added - ConstantMapItem.Column: added - ConstantReference.Column: added - CppInclude.Column: added - DefinitionInfo.Column: added - Enum.Column: added - EnumItem.Column: added - Field.Column: added - Function.Column: added - Include.Column: added - ListType.Column: added - MapType.Column: added - Namespace.Column: added - Position.Column: added - Position.String: added - Service.Column: added - ServiceReference.Column: added - SetType.Column: added - Struct.Column: added - TypeReference.Column: added - Typedef.Column: added --- go.uber.org/thriftrw/envelope/stream --- NEW PACKAGE --- go.uber.org/thriftrw/gen --- Compatible changes: - StreamGenerator: added --- go.uber.org/thriftrw/internal/envelope/exception --- Compatible changes: - (*ExceptionType).Decode: added - (*TApplicationException).Decode: added - (*TApplicationException).Encode: added - ExceptionType.Encode: added --- go.uber.org/thriftrw/plugin/api --- Compatible changes: - (*Argument).Decode: added - (*Argument).Encode: added - (*Feature).Decode: added - (*Function).Decode: added - (*Function).Encode: added - (*GenerateServiceRequest).Decode: added - (*GenerateServiceRequest).Encode: added - (*GenerateServiceResponse).Decode: added - (*GenerateServiceResponse).Encode: added - (*HandshakeRequest).Decode: added - (*HandshakeRequest).Encode: added - (*HandshakeResponse).Decode: added - (*HandshakeResponse).Encode: added - (*Module).Decode: added - (*Module).Encode: added - (*ModuleID).Decode: added - (*Plugin_Goodbye_Args).Decode: added - (*Plugin_Goodbye_Args).Encode: added - (*Plugin_Goodbye_Result).Decode: added - (*Plugin_Goodbye_Result).Encode: added - (*Plugin_Handshake_Args).Decode: added - (*Plugin_Handshake_Args).Encode: added - (*Plugin_Handshake_Result).Decode: added - (*Plugin_Handshake_Result).Encode: added - (*Service).Decode: added - (*Service).Encode: added - (*ServiceGenerator_Generate_Args).Decode: added - (*ServiceGenerator_Generate_Args).Encode: added - (*ServiceGenerator_Generate_Result).Decode: added - (*ServiceGenerator_Generate_Result).Encode: added - (*ServiceID).Decode: added - (*SimpleType).Decode: added - (*Type).Decode: added - (*Type).Encode: added - (*TypePair).Decode: added - (*TypePair).Encode: added - (*TypeReference).Decode: added - (*TypeReference).Encode: added - Feature.Encode: added - ModuleID.Encode: added - ServiceID.Encode: added - SimpleType.Encode: added --- go.uber.org/thriftrw/protocol --- Compatible changes: - BinaryStreamer: added --- go.uber.org/thriftrw/protocol/binary --- Compatible changes: - Default: added - EnvelopeV0Responder: added - EnvelopeV1Responder: added - NewStreamReader: added - NewStreamWriter: added - NoEnvelopeResponder: added - Protocol: added - Responder: added - StreamReader: added - StreamWriter: added --- go.uber.org/thriftrw/protocol/envelope --- NEW PACKAGE --- go.uber.org/thriftrw/protocol/stream --- NEW PACKAGE --- go.uber.org/thriftrw/thrifttest/streamtest --- NEW PACKAGE --- go.uber.org/thriftrw/version --- Incompatible changes: - Version: value changed from "1.28.0" to "1.29.0" ```

jparise commented Jun 25, 2021

View reviewed changes

idl/internal/parser.go Outdated Show resolved Hide resolved

idl: return ParseResult from internal.Parse()

478e823

This gives us room to grow the set of returned data without expanding the number of return values.

abhinav approved these changes Jun 25, 2021

View reviewed changes

abhinav added 2 commits June 25, 2021 09:30

config_test.go: Fix license date

802ed04

config_test.go: simplify assert.NoError

2b229c1

abhinav merged commit e4bf17f into thriftrw:dev Jun 25, 2021

jparise deleted the idl-config-pos branch June 25, 2021 16:40

jparise commented Jun 25, 2021

View reviewed changes

abhinav mentioned this pull request Aug 30, 2021

Release v1.29.0 #539

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

idl: record document positions on constant nodes #503

idl: record document positions on constant nodes #503

jparise commented Jun 25, 2021

codecov bot commented Jun 25, 2021 •

edited

Loading

abhinav left a comment

abhinav Jun 25, 2021

jparise Jun 25, 2021 •

edited

Loading

abhinav Jun 25, 2021

jparise Jun 25, 2021

abhinav Jun 25, 2021

jparise Jun 25, 2021 •

edited

Loading

idl: record document positions on constant nodes #503

idl: record document positions on constant nodes #503

Conversation

jparise commented Jun 25, 2021

codecov bot commented Jun 25, 2021 • edited Loading

Codecov Report

abhinav left a comment

Choose a reason for hiding this comment

abhinav Jun 25, 2021

Choose a reason for hiding this comment

jparise Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

abhinav Jun 25, 2021

Choose a reason for hiding this comment

jparise Jun 25, 2021

Choose a reason for hiding this comment

abhinav Jun 25, 2021

Choose a reason for hiding this comment

jparise Jun 25, 2021 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jun 25, 2021 •

edited

Loading

jparise Jun 25, 2021 •

edited

Loading

jparise Jun 25, 2021 •

edited

Loading