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

proposal: encoding/json: "nonil" struct tag to marshal nil slices and maps as non-null #27589

Open
pjebs opened this issue Sep 10, 2018 · 41 comments · May be fixed by #27813
Open

proposal: encoding/json: "nonil" struct tag to marshal nil slices and maps as non-null #27589

pjebs opened this issue Sep 10, 2018 · 41 comments · May be fixed by #27813

Comments

@pjebs
Copy link
Contributor

@pjebs pjebs commented Sep 10, 2018

There have been many reports about encoding a nil slice or map as [ ] or { } respectively.

See: #2278

Currently it is encoded as null. This is a source of bugs in many non-theoretical cases (eg consumers of JSON APIs). There are many issues on Google about others falling into this pitfall so it is at least not uncommon.

It would be nice if a struct tag can be used to signify that we intend the nil slice or map to be treated as an empty slice or map respectively for the purposes of marshalling.

Current solutions include: https://github.com/helloeave/json. This is not a nice solution because it requires the assistance of a third-party package.

This is how it's implemented in third party lib: homelight/json@7b6e442

My PR can be found here: #27813 (subject to an agreed struct tag name, which when decided, can be updated in the PR)

@mvdan mvdan changed the title Feature request: Struct tag for json to marshall null slices/maps as non-nil proposal: encoding/json: struct tag to marshal nil slices and maps as non-null Sep 10, 2018
@gopherbot gopherbot added this to the Proposal milestone Sep 10, 2018
@gopherbot gopherbot added the Proposal label Sep 10, 2018
@mvdan
Copy link
Member

@mvdan mvdan commented Sep 10, 2018

I've retitled this to be a proposal.

I'm not sure if it's a good idea, but it's certainly feasible and wouldn't add much new API. /cc @bradfitz @dsnet

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Sep 12, 2018

For slices and maps, it should be noted that the proposed struct tag is mutually exclusive to omitempty. For backwards compatibility, if both omitempty and the new struct tag is present, then omitempty takes precedence.

@pjebs pjebs closed this Sep 13, 2018
@pjebs pjebs reopened this Sep 13, 2018
@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Sep 22, 2018

Here is my pull request. I hope it gets accepted because I believe it adds value to the "Go" programming language.

#27813

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Sep 22, 2018

I called the struct tag: nonil

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Sep 24, 2018

"heed" is a unique word, at least in terms of Go's existing API.

If this is accepted (once it's reviewed together with all the existing JSON proposals), I suspect we'd want to change "heed" to something else.

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Sep 24, 2018

I scoured the dictionary and thesaurus for appropriate words that were short and exemplified the intention but couldn't find anything. Hopefully a better word can be found because I was initially hesitant of the word too - although admittedly it is growing on me.

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Sep 24, 2018

@bradfitz Any idea of the timeframe to when all the JSON proposals will be examined? Some of them are now 2+ years. The pivotal proposal is 7+ years.

@QtRoS
Copy link
Contributor

@QtRoS QtRoS commented Sep 25, 2018

@bradfitz @pjebs what about nullsafe or nullfree or nonull?
+1 that heednull is unacceptable.

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Sep 26, 2018

For Go2, omitempty should become omitzero. The zero value and what is colloquially called "empty" are different for maps and slices.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 3, 2018

Will add this to my JSON scan for this cycle.

@rsc
Copy link
Contributor

@rsc rsc commented Oct 3, 2018

Maybe emptynil.

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Oct 3, 2018

HEED
verb
1.
pay attention to; take notice of.

noun
1.
careful attention.
@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Oct 3, 2018

Seems like precisely the right word. It's not part of the Go vocabulary, but it seems like a great addition in various future contexts for programming languages in general.

eg. heed memory, heed security etc

@rsc
Copy link
Contributor

@rsc rsc commented Oct 10, 2018

heednull means "pay attention to null" by not respecting that the value is null and instead printing something other than "null"? That seems backward to me.

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Oct 10, 2018

If it was called heednil instead, then one can argue that the slice or map was nil, and you are instructing the encoder to "pay attention to it".

I then went from that step to heednull because it flowed off the tongue better. The null in heednull was not really a reference to the eventual json output null.

Perhaps due to the confusion, and ambiguity, heednil is closer to being "Go-like".

@rsc rsc added the Proposal-Hold label Oct 17, 2018
@rsc
Copy link
Contributor

@rsc rsc commented Oct 17, 2018

On hold for JSON sweep.

@pjebs
Copy link
Contributor Author

@pjebs pjebs commented Oct 17, 2018

What does that mean?

@dsnet
Copy link
Member

@dsnet dsnet commented Oct 17, 2018

The json package already has a bloated API surface. Any additions need to carefully considered together with all the other proposals. I alluded to the need for this: #27813 (comment)

The "JSON sweep" is a sweep through all the JSON proposal trying to come up with a comprehensive yay/nay to each proposal in an effort to balance features desired with not adding too much technical debt.

@tommie
Copy link

@tommie tommie commented Dec 31, 2018

Current solutions include: https://github.com/helloeave/json

This fork (which sadly I can't use because AppEngine doesn't have sync.Map, it seems) sets an encoder-wide option instead of using a struct tag. It feels to me that either an entire system is fine with having null, or there is an expectation that all slices are non-null. Why is the proposal to do this as a struct tag instead of an encoder option?

@QtRoS
Copy link
Contributor

@QtRoS QtRoS commented Dec 31, 2018

@tommie in my opinion the problem is that any system doesn't work in isolation. Complex software has complex integrations with different technologies and different expectations. Global flag is not versatile enough.

@tommie
Copy link

@tommie tommie commented Dec 31, 2018

Complex software has complex integrations with different technologies and different expectations. Global flag is not versatile enough.

That is a non-answer, IMHO (and somewhat belittling). I can see different complex software requiring different options, and AFAIK, this thread has not discussed the already existing solution yet.

  • Say you have a frontend that needs non-null results, and a backend that wants null results. It's not per-field, but per encoder invocation.
  • Say most fields are "arrays", but one field is an optional array. Then it's per-field.

Defining what the default behavior is could still be an encoder option, while allowing per-field overrides. The two solutions are not mutually exclusive.

The problem I'm trying to solve is interoperability with Javascript. Passing null means my browser-side code needs to handle an empty array in a non-idiomatic way, leading to more fragile code. It seems to me @pjebs is trying to solve a different problem with the struct tag solution, and I'm curious what that is.

@zhongliangong
Copy link

@zhongliangong zhongliangong commented Oct 10, 2019

Any updates on this?
I recall the json encoder already having the omitempty tag anyway, which changes the behavior of the json encoder via a struct tag.

Would make sense to have an empty array in json to be distinct from null, the same way that an empty slice in golang is distinct from nil.

@inluxc
Copy link

@inluxc inluxc commented Oct 10, 2019

This is happening to me. :(
Please fix it

@pd93
Copy link

@pd93 pd93 commented Oct 23, 2019

If you want it to marshal an empty slice as an empty JSON array ([]) instead of nil, you can just initialise the slice with:

mySlice := make([]<type>, 0)

This will cause json.Marshal() to write a [] instead of null. A nil slice is not the same as an empty slice and Golang is recognising that when encoding into JSON. This is surely behaving as it should?

Unless I'm missing something completely :')

Example: Go Playground
Credit: https://www.danott.co/posts/json-marshalling-empty-slices-to-empty-arrays-in-go

Edit: Made the code clearer

@VadimKulagin
Copy link

@VadimKulagin VadimKulagin commented Oct 24, 2019

I think the json tag option "notnull" or "expandnull" will be more clear.

In addition, it is worth updating the name of this issue, because this functionality can be useful not only for structure fields but also for encoding simple slices, maps, maps of slices, slices of maps, etc.

This can be implemented using Encoder setters, as in https://github.com/helloeave/json

@tommie
Copy link

@tommie tommie commented Oct 24, 2019

This is surely behaving as it should?

Yes, but it's more error-prone doing it at every call site. Simply nil is well-known to mean "empty slice" in Go, and making grammar decisions on the subtle difference between nil and []type(nil) is a bit scary.

@pd93
Copy link

@pd93 pd93 commented Oct 24, 2019

nil is well-known to mean "empty slice" in Go

@tommie This is simply not true. Consider the following snippet (go playground):

var strSlice1 []string
strSlice2 := []string{}
strSlice3 := make([]string, 0)
fmt.Println(strSlice1 == nil) // true
fmt.Println(strSlice2 == nil) // false
fmt.Println(strSlice3 == nil) // false
  • strSlice1 is declared, but not defined. It equals nil because no variable has been assigned to it yet. It is not an empty slice, it is a nil slice.
  • strSlice2 is declared and defined. It does not equal nil because it has had the value of an empty slice assigned to it.
  • strSlice3 is much the same as strSlice2. It's just been defined by using the built-in make command.

When Golang is marshalling your structure into JSON. It is recognising the distinction between a nil slice and an empty slice. They are fundamentally different and should not be recognised the same way.

Another example for you (go playground):

type MyStruct struct {
    Name string
}

// Causes nil pointer exception
func NewMyStruct1() (s *MyStruct) {
    s.Name = "struct1"
    return
}

// Works
func NewMyStruct2() (s *MyStruct) {
    s = &MyStruct{}
    s.Name = "struct2"
    return
}

When creating a new instance of a structure. We cannot simply declare it and then assign to one of its fields. (see NewMyStruct1()) This causes a nil pointer exception. It fails to assign because the variable is not an empty structure. It is a nil structure. There is no allocated memory for that variable, just a pointer that points to nothing (nil). In order for us to assign to the Name field, we must define the structure as well as declare it (see NewMyStruct2()).

The same principles apply to slices (they are essentially structures after-all). If you aren't initialising your slices properly, you're not writing your code correctly. Golang is just slightly more forgiving in terms of errors because there are no fields on a slice structure. So there is no opportunity to throw a nil pointer exception.


Again - unless I'm missing something entirely, this feature is essentially adding bloat to an already bloated library for zero benefit and is encouraging developers to write lazy code with uninitialised slices.

@tommie
Copy link

@tommie tommie commented Oct 24, 2019

Yes, there are many ways to create an empty slice, so the question is what the different cases mean. []int{} and make([]int, 0) are of no consequence to this FR, so I'll leave that. nil is what matters here. Go has documented opinions on

var strSlice1 []string

being the preferred form: https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

See also https://tour.golang.org/moretypes/12 for what we're teaching developers on one of the first "slides" about slices.

Which, to me, implies that a simple nil should be enough for code to interpret it as an empty slice.

@tommie
Copy link

@tommie tommie commented Oct 24, 2019

this feature is essentially adding bloat to an already bloated library

Perhaps, but whether something adds bloat to a common library is less interesting than what benefits it adds for all the users of the library. So I still think this discussion is well worth having.

for zero benefit

This is needlessly dismissive.

encouraging developers to write lazy code with uninitialised slices.

As discussed above: it's encouraged everywhere in Go.

@pd93
Copy link

@pd93 pd93 commented Oct 24, 2019

@tommie I don't think you quite understood my post. The line of code you just posted does not create an empty slice. Full stop. It simply creates a []string pointer that points to no information (aka a nil pointer). This is entirely different from a []string pointer that points to an empty string slice. They are not equivalent.

A nil pointer will give true when comparing it to nil.
A pointer to an empty slice does not.

The documentation you linked to literally re-enforces my point:

Note that there are limited circumstances where a non-nil but zero-length slice is preferred, such as when encoding JSON objects (a nil slice encodes to null, while []string{} encodes to the JSON array []).


whether something adds bloat to a common library is less interesting than what benefits it adds for all the users of the library

This doesn't add anything to the library. What you're searching for is already achievable by creating an empty slice instead of a nil slice.

This is needlessly dismissive.

Not trying to be rude. Just explaining why this is unnecessary. It literally doesn't add any functionality to the codebase and it adds another struct tag which needs documenting and explaining. This is the definition of technical debt.

As discussed above: it's encouraged everywhere in Go.

The documentation you linked specifically recommends my approach when encoding to JSON as quoted above.

@tommie
Copy link

@tommie tommie commented Oct 24, 2019

The line of code you just posted does not create an empty slice. Full stop.

I agree, but I have yet to see Go code that makes this distinction, outside of encoding/json.

It literally doesn't add any functionality to the codebase

I understand we can't break backwards compatibility by changing the default, so whatever the solution, it would add code, yes. And what we're discussing here is a subset of the current functionality (not being able to express a nil slice), so it doesn't really add functionality. I agree.

But it makes the normal case simpler (and safer, depending on how you see it: less chance of introducing an encoding bug). The person who introduces a struct in a codebase shouldn't have to tell everyone instantiating that struct to remember to initialize slices just because this struct happens to be JSON-encoded later in the code. We have a standard library that leads to fragile code, when it really doesn't have to. JSON has its origins in the Javascript environment, and passing a null where the data model suggests an array is not a common thing, unlike in Go. C.f. where a string cannot result in a null, but a *string can. nil being the zero value for a slice is a Go:ism that is foreign to most (all?) other JSON decoders.

it adds another struct tag

I'd be happy with just a json.Encoder option to treat all nil slices as empty. It's what https://github.com/helloeave/json does. It's already been shot down once in this thread, though I disagree with the argument that an encoder option is something global.

This is the definition of technical debt.

Or correcting an original design mistake, where focus was to make it expressive rather than easy to use.

The documentation you linked specifically recommends my approach when encoding to JSON as quoted above.

Yeah, I should have pointed that out. We cannot include the JSON part in this discussion, or we'll create a circular argument. The main point of the linked review comment is not about JSON, but about slice variables in general. The JSON paragraph is, IMHO, a side effect of the JSON library behaving unexpectedly.

I guess I'm asking for a different trade-off between improved usability and bloat than you are. I understand you don't see this as leading to improved user code, but I've exhausted my arguments.

@pd93
Copy link

@pd93 pd93 commented Oct 24, 2019

I have yet to see Go code that makes this distinction, outside of encoding/json

Okay, after re-reading everything in this thread, I'm beginning to understand your point of view a bit better. The style docs state that:

When designing interfaces, avoid making a distinction between a nil slice and a non-nil, zero-length slice, as this can lead to subtle programming errors.

and the standard fmt package does indeed behave like this as you can see in this example. This seems to directly contradict the previous paragraph that states that in the encoding/json library:

a nil slice encodes to null, while []string{} encodes to the JSON array [].

I agree that this seems like a design mistake. At the very best, it's inconsistent and I'm now curious why nil slices are preferred over empty slices anyway. I've run into this problem a fair few times, and I've always just used make() to fix it. What would be the consequence of enforcing empty slices globally as a code style?

I'd be happy with just a json.Encoder option

I think this was downvoted because developers wouldn't be able to turn it on/off on a per-structure basis. Though, I'd be curious to ask what @QtRoS's use-case is for marshalling to null in some cases, but not others. I think I'd lean towards the option over a struct tag if any change is necessary at all unless there was a compelling use case for null marshalling.

@tommie
Copy link

@tommie tommie commented Oct 25, 2019

This seems to directly contradict the previous paragraph that states that in the encoding/json library:

Yeah, I think the json paragraph must be seen as documenting rather than normative. Otherwise it makes very little sense there.

I'm now curious why nil slices are preferred over empty slices anyway.

I've wondered the same for a couple of years. :\ I also don't like that these are/were the idiomatic ways to initialize booleans, depending on initial value:

var b bool
b := true

Though now I can't find the reference to that. Maybe it was replaced by the "empty slice" section. Something, somewhere, used to say to prefer var style when initializing something to its zero value.

What would be the consequence of enforcing empty slices globally as a code style?

Since it's easier to instantiate nil (the zero value), I think it would lead to a language whose implementation makes one style easy, and prescribes another style. I'm not against it, but I'm not sure it would scale well as a code base grows.

Though, I'd be curious to ask what @QtRoS's use-case is for marshalling to null in some cases, but not others.

+1

@QtRoS
Copy link
Contributor

@QtRoS QtRoS commented Oct 26, 2019

Hi, @tommi @pd93
To be honest, I didn't remember saying:

marshalling to null in some cases, but not others

so I've reread whole conversation too. I guess that this phrase caused such interpretation:

Complex software has complex integrations with different technologies and different expectations. Global flag is not versatile enough.

But I meant something a bit different so let me explain it. In my experience a single piece of software (one application, one program) can have multiple integrations (built on top of JSON) with external software, for example:

  • SQL DB
  • NoSQL DB
  • External services built with the same technology (e.g. Go <-> Go)
  • External services built with different technology (e.g. Go <-> Java)
  • Plain text communication (via filesystem)
  • Frontend (HTTP)
  • etc.

(It's obviously not good practice, but it's possible in the real world)
Every external piece of software can have different expecations about null and [] in JSON. Satifsying all of them simulteneously is not possible with global application0-wide encoder option. So that's not versatile enough. That's it.

@lukebarton
Copy link

@lukebarton lukebarton commented Nov 5, 2019

Real world issue I've observed.

My issue

I have an json API contract that says my response must look like this:

{
  "foo": ["some"],
  "bar": ["values", "for the consumer"], 
  "baz": []
}

(empty-array expected over null)

So naturally I create a type to help me satisfy the contract

type MyResponse struct {
  Foo: []string `json:"foo"`,
  Bar: []string `json:"bar"`,
  Baz: []string `json:"baz"`,
}

Then someone else comes along and creates a mock response for testing

myMock := MyResponse{
  Foo: []string{"blah"},
  Bar: []string{"blah"},
}

The type checker is happy with a nil slice for Baz

json.marshal(myMock)

for already understood reasons, this results in

{
  "foo": ["blah"],
  "bar": ["blah"],
  "baz": null,
}

which, of course does not satisfy the contract as intended.

In conclusion of that, the type system is not doing anything wrong, but it's not helping us satisfy contracts we have in json.

Moving forward

So what can we do? The obvious choice is a constructor to initialise MyResponse -- but Go doesn't have any way of enforcing constructors, so it has to be opt-in as far as usage goes, which is going to be forgotten and the type checker still isn't going to complain to save us from ourselves. It's not a satisfactory solution.

Digging deeper

I began thinking about where the fault lies - is it in Go's nil slices? is it in the json encoder? is it in our usage of types?

I concluded that the error does lie in the choice the json encoder makes in choosing how to encode a nil-slice and I'll do my best to explain why I think that's the case.

The author of the encoder chose to encode a nil-slice as null -- but why? The code that returns null is inside a function which knows it's encoding a slice.

func (se sliceEncoder) encode(e *encodeState, v reflect.Value, opts encOpts) {
if v.IsNil() {
e.WriteString("null")
return
}
se.arrayEnc(e, v, opts)
}

The reason that decision was made seems to be because it's nil under the hood, and nil == null.

So now I want to consider, why is the zero value of a slice nil in the first place? Correct me if I'm wrong (I'm relatively new to Go) but it's a memory allocation optimisation, meaning Go only needs to allocate memory when it gets it's first values -- which is totally smart when designing a memory-efficient language!

So, I'll say that nil-slices are just an implementation detail of Go in it's aim of being memory efficient out-of-the-box as a programming language -- It looks like a slice, it quacks like a slice, but aha! it's a nil under the hood -- but it's still a slice.

Aha!

Now let's look at the perspective of the encoder - the encoder appears to be encoding the underlying nil to null. But now we understand that the slice being nil is just an implementation detail of Go lang. The encoder should be encoding the values in the context of their type eg. []string to [], and definitely not converting the underlying zero-value-of-a-slice-nil to it's closest json equivelent of null. In wonderful irony, representing the underlying nil (which is a memory allocation optimisation for Go) as null in json actually costs more bytes over-the-wire than the empty array version []!

So how do we address this misstep? Ideally, mirroring omitempty and as the less common behaviour, nullempty would encode empty arrays as null. But to be backward compatible, something like arrayempty and mapempty, or allowempty might be best.

Summary

In summary, nil-slices are an implementation detail of the Go language and nil-slices should be treated as plain-old slices by the json encoder - it's a slice and it's empty, and it should be represented in json as [] when specifying something like allowempty to enable this new behaviour in a backwards compatible way.

@lukebarton
Copy link

@lukebarton lukebarton commented Nov 7, 2019

I decided to do something productive and created a change over at:
https://go-review.googlesource.com/c/go/+/205897

I think it's an improvement over @pjebs PR (which can be found here: #27813) for the following reasons:

  • The previous PR had a naming issue which was unresolved. As such I spent a long time stewing over the name of the option, and ended up going with nilasempty which I think works very well to describe what it does, and even gives a clue as to why it exists
  • The encoder functions are entirely responsible for the encoding of their nil versions resulting in a cleaner implementation
  • There are separate tests

I think this is the cleanest, most straightforward change that could implement this feature in the existing encoding/json package. There are good, clean, readable tests too.

If the maintainers agree, I really think it'd be great to get this merged so that we can start benefiting from it's presence in the next suitable release! Let's put this shortcoming to bed 👍

@gopherbot
Copy link

@gopherbot gopherbot commented Nov 7, 2019

Change https://golang.org/cl/205897 mentions this issue: encoding/json: Add nilasempty option

@lukebarton
Copy link

@lukebarton lukebarton commented Nov 7, 2019

Okay, apparently this proposal needs to be accepted before it can progress. That's up to the powers that be. We've made our case for why this needs fixing and submitted a pretty good patch/change, which is clean, obvious, in keeping with the existing design of the encoding package and backward compatible. I don't think anything more can be done here.

There's also a code freeze until January. I don't know if that stops the proposal from being accepted or not. There's shippable code ready to go whenever things can move forward in any event.

@jacksgt
Copy link

@jacksgt jacksgt commented Nov 26, 2019

Thank you for submitting this proposal!

I think the name nilasempty is great, because it immediately tells what this struct tag does.

@pjebs pjebs changed the title proposal: encoding/json: struct tag to marshal nil slices and maps as non-null proposal: encoding/json: "nonil" struct tag to marshal nil slices and maps as non-null Feb 21, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented May 8, 2020

Change https://golang.org/cl/136761 mentions this issue: encoding/json: added "nonil" option for json encoding (slice/maps)

@as
Copy link
Contributor

@as as commented May 16, 2020

I provide an explanation in the similar issue here #37711 why I think a tag is not the right way to address this problem even in a third-party package.

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

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.