feat: cairo-lang-utils supports no_std #4759

tdelabro · 2024-01-08T08:45:13Z

supersede #3297

In this PR:

cairo-lang-utils can now be compiled to no_std
add the crate ensure-no_std to workspace
add a check for a correct build of ensure-no_std in the CI
Index and IndexMut impl of OrderedHashMap is changed, arg is now a reference, to match the inner type signature and avoid unnecessary clone
<Un>OrderedHash<Map/Set> types have no constraint over their generics. The constraint is only enforced at the impl code block level, where they are required
add new() for <Un>OrderedHash<Map/Set> types behind a std flag. They usesstd::collections::hash_map::RandomState as its HashBuilder types like it's implicitly done in std rust for their internal types.

Once this one is merged, I will do the follow up for cairo-lang-casm

This change is

orizi

Reviewed 6 of 75 files at r1, all commit messages.
Reviewable status: 6 of 75 files reviewed, 2 unresolved discussions (waiting on @tdelabro)

.github/workflows/ci.yml line 58 at r1 (raw file):

          - test -p cairo-lang-utils --no-default-features --features=serde 
          - test -p tests
    steps:

Suggestion:

          - test -p cairo-lang-utils
          - test -p cairo-lang-utils --no-default-features --features=serde          - test -p tests
    steps:

crates/bin/starknet-sierra-compile/Cargo.toml line 17 at r1 (raw file):

cairo-lang-sierra = { path = "../../cairo-lang-sierra", version = "2.4.1" }
cairo-lang-starknet = { path = "../../cairo-lang-starknet", version = "2.4.1" }
cairo-lang-utils = { path = "../../cairo-lang-utils", version = "2.4.1" }

why can the versions be removed?

Code quote:

cairo-lang-sierra = { path = "../../cairo-lang-sierra", version = "2.4.1" }
cairo-lang-starknet = { path = "../../cairo-lang-starknet", version = "2.4.1" }
cairo-lang-utils = { path = "../../cairo-lang-utils", version = "2.4.1" }

tdelabro · 2024-01-09T12:06:33Z

why can the versions be removed?

I though it was needless redundancy but after looking at the rust doc it seems that adding the version is required to be able to publish the crate on crates.io. My bad, I will revert those.

tdelabro · 2024-01-09T12:08:05Z

Suggestion:

      - test -p cairo-lang-utils
      - test -p cairo-lang-utils --no-default-features --features=serde          - test -p tests
steps:

@orizi Sorry I don't get what you are suggesting. What will be the behavior if I put both - test -p cairo-lang-utils --no-default-features --features=serde and - test -p tests on the same line?

orizi

Reviewed 58 of 75 files at r1, 12 of 12 files at r2, all commit messages.
Reviewable status: 74 of 76 files reviewed, 3 unresolved discussions (waiting on @tdelabro)

crates/cairo-lang-lowering/src/graph_algorithms/strongly_connected_components.rs line 31 at r2 (raw file):

    function_id: ConcreteFunctionWithBodyId,
) -> Vec<ConcreteFunctionWithBodyId> {
    compute_scc::<_, RandomState>(&ConcreteFunctionWithBodyPostInlineNode {

why is this a must now?

crates/cairo-lang-lowering/src/lower/mod.rs line 486 at r2 (raw file):

                .concrete_struct_members(structure.concrete_struct_id)
                .map_err(LoweringFlowError::Failed)?;
            let mut required_members = UnorderedHashMap::<_, _, RandomState>::from_iter(

i don't want us to have to do this ever.
how can this be avoided?

orizi

no - just remove the space at the end of the line.
also - answer in reviewable so we'd have threads.

Reviewable status: 74 of 76 files reviewed, 3 unresolved discussions (waiting on @tdelabro)

tdelabro

Reviewable status: 67 of 76 files reviewed, 3 unresolved discussions (waiting on @orizi)

.github/workflows/ci.yml line 58 at r1 (raw file):

          - test -p cairo-lang-utils --no-default-features --features=serde 
          - test -p tests
    steps:

Done.

crates/cairo-lang-lowering/src/lower/mod.rs line 486 at r2 (raw file):

Previously, orizi wrote…

i don't want us to have to do this ever.
how can this be avoided?

For compute_scc the simplest way it to not make mod graph_algos; no_std.
It is only used on cairo-lang-lowering which we don't plan to make no_std anytime soon.

So I put it behind a feature flag and we don't have to worry about it anymore.

For UnorderedHashMap you can just do UnorderedHashMap::<_, _>::from_iter. By not mentioning the last generic type, its default type will be used. Which is RandomState.

crates/cairo-lang-lowering/src/graph_algorithms/strongly_connected_components.rs line 31 at r2 (raw file):

Previously, orizi wrote…

why is this a must now?

Done.

orizi

Reviewed 4 of 9 files at r3, all commit messages.
Reviewable status: 71 of 76 files reviewed, 4 unresolved discussions (waiting on @tdelabro)

crates/cairo-lang-lowering/src/lower/mod.rs line 486 at r2 (raw file):

Previously, tdelabro (Timothée Delabrouille) wrote…

For compute_scc the simplest way it to not make mod graph_algos; no_std.
It is only used on cairo-lang-lowering which we don't plan to make no_std anytime soon.

So I put it behind a feature flag and we don't have to worry about it anymore.

For UnorderedHashMap you can just do UnorderedHashMap::<_, _>::from_iter. By not mentioning the last generic type, its default type will be used. Which is RandomState.

why do i need the first 2?

crates/cairo-lang-utils/src/graph_algos/feedback_set_test.rs line 94 at r3 (raw file):

    let fset =
        HashSet::<usize>::from_iter(calc_feedback_set::<_>(&IntegerNode { id: 0, graph }.into()));

why do i need <_> all around?

crates/cairo-lang-utils/src/graph_algos/strongly_connected_components.rs line 50 at r3 (raw file):

impl<Node: GraphNode> SccAlgoContext<Node> {
    fn new(target_node_id: Node::NodeId) -> Self {

the base impl of the type itself may be with no extra empty line.

Suggestion:

}
impl<Node: GraphNode> SccAlgoContext<Node> {
    fn new(target_node_id: Node::NodeId) -> Self {

orizi

Reviewed 2 of 9 files at r3.
Reviewable status: 73 of 76 files reviewed, 3 unresolved discussions (waiting on @tdelabro)

tdelabro

Reviewable status: 73 of 76 files reviewed, 3 unresolved discussions (waiting on @orizi)

crates/cairo-lang-lowering/src/lower/mod.rs line 486 at r2 (raw file):

Previously, orizi wrote…

why do i need the first 2?

I found this explanation helpful: https://stackoverflow.com/a/65620283/9967008

crates/cairo-lang-utils/src/graph_algos/feedback_set_test.rs line 94 at r3 (raw file):

Previously, orizi wrote…

why do i need <_> all around?

We don't anymore after it removed the mod from no_std.
I changed it back

crates/cairo-lang-utils/src/graph_algos/strongly_connected_components.rs line 50 at r3 (raw file):

Previously, orizi wrote…

the base impl of the type itself may be with no extra empty line.

Done.

orizi

Reviewed 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tdelabro)

crates/cairo-lang-lowering/src/lower/mod.rs line 486 at r2 (raw file):

Previously, tdelabro (Timothée Delabrouille) wrote…

It's explained quite clearly here: https://stackoverflow.com/a/65620283/9967008

my main contention was that i didn't need it previously. but ok.

crates/cairo-lang-utils/src/graph_algos/feedback_set_test.rs line 35 at r4 (raw file):

    fn compute_scc(&self) -> Vec<Self::NodeId> {
        compute_scc::<_>(self)
    }

Suggestion:

        compute_scc(self)
    }

crates/cairo-lang-utils/src/graph_algos/strongly_connected_components.rs line 64 at r4 (raw file):

    let mut ctx = SccAlgoContext::<_>::new(root.get_id());
    compute_scc_recursive(&mut ctx, root);
    ctx.result

Suggestion:

/// Computes the SCC (Strongly Connected Component) of the given node in its graph.
pub fn compute_scc<Node: GraphNode>(root: &Node) -> Vec<Node::NodeId> {
    let mut ctx = SccAlgoContext::new(root.get_id());
    compute_scc_recursive(&mut ctx, root);
    ctx.result

tdelabro

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

crates/cairo-lang-utils/src/graph_algos/feedback_set_test.rs line 35 at r4 (raw file):

    fn compute_scc(&self) -> Vec<Self::NodeId> {
        compute_scc::<_>(self)
    }

Done.

crates/cairo-lang-utils/src/graph_algos/strongly_connected_components.rs line 64 at r4 (raw file):

    let mut ctx = SccAlgoContext::<_>::new(root.get_id());
    compute_scc_recursive(&mut ctx, root);
    ctx.result

Done.

orizi

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tdelabro)

tdelabro force-pushed the cairo-lang-utils-no_std branch 3 times, most recently from c1841c3 to b8d790e Compare January 8, 2024 12:55

tdelabro added 3 commits January 8, 2024 13:56

feat: cairo-lang-utils supports no_std

9417e19

fix: add 'serde' features in utils import in crate that use it

95a9699

add features utils/schemars to cl-casm deps

ea92a1a

tdelabro force-pushed the cairo-lang-utils-no_std branch from b8d790e to ea92a1a Compare January 8, 2024 12:56

tdelabro added 2 commits January 8, 2024 13:58

fmt

bddf24d

test: add a run of no_std test of cl-utils to the CI

9c602e5

orizi approved these changes Jan 9, 2024

View reviewed changes

chore: add back version in local import in Cargo.toml

05235c9

orizi requested changes Jan 9, 2024

View reviewed changes

orizi reviewed Jan 9, 2024

View reviewed changes

tdelabro added 2 commits January 9, 2024 14:34

utils/graph_algos is std only

31fc070

fmt: remove extra space at end o line

d719c1f

tdelabro commented Jan 9, 2024

View reviewed changes

orizi requested changes Jan 9, 2024

View reviewed changes

orizi reviewed Jan 9, 2024

View reviewed changes

ori review

5ae930a

tdelabro commented Jan 9, 2024

View reviewed changes

orizi requested changes Jan 9, 2024

View reviewed changes

tdelabro commented Jan 9, 2024

View reviewed changes

remove more unecessary <_>

25c0499

orizi approved these changes Jan 9, 2024

View reviewed changes

orizi added this pull request to the merge queue Jan 9, 2024

Merged via the queue into starkware-libs:main with commit c609a8b Jan 9, 2024
40 checks passed

tdelabro deleted the cairo-lang-utils-no_std branch January 9, 2024 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cairo-lang-utils supports no_std #4759

feat: cairo-lang-utils supports no_std #4759

tdelabro commented Jan 8, 2024 •

edited by spapinistarkware

Loading

orizi left a comment

tdelabro commented Jan 9, 2024

tdelabro commented Jan 9, 2024

orizi left a comment

orizi left a comment

tdelabro left a comment

orizi left a comment

orizi left a comment

tdelabro left a comment •

edited

Loading

orizi left a comment

tdelabro left a comment

orizi left a comment

feat: cairo-lang-utils supports no_std #4759

feat: cairo-lang-utils supports no_std #4759

Conversation

tdelabro commented Jan 8, 2024 • edited by spapinistarkware Loading

orizi left a comment

Choose a reason for hiding this comment

tdelabro commented Jan 9, 2024

tdelabro commented Jan 9, 2024

orizi left a comment

Choose a reason for hiding this comment

orizi left a comment

Choose a reason for hiding this comment

tdelabro left a comment

Choose a reason for hiding this comment

orizi left a comment

Choose a reason for hiding this comment

orizi left a comment

Choose a reason for hiding this comment

tdelabro left a comment • edited Loading

Choose a reason for hiding this comment

orizi left a comment

Choose a reason for hiding this comment

tdelabro left a comment

Choose a reason for hiding this comment

orizi left a comment

Choose a reason for hiding this comment

tdelabro commented Jan 8, 2024 •

edited by spapinistarkware

Loading

tdelabro left a comment •

edited

Loading