Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added tail struct for struct constructions #4923

Merged
merged 1 commit into from
Jan 28, 2024

Conversation

TomerStarkware
Copy link
Collaborator

@TomerStarkware TomerStarkware commented Jan 25, 2024

This change is Reviewable

@TomerStarkware TomerStarkware force-pushed the tomer/structCtor_tail branch 2 times, most recently from 846c7f7 to 442cbdd Compare January 25, 2024 12:55
Copy link
Collaborator

@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.

Reviewed 3 of 7 files at r1.
Reviewable status: 3 of 8 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @TomerStarkware)


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

    if let Some(member_path) = &expr.member_path {
        // assert!(expr.n_snapshots == 0, "Member access should not have snapshots.");

?


crates/cairo-lang-lowering/src/test_data/struct line 16 at r1 (raw file):

//! > module_code
struct MyStruct {
    a: (),

revert


crates/cairo-lang-lowering/src/test_data/struct line 129 at r1 (raw file):

    b: felt252,
    c: (felt252, felt252),
}

Suggestion:

//! > function
fn foo(s: MyStruct) -> MyStruct {
  MyStruct { a: (), .. MyStruct { c: (1, 2), .. MyStruct { b: 9, .. s } } }
}

//! > function_name
foo

//! > module_code
#[derive(Copy)]
struct MyStruct {
  a: (),
  b: felt252,
  c: (felt252, felt252),
}

crates/cairo-lang-semantic/src/expr/compute.rs line 1662 at r1 (raw file):

            }
            ast::StructArg::StructArgTail(tail_expr_syntax) => {
                // Todo(TomerStarkware): remove tail expression from argument list.

Suggestion:

                // TODO(TomerStarkware): remove tail expression from argument list.

crates/cairo-lang-semantic/src/expr/objects.rs line 447 at r1 (raw file):

    pub concrete_struct_id: ConcreteStructId,
    pub members: Vec<(MemberId, ExprId)>,
    pub tail_struct: Option<ExprId>,

is that the name of the concept in rust?

@TomerStarkware TomerStarkware force-pushed the tomer/structCtor_tail branch 9 times, most recently from c3c54e9 to af3b53c Compare January 25, 2024 16:12
Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 9 files reviewed, 11 unresolved discussions (waiting on @orizi and @TomerStarkware)


crates/cairo-lang-formatter/src/node_properties.rs line 100 at r4 (raw file):

                true
            }

remove


crates/cairo-lang-formatter/src/node_properties.rs line 146 at r4 (raw file):

            }

            _ => false,

Suggestion:

            SyntaxKind::TokenDotDot
                if grandparent_kind(db, self) == Some(SyntaxKind::StructArgTail) =>
            {
                true
            }
            _ => false,

crates/cairo-lang-semantic/src/diagnostic.rs line 676 at r6 (raw file):

                                                                     all the fields in the struct \
                                                                     have already been specified."
                .into(),

Suggestion:

            SemanticDiagnosticKind::StructTailExpressionNoEffect => {
                "Struct update has no effect, all the fields in the struct have already been specified."
                .into(),
        }

crates/cairo-lang-semantic/src/expr/compute.rs line 1708 at r6 (raw file):

    if members.len() == member_exprs.len() {
        if let Some((_, base_struct_syntax)) = base_struct {
            return Err(ctx.diagnostics.report(&base_struct_syntax, StructTailExpressionNoEffect));

Consider changing to warning.

Code quote:

return Err(ctx.diagnostics.report(&base_struct_syntax, StructTailExpressionNoEffect));

crates/cairo-lang-semantic/src/expr/test_data/structure line 7 at r4 (raw file):


//! > function
#[cairofmt::skip]

Why skip?

Code quote:

#[cairofmt::skip]

crates/cairo-lang-semantic/src/expr/test_data/structure line 129 at r4 (raw file):

 --> lib.cairo:5:15
    A { a: 4, ..a }
              ^*^

Change Struct update to reflect the name you'll chose in the code.

Code quote:

//! > expected_diagnostics
error: Struct update has no effect, all the fields in the struct have already been specified.
 --> lib.cairo:5:15
    A { a: 4, ..a }
              ^*^

Copy link
Collaborator

@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.

Reviewed 1 of 1 files at r3, 1 of 4 files at r4.
Reviewable status: 3 of 9 files reviewed, 9 unresolved discussions (waiting on @gilbens-starkware and @TomerStarkware)


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

            (*member_id, lower_expr_to_var_usage(ctx, builder, *expr_id))
        }));
    if members.len() != members_vars_usages.len() {

add comment about the handling.


crates/cairo-lang-semantic/src/expr/objects.rs line 447 at r5 (raw file):

    pub concrete_struct_id: ConcreteStructId,
    pub members: Vec<(MemberId, ExprId)>,
    pub tail_struct: Option<ExprId>,

specifically - it being a struct is wholly uninteresting.

@TomerStarkware TomerStarkware force-pushed the tomer/structCtor_tail branch 3 times, most recently from eb2cdbb to 9462c55 Compare January 25, 2024 16:30
Copy link
Collaborator

@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.

Reviewed 1 of 4 files at r4, 1 of 2 files at r5, 2 of 3 files at r6, 1 of 1 files at r7, 1 of 2 files at r8, all commit messages.
Reviewable status: 7 of 9 files reviewed, 8 unresolved discussions (waiting on @gilbens-starkware and @TomerStarkware)


crates/cairo-lang-semantic/src/expr/compute.rs line 1708 at r6 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Consider changing to warning.

hmm - yeah - sounds sensible to me as well.


crates/cairo-lang-semantic/src/expr/objects.rs line 447 at r8 (raw file):

    pub concrete_struct_id: ConcreteStructId,
    pub members: Vec<(MemberId, ExprId)>,
    pub base_struct: Option<ExprId>,

doc what it is here as well - guesses arent clear.


tests/test_data/fib_struct.sierra line 30 at r8 (raw file):

drop<felt252>([1]) -> (); // 4
drop<felt252>([2]) -> (); // 5
struct_construct<Unit>() -> ([5]); // 6

why are there changes here?

Copy link
Collaborator Author

@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.

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


crates/cairo-lang-formatter/src/node_properties.rs line 100 at r4 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

remove

Done


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

Previously, orizi wrote…

?

removed


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

Previously, orizi wrote…

add comment about the handling.

Done.


crates/cairo-lang-lowering/src/test_data/struct line 16 at r1 (raw file):

Previously, orizi wrote…

revert

Done.


crates/cairo-lang-lowering/src/test_data/struct line 129 at r1 (raw file):

    b: felt252,
    c: (felt252, felt252),
}

Done.


crates/cairo-lang-semantic/src/expr/compute.rs line 1662 at r1 (raw file):

            }
            ast::StructArg::StructArgTail(tail_expr_syntax) => {
                // Todo(TomerStarkware): remove tail expression from argument list.

Done.


crates/cairo-lang-semantic/src/expr/compute.rs line 1708 at r6 (raw file):

Previously, orizi wrote…

hmm - yeah - sounds sensible to me as well.

Done.


crates/cairo-lang-semantic/src/expr/objects.rs line 447 at r1 (raw file):

Previously, orizi wrote…

is that the name of the concept in rust?

It was called tail struct in our parser,


crates/cairo-lang-semantic/src/expr/objects.rs line 447 at r5 (raw file):

Previously, orizi wrote…

specifically - it being a struct is wholly uninteresting.

changed to base_struct


crates/cairo-lang-semantic/src/expr/test_data/structure line 7 at r4 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Why skip?

so the struct won't fold to one line


crates/cairo-lang-semantic/src/expr/test_data/structure line 129 at r4 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Change Struct update to reflect the name you'll chose in the code.

Done.

@TomerStarkware TomerStarkware force-pushed the tomer/structCtor_tail branch 2 times, most recently from 04eec71 to 2bb82d0 Compare January 27, 2024 19:59
Copy link
Collaborator Author

@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.

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


crates/cairo-lang-semantic/src/expr/objects.rs line 447 at r8 (raw file):

Previously, orizi wrote…

doc what it is here as well - guesses arent clear.

Done.


tests/test_data/fib_struct.sierra line 30 at r8 (raw file):

Previously, orizi wrote…

why are there changes here?

changed back

Copy link
Collaborator

@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.

Reviewed 7 of 7 files at r9, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @TomerStarkware)


crates/cairo-lang-semantic/src/diagnostic.rs line 671 at r9 (raw file):

            }
            SemanticDiagnosticKind::StructBaseStructExpressionNotLast => {
                "The base struct must always be the last field.".into()

Suggestion:

                "The base struct must always be the last argument.".into()

crates/cairo-lang-semantic/src/expr/objects.rs line 448 at r9 (raw file):

    pub members: Vec<(MemberId, ExprId)>,
    // An optional base struct to copy missing members from in the struct construction.
    // For example `let x = MyStruct { a: 1, ..base }`.

Suggestion:

    /// The base struct to copy missing members from if provided.
    /// For example `let x = MyStruct { a: 1, ..base }`.

Copy link
Collaborator Author

@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.

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


crates/cairo-lang-semantic/src/diagnostic.rs line 671 at r9 (raw file):

            }
            SemanticDiagnosticKind::StructBaseStructExpressionNotLast => {
                "The base struct must always be the last field.".into()

Done.


crates/cairo-lang-semantic/src/expr/objects.rs line 448 at r9 (raw file):

    pub members: Vec<(MemberId, ExprId)>,
    // An optional base struct to copy missing members from in the struct construction.
    // For example `let x = MyStruct { a: 1, ..base }`.

Done.

Copy link
Collaborator

@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.

:lgtm:

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

Copy link
Contributor

@gilbens-starkware gilbens-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:

Reviewed 1 of 1 files at r3, 1 of 1 files at r7, 1 of 2 files at r8, 4 of 7 files at r9, 3 of 3 files at r10, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware)

@TomerStarkware TomerStarkware added this pull request to the merge queue Jan 28, 2024
Merged via the queue into main with commit e8a1e50 Jan 28, 2024
41 checks passed
@orizi orizi deleted the tomer/structCtor_tail branch February 4, 2024 08:37
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