-
Notifications
You must be signed in to change notification settings - Fork 203
Add the forcetypeassert linter #1142
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
Conversation
|
Don't think it was worth it to make all the changes; I'll go back and //nolint some of the ones that cannot be invalid casts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Type Assertion Errors Mask Key Existence
The Load, LoadAndDelete, and LoadOrStore methods now incorrectly conflate "key found" with "type assertion succeeded". Their ok/loaded return values now reflect the type assertion result, causing them to return false and a zero value when a key exists but has an unexpected type. This silently masks potential programming errors and is inconsistent with the Range method's panic behavior.
packages/envd/internal/utils/map.go#L21-L44
infra/packages/envd/internal/utils/map.go
Lines 21 to 44 in 38de2ed
| func (m *Map[K, V]) Load(key K) (value V, ok bool) { | |
| v, ok := m.m.Load(key) | |
| if ok { | |
| value, ok = v.(V) | |
| } | |
| return | |
| } | |
| func (m *Map[K, V]) LoadAndDelete(key K) (value V, loaded bool) { | |
| v, loaded := m.m.LoadAndDelete(key) | |
| if loaded { | |
| value, loaded = v.(V) | |
| } | |
| return | |
| } | |
| func (m *Map[K, V]) LoadOrStore(key K, value V) (actual V, loaded bool) { | |
| a, loaded := m.m.LoadOrStore(key, value) | |
| if loaded { | |
| actual, loaded = a.(V) | |
| } | |
| return | |
| } |
Bug: Type Assertion Failures Mask Errors
This update changes how type assertion failures are handled. Previously, these would panic, providing fail-fast behavior. Now, in dirtySortedKeys, a failed key.(int64) assertion silently appends 0 to the slice, potentially corrupting the sorted keys. Similarly, Map's Load, LoadAndDelete, and LoadOrStore methods now return zero values and false on assertion failure, making "key not found" indistinguishable from "key found with wrong type." This can obscure underlying type-related bugs.
packages/orchestrator/internal/sandbox/block/cache.go#L238-L240
infra/packages/orchestrator/internal/sandbox/block/cache.go
Lines 238 to 240 in 38de2ed
| m.dirty.Range(func(key, _ any) bool { | |
| n, _ := key.(int64) | |
| keys = append(keys, n) |
packages/envd/internal/utils/map.go#L21-L28
infra/packages/envd/internal/utils/map.go
Lines 21 to 28 in 38de2ed
| func (m *Map[K, V]) Load(key K) (value V, ok bool) { | |
| v, ok := m.m.Load(key) | |
| if ok { | |
| value, ok = v.(V) | |
| } | |
| return | |
| } |
# Conflicts: # .golangci.yml # packages/api/internal/orchestrator/placement/placement_benchmark_test.go
|
Too much work, punt on this. |
https://github.com/gostaticanalysis/forcetypeassert