Skip to content

fix add to handle a nil map as the first value - #325

Merged
itchyny merged 2 commits into
itchyny:mainfrom
yuefdev:fix/add-nil-map
Oct 20, 2026
Merged

itchyny merged 2 commits into
itchyny:mainfrom
yuefdev:fix/add-nil-map

Conversation

@yuefdev

@yuefdev yuefdev commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Running add on []any{map[string]any(nil), map[string]any{"x": 1}} panics: cloning the first map leaves a nil destination for the next maps.Copy.

Keep maps.Clone for the first map and initialise an empty map only when the clone is nil. Clone the first slice with append([]any{}, x...) so a typed nil slice produces an empty array rather than null with json.Marshal.

Regressions cover both nil maps and nil slices through the public query API, using add and add(.[]). The new slice cases fail before the follow-up and pass afterwards.

Validation: go test ./..., go vet ./..., changed-file gofmt and git diff --check pass. The race suite could not run for this follow-up because cgo is disabled in the current local environment.

Signed-off-by: Yusuf Efe <120668197+yuefdev@users.noreply.github.com>
Comment thread func.go Outdated
case nil:
v = maps.Clone(x)
m := make(map[string]any, len(x))
maps.Copy(m, x)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slower than maps.Clone because this rehashes and inserts. I would suggest checking for nil and assign map[string]any{} after maps.Clone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've restored maps.Clone(x) and only allocate map[string]any{} when the clone is nil (31af8e3). That keeps the fast cloning path for populated maps while retaining the nil-map fix. The existing regression passes for both add and add(.[]).

@itchyny

itchyny commented Oct 20, 2026

Copy link
Copy Markdown
Owner

We'll also need to avoid nil slice because I want to guarantee that the emit does not emit nil slice (which may cause confusing result with json.Marshal). Replace slices.Clone(x) with append([]any{}, x...).

Signed-off-by: Yusuf Efe <120668197+yuefdev@users.noreply.github.com>
@yuefdev

yuefdev commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

I've also switched the slice case to append([]any{}, x...) in 31af8e3, so a typed nil slice produces an empty, non-nil array. Added a public-API regression for both add and add(.[]); both cases fail before the change and pass afterwards.

go test ./..., go vet ./... and the changed-file formatting checks pass. I couldn't rerun the race suite in this environment because cgo is disabled. I've updated the PR description to reflect that.

@itchyny itchyny left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

@itchyny
itchyny merged commit 7125fe4 into itchyny:main Oct 20, 2026
11 checks passed
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