add inner use sort to formatter.

[dean-starkware]

This change is 

[mkaput]

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @dean-starkware, @Draggu, @gilbens-starkware, @orizi, and @piotmag769)

a discussion (no related file):
Quite a lot of unrelated (to me) changes is going on here. Can you please fill issue title & description and outline what are you changing, please?

---

[dean-starkware]

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @Draggu, @gilbens-starkware, @mkaput, @orizi, and @piotmag769)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

Quite a lot of unrelated (to me) changes is going on here. Can you please fill issue title & description and outline what are you changing, please?

In this pull request, we implemented sorting of the inner paths of use statements. Specifically, we now handle cases like use a::{a, b, c}, where the items inside the braces are sorted alphabetically. This ensures a consistent and readable structure for use paths across the codebase.

---

[mkaput]

Reviewable status: 0 of 12 files reviewed, 1 unresolved discussion (waiting on @Arcticae, @dean-starkware, @Draggu, @gilbens-starkware, @orizi, and @piotmag769)

a discussion (no related file):

Previously, dean-starkware wrote…

In this pull request, we implemented sorting of the inner paths of use statements. Specifically, we now handle cases like use a::{a, b, c}, where the items inside the braces are sorted alphabetically. This ensures a consistent and readable structure for use paths across the codebase.

1. Please put this description in PR description & update title so that the commit that will land on main will make sense
2. Why is this touching LS (and thus asking my team for review) then?

---

[orizi]

Reviewed 2 of 8 files at r19, all commit messages.
Reviewable status: 12 of 18 files reviewed, 4 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 884 at r7 (raw file):

Previously, dean-starkware wrote…

We decided not to deal with comments within the inner use, so it was not added.

add a test it is ignored.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 864 at r19 (raw file):

        // Sort the filtered nodes by their ident (text).
        sorted_leaf_and_single.sort_by_key(|node| {

apply recursive comparison here as well.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 923 at r19 (raw file):

                    children[start_idx..end_idx].sort_by(|first_node, second_node| {
                        let mut first_path = Some(
                            ast::ItemUse::from_syntax_node(self.db, first_node.clone())

refactor as discussed f2f.

[orizi]

Reviewed 3 of 8 files at r19, 1 of 1 files at r20, all commit messages.
Reviewable status: 15 of 18 files reviewed, 5 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

Cargo.lock line 1 at r20 (raw file):

# This file is automatically @generated by Cargo.

you probably ran a cargo update or something - please revert (as this is not a toml updating PR)

[mkaput]

Reviewed all commit messages.
Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @dean-starkware, @gilbens-starkware, and @orizi)

[mkaput]

Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @dean-starkware, @gilbens-starkware, and @orizi)

a discussion (no related file):

Previously, mkaput (Marek Kaput) wrote…

1. Please put this description in PR description & update title so that the commit that will land on main will make sense
2. Why is this touching LS (and thus asking my team for review) then?

ACK, but it'd be much better to put this description in the PR description field, so that this text will land in the commit that will land on main 😉

---

[mkaput]

Reviewed 2 of 14 files at r12, 2 of 7 files at r15.
Reviewable status: 15 of 18 files reviewed, 4 unresolved discussions (waiting on @dean-starkware, @gilbens-starkware, and @orizi)

[orizi]

Reviewed 3 of 8 files at r19.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

[dean-starkware]

Reviewable status: 14 of 18 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 884 at r7 (raw file):

Previously, orizi wrote…

add a test it is ignored.

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 864 at r19 (raw file):

Previously, orizi wrote…

apply recursive comparison here as well.

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 923 at r19 (raw file):

Previously, orizi wrote…

refactor as discussed f2f.

Done.

---

Cargo.lock line 1 at r20 (raw file):

Previously, orizi wrote…

you probably ran a cargo update or something - please revert (as this is not a toml updating PR)

Done.

[orizi]

Reviewed 3 of 4 files at r21.
Reviewable status: 16 of 18 files reviewed, 10 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 872 at r21 (raw file):

            let a_use_path = match extract_use_path(a_node, self.db) {
                Some(path) => path,
                None => return Ordering::Equal,

why Equal?

Code quote:

                None => return Ordering::Equal,

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 879 at r21 (raw file):

                None => return Ordering::Equal,
            };
            compare_use_paths(&a_use_path, &b_use_path, self.db)

maybe?

Suggestion:

            match (extract_use_path(a_node, self.db), extract_use_path(b_node, self.db)) {
                (None, None) => Ordering::Equal,
                (None, Some(_)) => Ordering::Less,
                (Some(_), None) => Ordering::Grater,
                (Some(a_path), Some(b_path)) => compare_use_paths(&a_path, &b_path, self.db),
            }
            

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 927 at r21 (raw file):

                        let b =
                            ast::ItemUse::from_syntax_node(self.db, b.clone()).use_path(self.db);
                        compare_use_paths(&a, &b, self.db)

probably just inline a and b.

Code quote:

 compare_use_paths(&a, &b, self.db)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1052 at r21 (raw file):

        (UsePath::Leaf(a_leaf), UsePath::Single(b_single)) => {
            let a_str = extract_ident(a_leaf, db);
            let b_str = extract_ident(b_single, db);

Suggestion:

            let a_str = a_leaf.ident(db).as_syntax_node().get_text_without_trivia(db);
            let b_str = b_single.ident(db).as_syntax_node().get_text_without_trivia(db);

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1064 at r21 (raw file):

        (UsePath::Single(a_single), UsePath::Leaf(b_leaf)) => {
            let a_str = extract_ident(a_single, db);
            let b_str = extract_ident(b_leaf, db);

and later as well.

Suggestion:

            let a_str = a_single.ident(db).as_syntax_node().get_text_without_trivia(db);
            let b_str = b_leaf.ident(db).as_syntax_node().get_text_without_trivia(db);

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1077 at r21 (raw file):

            let a_str = extract_ident(a_leaf, db);
            let b_str = extract_ident(b_leaf, db);
            a_str.cmp(&b_str)

fix tie break as well.
and add tests.

Suggestion:

            match a_leaf.extract_ident(db).cmp(&b_leaf.extract_ident(db)) {
                Ordering::Equal => match (a_leaf.alias_clause(db), b_leaf.alias_clause(db)) {
                    (OptionAliasClause::Empty(_), OptionAliasClause::Empty(_)) => Ordering::Equal,
                    (OptionAliasClause::Empty(_), OptionAliasClause::AliasClause(_)) => Ordering::Less,
                    (OptionAliasClause::AliasClause(_), OptionAliasClause::Empty(_)) => Ordering::Greater,
                    (OptionAliasClause::AliasClause(a_alias), OptionAliasClause::AliasClause(b_alias)) => {
                        a_alias.alias(db).text(db).cmp(&b_alias.alias(db).text(db))
                    }
                }
                other => other,
            }

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1128 at r21 (raw file):

                    .iter()
                    .min_by_key(|x| extract_ident(*x, db)) // Dereference `&UsePath` to `UsePath`
                    .cloned() // Convert &UsePath to UsePath            }

Suggestion:

                    .min_by_key(|x| extract_ident(*x, db))
                    .cloned()            }

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1160 at r21 (raw file):

        _ => "".to_string(),
    }
}

this function should be replaced with simple calls to:

node.ident(db).as_syntax_node().get_text_without_trivia(db)

the updated code is written above.

if you do want it:

trait IdentExtractor {
    fn extract_ident(&self, db: &dyn SyntaxGroup) -> String;
}
impl IdentExtractor for ast::UsePathLeaf {
    fn extract_ident(&self, db: &dyn SyntaxGroup) -> String {
        node.ident(db).as_syntax_node().get_text_without_trivia(db)
    }
}
impl IdentExtractor for ast::UsePathSingle {
    fn extract_ident(&self, db: &dyn SyntaxGroup) -> String {
        node.ident(db).as_syntax_node().get_text_without_trivia(db)
    }
}

Code quote:

/// Helper function to extract the first segment identifier for comparison.
fn extract_ident(path: &impl TypedSyntaxNode, db: &dyn SyntaxGroup) -> String {
    let syntax_node = path.as_syntax_node();

    match syntax_node.kind(db) {
        SyntaxKind::UsePathSingle => {
            let path_segment = ast::UsePathSingle::from_syntax_node(db, syntax_node).ident(db);
            match path_segment {
                PathSegment::Simple(simple_segment) => {
                    simple_segment.as_syntax_node().get_text_without_trivia(db)
                }
                PathSegment::WithGenericArgs(_) => "".to_string(),
            }
        }
        SyntaxKind::UsePathLeaf => {
            let path_segment = ast::UsePathLeaf::from_syntax_node(db, syntax_node).ident(db);
            match path_segment {
                PathSegment::Simple(simple_segment) => {
                    simple_segment.as_syntax_node().get_text_without_trivia(db)
                }
                PathSegment::WithGenericArgs(_) => "".to_string(),
            }
        }
        _ => "".to_string(),
    }
}

---

crates/cairo-lang-formatter/test_data/expected_results/sort_inner_use.cairo line 4 at r22 (raw file):

use a::{a, b as bee, c as cee, d};
use a::{a as bc, a as ab};

Suggestion:

use a::{a as ab, a as bc};

[dean-starkware]

Reviewable status: 15 of 18 files reviewed, 10 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 872 at r21 (raw file):

Previously, orizi wrote…

why Equal?

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 879 at r21 (raw file):

Previously, orizi wrote…

maybe?

Yes, this is exactly what I did :)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 927 at r21 (raw file):

Previously, orizi wrote…

probably just inline a and b.

After the lsat implementation update (according to the related CR comment), it shouldn't be inlined now.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1052 at r21 (raw file):

        (UsePath::Leaf(a_leaf), UsePath::Single(b_single)) => {
            let a_str = extract_ident(a_leaf, db);
            let b_str = extract_ident(b_single, db);

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1064 at r21 (raw file):

Previously, orizi wrote…

and later as well.

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1077 at r21 (raw file):

Previously, orizi wrote…

fix tie break as well.
and add tests.

I decided that in case of tie breaks (between leaves) we do not sort by the aliases.
This rust doesn't sort by aliases:
use a::{a as bc, a as ab};
remains:
use a::{a as bc, a as ab};
and not:
use a::{a as ab, a as bc};
I forgot to add a doc about it- added now, thanks.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1128 at r21 (raw file):

                    .iter()
                    .min_by_key(|x| extract_ident(*x, db)) // Dereference `&UsePath` to `UsePath`
                    .cloned() // Convert &UsePath to UsePath            }

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1160 at r21 (raw file):

Previously, orizi wrote…

this function should be replaced with simple calls to:

node.ident(db).as_syntax_node().get_text_without_trivia(db)

the updated code is written above.

if you do want it:

trait IdentExtractor {
    fn extract_ident(&self, db: &dyn SyntaxGroup) -> String;
}
impl IdentExtractor for ast::UsePathLeaf {
    fn extract_ident(&self, db: &dyn SyntaxGroup) -> String {
        node.ident(db).as_syntax_node().get_text_without_trivia(db)
    }
}
impl IdentExtractor for ast::UsePathSingle {
    fn extract_ident(&self, db: &dyn SyntaxGroup) -> String {
        node.ident(db).as_syntax_node().get_text_without_trivia(db)
    }
}

Great thanks.
Would like to know why this implementation (via traits) is considered to be better than using the extract_ident function.

---

crates/cairo-lang-formatter/test_data/expected_results/sort_inner_use.cairo line 4 at r22 (raw file):

use a::{a, b as bee, c as cee, d};
use a::{a as bc, a as ab};

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 15 of 18 files reviewed, 9 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 927 at r21 (raw file):

Previously, dean-starkware wrote…

After the lsat implementation update (according to the related CR comment), it shouldn't be inlined now.

it still should - the let statement don't shorted anything or add information.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1160 at r21 (raw file):

Previously, dean-starkware wrote…

Great thanks.
Would like to know why this implementation (via traits) is considered to be better than using the extract_ident function.

because it had a specific implementation per type - your implementation was less efficient and lost information.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 733 at r24 (raw file):

    }
}

doc all.
and please move below (in general - helper functions should be below the using functions.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 885 at r24 (raw file):

            match (a_use_path, b_use_path) {
                (Some(a_path), Some(b_path)) => {
                    // Compare the extracted `UsePath`s.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1159 at r24 (raw file):

                            UsePath::Single(single) => single.extract_ident(db),
                            _ => "".to_string(), /* Return an empty string if it's not Single or
                                                  * Leaf */

what if the child is a Multi?
test.

Code quote:

                            _ => "".to_string(), /* Return an empty string if it's not Single or
                                                  * Leaf */

[dean-starkware]

Reviewable status: 15 of 18 files reviewed, 9 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 927 at r21 (raw file):

Previously, orizi wrote…

it still should - the let statement don't shorted anything or add information.

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 733 at r24 (raw file):

Previously, orizi wrote…

doc all.
and please move below (in general - helper functions should be below the using functions.

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 885 at r24 (raw file):

            match (a_use_path, b_use_path) {
                (Some(a_path), Some(b_path)) => {
                    // Compare the extracted `UsePath`s.

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1159 at r24 (raw file):

Previously, orizi wrote…

what if the child is a Multi?
test.

Here's my insights after examining the alternatives, including implementation and different edge cases:

I decided that the multi should be placed first, regardless of what's inside. Therefore, we return "".to_string().
The reasons for this are:

1. It's complex to implement– a recursive function is required to handle a path of type multi in this case. The implementation would be similar to sort_inner_use. It's a complicated implementation with a long computation. This is in contrast to our approach– not sorting the entire {} but simply selecting the first element. I decided that, across the board, we will place the multi first. I also added an appropriate test to verify this.
2. We will deal with this when we implement the use merge. This is one of the reasons why the issue doesn’t exist in Rust and will be resolved with merging.

However, during debugging and reviewing various cases, I discovered a bug and an edge case that I hadn’t checked before. I added a test and adjusted the sort_inner_use function accordingly. (we ignored a multi within a multi and removed it from the final sorted result.)

I’d be happy to elaborate if necessary– but I’m confident this is the right approach, both in terms of implementation and the correctness of the code.

[orizi]

Reviewed all commit messages.
Reviewable status: 15 of 18 files reviewed, 9 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1159 at r24 (raw file):

Previously, dean-starkware wrote…

Here's my insights after examining the alternatives, including implementation and different edge cases:

I decided that the multi should be placed first, regardless of what's inside. Therefore, we return "".to_string().
The reasons for this are:

1. It's complex to implement– a recursive function is required to handle a path of type multi in this case. The implementation would be similar to sort_inner_use. It's a complicated implementation with a long computation. This is in contrast to our approach– not sorting the entire {} but simply selecting the first element. I decided that, across the board, we will place the multi first. I also added an appropriate test to verify this.
2. We will deal with this when we implement the use merge. This is one of the reasons why the issue doesn’t exist in Rust and will be resolved with merging.

However, during debugging and reviewing various cases, I discovered a bug and an edge case that I hadn’t checked before. I added a test and adjusted the sort_inner_use function accordingly. (we ignored a multi within a multi and removed it from the final sorted result.)

I’d be happy to elaborate if necessary– but I’m confident this is the right approach, both in terms of implementation and the correctness of the code.

properly doc - not as a n "after" multi line comment - but as a comment above ``_ => "".string()

in any case - multi seems way more likely to always be last than first.
a::{b, {c, d}} seems nicer than a::{{c, d}, b}.

in any case - can probably wait for node merging (as this sort of multi should just be dropped entirely)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1044 at r25 (raw file):

                    UsePath::Single(single) => single.extract_ident(db),
                    _ => "".to_string(), // Handle cases where it's neither leaf nor single
                }

doc adds nothing.
if you wish to doc something there - it should be the "why" not the "what"

Suggestion:

            let a_min = a_elements.iter().min_by_key(|path| {
                match path {
                    UsePath::Leaf(leaf) => leaf.extract_ident(db),
                    UsePath::Single(single) => single.extract_ident(db),
                    _ => "".to_string(),
                }
            });
            let b_min = b_elements.iter().min_by_key(|path| {
                match path {
                    UsePath::Leaf(leaf) => leaf.extract_ident(db),
                    UsePath::Single(single) => single.extract_ident(db),
                    _ => "".to_string(),
                }

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1068 at r25 (raw file):

            let b_str = b_single.extract_ident(db);

            // Compare the extracted identifiers.

useless comment.

Code quote:

            // Compare the extracted identifiers.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1092 at r25 (raw file):

            let b_str = b_leaf.extract_ident(db);

            // In case of tie breaks we do not sort by aliases (same methodology as Rust).

Suggestion:

            // Note that in case of tie breaks we do not sort by aliases.

[dean-starkware]

Reviewable status: 15 of 18 files reviewed, 9 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1159 at r24 (raw file):

Previously, orizi wrote…

properly doc - not as a n "after" multi line comment - but as a comment above ``_ => "".string()

in any case - multi seems way more likely to always be last than first.
a::{b, {c, d}} seems nicer than a::{{c, d}, b}.

in any case - can probably wait for node merging (as this sort of multi should just be dropped entirely)

• Changed doc.
• multi changed to be last.
• Added sort by aliases (+tests).

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1044 at r25 (raw file):

Previously, orizi wrote…

doc adds nothing.
if you wish to doc something there - it should be the "why" not the "what"

Yes, I agree. It was a comment I made for myself and forgot to delete before pushing.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1068 at r25 (raw file):

Previously, orizi wrote…

useless comment.

Done.

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1092 at r25 (raw file):

            let b_str = b_leaf.extract_ident(db);

            // In case of tie breaks we do not sort by aliases (same methodology as Rust).

Done.

[orizi]

Reviewed 3 of 3 files at r26, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1088 at r26 (raw file):

            let a_alias = a_leaf.extract_alias(db);
            let b_alias = b_leaf.extract_alias(db);
            a_str.cmp(&b_str).then(a_alias.cmp(&b_alias))

extract alias only on the case of equality.

Code quote:

            a_str.cmp(&b_str).then(a_alias.cmp(&b_alias))

[dean-starkware]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

crates/cairo-lang-formatter/src/formatter_impl.rs line 1088 at r26 (raw file):

Previously, orizi wrote…

extract alias only on the case of equality.

Done.