Skip to content

feat: Reach, Cache and Bitmap Structure Work#29

Open
zinic wants to merge 1 commit intomainfrom
reach
Open

feat: Reach, Cache and Bitmap Structure Work#29
zinic wants to merge 1 commit intomainfrom
reach

Conversation

@zinic
Copy link
Contributor

@zinic zinic commented Feb 5, 2026

  • Update reachability cache to focus on component traversal caching over membership caching
  • Introduce commutative bitmap structures to avoid having to bitwise or large roaring bitmaps
  • Introduce caching package with https://www.pdl.cmu.edu/ftp/Storage/nsdi24-SIEVE_abs.shtml implementation
  • Add and update tests

Summary by CodeRabbit

  • New Features

    • Added new public APIs to query component reach data for specific members in multiple directions.
    • Introduced pluggable cache system with multiple implementations and dynamic capacity allocation.
  • Refactor

    • Refactored internal reach data storage to use generic cache backend.
    • Simplified concurrency handling by removing explicit locking mechanisms.
    • Added new query methods to component graph interface.
  • Tests

    • Added comprehensive unit tests for cache implementations.

@coderabbitai
Copy link

coderabbitai bot commented Feb 5, 2026

Walkthrough

Introduces a generic cache abstraction layer with two implementations (NonExpiringMapCache and Sieve-based eviction), refactors algo/reach.go to use cache backend instead of direct map storage, generalizes CommutativeDuplex64 with parametric types, and adds accessor methods to ComponentGraph.

Changes

Cohort / File(s) Summary
Cache Abstraction & Implementations
cache/cache.go, cache/nemap.go, cache/sieve.go
Introduces generic Cache[K, V] interface with thread-safe Stats model; provides NonExpiringMapCache (capacity-limited map-based) and Sieve (eviction-based with visited flag tracking) implementations.
Cache Tests
cache/nemap_test.go, cache/sieve_test.go
Validates Put/Get/Delete semantics and eviction behavior for both cache implementations with generic type parameters.
Reachability Cache Refactoring
algo/reach.go
Replaces per-component in-memory maps with cache.Cache backend; renames Components field to internal components; changes inboundComponentReach and outboundComponentReach from maps to Cache types; updates Stats() return type; adds new public APIs ReachSliceOfComponentContainingMember() and ReachOfComponentContainingMember(); makes cache capacity dynamic (15% of graph nodes).
Cardinality Generics
cardinality/commutative.go
Replaces concrete CommutativeDuplex64 with generic DuplexCommutation[T] type alias and CommutativeDuplexes[T] container; removes old concrete type methods; adds generic Or/And composition and Contains-based evaluation.
Graph Component Methods
algo/scc.go
Adds public methods KnownMembers() and ComponentMembers(componentID) to ComponentGraph for member introspection.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Possibly related PRs

Poem

🐇 A cache so generic, it holds any treasure,
Sieve and maps both dancing to their measure,
Reachability now flows through backends bright,
While commutative duplexes take their flight! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the three main components of this changeset: reach (reachability cache updates), cache (new caching package with SIEVE implementation), and bitmap structures (commutative duplex structures).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch reach

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@algo/reach.go`:
- Around line 115-124: The function componentReachToCommutativeMemberReach
directly indexes s.components.componentMembers[reachableComponent] instead of
using the existing helper; change its body to use
s.components.CollectComponentMembers(reachableComponent, componentMembers) (same
pattern as componentReachToMemberReach) so collection is done via the safe
getter; ensure you keep the same return type and capacity initialization for
componentMembers and call CollectComponentMembers for each reachableComponent
inside the Each loop.
🧹 Nitpick comments (10)
cache/nemap_test.go (1)

10-38: Consider adding tests for edge cases and the full API.

The current tests cover basic Put/Get and update scenarios well. Consider adding tests for:

  • Get on a non-existent key (should return zero value and false)
  • Delete operation
  • Capacity behavior: what happens when Put is called on a new key after capacity is reached (the implementation silently drops the entry)
cache/nemap.go (2)

7-12: Complete the documentation comment.

The comment on line 7 is incomplete. Consider documenting the type's purpose, thread-safety guarantees, and behavior when capacity is reached.

📝 Suggested documentation
-// NonExpiringMapCache
+// NonExpiringMapCache is a thread-safe, bounded map cache that never expires entries.
+// When capacity is reached, new keys are silently dropped while existing keys can still be updated.
 type NonExpiringMapCache[K comparable, V any] struct {

21-31: Silent drop of new entries at capacity could cause subtle bugs.

When the cache is at capacity, Put silently ignores new keys without notifying the caller. This could lead to confusing behavior where callers assume their data was cached but subsequent Get calls return nothing.

Consider either:

  1. Returning a boolean indicating success
  2. Implementing an eviction policy (like LRU) to make room for new entries
  3. Documenting this behavior clearly if it's intentional
♻️ Option 1: Return success indicator
-func (s *NonExpiringMapCache[K, V]) Put(key K, value V) {
+func (s *NonExpiringMapCache[K, V]) Put(key K, value V) bool {
 	s.rwLock.Lock()
 	defer s.rwLock.Unlock()

 	if _, exists := s.store[key]; exists {
 		s.store[key] = value
+		return true
 	} else if int(s.stats.Size()) < s.stats.Capacity {
 		s.store[key] = value
 		s.stats.Put()
+		return true
 	}
+	return false
 }

Note: This would require updating the Cache interface signature.

cache/cache.go (1)

74-79: Consider adding documentation for the Cache interface.

The interface is clean and well-designed. Adding a brief doc comment would help consumers understand the contract.

📝 Suggested documentation
+// Cache defines a generic key-value cache with basic CRUD operations and statistics tracking.
+// Implementations must be safe for concurrent use.
 type Cache[K comparable, V any] interface {
 	Put(key K, value V)
 	Get(key K) (V, bool)
 	Delete(key K)
 	Stats() Stats
 }
cache/sieve.go (2)

37-51: Remove unnecessary type cast.

On line 39, int(s.stats.Capacity) casts an int to int, which is redundant since Capacity is already declared as int in the Stats struct.

♻️ Suggested fix
 func (s *Sieve[K, V]) putEntry(key K, value V) {
 	// Evict first if needed
-	if s.queue.Len() >= int(s.stats.Capacity) {
+	if s.queue.Len() >= s.stats.Capacity {
 		s.evict()
 	}

103-133: Consider returning an error instead of panicking on invariant violations.

The panics at lines 114 and 127 serve as invariant checks, which is reasonable for catching programming errors during development. However, in production, panics can be disruptive. If this cache is used in critical paths, consider either:

  1. Returning an error and gracefully handling the inconsistent state
  2. Adding a comment explaining these are intentional invariant checks that indicate a bug if triggered

The current approach is acceptable for development but worth documenting the rationale.

cache/sieve_test.go (1)

67-101: Consider adding a test for Delete operation.

The Delete method has special handling for when the deleted entry is at the hand position. A test covering this scenario would increase confidence in that edge case.

cardinality/commutative.go (2)

13-21: Redundant cardinality check in Contains.

The check duplex.Cardinality() > 0 on line 15 is likely redundant since duplex.Contains(value) would return false for an empty bitmap anyway. However, if Cardinality() is O(1) and Contains() is more expensive, this could be a valid optimization.


36-60: Add documentation clarifying the semantics of empty OR sets.

When s.or is empty, valueInOrSets returns false, making Contains always return false regardless of AND sets. This is mathematically consistent with treating an empty OR as a failing condition, but lacks documentation explaining this design choice. Since CommutativeDuplexes is not currently used elsewhere in the codebase and has no tests, clarify in a doc comment whether this behavior is intentional and whether AND-only queries are expected to be supported.

algo/reach.go (1)

236-237: Small graphs may get zero or minimal cache capacity.

For graphs with fewer than 7 nodes, maxCacheCap will be 0, effectively disabling caching. While this might be acceptable for tiny graphs, consider setting a minimum capacity to ensure at least some caching occurs.

♻️ Suggested fix with minimum capacity
-		maxCacheCap := int(float64(digraph.NumNodes()) * .15)
+		maxCacheCap := max(10, int(float64(digraph.NumNodes()) * .15))
 		return NewReachabilityCache(ctx, digraph, maxCacheCap), nil

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
algo/reach.go (1)

232-238: ⚠️ Potential issue | 🔴 Critical

Sieve cache crashes with zero capacity on first Put() operation.

When digraph.NumNodes() is 0, maxCacheCap becomes 0. The Sieve implementation will panic: putEntry() immediately triggers evict() (line 39: 0 >= 0), which attempts to access hand.Value on a nil pointer when the queue is empty (no guard against empty queue at line 108-111).

Either validate that maxCacheCap > 0 before creating the cache, or add a capacity check in evict() to handle the zero-capacity case gracefully.

🤖 Fix all issues with AI agents
In `@cache/sieve_test.go`:
- Around line 40-65: The test TestSieve_EvictWhenFull uses an assertions map and
iterates over it to call sieve.Put(), which makes insertion order
nondeterministic and can cause flakes; replace the map iteration with a
deterministic insertion sequence (e.g., a slice or explicit ordered calls) so
keys are inserted in a known order before asserting eviction, updating
references to cache.NewSieve, TestSieve_EvictWhenFull, and Put accordingly.
🧹 Nitpick comments (5)
cardinality/commutative.go (1)

13-21: Consider simplifying the cardinality check.

The Cardinality() > 0 check on line 15 is a micro-optimization—if a duplex is empty, Contains(value) would return false anyway. This optimization is valid if Cardinality() is O(1), but it adds slight cognitive overhead.

♻️ Optional simplification
 func (s DuplexCommutation[T]) Contains(value T) bool {
 	for _, duplex := range s {
-		if duplex.Cardinality() > 0 && duplex.Contains(value) {
+		if duplex.Contains(value) {
 			return true
 		}
 	}
 
 	return false
 }
algo/scc.go (1)

159-161: Consider bounds validation for componentID.

ComponentMembers directly indexes the slice without bounds checking. An invalid componentID from a caller would cause a panic. The existing CollectComponentMembers has the same pattern, so this is consistent with the codebase, but worth noting if this API is exposed to external callers.

cache/cache.go (1)

57-63: Delete() does not guard against negative size.

If Delete() is called more times than Put(), Size() will return a negative value. Consider adding a guard or documenting this as a caller responsibility.

🛡️ Optional guard against negative size
 func (s Stats) Delete() {
-	s.size.Add(-1)
+	// Only decrement if positive to prevent underflow
+	for {
+		current := s.size.Load()
+		if current <= 0 {
+			return
+		}
+		if s.size.CompareAndSwap(current, current-1) {
+			return
+		}
+	}
 }
cache/nemap_test.go (1)

28-38: Consider adding a test for capacity behavior.

The NonExpiringMapCache silently ignores Put() when at capacity. A test validating this behavior would improve coverage and document the expected semantics.

cache/nemap.go (1)

7-12: Incomplete doc comment.

The comment on line 7 is just the type name. Consider documenting the cache behavior, especially that new entries are silently dropped when at capacity (no eviction).

📝 Suggested documentation
-// NonExpiringMapCache
+// NonExpiringMapCache is a thread-safe cache that stores entries indefinitely.
+// When capacity is reached, new entries are silently dropped (no eviction).
+// Existing keys can always be updated regardless of capacity.
 type NonExpiringMapCache[K comparable, V any] struct {

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
algo/reach.go (1)

232-239: ⚠️ Potential issue | 🟡 Minor

Edge case: zero nodes results in zero cache capacity.

If digraph.NumNodes() returns 0, maxCacheCap will be 0. Combined with the Sieve cache issue flagged earlier, this would cause a panic on the first cache operation.

Consider adding a minimum capacity floor.

🛡️ Proposed fix to ensure minimum capacity
-		maxCacheCap := int(float64(digraph.NumNodes()) * .15)
+		maxCacheCap := max(1, int(float64(digraph.NumNodes()) * .15))
 		return NewReachabilityCache(ctx, digraph, maxCacheCap), nil
🤖 Fix all issues with AI agents
In `@algo/reach.go`:
- Around line 203-206: Fix the typo in the doc comment for
ReachOfComponentContainingMember: change "opreations" to "operations" in the
sentence that references ReachSliceOfComponentContainingMember and commutative
bitmap operations so the documentation reads correctly.

In `@cache/sieve.go`:
- Around line 103-133: The evict loop can panic when capacity is <= 0 because
s.queue.Back() may be nil; fix by validating capacity in the Sieve constructor
(NewSieve) to reject or normalize non-positive capacities (e.g., return an error
or panic if capacity <= 0) and also add a defensive nil-check in Sieve.evict
(guard before using hand or hand.Value — check s.queue and s.queue.Back() for
nil and return immediately) so Put/evict cannot dereference a nil element;
update references: NewSieve, Sieve.evict, Put, s.queue, hand, s.removeEntry.
🧹 Nitpick comments (2)
cache/nemap.go (1)

21-31: Silent failure when capacity is reached.

When the cache is at capacity and a new key is inserted, the Put operation silently does nothing. Consider returning a boolean or documenting this behavior, as callers may expect the value to be stored.

algo/reach.go (1)

39-42: Unused type ReachabilityCacheStats.

This struct appears to be unused after the refactoring. The Stats() method now returns cache.Stats instead. Consider removing this dead code.

♻️ Proposed removal
-type ReachabilityCacheStats struct {
-	Cached uint64
-	Hits   uint64
-}
-

Comment on lines +203 to 206
// ReachOfComponentContainingMember returns the reach of the component containing the given member and direction as a single
// bitwise ORed bitmap. For large scale digraphs use of this function may come at a high computational cost. If this function
// is utilized in a tight loop, consider utilizing ReachSliceOfComponentContainingMember with commutative bitmap opreations.
func (s *ReachabilityCache) ReachOfComponentContainingMember(member uint64, direction graph.Direction) cardinality.Duplex[uint64] {
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Typo in documentation.

Line 205: "opreations" should be "operations".

📝 Proposed fix
 // ReachOfComponentContainingMember returns the reach of the component containing the given member and direction as a single
-// bitwise ORed bitmap. For large scale digraphs use of this function may come at a high computational cost. If this function
-// is utilized in a tight loop, consider utilizing ReachSliceOfComponentContainingMember with commutative bitmap opreations.
+// bitwise ORed bitmap. For large scale digraphs use of this function may come at a high computational cost. If this function
+// is utilized in a tight loop, consider utilizing ReachSliceOfComponentContainingMember with commutative bitmap operations.
📝 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.

Suggested change
// ReachOfComponentContainingMember returns the reach of the component containing the given member and direction as a single
// bitwise ORed bitmap. For large scale digraphs use of this function may come at a high computational cost. If this function
// is utilized in a tight loop, consider utilizing ReachSliceOfComponentContainingMember with commutative bitmap opreations.
func (s *ReachabilityCache) ReachOfComponentContainingMember(member uint64, direction graph.Direction) cardinality.Duplex[uint64] {
// ReachOfComponentContainingMember returns the reach of the component containing the given member and direction as a single
// bitwise ORed bitmap. For large scale digraphs use of this function may come at a high computational cost. If this function
// is utilized in a tight loop, consider utilizing ReachSliceOfComponentContainingMember with commutative bitmap operations.
func (s *ReachabilityCache) ReachOfComponentContainingMember(member uint64, direction graph.Direction) cardinality.Duplex[uint64] {
🤖 Prompt for AI Agents
In `@algo/reach.go` around lines 203 - 206, Fix the typo in the doc comment for
ReachOfComponentContainingMember: change "opreations" to "operations" in the
sentence that references ReachSliceOfComponentContainingMember and commutative
bitmap operations so the documentation reads correctly.

Comment on lines 103 to 125
func (s *Sieve[K, V]) evict() {
hand := s.hand

// if nil assign it to the tail element in the list
if hand == nil {
hand = s.queue.Back()
}

entry, exists := s.store[hand.Value.(K)]

if !exists {
panic("sieve: evicting non-existent element")
}

for entry.visited.Load() {
entry.visited.Store(false)

if hand = hand.Prev(); hand == nil {
hand = s.queue.Back()
}

entry, exists = s.store[hand.Value.(K)]

if !exists {
panic("sieve: evicting non-existent element")
}
}

s.hand = hand.Prev()
s.removeEntry(entry)
}
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Potential infinite loop if capacity is 0.

If NewSieve is called with capacity <= 0, the first Put will call evict() on an empty queue, causing s.queue.Back() to return nil, which will panic at line 111 when accessing hand.Value.

Consider adding a capacity validation in the constructor.

🛡️ Proposed fix to validate capacity
 func NewSieve[K comparable, V any](capacity int) Cache[K, V] {
+	if capacity <= 0 {
+		panic("sieve: capacity must be greater than 0")
+	}
 	return &Sieve[K, V]{
 		store: make(map[K]*entry[K, V], capacity),
 		queue: list.New(),
 		stats: NewStats(capacity),
 	}
 }
📝 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.

Suggested change
func (s *Sieve[K, V]) evict() {
hand := s.hand
// if nil assign it to the tail element in the list
if hand == nil {
hand = s.queue.Back()
}
entry, exists := s.store[hand.Value.(K)]
if !exists {
panic("sieve: evicting non-existent element")
}
for entry.visited.Load() {
entry.visited.Store(false)
if hand = hand.Prev(); hand == nil {
hand = s.queue.Back()
}
entry, exists = s.store[hand.Value.(K)]
if !exists {
panic("sieve: evicting non-existent element")
}
}
s.hand = hand.Prev()
s.removeEntry(entry)
}
func NewSieve[K comparable, V any](capacity int) Cache[K, V] {
if capacity <= 0 {
panic("sieve: capacity must be greater than 0")
}
return &Sieve[K, V]{
store: make(map[K]*entry[K, V], capacity),
queue: list.New(),
stats: NewStats(capacity),
}
}
🤖 Prompt for AI Agents
In `@cache/sieve.go` around lines 103 - 133, The evict loop can panic when
capacity is <= 0 because s.queue.Back() may be nil; fix by validating capacity
in the Sieve constructor (NewSieve) to reject or normalize non-positive
capacities (e.g., return an error or panic if capacity <= 0) and also add a
defensive nil-check in Sieve.evict (guard before using hand or hand.Value —
check s.queue and s.queue.Back() for nil and return immediately) so Put/evict
cannot dereference a nil element; update references: NewSieve, Sieve.evict, Put,
s.queue, hand, s.removeEntry.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@cardinality/commutative.go`:
- Around line 36-59: The Contains logic treats empty OR sets as a failing
condition; update valueInOrSets in CommutativeDuplexes[T] so it returns true
when s.or is empty (i.e., treat “no OR constraints” as vacuously satisfied) by
checking len(s.or)==0 and returning true before iterating, ensuring Contains
becomes valueInOrSets(value) && valueInAndSets(value) with correct semantics;
alternatively, if the current behavior is intentional, add a clear doc comment
on CommutativeDuplexes.Contains (or the type) stating that callers must supply
at least one OR set.
🧹 Nitpick comments (4)
algo/scc.go (1)

159-161: Exposing mutable internal state — consider whether callers need a clone.

ComponentMembers returns the stored Duplex directly, so any caller that mutates the returned bitmap (e.g., Add, Remove, And) will silently corrupt the ComponentGraph's own membership data. CollectComponentMembers already touches the same reference via Or, but the surface area is more controlled there because the mutation target is the caller-owned members argument.

If you expect callers to treat this as read-only, a brief doc comment noting that contract would suffice. If mutation is plausible, returning s.componentMembers[componentID].Clone() is safer.

cache/nemap.go (2)

7-7: Incomplete doc comment.

The comment on line 7 is just the type name with no description. Consider adding a brief explanation of the cache's behavior (e.g., non-evicting, silently drops new keys at capacity).


21-31: Silent drop on capacity-full Put may surprise callers.

When the cache is at capacity and a new key is inserted, the entry is silently discarded. Callers have no way to know the put was a no-op. This is fine if intentional, but consider either returning a bool indicating whether the entry was stored, or documenting this behavior prominently. The current Cache interface returns void from Put, so at minimum a doc comment on this type would help.

algo/reach.go (1)

236-237: Cache capacity is 0 for small graphs (< 7 nodes).

int(float64(n) * .15) truncates to 0 for any graph with fewer than 7 nodes. A zero-capacity Sieve will never store entries, effectively disabling memoization. This isn't a correctness bug, but consider adding a minimum, e.g. max(1, ...), to ensure DFS memoization is always available.

Proposed fix
-		maxCacheCap := int(float64(digraph.NumNodes()) * .15)
+		maxCacheCap := max(1, int(float64(digraph.NumNodes()) * .15))

Comment on lines +36 to +59
func (s *CommutativeDuplexes[T]) valueInOrSets(value T) bool {
// Search each bitwise or set. Only one set is required to contain the value.
for _, orSet := range s.or {
if orSet.Contains(value) {
return true
}
}

return false
}

func (s *CommutativeDuplexes[T]) valueInAndSets(value T) bool {
// Search each bitwise and set. Each and set must contain the value.
for _, andSet := range s.and {
if !andSet.Contains(value) {
return false
}
}

return true
}

func (s *CommutativeDuplexes[T]) Contains(value T) bool {
return s.valueInOrSets(value) && s.valueInAndSets(value)
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Search for CommutativeDuplexes usage patterns across the codebase
rg -n --type=go 'CommutativeDuplexes' -C 3

Repository: SpecterOps/DAWGS

Length of output: 1918


🏁 Script executed:

#!/bin/bash
# Search for instantiation and usage of CommutativeDuplexes
rg -n --type=go 'new\(CommutativeDuplexes' -A 10

Repository: SpecterOps/DAWGS

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Search for calls to Contains method
rg -n --type=go '\.Contains\(' -B 3 -A 1 cardinality/

Repository: SpecterOps/DAWGS

Length of output: 3187


🏁 Script executed:

#!/bin/bash
# Look for test files related to commutative
find . -name '*test*.go' | xargs grep -l -i commutative

Repository: SpecterOps/DAWGS

Length of output: 44


🏁 Script executed:

#!/bin/bash
# Search for Or() method calls to see if they're always present
rg -n --type=go '\.Or\(' -B 5 -A 2

Repository: SpecterOps/DAWGS

Length of output: 8132


🏁 Script executed:

#!/bin/bash
# Look at the complete commutative.go file
cat -n cardinality/commutative.go

Repository: SpecterOps/DAWGS

Length of output: 1846


🏁 Script executed:

#!/bin/bash
# Search for calls to .Or( and .And( methods in commutative context
rg -n --type=go '\.Or\(|\.And\(' cardinality/commutative.go -B 5 -A 2

Repository: SpecterOps/DAWGS

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Search for CommutativeDuplexes in test files
find . -name '*.go' -type f | xargs grep -l 'CommutativeDuplexes'

Repository: SpecterOps/DAWGS

Length of output: 88


🏁 Script executed:

#!/bin/bash
# Check if there are any recent commits or comments that might explain the design intent
git log --oneline cardinality/commutative.go | head -10

Repository: SpecterOps/DAWGS

Length of output: 172


🏁 Script executed:

#!/bin/bash
# Check the git diff/blame for the Contains method to see when it was added
git log -p --follow -S "func (s *CommutativeDuplexes" cardinality/commutative.go | head -50

Repository: SpecterOps/DAWGS

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Check the commit that introduced CommutativeDuplexes to see the full context
git show 330bdb5 --stat | head -30

Repository: SpecterOps/DAWGS

Length of output: 1788


🏁 Script executed:

#!/bin/bash
# Look for any doc comments or README explaining CommutativeDuplexes
rg -n --type=go 'CommutativeDuplexes' -B 5 cardinality/

Repository: SpecterOps/DAWGS

Length of output: 1539


Contains returns false when no OR sets are present — confirm if this is intentional

The asymmetry between valueInOrSets and valueInAndSets is a real design flaw: when s.or is empty, Contains will always return false regardless of AND set constraints. This follows the "all OR sets must match" pattern rather than "at least one must match", contradicting typical OR semantics.

If OR sets are meant to be optional (no OR constraint when empty), valueInOrSets should return true when s.or is empty, matching the vacuous truth pattern used in valueInAndSets:

Suggested fix
 func (s *CommutativeDuplexes[T]) valueInOrSets(value T) bool {
+	if len(s.or) == 0 {
+		return true
+	}
+
 	// Search each bitwise or set. Only one set is required to contain the value.
 	for _, orSet := range s.or {
 		if orSet.Contains(value) {
 			return true
 		}
 	}
 
 	return false
 }

If the current behavior is intentional (callers must always add at least one OR set), add a doc comment clarifying this precondition.

🤖 Prompt for AI Agents
In `@cardinality/commutative.go` around lines 36 - 59, The Contains logic treats
empty OR sets as a failing condition; update valueInOrSets in
CommutativeDuplexes[T] so it returns true when s.or is empty (i.e., treat “no OR
constraints” as vacuously satisfied) by checking len(s.or)==0 and returning true
before iterating, ensuring Contains becomes valueInOrSets(value) &&
valueInAndSets(value) with correct semantics; alternatively, if the current
behavior is intentional, add a clear doc comment on CommutativeDuplexes.Contains
(or the type) stating that callers must supply at least one OR set.

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.

1 participant