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

Parser cannot handle multi-line option strings #200

Closed
bufdev opened this issue May 22, 2019 · 6 comments · Fixed by #207
Closed

Parser cannot handle multi-line option strings #200

bufdev opened this issue May 22, 2019 · 6 comments · Fixed by #207

Comments

@bufdev
Copy link
Contributor

bufdev commented May 22, 2019

https://github.com/googleapis/googleapis/blob/6ea045ad2ed95863557b9856760fa8760d8daee0/google/cloud/language/v1/language_service.proto#L34

$ goprotoc -o /dev/null google/cloud/language/v1/language_service.proto
google/cloud/language/v1/language_service.proto:36:5: syntax error: unexpected string literal, expecting ';'
@jhump
Copy link
Owner

jhump commented May 22, 2019

Oh, wow, I've never seen this before. Nice find!

Looks like it's not just option strings -- it's all string literals:

syntax = "prot"
        "o3";

message Foo{}

@bufdev
Copy link
Contributor Author

bufdev commented May 22, 2019

Yep - I'm masquerading like I know something about compilers, but I'll be honest, it's a course I really regret not taking in college, so the modifications I'm making to proto.y are just following the examples and extrapolating. This one might be out of my league without a lot of work, can you tackle it?

@jhump
Copy link
Owner

jhump commented May 23, 2019

This one's going to be a bear. I was hoping I could just hack this into the lexer, but it looks like protoc also supports trailing comments after each string piece. I'm not even sure what to do with those comments. I think I'll just accumulate all of them, separated by newlines, and attach it to the resulting combined string literal AST node as a trailing comment.

@bufdev
Copy link
Contributor Author

bufdev commented May 23, 2019

I wonder what protoc does, I can do some investigation if you’d like?

@jhump
Copy link
Owner

jhump commented May 23, 2019

Ok. Not nearly as bad as I thought. I did not look into what protoc does. I re-used the same pattern for handling negative floats (which are a composite in the AST of a unary minus node and a float literal node). That will end up losing comments in between in source info. (But I think that's okay; I know protoc loses a lot of comments, likely due to little things like this, since not every element in the grammar/AST maps to a single field in the descriptor hierarchy.)

If you do have a chance to look that would be cool. For now, I think #207 should be good enough. Want to test it with your prototool integration test script?

Also, wasn't there another bug, about leading comments missing trailing newlines? Do you want to file that? (I think it's a lot lower priority, but if I might get around to it...)

@bufdev
Copy link
Contributor Author

bufdev commented May 23, 2019

Yep I'll run this through my script right now and make sure it works.

I'll do some more integration testing on the SourceCodeInfos in a bit and let you know - I want to make sure I understand the specific difference, if any. I'm also going to go through https://github.com/emicklei/proto/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aclosed and do equivalent test cases for desc/protoparse (and I'll put up a PR as I create them, unless you aren't interested) - emicklei/proto has effectively been widely integration tested via usage of Prototool for a year, so it has seen a lot of edge cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants