go-toml v2: requirements

[pelletier]

The goal of this discussion is to gather requirements and feedback on what should go into go-toml v2.

Go-toml has been plagued by several bad design decisions made early on, as this project was just an experiment for me to learn Go and write a parser. As more and more people are migrating from the now unsupported BurntSushi/toml, those issues are becoming bigger blockers for the library adoption.

See also:

• go-toml v2: unmarshaler

Problems

Exposed internals

The toml.Tree structure is publicly exposed, but only partially. This creates all sort of confusion and makes fixing the situation impossible without breaking backward compatibility. Just recently we merged a couple changes to expose some functions to unblock some use cases (#466) but that is not a long term solution.

Poorly designed API

The Set* and Get* method families are the only official way to get data in/out of a Tree. They are too simplistic in their path access, and they don't account for the fact that data in TOML documents can be heterogeneous. Now that the spec is stable, it seems like a good time to revisit the Tree structure to provide better access patterns.

Unmarshaling process is inside-out

This ticket is the main representation of this issue: #252. The process basically creates structs as it parses the file starting at the leaf objects, then assemble them together and eventually set the values on the unamarshal target. This causes overwriting all fields initially set in the target, and cannot be fixed without completely changing the Unmarshal process.

Marshaling speed

Marshal and Unmarshal (and there sibling methods on Encoder and Decoder) are slow (#167). The main reason for their slowness is that they both require building a toml.Tree first, then un/marshaling it to the right structure. This creates a considerable amount of overhead, and is not strictly necessary.

Decoding information loss

When decoding to a Tree, a lot of information is lost on how the file was composed. This completely prevents a common pattern of loading a file, make some edits, encode the file back. Because information is lost, the encoded modified file may look very different than the loaded file (outside of its purposeful modifications). Issues related to this problem: https://github.com/pelletier/go-toml/milestone/5.

Goals

The first two problems require breaking backward compatibility, so they need to be addressed in part of a "v2" operation. The others can be worked internally, but would require deep changes to the lexer/parser so I feel like they should be considered holistically.

Efficient marshaling

Marshaling is one of the main use-cases of go-toml. Got some TOML doc, map it to a Go struct, do some work, maybe turn it back to bytes. This needs to be an efficient process for projects with large amount of files like Hugo. This process doesn't care much about the final structure of the document once marshaled.

• The un/marshaling process should aim to be fast. In its design it should minimize allocations and pave the way for a zero-copy mode for strings.
• Those processes should still be configurable, same as encoding/json in the standard library.
• Meaningful errors by default. The errors reported by those processes should be meaningful and provide pointer to the user on how to troubleshoot them. Optionally can be turned off for performance.

Non-goals

• This process does not need to preserve order.
• Whitespace is not preserved.

Document editing

The second main use cases for go-toml. This process is about working on a TOML document while respecting its structure. This means preserving comments, key types, table types, ordering, etc.

• The API for manipulating a document should be easy to use. No casting data on every method call.
• The user can walk a TOML document when they don't know its structure.
• User has control over the ordering and kind of elements in their document.
• All comments are preserved.

Non-goals

• Whitespace is not guaranteed to be preserved.

[vincentserpoul]

This process does not need to preserve order.

Does it mean the marshaling won't preserve the order? I know that this was actually one of the argument against using go-toml for mozilla/sops in the first place.

On another topic, would you like to add some additional linting (via golangci)?

[pelletier]

Does it mean the marshaling won't preserve the order? I know that this was actually one of the argument against using go-toml for mozilla/sops in the first place.

My thinking is that the order should be whatever reflect gives us. So doing Unmarshal -> Marshal the order might be changed if the original file was not in the same order as what reflect returns. Would that work for your use-case?

On another topic, would you like to add some additional linting (via golangci)?

I'm open to it!

[vincentserpoul]

My thinking is that the order should be whatever reflect gives us. So doing Unmarshal -> Marshal the order might be changed if the original file was not in the same order as what reflect returns. Would that work for your use-case?

sops is used to encrypt values (only values, not keys) in yaml/json/toml, in order to be able to store them without any security issues.
The usage flows are the following:
toml with encrypted values | sops > toml w clear values
toml with clear values | sops > toml w encrypted values

So if you don't see the file with the same comments or in a different order, it's ok but definitely not ideal, better if it preserves order and comments.

On another topic, would you like to add some additional linting (via golangci)?

I'm open to it!

That might be very noisy at the beginning, as there usually are quite a few things to fix, but definitely worth it in the long run imho.
I'll do a PR, you can decide then :)

[pelletier]

Ah so for that use case wouldn't the Document structure be the solution? Preserves the structure and allows you to modify some of the values directly.

[moorereason]

I agree with pretty much every word of this. 👍

For the Efficient marshaling goal, I would add that comments would not be preserved.

PS - You've probably already seen this, but I'll leave this link here: https://dave.cheney.net/high-performance-json.html. Dave's goals may be different, but it has some interesting ideas to consider.

Cc: @bep

[pelletier]

Absolutely! Edited the post to explicitly call out conserving comments in document editing.

One thing that will need to be figured out is how comments are "attached" to elements, if at all. For example:

[table]
# Comment A

# Comment B
# Comment C
key = "value" # Comment D

Which comments should be considered key's comment? D? B+C+D? A+B+C+D? None? I reckon will be at least partially influenced by the API for Document, which I haven't started sketching yet.

[renaissanceGeek]

I'm glad I saw this thread because my team is starting a project that will involve editing toml files and saving them out again and we're debating whether we need to implement our own parser to preserve comments. If you have a design in mind for the Document API, perhaps we can contribute back our implementation of it.

[pelletier]

I don't have a design for the Document API yet, suggestions/contributions are most welcomed!

[pelletier]

@renaissanceGeek now that v2.0.0-beta.3 is out, my next focus is on the Document API. What are the requirements on your end?

[renaissanceGeek]

Thank you so much for your hard work on v2.0.0! 👏

We ended up deferring the toml editing features in our application, so we haven't documented detailed requirements yet except that preserving comments is one of them. But as you pointed out earlier, the devil's in the details there--what makes sense in determining which node to associate the comment to?

My instinctive inclination is to associate all comments with whatever element comes next, and perhaps have separate attributes for inline vs whole-line comments. So perhaps comments []string and inlineComment string. In your example above, the properties of the key element would be something like

{
key: "key",
value: "value",
comments: ["Comment A", "Comment B", "Comment C"],
inlineComment: "Comment D",
}

If the file were read in then written back out, it would lose the extra space between Comment A and Comment B, but that seems reasonable since you declared preserving whitespace to be an explicit non-goal. :-)

[bep]

This all looks great to me, and @pelletier -- you doing this upgrade is very much appreciated, and I'm sure I'm not alone thinking that.

[pelletier]

Thank you, kind words are always motivating! It is long overdue; I'm quite surprised how long people have put up with those deficiencies. The hard part is finding time to work on this, which for now is going to be "a little bit every day".

[pelletier]

Update on this: I have a branch at https://github.com/pelletier/go-toml/tree/v2-wip that has a brand new scanner/parser and implements toml.Unmarshal.

More detailed write up to come, and definitely more work to do (especially on performance), but first benchmark looks decent:

~/s/g/p/g/benchmark$ go test -bench=.
goos: linux
goarch: amd64
pkg: github.com/pelletier/go-toml/v2/benchmark
cpu: Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz
BenchmarkUnmarshalSimple/v2-8         	 1607115	       742.0 ns/op
BenchmarkUnmarshalSimple/v1-8         	  307977	      3915 ns/op
BenchmarkUnmarshalSimple/bs-8         	  516754	      2330 ns/op
BenchmarkReferenceFile/v2-8           	    9604	    129158 ns/op	  111422 B/op	    1381 allocs/op
BenchmarkReferenceFile/v1-8           	    4521	    263808 ns/op	  130566 B/op	    2649 allocs/op
BenchmarkReferenceFile/bs-8           	    4070	    296271 ns/op	   80784 B/op	    1729 allocs/op

(bs = BurntSushi's, v1 is go-toml master, v2 is this branch)

This version builds an AST. @renaissanceGeek plan is to build a Document on top of it. Do you think this makes sense?

[pelletier]

A quick round of removing the very low hanging fruits makes me believe this is a reasonable design.

~/s/g/p/g/benchmark$ go test -bench=.
goos: linux
goarch: amd64
pkg: github.com/pelletier/go-toml/v2/benchmark
cpu: Intel(R) Core(TM) i7-7700 CPU @ 3.60GHz
BenchmarkUnmarshalSimple/v2-8         	 1724246	       695.6 ns/op
BenchmarkUnmarshalSimple/v1-8         	  304478	      3900 ns/op
BenchmarkUnmarshalSimple/bs-8         	  515581	      2405 ns/op
BenchmarkReferenceFile/v2-8           	   39885	     29476 ns/op	   37064 B/op	     152 allocs/op
BenchmarkReferenceFile/v1-8           	    4150	    265336 ns/op	  130566 B/op	    2649 allocs/op
BenchmarkReferenceFile/bs-8           	    4014	    294638 ns/op	   80791 B/op	    1729 allocs/op

(note this is without duplicates and key type checking)

[pelletier]

Here is a write up of the unmarshaler implementation: #488

[moorereason]

@pelletier,
How open are you to other changes to the API? For example:

• Rename Decoder.Strict(bool) as Decoder.DisallowUnknownFields(), which is more intuitive and would match stdlib json Decoder.
• Rename Encoder.Indentation(string) to Encoder.SetIndent(string) to match stdlib json.

[moorereason]

@pelletier, did you intend to implement this change for v2?

Rename Decoder.Strict(bool) as Decoder.DisallowUnknownFields(), which is more intuitive and would match stdlib json Decoder.

[pelletier]

Completely missed that! Will do.

[pelletier]

#731

[pelletier]

Very open to any change! I think those make total sense, will put them in.