roc-lang / roc

A fast, friendly, functional language.
https://roc-lang.org
Universal Permissive License v1.0
4.46k stars 313 forks source link

Support "early return" version of if-else statements #7060

Closed snobee closed 2 months ago

snobee commented 2 months ago

Addresses #7039.

I'm not familiar with the roc compiler so I might take a while to figure out the changes to the parser needed for this, but I thought I'd open a draft now

Any feedback is appreciated but not needed rn

snobee commented 2 months ago

Should I add a test for this as well?

smores56 commented 2 months ago

@snobee I'd add a test case (or cases) in https://github.com/roc-lang/roc/blob/main/crates/compiler/test_syntax/tests/test_fmt.rs that ensure that we handle the 4 below cases in the way we want:

1) Un-indented else keyword, un-indented block: probably a parse error, maybe indent the block? 2) Un-indented else keyword, indented block: valid, but removes a newline below the else keyword 3) Indented else keyword, un-indented block: valid, but adds a missing newline below the else keyword 4) Indented else keyword, indented block: probably just un-indent the block, or maybe a parse error?

No need to add cases if any of these are already represented. Also, the above are guesses at the desired behavior, do what you think is correct.

smores56 commented 2 months ago
module []

testFn = \abc ->
    if abc > 3 then
        456
        else

    123

This doesn't seem to be parsing/formatting for me, with or without the newline after the else

snobee commented 2 months ago

@smores56 I don't get the same issue. Does it give you an error?

smores56 commented 2 months ago

Nevermind, I must have done something wrong. I now have all of the above test cases formatting correctly except for the first, which is expected to fail parsing. Here's the error for that code:

module []

testFn = \abc ->
    if abc > 3 then
        456
    else

    123
smores@smortress:~/dev/roc$ cargo run --bin roc -- format test_1.roc
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/roc format test_1.roc`
We ran into an issue while compiling your code.
Sadly, we don't have a pretty error message for this case yet.
If you can't figure out the problem from the context below, please reach out at <https://roc.zulipchat.com>
Unexpected parse failure when parsing this formatting:

"module []\n\ntestFn = \\abc ->\n    if abc > 3 then\n        456\n    else\n\n    123\n"

Parse error was:

Expr(Closure(Body(If(IndentElseBranch(@68), @32), @32), @20), @9)

Location: crates/cli/src/format.rs:188:9
smores56 commented 2 months ago

So that means this is good to go! I'll approve, and wait for you to let me know if you feel like adding tests. If not, it's fine, the feature isn't really prone to failure IMO.

smores56 commented 2 months ago

Thanks for the contribution! I've wanted a feature like this for a while, I'm sure others will appreciate the cleanup this can provide.