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

Implement the return keyword #7173

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

smores56
Copy link
Collaborator

@smores56 smores56 commented Oct 20, 2024

Closes #7104

This adds the return keyword to Roc, which allows returning early from anywhere in a function body. It unblocks the new try keyword, which is implemented using return : #7087

Here's a demo of the feature:

module []

staticValueDef =
    someVal =
        if 10 > 5 then
            x = 5
            return x
        else
            6

    someVal + 2

expect staticValueDef == 5

earlyReturnWithUnusedStatements = \n ->
    return ["$(Num.toStr n)"]

    if n > 3 then
        ["first", "with", "some", "items"]
    else
        ["other", "with", "multiple", "items"]

expect earlyReturnWithUnusedStatements 7 == ["7"]
expect earlyReturnWithUnusedStatements 3 == ["3"]

earlyReturnNested = \n ->
    (x, y) =
        if n > 5 then
            z = "early return!"
            return z
            (n, n - 2)
        else
            (n, n + 2)

    "did not early return with $(Num.toStr x), $(Num.toStr y)"

expect earlyReturnNested 7 == "early return!"
expect earlyReturnNested 3 == "did not early return with 3, 5"

earlyReturnOnlyReturn = \x ->
    return x

expect earlyReturnOnlyReturn 20 == 20
expect earlyReturnOnlyReturn "abc" == "abc"

Let's test this:

smores@smortress:~/dev/roc$ cargo run --bin roc -- test ~/dev/return-test.roc
...

── UNREACHABLE CODE in /home/smores/dev/return-test.roc ────────────────────────

This code won't run because it follows a return statement:

18│>      if n > 3 then
19│>          ["first", "with", "some", "items"]
20│>      else
21│>          ["other", "with", "multiple", "items"]


── UNREACHABLE CODE in /home/smores/dev/return-test.roc ────────────────────────

This code won't run because it follows a return statement:

31│              (n, n - 2)
                 ^^^^^^^^^^


── RETURN OUTSIDE OF FUNCTION in /home/smores/dev/return-test.roc ──────────────

This return statement doesn't belong to a function:

7│              return x
                ^^^^^^^^

────────────────────────────────────────────────────────────────────────────────

0 errors and 3 warnings found in 266 ms
.

Running tests…

────────────────────────────────────────────────────────────────────────────────
0 failed and 7 passed in 266 ms.

You'll notice that there's a static value that has a return in it. This PR marks those as warnings because we can still treat them as "early returns" to the end of the def block, pretty convenient that the implementation worked out that way!

Copy link
Collaborator

@joshuawarner32 joshuawarner32 left a comment

Choose a reason for hiding this comment

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

Parser changes LGTM

crates/compiler/parse/src/ast.rs Outdated Show resolved Hide resolved
@smores56 smores56 marked this pull request as ready for review October 25, 2024 06:03
@smores56 smores56 changed the title [ADDING TESTS] Implement the return keyword Implement the return keyword Oct 25, 2024
crates/compiler/can/src/def.rs Outdated Show resolved Hide resolved
crates/compiler/test_gen/src/gen_return.rs Show resolved Hide resolved
crates/repl_ui/src/repl_state.rs Outdated Show resolved Hide resolved
crates/reporting/src/error/canonicalize.rs Show resolved Hide resolved
crates/reporting/src/error/canonicalize.rs Show resolved Hide resolved
crates/reporting/src/error/canonicalize.rs Outdated Show resolved Hide resolved
alloc.concat([
alloc.reflow("But I need every "),
alloc.keyword("return"),
alloc.reflow(" statement in that function to return:"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

minor: this should probably say "but I need every branch of this function..."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you're getting at, but I think this is misleading when we have if expressions/statements and early returns. For example, in this function:

myFunc = \x ->
    y =
        if x == 5 then
            return "early return"
        else
            x + 2

    if y == 4 then
        (y, 1)
    else
        (y, 2)

Your suggested error would imply that the branches of the y expression should both be (Num *, Num *) even though the branches of y are actually Num *.

I'm not sure of a better way to express this, so I'll leave it for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I follow, those are not the branches of the function, they are the branches of a def inside the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. I think since "branches" refers usually to if/when branches, referring to the function's "branches" is a little confusing I expect, even if I personally can figure out what you mean. It seems like the most precise phrasing would be "every place your function can return from should return XYZ". Would that be worse or better in your eyes?

@agu-z
Copy link
Collaborator

agu-z commented Oct 25, 2024

We should probably add tests to test_reporting to make sure the errors don't break when we make other changes.

@smores56
Copy link
Collaborator Author

I've added tests for test_reporting, meaning all PR comments have been addressed (or at least responded to).

@@ -715,7 +726,6 @@ pub fn canonicalize_expr<'a>(

let output = Output {
references,
tail_call: None,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These were removed during my attempts to fix tail calls, but didn't need to be. However, their default value is None anyway, so I kept them out.

@smores56
Copy link
Collaborator Author

@ayazhafiz I've responded to all of your comments, let me know if there's anything else you want to have fixed.

P.S. I used all of your wording suggestions, but there are a couple articles different, I hope you don't mind.

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.

Add a return Keyword
4 participants