perf: ObjectGraph remove unused fields and methods

[TimothyMakkison]

Remove unused methods and fields from ObjectGraph

• Remove IObjectGraph
• Make ObjectGraph into a struct

Before

After

[thomhurst]

Summary

This PR removes unused fields and methods from ObjectGraph, converting it from a class with IObjectGraph interface to a readonly struct with simplified public API.

Critical Issues

1. Thread Safety Concern - Potential Data Race

Location: TUnit.Core/Discovery/ObjectGraph.cs:58

The PR removes locking from GetObjectsAtDepth():

Before:

lock (objects)
{
    return objects.ToArray();
}

After:

return objects;

Issue: The HashSet<object> values in _objectsByDepth are still being mutated during discovery (see ObjectGraphDiscoverer.cs:205-209 where AddToDepth modifies the HashSets). Removing the lock and defensive copy means callers get a direct reference to a mutable HashSet that may be modified concurrently during discovery.

Risk: If GetObjectsAtDepth() is called while discovery is still in progress (e.g., from another thread), this could cause:

• InvalidOperationException if iterating while the HashSet is modified
• Missing or duplicate objects in iteration

Required Fix: Either:

1. Keep the defensive copy: return objects.ToArray(); (safer, matches previous behavior), OR
2. Document that ObjectGraph must only be accessed after discovery completes, AND verify all call sites respect this

2. Missing Parameter in Constructor Calls

Location: TUnit.Core/Discovery/ObjectGraphDiscoverer.cs:131, 153

The PR removes the allObjects parameter from the ObjectGraph constructor, but ObjectGraphDiscoverer still creates and populates the allObjects HashSet (lines 103, 137, 208) even though it's no longer used.

Issue: This creates unnecessary allocations and lock contention for data that's immediately discarded.

Suggestion: Since AllObjects, ObjectsByDepth, and MaxDepth properties were unused and are now removed, also remove the allObjects HashSet and allObjectsLock from ObjectGraphDiscoverer.DiscoverObjectGraph() and DiscoverNestedObjectGraph() to eliminate the wasted work.

Suggestions

3. Struct Default Constructor Pitfall

Making ObjectGraph a struct means default(ObjectGraph) or new ObjectGraph() creates an instance with null _objectsByDepth and _sortedDepthsDescending, which will throw NullReferenceException if methods are called on it.

Mitigation: Consider adding null checks in GetObjectsAtDepth() and GetDepthsDescending(), or document that default struct instances are invalid.

4. Return Type Changed from IEnumerable to IReadOnlyCollection

Location: TUnit.Core/Discovery/ObjectGraph.cs:47

GetObjectsAtDepth() return type changed from IEnumerable<object> to IReadOnlyCollection<object>. This is a breaking change to the public contract (even though internal), but seems safe since:

• No external usages found via grep
• Only consumed in ObjectLifecycleService.cs:426 which just iterates it

Verify: Confirm this doesn't break reflection-based consumers or serialization.

Previous Review Status

Unable to retrieve previous comments due to API scope limitations.

Verdict

⚠️ REQUEST CHANGES - Critical thread safety issue (#1) must be addressed. Issue #2 is a performance optimization that should be fixed while we're here.

[TimothyMakkison]

1. Thread Safety Concern - Potential Data Race

GetObjectsByDepth is never mutated by its called and the ObjectGraph is only used in that function call.

2. Missing Parameter in Constructor Calls

Correct, I'm working on a different PR to remove this and possibly make ObjectGraphDiscoverer single threaded. I don't think it needs the locks or Concurrent collections. I might be missing something here.

3. Struct Default Constructor Pitfall

default is never used for ObjectGraph

[thomhurst]

Summary

This PR removes unused fields and methods from ObjectGraph, converts it from a class to a struct, and removes the IObjectGraph interface.

Critical Issues

1. Struct containing mutable reference type violates best practices

In TUnit.Core/Discovery/ObjectGraph.cs, the PR converts ObjectGraph from a sealed class to a readonly struct:

internal readonly struct ObjectGraph
{
    private readonly ConcurrentDictionary<int, HashSet<object>> _objectsByDepth;
    // ...
}

Problem: Structs should generally not contain mutable reference types like ConcurrentDictionary<,>. While the readonly modifier prevents reassignment of the field itself, it doesn't prevent mutation of the dictionary's contents. This creates a potentially confusing API where a "readonly struct" actually contains mutable state.

Why this matters:

• Violates struct design guidelines (structs should be immutable value types)
• Creates confusion about mutability semantics
• Struct copying behavior with reference type fields can be surprising
• The performance benefit is minimal (one less pointer indirection, but you still heap-allocate the dictionary)

Recommendation: Keep ObjectGraph as a sealed class. The performance improvement from making it a struct is negligible since the heavy data (ConcurrentDictionary, HashSets) remains on the heap regardless. If the goal is to remove the interface abstraction, that's fine - but keep it as a class.

2. Thread safety concern in GetObjectsAtDepth

The PR removes the lock when returning objects:

Before:

public IEnumerable<object> GetObjectsAtDepth(int depth)
{
    if (!_objectsByDepth.TryGetValue(depth, out var objects))
    {
        return [];
    }
    
    lock (objects)
    {
        return objects.ToArray();
    }
}

After:

public IReadOnlyCollection<object> GetObjectsAtDepth(int depth)
{
    if (!_objectsByDepth.TryGetValue(depth, out var objects))
    {
        return [];
    }
    
    return objects; // HashSet<object> exposed directly
}

Analysis: This change is probably safe IF the HashSets are never modified after ObjectGraph construction. Looking at ObjectGraphDiscoverer.cs:131 and :153, the ObjectGraph is created AFTER discovery completes, so concurrent modification shouldn't occur. However:

• Returning the mutable HashSet<object> directly (cast to IReadOnlyCollection<object>) means callers could cast it back to HashSet and modify it
• This violates encapsulation and the "read-only" contract
• The previous defensive copy (.ToArray()) was safer, though more expensive

Recommendation: If performance is critical here, document clearly in the method/class that the returned collections MUST NOT be modified. Better yet, keep the defensive copy or switch to returning ImmutableHashSet at construction time.

Suggestions

None - fix the critical issues above first.

Verdict

⚠️ REQUEST CHANGES - Critical issues found

The struct conversion introduces design issues that outweigh the minimal performance benefits. The removal of unused methods/fields is good, but the architectural change needs reconsideration.

[TimothyMakkison]

Okay, looking closer I'm pretty sure everything but the logic used by TestContext.TrackedObjects can internally use Dictionary

I'll make a new PR when this is merged