internal/core/convert: fix panic when encoding pointer-receiver marshalers

When a value type's pointer implements an interface like json.Marshaler,
the implements function correctly returns needAddr=true, but fromGoValue
ignored that return value. The subsequent reflect.TypeAssert on the
non-pointer value returned nil, causing a panic on the method call.

Use the needAddr return from implements to take the address of the value
before the type assertion, matching the intent of the implements API.

Note that encoding/json/v1 does not call pointer-receiver methods on
non-pointer values which are not addressable,
but encoding/json/v2 fixes this inconsistency and always calls them,
even when this incurs an extra allocation. We follow the v2 behavior.

While this is technically a behavior change, it is one where we are
aligning with Go's json/v2 design, and fixing an edge case inconsistency
which very few users should be running into.
If a user really does want the old behavior, they can do so by
wrapping the value in a type which hides the encoding method.

This regression was introduced by me in https://cuelang.org/cl/1226577,
where I did a refactor to reflect.Value by borrowing ideas from json/v2,
but I had forgotten about this edge case of addressable values.

All three non-pointer test cases panic without the fix.

Fixes #4303.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Change-Id: Iee398ffa7d5b8788f6f70bd7e6897cd75a31cdab
Reviewed-on: https://cue.gerrithub.io/c/cue-lang/cue/+/1233358
Unity-Result: CUE porcuepine <cue.porcuepine@gmail.com>
Reviewed-by: Roger Peppe <rogpeppe@gmail.com>
TryBot-Result: CUEcueckoo <cueckoo@cuelang.org>

Daniel Martí 2 months ago f087406c a6075bdd

+47 -22

2 changed files

expand all

internal

core

convert

go.go

go_test.go

+24 -22

internal/core/convert/go.go

··· 269 269 return n 270 270 } 271 271 272 - if _, ok := implements(typ, typesInterface); ok { 273 - v, _ := reflect.TypeAssert[types.Interface](val) 272 + if v, ok := typeAssert[types.Interface](val, typesInterface); ok { 274 273 t := v.Core() 275 274 // TODO: panic if not the same runtime. 276 275 return t.V 277 276 } 278 - if _, ok := implements(typ, astExpr); ok { 279 - v, _ := reflect.TypeAssert[ast.Expr](val) 277 + if v, ok := typeAssert[ast.Expr](val, astExpr); ok { 280 278 return compileExpr(ctx, v) 281 279 } 282 - if _, ok := implements(typ, jsonMarshaler); ok { 283 - v, _ := reflect.TypeAssert[json.Marshaler](val) 280 + if v, ok := typeAssert[json.Marshaler](val, jsonMarshaler); ok { 284 281 b, err := v.MarshalJSON() 285 282 if err != nil { 286 283 return ctx.AddErr(errors.Promote(err, "json.Marshaler")) ··· 291 288 } 292 289 return compileExpr(ctx, expr) 293 290 } 294 - if _, ok := implements(typ, textMarshaler); ok { 295 - v, _ := reflect.TypeAssert[encoding.TextMarshaler](val) 291 + if v, ok := typeAssert[encoding.TextMarshaler](val, textMarshaler); ok { 296 292 b, err := v.MarshalText() 297 293 if err != nil { 298 294 return ctx.AddErr(errors.Promote(err, "encoding.TextMarshaler")) ··· 300 296 str := strings.ToValidUTF8(string(b), string(utf8.RuneError)) 301 297 return &adt.String{Src: src, Str: str} 302 298 } 303 - if _, ok := implements(typ, goError); ok { 304 - v, _ := reflect.TypeAssert[error](val) 299 + if v, ok := typeAssert[error](val, goError); ok { 305 300 errs, ok := v.(errors.Error) 306 301 if !ok { 307 302 errs = ctx.Newf("%s", v.Error()) ··· 557 552 topSentinel = ast.NewIdent("_") 558 553 ) 559 554 560 - // implements is like t.Implements(ifaceType) but checks whether 561 - // either t or reflect.PointerTo(t) implements the interface. 562 - // It also returns false for the case where t is an interface type. 563 - func implements(t, ifaceType reflect.Type) (needAddr, ok bool) { 555 + // typeAssert is similar to [reflect.TypeAssert] except that 556 + // it will also convert v to a pointer if its pointer type implements 557 + // the given interface T, allocating a value if necessary. 558 + func typeAssert[T any](v reflect.Value, t reflect.Type) (T, bool) { 559 + vt := v.Type() 564 560 switch { 565 - case t.Kind() == reflect.Interface: 566 - return false, false 567 - case t.Implements(ifaceType): 568 - return false, true 569 - case reflect.PointerTo(t).Implements(ifaceType): 570 - return true, true 571 - default: 572 - return false, false 561 + case vt.Kind() == reflect.Interface: 562 + return *new(T), false 563 + case vt.Implements(t): 564 + return reflect.TypeAssert[T](v) 565 + case reflect.PointerTo(vt).Implements(t): 566 + if v.CanAddr() { 567 + v = v.Addr() 568 + } else { 569 + p := reflect.New(vt) 570 + p.Elem().Set(v) 571 + v = p 572 + } 573 + return reflect.TypeAssert[T](v) 573 574 } 575 + return *new(T), false 574 576 } 575 577 576 578 func fromGoType(ctx *adt.OpContext, allowNullDefault bool, t reflect.Type) (e ast.Expr, expr adt.Expr) {

+23

internal/core/convert/go_test.go

··· 18 18 19 19 import ( 20 20 "encoding" 21 + "encoding/json" 21 22 "math/big" 22 23 "testing" 23 24 "time" ··· 45 46 } 46 47 47 48 var _ encoding.TextMarshaler = &textMarshaller{} 49 + 50 + type jsonMarshaller struct { 51 + b string 52 + } 53 + 54 + func (j *jsonMarshaller) MarshalJSON() ([]byte, error) { 55 + return []byte(`"` + j.b + `"`), nil 56 + } 57 + 58 + var _ json.Marshaler = &jsonMarshaller{} 59 + 60 + type ptrError struct { 61 + msg string 62 + } 63 + 64 + func (e *ptrError) Error() string { return e.msg } 65 + 66 + var _ error = &ptrError{} 48 67 49 68 func TestConvert(t *testing.T) { 50 69 type key struct { ··· 234 253 {[]encoding.TextMarshaler{&textMarshaller{b: "bar"}}, 235 254 "(#list){\n 0: (string){ \"bar\" }\n}"}, 236 255 {stringType("\x80"), `(string){ "�" }`}, 256 + {jsonMarshaller{b: "bar"}, `(string){ "bar" }`}, 257 + {&jsonMarshaller{b: "bar"}, `(string){ "bar" }`}, 258 + {textMarshaller{b: "bar"}, `(string){ "bar" }`}, 259 + {ptrError{msg: "bad"}, "(_|_){\n // [eval] bad\n}"}, 237 260 } 238 261 r := runtime.New() 239 262 for _, tc := range testCases {

Configure Feed

Configure Feed