Make get_children an iterator

[eytan-starkware]

Improve performance by making get_children untracked and return an iterator. Stability of the computation comes from SyntaxNode now being tracked and interned on creation.

[eytan-starkware]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• (optimization): Using SyntaxNodeBuilder pattern for get_children #8529
• Make get_children an iterator #8527 👈 (View in Graphite)
• Keeping SyntaNode's kind and parent kind for optimization #8515 : 1 other dependent PR (#8578 )
• Keeping SyntaxNode parent's data id for optimizations #8513
• Making Stable Syntax Pointers just a wrapper around syntax nodes #8501 : 1 other dependent PR (#8507 )
• Tracking SyntaxNode and interning through a tracked function #8489
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[reviewable-StarkWare]

This change is 

[orizi]

@orizi reviewed all commit messages.
Reviewable status: 0 of 24 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

---

crates/cairo-lang-defs/src/db.rs line 1354 at r1 (raw file):

            ast::UsePath::Single(use_path) => stack.push(use_path.use_path(db)),
            ast::UsePath::Multi(use_path) => stack
                .extend(use_path.use_paths(db).elements(db).collect::<Vec<_>>().into_iter().rev()),

can't it be a DoubleEndedIter?

Code quote:

            ast::UsePath::Multi(use_path) => stack
                .extend(use_path.use_paths(db).elements(db).collect::<Vec<_>>().into_iter().rev()),

[orizi]

Reviewable status: 0 of 24 files reviewed, 2 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware)

---

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

        let leading = children.next().unwrap();
        let token = children.next().unwrap();
        let trailing = children.next().unwrap();

Suggestion:

        let Some([leading, token, trailing]) = children.collect_array() else {
            panic!("Terminal node should have 3 children.");
        };

[eytan-starkware]

Reviewable status: 0 of 24 files reviewed, 2 unresolved discussions (waiting on @orizi and @TomerStarkware)

---

crates/cairo-lang-defs/src/db.rs line 1354 at r1 (raw file):

Previously, orizi wrote…

can't it be a DoubleEndedIter?

Done.

---

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

        let leading = children.next().unwrap();
        let token = children.next().unwrap();
        let trailing = children.next().unwrap();

Done.

[orizi]

@orizi reviewed 3 of 24 files at r1, 8 of 11 files at r2, all commit messages.
Reviewable status: 11 of 24 files reviewed, 5 unresolved discussions (waiting on @TomerStarkware)

---

crates/cairo-lang-syntax/src/node/mod.rs line 692 at r2 (raw file):

#[salsa::tracked(returns(ref))]
fn children_key_indices<'db>(

doc.

won't it be better to have the parent's green-id as key?

---

crates/cairo-lang-defs/src/patcher.rs line 91 at r2 (raw file):

                    RewriteNode::Modified(ModifiedNode { children: None }),
                    left_idx,
                ));

this seems unneeded - but maybe never was needed.

Code quote:

                new_children.extend(itertools::repeat_n(
                    RewriteNode::Modified(ModifiedNode { children: None }),
                    left_idx,
                ));

---

crates/cairo-lang-defs/src/patcher.rs line 94 at r2 (raw file):

                // The number of children between the first and last nonempty nodes.
                let num_middle = right_idx - left_idx + 1;

isn't this children.len() now?
(and you maybe no longer require .enumerate()?)

Code quote:

right_idx - left_idx + 1;

---

crates/cairo-lang-syntax/src/node/element_list.rs line 36 at r2 (raw file):

impl<'db, T: TypedSyntaxNode<'db>> ElementList<'db, T, 2> {
    pub fn elements_vec(&self, db: &'db dyn Database) -> Vec<T> {
        self.node.get_children(db).step_by(2).map(|x| T::from_syntax_node(db, x)).collect()

why is this change required?

Code quote:

        self.node.get_children(db).step_by(2).map(|x| T::from_syntax_node(db, x)).collect()

[orizi]

@orizi reviewed 10 of 24 files at r1, 3 of 11 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware)

---

crates/cairo-lang-parser/src/printer.rs line 259 at r2 (raw file):

            self.print_tree(&expected_child.name, &child, indent, false, under_top_level);
        }
        let (last_child, last_expected_child) = iter.next().unwrap();

Suggestion:

    let mut iter = zip_eq(children, expected_children);
    let (last_child, last_expected_child) = iter.next_back().unwrap();
    for (child, expected_child) in iter {
        self.print_tree(&expected_child.name, child, indent, false, under_top_level);
    }

[eytan-starkware]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi and @TomerStarkware)

---

crates/cairo-lang-defs/src/patcher.rs line 91 at r2 (raw file):

Previously, orizi wrote…

this seems unneeded - but maybe never was needed.

Not for this PR

---

crates/cairo-lang-defs/src/patcher.rs line 94 at r2 (raw file):

Previously, orizi wrote…

isn't this children.len() now?
(and you maybe no longer require .enumerate()?)

Maybe, but the indices are being used in other parts of the code, so if it is not important at the moment let's leave it

---

crates/cairo-lang-syntax/src/node/element_list.rs line 36 at r2 (raw file):

Previously, orizi wrote…

why is this change required?

It wasn't. Reverted it.

---

crates/cairo-lang-syntax/src/node/mod.rs line 692 at r2 (raw file):

Previously, orizi wrote…

doc.

won't it be better to have the parent's green-id as key?

Done
I am trying to pre-calculate things, but parent green_id wont help as far as I can tell (or did you mean param for the function). I actually changed this a bit to prevent calling green.long(db) in the iterator which helped performance as far as I can tell.

---

crates/cairo-lang-parser/src/printer.rs line 259 at r2 (raw file):

            self.print_tree(&expected_child.name, &child, indent, false, under_top_level);
        }
        let (last_child, last_expected_child) = iter.next().unwrap();

It doesn't seem that ZipEq implements DoubleEndedIterator

[orizi]

@orizi reviewed all commit messages.
Reviewable status: 22 of 25 files reviewed, 3 unresolved discussions (waiting on @TomerStarkware)

---

crates/cairo-lang-parser/src/printer.rs line 259 at r3 (raw file):

            self.print_tree(&expected_child.name, &child, indent, false, under_top_level);
        }
        let (last_child, last_expected_child) = iter.next().unwrap();

Suggestion:

        let last_child = children.next_back().unwrap();
        let last_expected_child = expected_children.next_back().unwrap();
        for (child, expected_child) in zip_eq(children, expected_children) {
            self.print_tree(&expected_child.name, &child, indent, false, under_top_level);
        }
        let (last_child, last_expected_child) = iter.next().unwrap();

[orizi]

@orizi reviewed 1 of 3 files at r3.
Reviewable status: 23 of 25 files reviewed, 3 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware)

---

crates/cairo-lang-syntax/src/node/mod.rs line 49 at r3 (raw file):

        /// Which fields are used is determined by each SyntaxKind.
        /// For example, a function item might use the name of the function.
        key_fields: Arc<[GreenId<'db>]>,

wrap with a type and implement update for the wrapper type instead of multiple such impls.

Code quote:

        key_fields: Arc<[GreenId<'db>]>,

[orizi]

@orizi reviewed 2 of 3 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware)

[eytan-starkware]

Reviewable status: 24 of 25 files reviewed, 2 unresolved discussions (waiting on @orizi and @TomerStarkware)

---

crates/cairo-lang-syntax/src/node/mod.rs line 49 at r3 (raw file):

Previously, orizi wrote…

wrap with a type and implement update for the wrapper type instead of multiple such impls.

Done.

---

crates/cairo-lang-parser/src/printer.rs line 259 at r3 (raw file):

            self.print_tree(&expected_child.name, &child, indent, false, under_top_level);
        }
        let (last_child, last_expected_child) = iter.next().unwrap();

Done.

[orizi]

@orizi reviewed 2 of 4 files at r4, all commit messages.
Reviewable status: 24 of 26 files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

---

crates/cairo-lang-syntax/src/node/mod.rs line 89 at r4 (raw file):

    metadata: &'a [ChildKeyData<'a>],
    front: usize,
    back: usize,

and make it the actual iterator.

Suggestion:

    slice: &'a [ChildKeyData<'a>],

[orizi]

@orizi reviewed 2 of 4 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @eytan-starkware and @TomerStarkware)

---

crates/cairo-lang-syntax/src/node/mod.rs line 428 at r4 (raw file):

            );
        }
        panic!("Child index {} out of bounds", target_index)

Suggestion:

        panic!("Child index {target_index} out of bounds")