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

Add return types to procedures #1197

Merged
merged 46 commits into from
Jan 17, 2023
Merged

Add return types to procedures #1197

merged 46 commits into from
Jan 17, 2023

Conversation

jubnzv
Copy link
Contributor

@jubnzv jubnzv commented Oct 27, 2022

This PR introduces the return keyword and the Return statement in the AST:

procedure foo() -> String
  a = my_string;
  return a;
end

Design decisions in the current implementation:

  • return takes as an argument a single variable with the return value. It seems natural for me to use this approach in the ANF-based language.
  • Don't support empty return statements (return;) for early exit when the procedure has no a return type. e.g. the following code is forbidden:
procedure no_return()
  match something with
  | 0 => (* do something *)
  | _ => return; (* typechecking error: cannot use an empty return for early exit *)
  end
end

TODO:

  • Eval support
  • Forbid usage of such procedures in Iterate statements
  • Dynamic behavior tests

Closes #578

@jubnzv jubnzv marked this pull request as ready for review November 17, 2022 11:04
@jubnzv jubnzv requested a review from jjcnn November 17, 2022 11:04
Copy link
Contributor

@jjcnn jjcnn left a comment

Choose a reason for hiding this comment

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

First of all, we need a fix to the concrete syntax for return statements.

Second, we need to restrict return types to non-maps, for consistency.

Third, I'd like a few more checks and tests:

  • Code that follows a return statement is dead, so shouldn't be allowed (note that this may not be needed, depending on how we change the concrete return syntax). For tests, we should also test that this works inside match statements.
  • For procedures with return types all branches of the procedure body should return a value. (Same caveat about return syntax as before)
  • Test: A procedure with a return type that is an address type should be able to return a value of a more specific type than the specified return type, and should not be able to return a value of a more general type than the specified return type.
  • Test: A returning procedure that calls another returning procedure. This is relevant to test because of the way EvalUtil.ml handles return values. Also add a test where the calls go returning->non-returning->returning, just in case.

src/base/Cashflow.ml Outdated Show resolved Hide resolved
## RETURN
##

<YOUR SYNTAX ERROR MESSAGE HERE>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add better error messages to all the new cases in ParserFaults.messages, but let's wait until all the syntax changes of v0.14 are ready.

src/base/ScillaLexer.mll Outdated Show resolved Hide resolved
src/base/ScillaParser.mly Outdated Show resolved Hide resolved
src/base/TypeChecker.ml Outdated Show resolved Hide resolved
src/base/TypeChecker.ml Outdated Show resolved Hide resolved
@jubnzv
Copy link
Contributor Author

jubnzv commented Nov 28, 2022

Thanks for the review!

First of all, we need a fix to the concrete syntax for return statements.

We should discuss the better syntax, which will be more convenient to developers. I suppose, we want to left semantics and abstract syntax as they're implemented.

Second, we need to restrict return types to non-maps, for consistency.

Done.

Code that follows a return statement is dead, so shouldn't be allowed (note that this may not be needed, depending on how we change the concrete return syntax). For tests, we should also test that this works inside match statements.

Done for the current implementation.

For procedures with return types all branches of the procedure body should return a value. (Same caveat about return syntax as before)

Done.

Test: A procedure with a return type that is an address type should be able to return a value of a more specific type than the specified return type, and should not be able to return a value of a more general type than the specified return type.

Could you please provide a minimal example for this?
As I understood, the actual return address may do not contain fields specified in the procedure signature?

Test: A returning procedure that calls another returning procedure. This is relevant to test because of the way EvalUtil.ml handles return values. Also add a test where the calls go returning->non-returning->returning, just in case.

Done. The test for dynamic behavior of returns (procedure-return-1.scilla) was improved.

@jjcnn
Copy link
Contributor

jjcnn commented Nov 28, 2022

Test: A procedure with a return type that is an address type should be able to return a value of a more specific type than the specified return type, and should not be able to return a value of a more general type than the specified return type.

Could you please provide a minimal example for this?
As I understood, the actual return address may do not contain fields specified in the procedure signature?

No, it's the other way around: The actual return address must contain all the fields specified in the procedure signature, but may contain more fields. In other words, the actual address must be assignable to the address in the procedure signature.

I think you've implemented the typecheck correctly, but I'd like there to be a test case for it, e.g.:

procedure (x : ByStr20 with contract field y : Uint128, field y : Bool end) : ByStr20 with contract field x : Uint128 end
  _return := x
end

Also, ideally, a negative test the other way around, just to make sure we're using assignable the right way around.

It's not necessary to do extensive testing of type_assignable here, though, because we have fairly extensive test cases for that elsewhere.

Copy link
Contributor

@rrw-zilliqa rrw-zilliqa left a comment

Choose a reason for hiding this comment

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

Looks (broadly) good to me, in that I can't see any obvious bugs...

src/base/SanityChecker.ml Outdated Show resolved Hide resolved
src/base/TypeChecker.ml Outdated Show resolved Hide resolved
@jubnzv jubnzv merged commit 8880cad into master Jan 17, 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.

Procedures to return values
3 participants