fix: pre-check element types in NewGoMap/NewGoSlice (SLT-19)#75
Conversation
ToValue runs checkCollectionElemTypes before wrapping a map/slice, so an unsupported static element type (chan, complex, ...) is rejected up front. The public NewGoMap/NewGoSlice constructors skipped that check: they validated only the Kind, then built a wrapper that panicked later inside Items/Keys/Index/iteration — methods that cannot return an error. This violated the "methods that can't return errors must never reach panic" invariant for hosts that build wrappers via the direct constructors (the ToValue path was already safe). Run the same pre-check in both constructors and panic at construction if it fails — consistent with their existing documented panic-on-misuse contract (e.g. panic when given a non-map) and moving the failure to a deterministic point the host controls, before any script-reachable method call. Test-first: TestConstructorPrechecksElemTypes asserts the constructors panic for unsupported element types and still build supported ones. Backlog: SLT-19. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 11 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 11 |
| Duplication | 0 |
🟢 Coverage 100.00% diff coverage · +0.02% coverage variation
Metric Results Coverage variation ✅ +0.02% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (0b852e8) 2157 1955 90.64% Head commit (4aff5bc) 2161 (+4) 1959 (+4) 90.65% (+0.02%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#75) 4 4 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #75 +/- ##
==========================================
+ Coverage 87.67% 87.70% +0.02%
==========================================
Files 8 8
Lines 1680 1684 +4
==========================================
+ Hits 1473 1477 +4
Misses 127 127
Partials 80 80 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
What
ToValuerunscheckCollectionElemTypesbefore wrapping a map/slice, so an unsupported static element type (chan,complex, …) is rejected up front with a clean error. The publicNewGoMap/NewGoSliceconstructors skipped that check — they validated only theKind, then built a wrapper that panicked later insideItems/Keys/Index/iteration, methods that cannot return an error.This violated the repo invariant "methods that can't return errors must never reach panic" for any host that builds wrappers via the direct constructors (the
ToValuepath was already safe).Fix
Run the same
checkCollectionElemTypesCachedpre-check in both constructors and panic at construction if it fails — consistent with their existing documented panic-on-misuse contract (they already panic when given a non-map/non-slice), and moving the failure to a deterministic, host-controlled point before any script-reachable method call.Test-first
TestConstructorPrechecksElemTypesasserts the constructors panic for unsupported element types (map/slice/array with chan/complex elements) and still build supported ones (including dynamically-checkedinterface{}). Fails before the fix.Verification
go test -race -count=2 ./...,go vet,gofmt -lclean, Dockergolang:1.19race run green.Backlog: SLT-19