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

Correctly marshal/unmarshal struct fields of slice type with nil values to/from CborNull #27

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NexZhu
Copy link
Contributor

@NexZhu NexZhu commented Jul 10, 2020

Currently struct fields of slice type with nil values will be marshalled into empty slice, not Null representation in CBOR.

For example:

type Example struct {
	slice []byte
}

e1 := Example{
	slice: nil,
}

e2 := Example{
	slice: []byte{},
}

Currently, e1 and e2 will be marshalled into the same byte sequence, which will all be unmarshalled into the same value as e2. So below will fail, which is undesirable IMO:

assert.Equal(e1, e1MarshalledThenUnmarshalled)

This PR fix it by correctly marshal/unmarshal struct fields of slice type with nil values to/from CborNull

@Stebalien
Copy link
Collaborator

I believe this is intentional given that go treats nil and [] the same way. Otherwise, some empty arrays will encode to an empty cbor array, and others to Null. We also reject Null on decode.

@NexZhu
Copy link
Contributor Author

NexZhu commented Jul 24, 2020

I believe this is intentional given that go treats nil and [] the same way. Otherwise, some empty arrays will encode to an empty cbor array, and others to Null. We also reject Null on decode.

fmt.Println([]int{} == nil) // prints false
fmt.Println(len([]int{})) // prints 0
fmt.Println(len(nil)) // error

I think these all show that Go doesn't treat them exactly the same way, the behavior of append is just a special case.

Also, it can be useful to represent them differently in CBOR (which is my case), for example:

b1 := Block{
	Timestamp: []byte,
        ... some other block header fields...
	Transactions: nil, // means b1 is used to transfer block header fields, transactions are omitted
}

b2 := Block{
	Timestamp: []byte,
        ... some other block header fields...
	Transactions: []Transaction{}, // means block b2 contains 0 transaction
}

@Stebalien
Copy link
Collaborator

While it is possible to distinguish between a nil slice and a non-nil slice, doing so in this case would just lead to more confusion given that most go programs use them interchangeably. If you need to distinguish between "empty" and "nil", I recommend implementing support for *[]foo (pointer to slice).

Note: nil and a nil slice aren't quite the same thing. nil is 0. ([]T)(nil) (nil slice) is actually SliceHeader{Data: 0, Len: 0, Cap: 0}.

fmt.Println([]int{} == []int{}) // doesn't compile
fmt.Println(len([]int{})) // prints 0
fmt.Println(len(([]int)(nil))) // prints 0

@NexZhu
Copy link
Contributor Author

NexZhu commented Jul 25, 2020

While it is possible to distinguish between a nil slice and a non-nil slice, doing so in this case would just lead to more confusion given that most go programs use them interchangeably. If you need to distinguish between "empty" and "nil", I recommend implementing support for *[]foo (pointer to slice).

SGTM, is there a plan to implement support for pointer to slice? @whyrusleeping

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 this pull request may close these issues.

2 participants