Skip to content

Fix Set on nil nested maps - #191

Merged
hanzei merged 4 commits into
stretchr:masterfrom
yuefdev:fix/nil-nested-map
Oct 23, 2026
Merged

hanzei merged 4 commits into
stretchr:masterfrom
yuefdev:fix/nil-nested-map

Conversation

@yuefdev

@yuefdev yuefdev commented Oct 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Map{"nested": map[string]any(nil)}.Set("nested.answer", 42) panics instead of creating the nested map. The same happens with a nil objx.Map.

Treat both like a missing map when setting a nested value. Adds a regression test for both types and plain nil.

Checked with go test -race ./..., go vet ./..., golangci-lint v2.14.0 and go generate ./... (no changes). Lint and generation ran in an LF checkout.

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

@hanzei hanzei left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Non-blocking suggestions:

  1. The same panic is still reachable behind a slice index. The branch is guarded by len(indexes) == 0, so a nil map reached through [i] still crashes:
    objx.Map{"a": []any{map[string]any(nil)}}.Set("a[0].x", 1)   // panic
    objx.Map{"a": []map[string]any{nil}}.Set("a[0].x", 1)         // panic
    For comparison, []any{nil} with the same Set is a silent no-op. One fix: at the top of the map[string]any case, return nil, false when curMSI == nil && action == actionSet. That matches the plain-nil behaviour rather than auto-creating maps inside slices. Your call whether it fits here or in a follow-up.
  2. A nil objx.Map is replaced with a map[string]any{}, not an objx.Map{}. That's consistent with how non-map values are overwritten, and ObjxMap() handles both, so I'd keep it. An assert.IsType in the test would pin it down.
  3. Only the map subtest reproduces the bug. The nil and objxMap cases already passed before the fix, presumably because Map.Set unwraps a nil objx.Map. They're fine as regression coverage.

@yuefdev

yuefdev commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

@hanzei I've included the slice case here as well (9b3fd94). Setting through a nil map inside a slice is now a no-op, matching []any{nil}. The regressions cover both interface and typed slices, plus a nil root map. The existing nested-map test also checks that the replacement remains a map[string]any.

go test ./... and go vet ./... pass locally. I reproduced the nil-map panic before adding the guard. Race testing is blocked here because cgo is disabled, and golangci-lint isn't installed.

@hanzei hanzei left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With the nil check at the top of access, Set on a nil top-level Map now does nothing instead of panicking. objx.New(badInput).Set("a", 1) would drop the write without any error. I'd keep the panic, as a nil Go map and MergeHere do, and check only for nil maps reached through a slice index:

 		curMSI := current.(map[string]any)
-		if curMSI == nil && action == actionSet {
-			return nil, false
-		}
@@
 		}
+		// A nil map inside a slice cannot be replaced from here.
+		if action == actionSet {
+			if v := reflect.ValueOf(current); v.Kind() == reflect.Map && v.IsNil() {
+				return nil, false
+			}
+		}
 	}

TestAccessorsAccessSetNilMap would then use assert.Panics(t, func() { m.Set("answer", 42) }). The tests, vet and lint pass with both changes.


Generated by Claude Code

@yuefdev

yuefdev commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Moved the nil-map guard after slice indexing, so a nil root Map panics again while nil maps inside slices remain a no-op. Updated the regression test. go test ./... and go vet ./... pass.

@hanzei hanzei left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@hanzei
hanzei merged commit c31179b into stretchr:master Oct 23, 2026
4 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