Insert explicit return expressions in functions/macros/do-blocks

This patch make sure that function and macro definitions, as well as
do-blocks, end with an explicit `return` statement before the last
expression in the body. The following exceptions are made:

 - If the last expression is a `for` or `while` loop (which both always
   evaluate to `nothing`) `return nothing` is added *after* the loop.
 - If the last expression is a `if` or `try` block the `return` is only
   added in case there is no `return` inside any of the branches.
 - If the last expression is a `let` or `begin` block the `return` is
   only added in case there is no `return` inside the block.
 - If the last expression is a function call, and the function name is
   `throw`, `rethrow`, or `error`, no `return` is added. This is because
   it is pretty obvious that these calls terminate the function without
   the explicit `return`.

Closes #43.

Author: fredrikekre

README.md

@@ -244,6 +244,7 @@ This is a list of things that Runic currently is doing:
  - [Line width limit](#line-width-limit)
  - [Newlines in blocks](#newlines-in-blocks)
  - [Indentation](#indentation)
+ - [Explicit `return`](#explicit-return)
  - [Spaces around operators, assignment, etc](#spaces-around-operators-assignment-etc)
  - [Spaces around keywords](#spaces-around-keywords)
  - [Multiline listlike expressions](#multiline-listlike-expressions)
@@ -399,6 +400,42 @@ x = a + b *
     d
 ```
 
+### Explicit `return`
+
+Explicit `return` statements are ensured in function/macro definitions as well as in
+`do`-blocks by adding `return` in front of the last expression, with some exceptions listed
+below.
+
+ - If the last expression is a `for` or `while` loop (which both always evaluate to
+   `nothing`) `return nothing` is added *after* the loop.
+ - If the last expression is a `if` or `try` block the `return` is only added in case
+   there is no `return` inside any of the branches.
+ - If the last expression is a `let` or `begin` block the `return` is only added in case
+   there is no `return` inside the block.
+ - If the last expression is a function call, and the function name is `throw`, `rethrow`,
+   or `error`, no `return` is added. This is because it is pretty obvious that these calls
+   terminate the function without the explicit `return`.
+
+```diff
+ function f(n)
+-    sum(rand(n))
++    return sum(rand(n))
+ end
+
+ macro m(args...)
+-    :(generate_expr(args...))
++    return :(generate_expr(args...))
+ end
+
+ function g()
+-    open("/dev/random", "r") do f
+-        read(f, 8)
++    return open("/dev/random", "r") do f
++        return read(f, 8)
+     end
+ end
+```
+
 ### Spaces around operators, assignment, etc
 
 Runic formats spaces around infix operators, assignments, comparison chains, and type

src/JuliaSyntax.jl

@@ -48,4 +48,5 @@ function _show_green_node(io, node, indent, pos, str, show_trivia)
             p += x.span
         end
     end
+    return
 end

src/ToggleableAsserts.jl

@@ -36,7 +36,7 @@ end
 const toggle_lock = ReentrantLock()
 
 function enable_assert(enable::Bool)
-    @lock toggle_lock begin
+    return @lock toggle_lock begin
         if assert_enabled() != enable
             @eval Runic assert_enabled() = $enable
         end

src/runestone.jl

@@ -3371,24 +3371,58 @@ function return_node(ctx::Context, ret::Node)
     return Node(JuliaSyntax.SyntaxHead(K"return", 0), kids)
 end
 
+function has_return(node::Node)
+    kids = verified_kids(node)
+    if kind(node) in KSet"let catch else finally"
+        idx = findfirst(x -> kind(x) === K"block", kids)::Int
+        if kind(node) === K"let"
+            idx = findnext(x -> kind(x) === K"block", kids, idx + 1)::Int
+        end
+        return has_return(kids[idx])
+    elseif kind(node) in KSet"try if elseif"
+        # Look for the initial try/if block and then for
+        # catch/else/finally (for try) or elseif/else (for if).
+        pred = function(x)
+            return !is_leaf(x) && kind(x) in KSet"catch else finally elseif block"
+        end
+        idx = findfirst(pred, kids)
+        while idx !== nothing
+            has_return(kids[idx]) && return true
+            idx = findnext(pred, kids, idx + 1)
+        end
+        return false
+    elseif kind(node) === K"block"
+        # Don't care whether this is the last expression,
+        # that is the job of a linter or something I guess.
+        return findfirst(x -> kind(x) === K"return", kids) !== nothing
+    else
+        error("unreachable")
+    end
+end
+
 function explicit_return_block(ctx, node)
     @assert kind(node) === K"block"
     kids = verified_kids(node)
-    rexpr_idx = findlast(!JuliaSyntax.is_whitespace, kids)
-    rexpr_idx === nothing && return nothing
-    rexpr = kids[rexpr_idx]
+    kids′ = kids
+    rexpr_idx = findlast(!JuliaSyntax.is_whitespace, kids′)
+    if rexpr_idx === nothing
+        # Empty block. TODO: Perhaps add `return nothing`?
+        return nothing
+    end
+    rexpr = kids′[rexpr_idx]
     kind(rexpr) === K"return" && return nothing
     if is_leaf(rexpr) ||
             kind(rexpr) in KSet"call dotcall tuple vect ref hcat typed_hcat vcat typed_vcat \
                 ? && || :: juxtapose <: >: comparison string . -> comprehension do macro \
-                typed_comprehension where parens curly function quote global local =" ||
-            is_string_macro(rexpr) ||
-            is_assignment(rexpr) ||
+                typed_comprehension where parens curly function quote global local = macrocall" ||
+            is_string_macro(rexpr) || is_assignment(rexpr) ||
+            (kind(rexpr) in KSet"let if try" && !has_return(rexpr)) ||
+            (is_begin_block(rexpr) && !has_return(rexpr)) ||
             kind(rexpr) === K"quote" && JuliaSyntax.has_flags(rexpr, JuliaSyntax.COLON_QUOTE)
         # The cases caught in this branch are simple, just wrap the last expression in a
         # return node. Also make sure the previous node is a K"NewlineWs".
         for i in 1:(rexpr_idx - 1)
-            accept_node!(ctx, kids[i])
+            accept_node!(ctx, kids′[i])
         end
         # If this is a call node, and the call the function name is `throw` or `error` we
         # bail because `return throw(...)` looks kinda stupid.
@@ -3403,12 +3437,12 @@ function explicit_return_block(ctx, node)
                 fname = String(read_bytes(ctx, call_kids[fname_idx]))
                 seek(ctx.fmt_io, p)
             end
-            if fname === "throw" || fname === "error"
+            if fname === "throw" || fname === "rethrow" || fname === "error"
                 return nothing
             end
         end
         # We will make changes so copy
-        kids′ = copy(kids)
+        kids′ = kids′ === kids ? copy(kids) : kids′
         # Make sure the previous node is a K"NewlineWs"
         if !kmatch(kids′, KSet"NewlineWs", rexpr_idx - 1)
             spn = 0
@@ -3445,10 +3479,10 @@ function explicit_return_block(ctx, node)
         return make_node(node, kids′)
     elseif kind(rexpr) in KSet"for while"
         # For `for` and `while` loops we add `return nothing` after the block.
-        @assert kind(kids[end]) === K"NewlineWs"
+        @assert kind(kids′[end]) === K"NewlineWs"
         @assert kind(last_leaf(rexpr)) === K"end"
         insert_idx = lastindex(kids)
-        kids′ = copy(kids)
+        kids′ = kids′ === kids ? copy(kids) : kids′
         for i in 1:(insert_idx - 1)
             accept_node!(ctx, kids′[i])
         end
@@ -3469,16 +3503,6 @@ function explicit_return_block(ctx, node)
         )
         insert!(kids′, insert_idx + 1, retnode)
         return make_node(node, kids′)
-    elseif kind(rexpr) in KSet"if try let block macrocall $"
-        # These have been seen in the wild.
-        # TODO:
-        #  - `if` and `try` could be recursed into, but is that really so nice? `return if`?
-        #    Perhaps if no branches have `return`?
-        #    `if` without `else` could have `return nothing` at the end.
-        #  - `macrocall` could be inspected and have return if the argument is simple, e.g.
-        #    `@inbounds x[i]` -> `return @inbounds x[i]`.
-        #  - block (begin-end) and let could be recursed into?
-        #  - `$` could be anything but only used in eval so not super common...
     else
         # error("Unhandled node in explicit_return_block: $(kind(rexpr))")
     end

test/runtests.jl

@@ -1238,8 +1238,17 @@ end
                 "f() do\n        return x\n    end", "f() do x\n        return x\n    end",
                 "function f()\n        return x\n    end",
                 "function ()\n        return x\n    end",
-                "quote\n        x\n    end",
+                "quote\n        x\n    end", "begin\n        x\n    end",
+                "let\n        x\n    end", "let x = 42\n        x\n    end",
                 "x = 1", "x += 1", "x -= 1", "global x = 1", "local x = 1",
+                "@inbounds x[i]", "@inline f(x)",
+                "if c\n        x\n    end",
+                "if c\n        x\n    else\n        y\n    end",
+                "if c\n        x\n    elseif d\n        z\n    else\n        y\n    end",
+                "try\n        x\n    catch\n        y\n    end",
+                "try\n        x\n    catch e\n        y\n    end",
+                "try\n        x\n    catch\n        y\n    finally\n        z\n    end",
+                "try\n        x\n    catch\n        y\n    else\n        z\n    finally\n        z\n    end",
             )
             @test format_string("$f\n    $r\nend") == "$f\n    return $r\nend"
             @test format_string("$f\n    x;$r\nend") == "$f\n    x\n    return $r\nend"
@@ -1249,15 +1258,34 @@ end
                 format_string("$f\n    return $f\n        return $r\n    end\nend")
         end
         # If the last expression is a call to throw or error there should be no return
-        for r in ("throw(ArgumentError())", "error(\"foo\")")
+        for r in ("throw(ArgumentError())", "error(\"foo\")", "rethrow()")
             @test format_string("$f\n    $r\nend") == "$f\n    $r\nend"
         end
-        # These cases append `return nothing` to the end
+        # `for` and `while` append `return nothing` to the end
         for r in ("for i in I\n    end", "while i in I\n    end")
             @test format_string("$f\n    $r\nend") == "$f\n    $r\n    return nothing\nend"
             @test format_string("$f\n    $r\n    # comment\nend") ==
                 "$f\n    $r\n    # comment\n    return nothing\nend"
         end
+        # if/let/begin/try with a `return` inside should be left alone
+        for r in (
+                "if c\n        return x\n    end",
+                "if c\n        return x\n    else\n        y\n    end",
+                "if c\n        x\n    else\n        return y\n    end",
+                "if c\n        return x\n    elseif d\n        y\n    else\n        y\n    end",
+                "if c\n        x\n    elseif d\n        return y\n    else\n        z\n    end",
+                "if c\n        x\n    elseif d\n        y\n    else\n        return z\n    end",
+                "let\n        return x\n    end",
+                "let x = 1\n        return x\n    end",
+                "begin\n        return x\n    end",
+                "try\n        return x\n    catch\n        y\n    end",
+                "try\n        x\n    catch e\n        return y\n    end",
+                "try\n        x\n    catch\n        y\n    finally\n        return z\n    end",
+                "try\n        x\n    catch\n        y\n    else\n        return z\n    finally\n        z\n    end",
+            )
+            str = "$f\n    $r\nend"
+            @test format_string(str) == str
+        end
     end
 end