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

Don't panic with 'omitempty' and uncomparable type #361

Merged
merged 1 commit into from Jul 28, 2022
Merged

Conversation

arp242
Copy link
Collaborator

@arp242 arp242 commented Jul 28, 2022

Fixes #360

@kkHAIKE
Copy link
Contributor

kkHAIKE commented Jul 30, 2022

i think use Value.IsZero is better..😊

@arp242
Copy link
Collaborator Author

arp242 commented Jul 30, 2022

i think use Value.IsZero is better..blush

You mean checking if the struct has the IsZero() bool method? I looked at that before in #353, and decided to go with this method. Is there any particular reason why you think IsZero() is better?

@kkHAIKE
Copy link
Contributor

kkHAIKE commented Jul 31, 2022

reflect.Value.IsZero() can use check struct fields all zero value.
https://github.com/golang/go/blob/f32519e5fbcf1b12f9654a6175e5e72b09ae8f3a/src/reflect/value.go#L1592-L1598

than not need use Comparable() to check..

@arp242
Copy link
Collaborator Author

arp242 commented Aug 2, 2022

I had to go back and look why I didn't use it @kkHAIKE, but the problem is that it considers e.g. []string{} to be non-zero, whereas we want to check the length.

Example program:

package main

import (
	"fmt"
	"reflect"
)

func main() {
	type Strukt struct{ Slice []int }

	a := reflect.ValueOf(Strukt{Slice: []int{1}})
	b := reflect.ValueOf(Strukt{Slice: []int{}})
	c := reflect.ValueOf(Strukt{})

	fmt.Printf("%-10s %-8s %s\n", "", "isZero", "isEmpty")
	fmt.Printf("%-10s %-8v %v\n", "values", a.IsZero(), isEmpty(a))
	fmt.Printf("%-10s %-8v %v\n", "[]int{}", b.IsZero(), isEmpty(b))
	fmt.Printf("%-10s %-8v %v\n", "no value", c.IsZero(), isEmpty(c))

}

func isEmpty(rv reflect.Value) bool {
	switch rv.Kind() {
	case reflect.Array, reflect.Slice, reflect.Map, reflect.String:
		return rv.Len() == 0
	case reflect.Struct:
		if rv.Type().Comparable() {
			return reflect.Zero(rv.Type()).Interface() == rv.Interface()
		}
		// Not comparable; need to loop over all fields and call isEmpty()
		// recursively so that we check the length for slices etc.
		for i := 0; i < rv.NumField(); i++ {
			if isEmpty(rv.Field(i)) {
				return true
			}
		}
	case reflect.Bool:
		return !rv.Bool()
	}
	return false
}

Outputs:

           isZero   isEmpty
values     false    false
[]int{}    false    true
no value   true     true

@kkHAIKE
Copy link
Contributor

kkHAIKE commented Aug 2, 2022

oh.. sorry, i forgot this case...change the impl to that

i think Array always not be length 0

func isEmpty(rv reflect.Value) bool {
	switch rv.Kind() {
	case Array:
		for i := 0; i < v.Len(); i++ {
			if !isEmpty(rv.Index(i)) {
				return false
			}
		}
		return true
	case reflect.Slice, reflect.Map:
		return rv.Len() == 0
	case reflect.Struct:
		for i := 0; i < rv.NumField(); i++ {
			if !isEmpty(rv.Field(i)) {
				return false
			}
		}
		return true
	}
	return rv.IsZero()
}

@arp242
Copy link
Collaborator Author

arp242 commented Aug 2, 2022

Cheers, thanks; I mentioned the discussion in #360; might be a few weeks before I look at this, but happy to accept PRs as long as they add decent test cases!

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.

Encoding: 'omitempty' will panic if the struct contains a uncomparable type
2 participants