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

encoding/prototext: parser should allow whitespace between minus sign and number in negative numeric literal #1526

Closed
jhump opened this issue Mar 2, 2023 · 3 comments

Comments

@jhump
Copy link
Contributor

jhump commented Mar 2, 2023

The text format specification explicitly states that whitespace and comments are allowed between the minus sign and the subsequent numeric component of a negative numeric literal. I can confirm that the C++ implementation correctly adheres to this. But the Go implementation in prototext does not.

Here's a trivial example.

// test.proto
syntax = "proto3";

message Foo {
  int32 id = 1;
}

Compile the above to Go via command like so:

protoc --go_out=paths=source_relative,Mtest.proto=foo.com/test/main:. test.proto

Here's the rest of the Go program

// main.go (in same directory as test.pb.go generated from above)
package main

import (
  "fmt"
  "google.golang.org/protobuf/encoding/prototext"
)

func main() {
  data := "id : - 23"
  var msg Foo
  err := prototext.Unmarshal([]byte(data), &msg)
  if err != nil {
    panic(err)
  }
  fmt.Println(&msg)
}

The above program panics with the following error:

proto: syntax error (line 1:6): invalid scalar value: -

I would instead expect this to echo back the message data with output like so:

id:-23
@puellanivis
Copy link
Collaborator

Unexpected standards esotericisms. This reminds me a whole lot about how technically you can embed comments into email addresses as well, except that no one actually supports that.

@cybrcodr
Copy link
Contributor

cybrcodr commented Mar 2, 2023

Iirc, they didn't have the textproto spec publicized yet at the time this was written and no compliance test for it too...
https://github.com/protocolbuffers/protobuf-go/blob/master/internal/encoding/text/decode_number.go#L79-L81

Here's a possible solution. parseNumber func needs to account for the extra whitespace and/or comments. It may also need to produce the bytes/string for properly setting Token.str field in parseNumberValue.

I currently don't have the bandwidth to work on this. If anyone has, feel free to send CL.

@jhump
Copy link
Contributor Author

jhump commented Mar 3, 2023

I currently don't have the bandwidth to work on this. If anyone has, feel free to send CL.

@cybrcodr, I got ya. https://go-review.googlesource.com/c/protobuf/+/473015
Thanks for the pointer!

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

No branches or pull requests

3 participants