Skip to content

Comments

fix: DependsOnConflictAnalyzer reports all dependency cycles (#4862)#4913

Merged
thomhurst merged 1 commit intomainfrom
fix/depends-on-analyzer-cycles
Feb 19, 2026
Merged

fix: DependsOnConflictAnalyzer reports all dependency cycles (#4862)#4913
thomhurst merged 1 commit intomainfrom
fix/depends-on-analyzer-cycles

Conversation

@thomhurst
Copy link
Owner

Summary

  • Replace the Chain-based dependency traversal in DependsOnConflictAnalyzer with a proper DFS using a visited set and backtracking path list
  • The old GetDependencies method returned immediately upon finding any already-traversed method, which prevented exploring other branches of the dependency graph and caused methods involved in cycles to go unreported
  • Now all test methods that participate in dependency cycles correctly receive a TUnit0033 diagnostic, including those previously missed in class-level [DependsOn] scenarios with multiple test methods

Closes #4862

Test plan

  • All 11 existing DependsOnConflictAnalyzerTests pass (verified on net9.0)
  • Updated Direct_Conflict_Other_Class_Raises_Error and Direct_Conflict_Other_Class_Raises_Error_GenericAttribute tests to expect diagnostics on all 4 test method locations (previously only 2 of 4 were reported)
  • Existing tests for no-conflict scenarios, base class scenarios, nested cycles, deep nested direct/indirect conflicts all continue to pass unchanged

Replace the Chain-based traversal with a proper DFS using a visited set
and backtracking path. The old approach returned immediately upon finding
any already-traversed method, which prevented exploring other branches
and caused methods involved in cycles to go unreported.
@claude
Copy link
Contributor

claude bot commented Feb 19, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

The DFS-based cycle detection is a clean and correct replacement for the previous Chain-based traversal. A few things worth acknowledging:

  • Algorithm correctness: The shared visited set (not backtracked alongside path) is intentional and sound. Since the dependency graph is static, if a node has no path back to targetMethod via one branch, it won't have one via another branch either — standard DFS reachability.
  • Path reconstruction: The new[] { method }.Concat(cyclePath).Concat(new[] { method }) pattern correctly builds the full cycle description including both endpoints.
  • continue instead of return: Switching early exits from return chain to continue is the key fix that allows the DFS to explore all dependency branches rather than short-circuiting on the first unresolvable attribute — this directly addresses the root cause of bug: DependsOnConflictAnalyzer only reports first dependency cycle #4862.
  • Test updates: The test changes appropriately expand expected diagnostics from 2 to 4 for the cross-class dependency cycle scenarios, matching the corrected behavior.

@thomhurst thomhurst merged commit 916e019 into main Feb 19, 2026
13 of 14 checks passed
@thomhurst thomhurst deleted the fix/depends-on-analyzer-cycles branch February 19, 2026 01:08
This was referenced Feb 22, 2026
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.

bug: DependsOnConflictAnalyzer only reports first dependency cycle

1 participant