Fix escaping the backslash character in text marshaling

[fcelda]

When marshaling structures into text representation, capnp-go doesn't escape the backslash characters.

See the following reproducer:

using Go = import "/go.capnp";
@0xd2f8840130e47262;
$Go.package("main");
$Go.import("bug");

struct Bug {
  data @0 :Data;

package main

import (
	"fmt"

	"capnproto.org/go/capnp/v3"
	"capnproto.org/go/capnp/v3/schemas"
)

func main() {
	arena := capnp.SingleSegment(nil)
	_, seg, _ := capnp.NewMessage(arena)
	bug, _ := NewBug(seg)

	bug.SetData([]byte{92, 0})

	RegisterSchema(schemas.DefaultRegistry)
	fmt.Println(bug) // prints (data = "\\x00") instead of (data = "\\\x00")
}

I'm attaching a fix for the bug and extended unit tests.

[davidhubbard]

Thank you for submitting a pull request! I'd much rather see needsEscape() altered to detect backslash, double quote and single quote, preferring to keep both the switch and needsEscape() in sync.

[fcelda]

Thank you for the review, @davidhubbard. I returned to the previous version of the code and just updated needsEscape to handle the missing characters. I don't find it super readable but that's probably just a preference. Let me know if this is what you imagined.

As a side note, I don't know what kind of a quoted string is this expected to produce. This will produce a string literal that is valid in Go. However I looked at quoted string parsing in capnp and the quoting there is incompatible. For instance, \' is not a valid escape sequence in capnp.

I don't mind too much, I just want to get the backslash fixed because that is a regression from zombiezen.com/go/capnproto2.

[davidhubbard]

Thanks! Yes, this is what I was hoping for. I understand what you mean about reading the code - it makes sense to me, but I'm still happy for the detailed feedback.

For your question about quoted string parsing in capnp I think what @lthibault said on matrix sums it up: "Do you guys have any thoughts about #546? My understanding of that feature was that it was intended to print data to the screen (a la Println), not to serialize."

In short, I believe everyone is in favor of this change because:

• It fixes backslash quoting (a regression from https://pkg.go.dev/zombiezen.com/go/capnproto2)
• We're always grateful for pull requests, and this one is well written
• It fixes the underlying issue that needsEscape() was not in sync with Append()

And I note the caveats we've unconvered:

• This wasn't intended to be fed into capnp string parsing, I think it was just for printing to the screen. Specifically \' is not a valid escape sequence in capnp.
• This code could use some love along the lines of becoming more readable (or maybe just some comments?)

[fcelda]

Thank you for detailed comments and merging.

[fcelda]

Thank you, @davidhubbard. Please, would it be possible to tag a new release with these changes included?

[lthibault]

@fcelda There are some regressions on the tip of main, so I'm not quite comfortable tagging a new release. However, I think you can run go get capnproto.org/go/capnp/v3@66b5b8e003bad68348fa7619a03d99afc729eb5f if you want to pin to this version. The regressions have to do with pipelined RPC calls, so if you're not using those, you should be fine.

[fcelda]

That's what I did. Understood. Thank you!