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

Feat/return empty #3134

Merged
merged 36 commits into from
May 22, 2023
Merged

Conversation

gaetbout
Copy link
Contributor

@gaetbout gaetbout commented May 13, 2023

The return and break expression have to return something, and this something is mandatory.
In the case of returning unit type, it was mandatory to make it explicit by adding ();

return ();
break ();

With this PR it is not mandatory anymore. These will be added automatically:

return;
break;

This should fix #3034


This change is Reviewable

@gaetbout
Copy link
Contributor Author

gaetbout commented May 13, 2023

@spapinistarkware Thanks for all the help you provided, I hope this fits SW requirements and solves the issue

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 18 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gaetbout)


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

        | semantic::Statement::Break(semantic::StatementBreak { expr_option, stable_ptr }) => {
            log::trace!("Lowering a return | break statement.");
            match expr_option {

Only ret_var is different. Unite two branches as much as possible.


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

                        }
                        Statement::Continue(_) => (),
                        Statement::Return(stmt) => match stmt.expr_option {

Use .map()


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

                return Err(ctx.diagnostics.report(return_syntax, ReturnNotAllowedInsideALoop));
            }

More code reuse. Less indentation. For now example, in both branches, I would just do
let (expr_option, expr_ty) = match ...
old code...

@gaetbout
Copy link
Contributor Author

@spapinistarkware due to type mismatch I had to modify compute.rs.
Probably I'm missing smthng, let me know

Copy link
Contributor

@spapinistarkware spapinistarkware 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 3 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gaetbout)

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 15 of 18 files at r1, 1 of 3 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gaetbout)

a discussion (no related file):
please add diagnostics tests that break;and return; provides sensible diagnostics when they don't make sense.


@gaetbout
Copy link
Contributor Author

Hi @orizi
I hope I did what you were asking me 😅
Otherwise, could you please provide some pointer on what I should be looking at?

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 all commit messages.
Reviewable status: 19 of 20 files reviewed, 3 unresolved discussions (waiting on @gaetbout and @spapinistarkware)


crates/cairo-lang-semantic/src/expr/test_data/loop line 137 at r6 (raw file):

//! > module_code

//! > expected_diagnostics

some diagnostic needs to appear here - () is not a valid bool.

Code quote:

//! > expected_diagnostics

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.

looks good - just one last comment :)

Reviewable status: 19 of 20 files reviewed, 2 unresolved discussions (waiting on @gaetbout and @spapinistarkware)

@gaetbout
Copy link
Contributor Author

@orizi
As no test is red, it appears I'm missing something somewhere, isn't it?

Copy link
Contributor

@spapinistarkware spapinistarkware left a comment

Choose a reason for hiding this comment

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

Ori left a comment

Reviewable status: 19 of 20 files reviewed, 2 unresolved discussions (waiting on @gaetbout and @orizi)


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

                            WrongReturnType { expected_ty, actual_ty: expr_ty },
                        );
                    }

This should be outside

Code quote:

                    let expr_ty = expr.ty();
                    let expected_ty = ctx
                        .get_signature(
                            return_syntax.stable_ptr().untyped(),
                            UnsupportedOutsideOfFunctionFeatureName::ReturnStatement,
                        )?
                        .return_type;
                    if !expected_ty.is_missing(db)
                        && !expr_ty.is_missing(db)
                        && ctx.resolver.inference().conform_ty(expr_ty, expected_ty).is_err()
                    {
                        ctx.diagnostics.report(
                            &expr_syntax,
                            WrongReturnType { expected_ty, actual_ty: expr_ty },
                        );
                    }

@gaetbout
Copy link
Contributor Author

gaetbout commented May 17, 2023

@orizi
Should be fine now 😄
Tried to extract some code out but due to miss-matched type I couldn't...
Maybe there is a way to make the last argument matching?
(Option<Id>, TypeId, ???)
Couldn't find a matching type between OptionExprClauseEmpty and ExprClause
Edit:
For the report to work

ctx.diagnostics.report(
    &expr_syntax,
    WrongReturnType { expected_ty, actual_ty: expr_ty },
);

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 2 files at r7, all commit messages.
Reviewable status: 19 of 20 files reviewed, 2 unresolved discussions (waiting on @gaetbout and @spapinistarkware)


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

                            WrongReturnType { expected_ty, actual_ty: expr_ty },
                        );
                    }

extract this into a closure or something just before (as it is basically the same code

Code quote:

                    let expected_ty = ctx
                        .get_signature(
                            return_syntax.stable_ptr().untyped(),
                            UnsupportedOutsideOfFunctionFeatureName::ReturnStatement,
                        )?
                        .return_type;
                    if !expected_ty.is_missing(db)
                        && !expr_ty.is_missing(db)
                        && ctx.resolver.inference().conform_ty(expr_ty, expected_ty).is_err()
                    {
                        ctx.diagnostics.report(
                            &empty_clause,
                            WrongReturnType { expected_ty, actual_ty: expr_ty },
                        );
                    }

@gaetbout
Copy link
Contributor Author

@orizi
Should be fine now 🥳

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 2 of 2 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gaetbout)

@spapinistarkware spapinistarkware added this pull request to the merge queue May 22, 2023
Merged via the queue into starkware-libs:main with commit f5da789 May 22, 2023
milancermak pushed a commit to milancermak/cairo that referenced this pull request May 23, 2023
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.

feat: return with no expression.
3 participants