chore+feat: refactor triplestore and cardinality types#36
Conversation
WalkthroughThis PR refactors the cardinality interface hierarchy to separate mutable and immutable operations: Provider[T] becomes read-only with Clone(), while new MutableProvider[T] handles mutating operations (Add, Clear). All cardinality implementations are updated to return MutableProvider[T], and type assertions are added where Clone() now returns Provider[T]. The Triplestore container is redesigned from bitmap-based storage to edge-centric indices with slice-based access. Dependencies are updated. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
cardinality/roaring32.go (1)
70-72:bitmap32.Add()misses the single-value optimisation added tobitmap64.Add()This PR updated
bitmap64.Add()to branch onlen(values)(no-op for 0, directbitmap.Addfor 1,AddManyfor >1), butbitmap32.Add()still unconditionally callsAddManyfor all arities, including the single-element hot path.♻️ Align bitmap32.Add() with bitmap64.Add()
func (s bitmap32) Add(values ...uint32) { - s.bitmap.AddMany(values) + switch len(values) { + case 0: + case 1: + s.bitmap.Add(values[0]) + default: + s.bitmap.AddMany(values) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cardinality/roaring32.go` around lines 70 - 72, bitmap32.Add currently calls s.bitmap.AddMany for all arities; change it to mirror bitmap64.Add by branching on len(values): return immediately for 0, call s.bitmap.Add(values[0]) for a single value, and call s.bitmap.AddMany(values) only when len(values) > 1 so the single-value hot path uses the direct Add method.container/triplestore.go (1)
204-210:Degreescounts distinct adjacent nodes, not edge count.Since
adjacent()returns deduplicated node IDs (via a bitmap), multi-edges between the same node pair are collapsed. If degree should represent the number of incident edges (standard graph theory definition), this undercounts for multigraphs. If this is intentional, a clarifying comment would help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@container/triplestore.go` around lines 204 - 210, Degrees currently returns the number of distinct adjacent nodes by calling s.adjacent(node, direction), which de-duplicates multi-edges via a bitmap and thus undercounts in multigraphs; either update Degrees to count incident edges instead (iterate the underlying edge storage or add/consume a method that returns incident edges or edge IDs and return that length) or, if node-degree (unique neighbors) is intentional, add a clarifying comment above triplestore.Degrees stating it returns the number of distinct neighboring nodes (not total incident edges) and document behavior for multiedges; locate the logic in the Degrees function and the s.adjacent(node, direction) call to implement the chosen fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@algo/reach.go`:
- Line 281: The current code does a naked assertion of Clone() to
cardinality.Duplex[uint64] (reachBitmap :=
s.ReachOfComponentContainingMember(node,
direction).Clone().(cardinality.Duplex[uint64])) which will panic if Clone()
returns the narrower cardinality.Provider[uint64]; change it to assert to
cardinality.Provider[uint64] first and handle both cases safely: perform a type
switch or ok-check on the Clone() result, use it directly if it's already a
Duplex, otherwise take the Provider and construct or copy into a new Duplex
before using it (so XorReach and other hot-path code always receive a safe,
mutable Duplex without risking a panic).
In `@cardinality/lock.go`:
- Around line 103-108: threadSafeDuplex.Clone() and threadSafeSimplex.Clone()
perform unchecked assertions on s.provider.Clone() which can panic if Clone()
returns a narrower Provider[T]; change both to do a safe type-check: call cloned
:= s.provider.Clone(), attempt a type assertion (or type switch) to
Duplex[T]/Simplex[T] with the comma-ok form, and only wrap with
ThreadSafeDuplex/ThreadSafeSimplex when the assertion succeeds; otherwise return
cloned (the Provider[T]) directly so immutable/narrow implementations don't
cause a panic.
In `@container/adjacencymap.go`:
- Around line 122-125: The code currently does a naked type assertion on
outboundAdjacent.Clone() to cardinality.Duplex[uint64] which can panic if
Clone() returns an immutable Provider; change this to a safe check (e.g. cloned,
ok := outboundAdjacent.Clone().(cardinality.Duplex[uint64])) and if ok use
cloned, otherwise allocate a new mutable Duplex and copy/populate its contents
from the returned Provider before calling Or(inboundAdjacent); ensure you never
mutate the original outboundAdjacent provider and keep the subsequent
Or(inboundAdjacent) on the mutable Duplex (refer to outboundAdjacent.Clone(),
cardinality.Duplex[uint64], combinedAdjacent, and Or).
In `@container/triplestore.go`:
- Around line 56-58: The public method triplestore.Sort() currently panics and
will crash callers via the MutableTriplestore interface; replace the panic with
a safe implementation: either implement the actual sort logic on the
triplestore's internal slice/map of triples (use a stable sort on the slice
field used to store triples inside type triplestore) or, if sorting isn't ready,
change Sort() to be a no-op (remove panic) so it safely returns, or
alternatively change the MutableTriplestore.Sort signature to return an error
and update callers to handle it; locate the method named Sort on the triplestore
type and the MutableTriplestore interface to make the matching change.
- Around line 93-96: AddNode currently unconditionally overwrites
s.startIndex[node] and s.endIndex[node], wiping any indices added earlier (e.g.,
by AddEdge); change AddNode so it only initializes those map entries when the
key is absent instead of always setting them to nil — locate the
triplestore.AddNode method and use a presence check (comma-ok) on s.startIndex
and s.endIndex and only set the entry when it does not already exist.
- Around line 200-202: EachAdjacentNode currently discards the result of
s.adjacent(node, direction) and never calls the delegate; fix by capturing the
returned adjacent nodes (from s.adjacent(node, direction)), iterating over that
slice, invoking delegate(adjacent) for each element and stopping iteration if
delegate returns false so semantics match a typical walker; update the
EachAdjacentNode implementation to iterate the returned list and honor the
delegate's boolean return.
- Around line 174-194: The adjacent() function currently adds both edge.Start
and edge.End in the default (both-directions) branch, which incorrectly includes
the queried node itself; change the logic in adjacent() (and use
adjacentEdgeIndices/edges to find the edge) so that for each edge you only add
the opposite endpoint: if edge.Start == node then nodes.Add(edge.End) else if
edge.End == node then nodes.Add(edge.Start); alternatively skip adding any
endpoint equal to node. This ensures the queried node is not returned in its own
adjacency list.
In `@graph/types.go`:
- Line 185: The unchecked type assertion clone.bitmaps[kind] =
kindBitmap.Clone().(cardinality.Duplex[uint64]) can panic if Clone() returns a
Provider[uint64] that is not a Duplex; replace it with a checked assertion: call
kindBitmap.Clone(), assign to a local (e.g., cloned := kindBitmap.Clone()), then
use a type assertion with the comma-ok form or a type switch to detect whether
cloned implements cardinality.Duplex[uint64]; if it does, store it in
clone.bitmaps[kind], otherwise handle the fallback explicitly (either
create/convert to a Duplex when possible or panic with a clear diagnostic
message referencing kind and the concrete type) so future immutable or
non-Duplex providers do not cause silent runtime crashes.
---
Nitpick comments:
In `@cardinality/roaring32.go`:
- Around line 70-72: bitmap32.Add currently calls s.bitmap.AddMany for all
arities; change it to mirror bitmap64.Add by branching on len(values): return
immediately for 0, call s.bitmap.Add(values[0]) for a single value, and call
s.bitmap.AddMany(values) only when len(values) > 1 so the single-value hot path
uses the direct Add method.
In `@container/triplestore.go`:
- Around line 204-210: Degrees currently returns the number of distinct adjacent
nodes by calling s.adjacent(node, direction), which de-duplicates multi-edges
via a bitmap and thus undercounts in multigraphs; either update Degrees to count
incident edges instead (iterate the underlying edge storage or add/consume a
method that returns incident edges or edge IDs and return that length) or, if
node-degree (unique neighbors) is intentional, add a clarifying comment above
triplestore.Degrees stating it returns the number of distinct neighboring nodes
(not total incident edges) and document behavior for multiedges; locate the
logic in the Degrees function and the s.adjacent(node, direction) call to
implement the chosen fix.
| func (s *ReachabilityCache) XorReach(node uint64, direction graph.Direction, duplex cardinality.Duplex[uint64]) { | ||
| // Reach bitmap will contain the member due to resolution of component reach | ||
| reachBitmap := s.ReachOfComponentContainingMember(node, direction).Clone() | ||
| reachBitmap := s.ReachOfComponentContainingMember(node, direction).Clone().(cardinality.Duplex[uint64]) |
There was a problem hiding this comment.
Same unchecked .(cardinality.Duplex[uint64]) assertion on Clone() result
Same pattern as in graph/types.go:185 — ReachOfComponentContainingMember returns Duplex[uint64], Clone() now deliberately returns the narrower Provider[uint64], and the naked assertion to Duplex[uint64] will panic if an immutable clone implementation is introduced. The impact here is particularly acute because XorReach is called on the hot algorithmic path.
🛡️ Safer assertion form
- reachBitmap := s.ReachOfComponentContainingMember(node, direction).Clone().(cardinality.Duplex[uint64])
+ cloned, ok := s.ReachOfComponentContainingMember(node, direction).Clone().(cardinality.Duplex[uint64])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() returned non-Duplex type for XorReach"))
+ }
+ reachBitmap := cloned🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@algo/reach.go` at line 281, The current code does a naked assertion of
Clone() to cardinality.Duplex[uint64] (reachBitmap :=
s.ReachOfComponentContainingMember(node,
direction).Clone().(cardinality.Duplex[uint64])) which will panic if Clone()
returns the narrower cardinality.Provider[uint64]; change it to assert to
cardinality.Provider[uint64] first and handle both cases safely: perform a type
switch or ok-check on the Clone() result, use it directly if it's already a
Duplex, otherwise take the Provider and construct or copy into a new Duplex
before using it (so XorReach and other hot-path code always receive a safe,
mutable Duplex without risking a panic).
| func (s threadSafeDuplex[T]) Clone() Provider[T] { | ||
| s.lock.Lock() | ||
| defer s.lock.Unlock() | ||
|
|
||
| return ThreadSafeDuplex(s.provider.Clone()) | ||
| return ThreadSafeDuplex(s.provider.Clone().(Duplex[T])) | ||
| } |
There was a problem hiding this comment.
Clone() in the core thread-safe wrapper uses unchecked type assertions
Both threadSafeDuplex.Clone() (line 107) and threadSafeSimplex.Clone() (line 154) assert the result of s.provider.Clone() to a more specific interface without a safety check. Since Clone() now deliberately returns the narrower Provider[T] to accommodate future immutable implementations, these assertions are the origin of a pattern that propagates across the whole codebase. Any Duplex[T] or Simplex[T] whose Clone() returns an immutable view will panic here and in every wrapper wrapping it.
🛡️ Safer assertion form for both wrappers
func (s threadSafeDuplex[T]) Clone() Provider[T] {
s.lock.Lock()
defer s.lock.Unlock()
- return ThreadSafeDuplex(s.provider.Clone().(Duplex[T]))
+ cloned, ok := s.provider.Clone().(Duplex[T])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() on Duplex[%T] returned non-Duplex type %T", *new(T), s.provider))
+ }
+ return ThreadSafeDuplex(cloned)
} func (s threadSafeSimplex[T]) Clone() Provider[T] {
s.lock.Lock()
defer s.lock.Unlock()
- return ThreadSafeSimplex(s.provider.Clone().(Simplex[T]))
+ cloned, ok := s.provider.Clone().(Simplex[T])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() on Simplex[%T] returned non-Simplex type %T", *new(T), s.provider))
+ }
+ return ThreadSafeSimplex(cloned)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cardinality/lock.go` around lines 103 - 108, threadSafeDuplex.Clone() and
threadSafeSimplex.Clone() perform unchecked assertions on s.provider.Clone()
which can panic if Clone() returns a narrower Provider[T]; change both to do a
safe type-check: call cloned := s.provider.Clone(), attempt a type assertion (or
type switch) to Duplex[T]/Simplex[T] with the comma-ok form, and only wrap with
ThreadSafeDuplex/ThreadSafeSimplex when the assertion succeeds; otherwise return
cloned (the Provider[T]) directly so immutable/narrow implementations don't
cause a panic.
| combinedAdjacent := outboundAdjacent.Clone().(cardinality.Duplex[uint64]) | ||
| combinedAdjacent.Or(inboundAdjacent) | ||
|
|
||
| return combinedAdjacent |
There was a problem hiding this comment.
Unchecked .(cardinality.Duplex[uint64]) assertion on Clone() result
Same pattern as graph/types.go:185 — outboundAdjacent is Duplex[uint64], Clone() returns Provider[uint64], and the naked assertion will panic if any Duplex[uint64] implementation's Clone() returns an immutable provider. Cloning here is essential (to avoid mutating the live adjacency map entry), so a panic would silently corrupt graph traversal.
🛡️ Safer assertion form
- combinedAdjacent := outboundAdjacent.Clone().(cardinality.Duplex[uint64])
+ cloned, ok := outboundAdjacent.Clone().(cardinality.Duplex[uint64])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() on Duplex[uint64] returned non-Duplex type %T", outboundAdjacent))
+ }
+ combinedAdjacent := cloned📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| combinedAdjacent := outboundAdjacent.Clone().(cardinality.Duplex[uint64]) | |
| combinedAdjacent.Or(inboundAdjacent) | |
| return combinedAdjacent | |
| cloned, ok := outboundAdjacent.Clone().(cardinality.Duplex[uint64]) | |
| if !ok { | |
| panic(fmt.Sprintf("cardinality: Clone() on Duplex[uint64] returned non-Duplex type %T", outboundAdjacent)) | |
| } | |
| combinedAdjacent := cloned | |
| combinedAdjacent.Or(inboundAdjacent) | |
| return combinedAdjacent |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/adjacencymap.go` around lines 122 - 125, The code currently does a
naked type assertion on outboundAdjacent.Clone() to cardinality.Duplex[uint64]
which can panic if Clone() returns an immutable Provider; change this to a safe
check (e.g. cloned, ok := outboundAdjacent.Clone().(cardinality.Duplex[uint64]))
and if ok use cloned, otherwise allocate a new mutable Duplex and copy/populate
its contents from the returned Provider before calling Or(inboundAdjacent);
ensure you never mutate the original outboundAdjacent provider and keep the
subsequent Or(inboundAdjacent) on the mutable Duplex (refer to
outboundAdjacent.Clone(), cardinality.Duplex[uint64], combinedAdjacent, and Or).
| func (s *triplestore) Sort() { | ||
| panic("not implemented") | ||
| } |
There was a problem hiding this comment.
Sort() panics at runtime — exposed via public interface.
This method is part of the MutableTriplestore interface and will crash any caller. If the implementation is deferred, consider returning an error or removing it from the interface until it's ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/triplestore.go` around lines 56 - 58, The public method
triplestore.Sort() currently panics and will crash callers via the
MutableTriplestore interface; replace the panic with a safe implementation:
either implement the actual sort logic on the triplestore's internal slice/map
of triples (use a stable sort on the slice field used to store triples inside
type triplestore) or, if sorting isn't ready, change Sort() to be a no-op
(remove panic) so it safely returns, or alternatively change the
MutableTriplestore.Sort signature to return an error and update callers to
handle it; locate the method named Sort on the triplestore type and the
MutableTriplestore interface to make the matching change.
| func (s *triplestore) AddNode(node uint64) { | ||
| s.nodes.Add(node) | ||
| s.startIndex[node] = nil | ||
| s.endIndex[node] = nil | ||
| } |
There was a problem hiding this comment.
AddNode overwrites existing edge indices, destroying data.
Setting s.startIndex[node] = nil and s.endIndex[node] = nil unconditionally wipes any previously recorded edge indices for that node. If AddNode is called after AddEdge has already indexed edges for that node, those index entries are lost.
🐛 Proposed fix — only initialize if absent
func (s *triplestore) AddNode(node uint64) {
- s.startIndex[node] = nil
- s.endIndex[node] = nil
+ if _, exists := s.startIndex[node]; !exists {
+ s.startIndex[node] = nil
+ }
+ if _, exists := s.endIndex[node]; !exists {
+ s.endIndex[node] = nil
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *triplestore) AddNode(node uint64) { | |
| s.nodes.Add(node) | |
| s.startIndex[node] = nil | |
| s.endIndex[node] = nil | |
| } | |
| func (s *triplestore) AddNode(node uint64) { | |
| if _, exists := s.startIndex[node]; !exists { | |
| s.startIndex[node] = nil | |
| } | |
| if _, exists := s.endIndex[node]; !exists { | |
| s.endIndex[node] = nil | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/triplestore.go` around lines 93 - 96, AddNode currently
unconditionally overwrites s.startIndex[node] and s.endIndex[node], wiping any
indices added earlier (e.g., by AddEdge); change AddNode so it only initializes
those map entries when the key is absent instead of always setting them to nil —
locate the triplestore.AddNode method and use a presence check (comma-ok) on
s.startIndex and s.endIndex and only set the entry when it does not already
exist.
| func (s *triplestore) adjacent(node uint64, direction graph.Direction) []uint64 { | ||
| nodes := cardinality.NewBitmap64() | ||
|
|
||
| s.adjacentEdgeIndices(node, direction).Each(func(edgeIndex uint64) bool { | ||
| if edge := s.edges[edgeIndex]; !s.deletedEdges.Contains(edge.ID) { | ||
| switch direction { | ||
| case graph.DirectionOutbound: | ||
| nodes.Add(edge.End) | ||
| for _, edgeIndex := range s.adjacentEdgeIndices(node, direction) { | ||
| edge := s.edges[edgeIndex] | ||
|
|
||
| case graph.DirectionInbound: | ||
| nodes.Add(edge.Start) | ||
| switch direction { | ||
| case graph.DirectionOutbound: | ||
| nodes.Add(edge.End) | ||
|
|
||
| default: | ||
| nodes.Add(edge.End) | ||
| nodes.Add(edge.Start) | ||
| } | ||
| } | ||
| case graph.DirectionInbound: | ||
| nodes.Add(edge.Start) | ||
|
|
||
| return true | ||
| }) | ||
| default: | ||
| nodes.Add(edge.End) | ||
| nodes.Add(edge.Start) | ||
| } | ||
| } | ||
|
|
||
| return nodes | ||
| return nodes.Slice() | ||
| } |
There was a problem hiding this comment.
adjacent() default case includes the queried node itself in its own adjacency list.
For DirectionOutbound and DirectionInbound, only the other endpoint is added (correct). But the default/both-directions case adds both edge.Start and edge.End for every incident edge, which means the queried node is always included in its own result.
🐛 Proposed fix — exclude the queried node, or pick the opposite endpoint
func (s *triplestore) adjacent(node uint64, direction graph.Direction) []uint64 {
nodes := cardinality.NewBitmap64()
for _, edgeIndex := range s.adjacentEdgeIndices(node, direction) {
edge := s.edges[edgeIndex]
switch direction {
case graph.DirectionOutbound:
nodes.Add(edge.End)
case graph.DirectionInbound:
nodes.Add(edge.Start)
default:
- nodes.Add(edge.End)
- nodes.Add(edge.Start)
+ if edge.Start == node {
+ nodes.Add(edge.End)
+ }
+ if edge.End == node {
+ nodes.Add(edge.Start)
+ }
}
}
return nodes.Slice()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/triplestore.go` around lines 174 - 194, The adjacent() function
currently adds both edge.Start and edge.End in the default (both-directions)
branch, which incorrectly includes the queried node itself; change the logic in
adjacent() (and use adjacentEdgeIndices/edges to find the edge) so that for each
edge you only add the opposite endpoint: if edge.Start == node then
nodes.Add(edge.End) else if edge.End == node then nodes.Add(edge.Start);
alternatively skip adding any endpoint equal to node. This ensures the queried
node is not returned in its own adjacency list.
| func (s *triplestore) EachAdjacentNode(node uint64, direction graph.Direction, delegate func(adjacent uint64) bool) { | ||
| s.adjacent(node, direction).Each(delegate) | ||
| s.adjacent(node, direction) | ||
| } |
There was a problem hiding this comment.
EachAdjacentNode never invokes the delegate — completely broken.
s.adjacent(node, direction) computes and returns the adjacent node list, but the result is discarded and delegate is never called. This silently does nothing for any caller.
🐛 Proposed fix
func (s *triplestore) EachAdjacentNode(node uint64, direction graph.Direction, delegate func(adjacent uint64) bool) {
- s.adjacent(node, direction)
+ for _, adjacentNode := range s.adjacent(node, direction) {
+ if !delegate(adjacentNode) {
+ return
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *triplestore) EachAdjacentNode(node uint64, direction graph.Direction, delegate func(adjacent uint64) bool) { | |
| s.adjacent(node, direction).Each(delegate) | |
| s.adjacent(node, direction) | |
| } | |
| func (s *triplestore) EachAdjacentNode(node uint64, direction graph.Direction, delegate func(adjacent uint64) bool) { | |
| for _, adjacentNode := range s.adjacent(node, direction) { | |
| if !delegate(adjacentNode) { | |
| return | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@container/triplestore.go` around lines 200 - 202, EachAdjacentNode currently
discards the result of s.adjacent(node, direction) and never calls the delegate;
fix by capturing the returned adjacent nodes (from s.adjacent(node, direction)),
iterating over that slice, invoking delegate(adjacent) for each element and
stopping iteration if delegate returns false so semantics match a typical
walker; update the EachAdjacentNode implementation to iterate the returned list
and honor the delegate's boolean return.
| clone := NewThreadSafeKindBitmap() | ||
| for kind, kindBitmap := range s.bitmaps { | ||
| clone.bitmaps[kind] = kindBitmap.Clone() | ||
| clone.bitmaps[kind] = kindBitmap.Clone().(cardinality.Duplex[uint64]) |
There was a problem hiding this comment.
Unchecked type assertion will panic if Clone() returns a non-Duplex provider
kindBitmap.Clone() now returns Provider[uint64] by design, explicitly to leave room for immutable clone implementations. Immediately asserting .(cardinality.Duplex[uint64]) with no fallback will panic the moment any Duplex[uint64] implementation returns a narrower (e.g. immutable) clone. All current concrete types are safe, but this is a latent trap for the future.
Consider a checked assertion with an explicit panic message to aid diagnostics:
🛡️ Safer assertion form
- clone.bitmaps[kind] = kindBitmap.Clone().(cardinality.Duplex[uint64])
+ cloned, ok := kindBitmap.Clone().(cardinality.Duplex[uint64])
+ if !ok {
+ panic(fmt.Sprintf("cardinality: Clone() on Duplex[uint64] returned non-Duplex type %T", kindBitmap))
+ }
+ clone.bitmaps[kind] = cloned📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| clone.bitmaps[kind] = kindBitmap.Clone().(cardinality.Duplex[uint64]) | |
| cloned, ok := kindBitmap.Clone().(cardinality.Duplex[uint64]) | |
| if !ok { | |
| panic(fmt.Sprintf("cardinality: Clone() on Duplex[uint64] returned non-Duplex type %T", kindBitmap)) | |
| } | |
| clone.bitmaps[kind] = cloned |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@graph/types.go` at line 185, The unchecked type assertion clone.bitmaps[kind]
= kindBitmap.Clone().(cardinality.Duplex[uint64]) can panic if Clone() returns a
Provider[uint64] that is not a Duplex; replace it with a checked assertion: call
kindBitmap.Clone(), assign to a local (e.g., cloned := kindBitmap.Clone()), then
use a type assertion with the comma-ok form or a type switch to detect whether
cloned implements cardinality.Duplex[uint64]; if it does, store it in
clone.bitmaps[kind], otherwise handle the fallback explicitly (either
create/convert to a Duplex when possible or panic with a clear diagnostic
message referencing kind and the concrete type) so future immutable or
non-Duplex providers do not cause silent runtime crashes.
1 participant
Summary by CodeRabbit
New Features
API Changes
Chores