Skip to content

Sort error summary output by count then name#4141

Closed
anushamukka-dev wants to merge 1 commit into
facebook:mainfrom
anushamukka-dev:export-D111620758
Closed

Sort error summary output by count then name#4141
anushamukka-dev wants to merge 1 commit into
facebook:mainfrom
anushamukka-dev:export-D111620758

Conversation

@anushamukka-dev

Copy link
Copy Markdown
Contributor

Summary:
The error summary printed by print_error_summary ordered files and per-file error kinds by count alone, leaving ties in a nondeterministic map-iteration order. This made the output unstable and harder to read across runs.

Add a secondary sort key so entries with equal counts break ties by name: per-file error kinds tie-break on ErrorKind::to_name, and files tie-break on the path's display string. This makes the summary deterministic and human-readable.

Because ModulePath does not implement Ord, the file-level tie-break compares display strings, which requires a Display bound on the generic collect_and_sort helper (its sole caller passes ModulePath, which is Display). The stale TODO requesting this behavior is removed.

Differential Revision: D111620758

Summary:
The error summary printed by `print_error_summary` ordered files and per-file error kinds by count alone, leaving ties in a nondeterministic map-iteration order. This made the output unstable and harder to read across runs.

Add a secondary sort key so entries with equal counts break ties by name: per-file error kinds tie-break on `ErrorKind::to_name`, and files tie-break on the path's display string. This makes the summary deterministic and human-readable.

Because `ModulePath` does not implement `Ord`, the file-level tie-break compares display strings, which requires a `Display` bound on the generic `collect_and_sort` helper (its sole caller passes `ModulePath`, which is `Display`). The stale TODO requesting this behavior is removed.

Differential Revision: D111620758
@meta-cla meta-cla Bot added the cla signed label Jul 13, 2026
@meta-codesync

meta-codesync Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

@anushamukka-dev has exported this pull request. If you are a Meta employee, you can view the originating Diff in D111620758.

@NathanTempest

Copy link
Copy Markdown
Contributor

Thanks for the cleanup. Making the summary deterministic by tie-breaking on name (count desc → name asc) is exactly right, and it resolves the stale TODO it removes. Bonus points for sort_by_cached_key caching the total + path string instead of recomputing per comparison, and Reverse reads much cleaner than the old negation.

One optional follow-up: since this changes ordering behavior, a small unit test on collect_and_sort asserting the count-desc / name-asc tie-break would lock it in (print_error_summary is stderr-only, but the helper returns a Vec, so it's easily testable via a #[cfg(test)] module). Not a blocker but happy to add it on our end. LGTM otherwise.

@meta-codesync meta-codesync Bot closed this in e6808ea Jul 14, 2026
@meta-codesync meta-codesync Bot added the Merged label Jul 14, 2026
@meta-codesync

meta-codesync Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

This pull request has been merged in e6808ea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants