Skip to content

Conversation

@orizi
Copy link
Collaborator

@orizi orizi commented Jan 17, 2026

Summary

Refactored variable state management in Sierra by introducing an EditState trait to replace the standalone take_args and put_results functions. This change improves code organization by moving the state manipulation logic into a trait implementation for OrderedHashMap<VarId, V>.


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?

The previous approach used standalone functions to manipulate variable state, which required returning a new state object each time. This led to more verbose code with multiple variable reassignments. The new trait-based approach allows for more idiomatic in-place modifications, making the code cleaner and easier to maintain.


What was the behavior or documentation before?

Previously, state manipulation required calling standalone functions take_args and put_results that would return a new state object, requiring explicit reassignment:

let (statement_refs, taken_refs) = take_args(entry.refs, ref_ids)?;
entry.refs = statement_refs;

What is the behavior or documentation after?

Now, state manipulation is done through the EditState trait methods that modify the state in-place:

let taken_refs = entry.refs.take_vars(ref_ids)?;

This eliminates the need for explicit reassignment and makes the code more concise.


Additional context

This change maintains the same functionality while improving code organization. The implementation updates all relevant code in the Sierra codebase to use the new trait methods, including tests that verify the behavior remains consistent.

@reviewable-StarkWare
Copy link

This change is Reviewable

@orizi orizi marked this pull request as ready for review January 17, 2026 08:13
@orizi orizi force-pushed the orizi/01-17-refactor_sierra-to-casm_made_edit_state_based_on_mut_self_ branch from 587af38 to 79e3410 Compare January 20, 2026 10:19
@orizi orizi force-pushed the orizi/01-17-performance_orizi_made_allocations_for_state_edits_more_efficient branch from 561ab13 to ea40ca4 Compare January 20, 2026 10:19
@orizi orizi changed the base branch from orizi/01-17-performance_orizi_made_allocations_for_state_edits_more_efficient to graphite-base/9495 January 20, 2026 10:50
Copy link
Contributor

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

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


crates/cairo-lang-sierra/src/edit_state.rs line 28 at r1 (raw file):

/// Trait for editing the state of variables in a map.
pub trait EditState<V> {
    /// Extracts out the given ids from the map, failing if some id is missing.

Extracts = removes. Please improve comment to be clear


crates/cairo-lang-sierra/src/edit_state_test.rs line 6 at r1 (raw file):

use crate::edit_state::{EditState, EditStateError};
use crate::ids::VarId;

The tests seem roundabout. Why hide the map behind a new take and put functions, and then create the map with a from?

@orizi orizi force-pushed the orizi/01-17-refactor_sierra-to-casm_made_edit_state_based_on_mut_self_ branch from 79e3410 to 4dbf235 Compare January 20, 2026 11:51
@orizi orizi force-pushed the graphite-base/9495 branch from ea40ca4 to 020210a Compare January 20, 2026 11:51
@orizi orizi changed the base branch from graphite-base/9495 to main January 20, 2026 11:51
Copy link
Collaborator

@TomerStarkware TomerStarkware left a comment

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: all files reviewed, 2 unresolved discussions (waiting on @orizi).

SIERRA_UPDATE_PATCH_CHANGE_TAG=No interface changes.
@orizi orizi force-pushed the orizi/01-17-refactor_sierra-to-casm_made_edit_state_based_on_mut_self_ branch from 4dbf235 to a5ffe57 Compare January 20, 2026 12:11
Copy link
Collaborator Author

@orizi orizi left a comment

Choose a reason for hiding this comment

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

@orizi made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @eytan-starkware).


crates/cairo-lang-sierra/src/edit_state.rs line 28 at r1 (raw file):

Previously, eytan-starkware wrote…

Extracts = removes. Please improve comment to be clear

Done.


crates/cairo-lang-sierra/src/edit_state_test.rs line 6 at r1 (raw file):

Previously, eytan-starkware wrote…

The tests seem roundabout. Why hide the map behind a new take and put functions, and then create the map with a from?

because it makes the tests short.

Copy link
Contributor

@eytan-starkware eytan-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@orizi orizi added this pull request to the merge queue Jan 20, 2026
Merged via the queue into main with commit 5cd5ce6 Jan 20, 2026
55 checks passed
@orizi orizi deleted the orizi/01-17-refactor_sierra-to-casm_made_edit_state_based_on_mut_self_ branch January 20, 2026 15:55
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.

5 participants