-
-
Notifications
You must be signed in to change notification settings - Fork 176
Fix: the typ of emptyInterface does not match ptr(#519, #510) #551
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
base: master
Are you sure you want to change the base?
Conversation
BenchmarkBefore fixing:
After fixing:
TestingPassed all encode tests except Issue147
Although Issue147 was wrong when testing, I think it was as expected. 'json_test.testNullStr' should return type testNullStr string
func (v *testNullStr) MarshalJSON() ([]byte, error) {
if *v == "" {
return []byte("null"), nil
}
return []byte(*v), nil
}
func TestIssue147(t *testing.T) {
type T struct {
Field1 string `json:"field1"`
Field2 testNullStr `json:"field2,omitempty"`
}
got, err := json.Marshal(T{
Field1: "a",
Field2: "b",
})
if err != nil {
t.Fatal(err)
}
expect, _ := stdjson.Marshal(T{
Field1: "a",
Field2: "b",
})
if !bytes.Equal(expect, got) {
t.Fatalf("expect %q but got %q", string(expect), string(got))
}
} |
@goccy Could you take a look at this PR when you have a moment? |
encode.go
Outdated
rv := reflect.ValueOf(v) | ||
// struct to *struct,fix: https://github.com/goccy/go-json/issues/519 | ||
if rv.Kind() == reflect.Struct { | ||
newV := reflect.New(rv.Type()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this approach, allocation occurs every time in the case of a struct, which is not ideal in terms of performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goccy Is it feasible to change 'rv.Kind() == reflect.Struct' to 'rv.Kind() == reflect.Struct && rv.NumField() == 1'?
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #551 +/- ##
==========================================
- Coverage 76.73% 74.71% -2.03%
==========================================
Files 55 55
Lines 18926 18935 +9
==========================================
- Hits 14523 14147 -376
- Misses 3771 4108 +337
- Partials 632 680 +48 🚀 New features to boost your workflow:
|
fix: The typ of emptyInterface does not match ptr
Issue:
When struct A contains a single field that is a pointer to struct B (e.g.,
A { B *B }
),encoding an instance ofA{}
into our customemptyInterface
causes a value-pointer mismatch:emptyInterface.typ
points to the type descriptor ofA
emptyInterface.ptr
points directly to the embeddedB
field (type*B
)Solution:
Encode
&A{}
(pointer to struct) instead ofA{}
(struct value). This ensures:EmptyInterface.typ
correctly points to*A
EmptyInterface.ptr
holds the address of theA
instance, which contains a valid*B
fieldFixes #519, #510