Sitelet https://github.com/starkware-libs/cairo/pull/10123
Skip to content

bugfix(formatter): Sort merged items via compare_names. - #10123

Merged
orizi merged 1 commit into
mainfrom
orizi/06-18-bugfix_formatter_sort_merged_items_via_compare_names
Jun 18, 2026
Merged

orizi merged 1 commit into
mainfrom
orizi/06-18-bugfix_formatter_sort_merged_items_via_compare_names

Conversation

@orizi

@orizi orizi commented Jun 18, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fixes incorrect leaf ordering when merging use items with allow_duplicate_uses disabled. Previously, leaves were converted to strings before deduplication and sorting, which meant the sort operated on formatted strings (e.g., "x as y") rather than on the structured name/alias fields. This caused self to not be placed first in merged braces groups. Now, sorting and deduplication happen directly on Leaf structs, sorting by name (using compare_names) and then by alias, before formatting. A Display impl and PartialEq/Eq derives were added to Leaf to support this.

A new test case (use_merge_no_sort.cairo) covers the scenario where module-level sorting is disabled but merged braces must still honor the self-first convention.


Type of change

Please check one:

  • Bug fix (fixes incorrect behavior)
  • New feature
  • Performance improvement
  • Documentation change with concrete technical impact
  • Style, wording, formatting, or typo-only change

Why is this change needed?

When merging use paths with duplicate-use checking enabled, leaves like self, b, and z under the same prefix were not being ordered correctly. The old code sorted string representations of leaves (e.g., "self", "b", "z") after formatting them, but compare_names (which enforces self-first ordering) was never applied to leaves — only to the string-level sort. This meant the output could be {b, self, z} instead of the correct {self, b, z}.


What was the behavior or documentation before?

Given:

use a::z;
use a::self;
use a::b;

The formatter could produce use a::{b, self, z}; instead of use a::{self, b, z}; when merging was enabled but module-level sorting was disabled.


What is the behavior or documentation after?

The formatter now correctly produces:

use a::{self, b, z};

Leaf nodes are sorted using compare_names on their name field (with alias as a tiebreaker) before being formatted and merged into braces groups.


Related issue or discussion (if any)

None.


Additional context

The Leaf struct now derives PartialEq and Eq (required for dedup) and implements Display to avoid duplicating the name as alias formatting logic.

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

orizi commented Jun 18, 2026 •

Copy link
Copy Markdown
Collaborator Author

@orizi
orizi marked this pull request as ready for review June 18, 2026 07:28
@cursor

cursor Bot commented Jun 18, 2026 •

Copy link
Copy Markdown

PR Summary

Low Risk
Formatter-only output change for merged use statements; no runtime or semantic impact beyond formatted text.

Overview
Fixes incorrect ordering inside merged use {…} groups when duplicate-use dedup is on: leaves are now sorted and deduped on Leaf structs (name via compare_names, then alias) instead of lexicographically sorting pre-formatted strings, so conventions like self first and * first apply even when module-level use sorting is disabled.

Adds Display on Leaf for formatting, PartialEq/Eq for dedup, extends compare_names to treat * like compare_use_paths, and adds use_merge_no_sort formatter test coverage.

Reviewed by Cursor Bugbot for commit 294280c. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 83e0aeffdd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/cairo-lang-formatter/src/formatter_impl.rs
@orizi
orizi force-pushed the orizi/06-18-bugfix_formatter_sort_merged_items_via_compare_names branch from 83e0aef to 294280c Compare June 18, 2026 08:17

@orizi orizi left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@orizi resolved 1 discussion.
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on eytan-starkware and TomerStarkware).

@TomerStarkware TomerStarkware left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

:lgtm:

@TomerStarkware reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on eytan-starkware).

@orizi
orizi added this pull request to the merge queue Jun 18, 2026
Merged via the queue into main with commit 15dc17c Jun 18, 2026
55 checks passed
@orizi
orizi deleted the orizi/06-18-bugfix_formatter_sort_merged_items_via_compare_names branch June 21, 2026 12:15
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.

3 participants