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

does not build with Go HEAD #506

Open
ianlancetaylor opened this issue May 8, 2024 · 8 comments
Open

does not build with Go HEAD #506

ianlancetaylor opened this issue May 8, 2024 · 8 comments

Comments

@ianlancetaylor
Copy link

This package does some extremely unsafe and unsupported operations. One of them has broken with Go HEAD, the future 1.23 release.

# github.com/goccy/go-json/internal/encoder.test
github.com/goccy/go-json/internal/encoder.(*Compiler).structCode: relocation target reflect.ifaceIndir not defined
github.com/goccy/go-json/internal/encoder.(*Compiler).isNilableType: relocation target reflect.ifaceIndir not defined
FAIL	github.com/goccy/go-json/internal/encoder [build failed]

The internal function reflect.ifaceIndir no longer exists. This package should not be referencing it.

If there are performance issues with the reflect package, please file them upstream. Please don't try to work around them using unsupported operations. Thanks.

gopherbot pushed a commit to golang/build that referenced this issue May 9, 2024
The buildstats package (used by coordinator for its internal needs)
currently fails to build at Go tip because one of its dependencies
relies on unexported internals of the reflect package not changing.
See goccy/go-json#506.

The coordinator is deployed with Go 1.21 now, and probably won't ever
get to be deployed with Go 1.23 since it will be deleted after the LUCI
migration is complete and it stops being needed. Delete the buildstats
implementation for now to avoid the breakage in x/build at Go tip.

Change-Id: Ica48eeb5ebe9f82cedda9385ced5d7a00d32b377
Reviewed-on: https://go-review.googlesource.com/c/build/+/584299
Reviewed-by: Michael Knyszek <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
gopherbot pushed a commit to golang/go that referenced this issue May 9, 2024
CL 583755 removed all uses of the ifaceIndir function,
and the function itself. Unfortunately, ifaceIndir is accessed
using go:linkname by the popular github.com/goccy/go-json package.
A bug has been filed to fix this upstream:
goccy/go-json#506
Until that bug is fixed and the fix is distributed,
keep this function available.
With luck we can remove this in the 1.24 release.

For #67279

Change-Id: I15fccf82d7a172a0b15cdbefb0a0a48381998938
Reviewed-on: https://go-review.googlesource.com/c/go/+/584676
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Ian Lance Taylor <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Ian Lance Taylor <[email protected]>
@mvdan
Copy link

mvdan commented May 10, 2024

This library has 47 go:linkname lines, particularly in https://github.com/goccy/go-json/blob/5e2ae3f23c1db71990844a230a4d11559efe128e/internal/runtime/rtype.go, which I find very surprising for a JSON library. Please transition away from them like Ian says - if the standard library is lacking features or isn't as performant as you would like it to be, file bugs or patches upstream.

Alternatively, I'm not sure that much can be done in terms of new Go versions not breaking you. It is bound to happen when you depend on dozens of internal reflect and runtime implementation details. @ianlancetaylor personally, I would not restore functions like ifaceIndir, not even for one release cycle - we might otherwise give the impression that go:linkname is somehow covered by the Go1 compatibility guarantee.

@goccy
Copy link
Owner

goccy commented May 10, 2024

@ianlancetaylor Thank you for notification to me. Since the current code related to the reflect package can be replaced with the use of only public APIs, I plan to address this soon.

@goccy
Copy link
Owner

goccy commented May 10, 2024

Previously, there was a trick implemented to avoid the forced escaping of argument values with reflect.ValueOf. However, since that code is no longer present, we can now use the reflect package normally.

@egonelbre
Copy link

egonelbre commented May 10, 2024

Also, as a general recommendation. Have every linkname & poking into Go runtime protected by a specific Go version tag, because there's no guarantee on how map, reflect etc. internals are implemented in the future. (e.g. see upcoming swissmap in https://go-review.googlesource.com/c/go/+/580778).

@goccy
Copy link
Owner

goccy commented May 10, 2024

Thank you. I'm already aware of that, and I implemented it with that understanding.

@trim21
Copy link
Contributor

trim21 commented Jul 16, 2024

Is there a way to iter a map with zero-alloc? reflect.Value.MapRange or reflect.Value.MapKeys both alloc O(len(m))

@ianlancetaylor
Copy link
Author

@trim21 Yes: use the MapIter.SetIterKey or MapIter.SetIterValue method.

@trim21
Copy link
Contributor

trim21 commented Jul 16, 2024

Thanks. for future readers, it would look like this:

it will have 3 allocs/op for map with any size

func(ctx *Ctx, b []byte, rv reflect.Value) ([]byte, error) {
		if rv.IsNil() {
			return appendNull(b), nil
		}

		size := rv.Len()
		b = appendArrayBegin(b, int64(size))

		iter := rv.MapRange()

		kv := reflect.New(keyType).Elem()
		vv := reflect.New(valueType).Elem()

		for iter.Next() {
			kv.SetIterKey(iter)
			b, err = keyEncoder(ctx, b, kv)
			if err != nil {
				return b, err
			}

			vv.SetIterValue(iter)
			b, err = valueEncoder(ctx, b, vv)
			if err != nil {
				return b, err
			}
		}

		return append(b, '}'), nil
	}

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

No branches or pull requests

5 participants