Skip to content

Conversation

naoNao89
Copy link
Contributor

@naoNao89 naoNao89 commented Oct 1, 2025

This PR fixes incorrect loop reporting in tsort by reporting only the minimal cycle subpath and removing the precise back-edge that closes the cycle. No performance refactors or unrelated changes are included.

Context

  • Addresses incorrect minimal cycle reporting discussed in Issue tsort: incorrect loop detection #8743
  • Keeps existing DFS structure; does not introduce ID interning / CycleDetector / HashSet changes
  • Excludes unrelated df test changes and GNU-compat test suite additions

What changed

  • When detecting a cycle, reconstruct and report only the true cycle nodes (no non-cycle prefix from the DFS path)
  • Remove the exact closing back-edge (last -> first) for the detected minimal cycle to progress deterministically
  • Maintain deterministic behavior by sorting node iteration for cycle detection

Why

  • Aligns error reporting structure more closely with GNU behavior by printing the minimal loop content
  • Ensures subsequent traversal remains coherent when multiple cycles exist by removing the precise back-edge that closes the reported cycle

Non-goals of this PR

  • No architectural/performance refactoring (no iterative DFS, no ID interning, no HashSet successor storage)
  • No additional test infrastructure or GNU-compat suite changes
  • No unrelated test changes (e.g., df tests)

Testing

  • Ran cargo test --package uu_tsort and the top-level cargo test tests locally; both succeeded
  • Existing tsort tests continue to pass

Follow-up PRs in planned split

  • PR 2: Iterative DFS and ID interning for cycle detection (performance-focused)
  • PR 3: Test enhancements including Linux-only GNU compatibility tests
  • PR 4: Unrelated df test robustness improvements

This focused PR should be easier to review and merge, addressing the core correctness issue independently.

@naoNao89 naoNao89 marked this pull request as draft October 1, 2025 21:09
Copy link

github-actions bot commented Oct 1, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

…oval

- Report only minimal cycle subpath when loop is detected
- Remove exact closing back-edge (last -> first) for deterministic progress
- Maintain existing DFS structure without architectural changes
- Address issue with incorrect loop detection
@naoNao89 naoNao89 force-pushed the pr1-tsort-minimal-cycle branch from 7a4ae04 to e590069 Compare October 1, 2025 21:41
Copy link

github-actions bot commented Oct 1, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

Copy link

codspeed-hq bot commented Oct 1, 2025

CodSpeed Performance Report

Merging #8786 will improve performances by 89.99%

Comparing naoNao89:pr1-tsort-minimal-cycle (f8db53b) with main (1331ff1)1

Summary

⚡ 4 improvements
✅ 71 untouched
🆕 2 new
⏩ 73 skipped2

Benchmarks breakdown

Benchmark BASE HEAD Change
fold_custom_width[50000] 60.8 ms 46.6 ms +30.51%
fold_many_lines[100000] 153.1 ms 125.3 ms +22.11%
🆕 nl_large_file[10] N/A 128.1 ms N/A
🆕 nl_many_lines[100000] N/A 101.6 ms N/A
tsort_input_parsing_heavy[50000] 12.8 s 6.7 s +89.99%
tsort_wide_dag[100000] 135.5 ms 130.1 ms +4.13%

Footnotes

  1. No successful run was found on main (f08f4d0) during the generation of this report, so 1331ff1 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

  2. 73 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@naoNao89 naoNao89 marked this pull request as ready for review October 1, 2025 23:56
Clean up cycle detection code by removing redundant comments that
don't add meaningful value to the implementation.
Copy link

github-actions bot commented Oct 3, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/tee (passes in this run but fails in the 'main' branch)

Copy link

github-actions bot commented Oct 4, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

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