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` 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 macro call the `return` is only added in
   case there is no `return` inside the macro.
 - 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`.

Since adding `return` changes the expression that a macro will see, this
rule is disabled for function definitions inside of macros with the
exception of some known ones from Base (e.g. `@inline`, `@generated`,
...).

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,65 @@ 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` 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 macro call, the `return` is only added in case there is no
+   `return` inside the macro.
+ - If the last expression is a function call, and the function name is (or contains) `throw`
+   or `error`, no `return` is added. This is because it is already obvious that these calls
+   terminate the function and don't return any value.
+
+Note that adding `return` changes the expression in a way that is visible to macros.
+Therefore it is, in general, not valid to add `return` to a function defined inside a macro
+since it isn't possible to know what the macro will expand to. For this reason this
+formatting rule is disabled for functions defined inside macros with the exception of some
+known and safe ones from Base (e.g. `@inline`, `@generated`, ...).
+
+For the same reason mentioned above, if the last expression in a function is a macro call it
+isn't valid to step in and add `return` inside. Instead the `return` will be added in front
+of the macro call like any other expression (unless there is already a `return` inside of
+the macro as described above).
+
+Examples:
+```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
+```
+
+#### Potential changes
+ - If the last expression is a `if` or `try` block it might be better to
+   recurse into the branches and add `return` there. Looking at real code, if a
+   function ends with an `if` block, it seems about 50/50 whether adding return
+   *after* the block or adding return inside the branches is the best choice.
+   Quite often `return if` is not the best but at least Runic's current
+   formatting will force to think about the return value.
+   See issue [#52](https://github.com/fredrikekre/Runic.jl/issues/52).
+
 ### 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/Runic.jl

@@ -60,6 +60,7 @@ function Base.show(io::IO, ::MIME"text/plain", node::Node)
     show(io, node)
     println(io)
     _show_green_node(io, node, "", 1, nothing, true)
+    return
 end
 
 function Base.show(io::IO, node::Node)
@@ -136,6 +137,7 @@ mutable struct Context
     next_sibling::Union{Node, Nothing}
     # parent::Union{Node, Nothing}
     lineage_kinds::Vector{JuliaSyntax.Kind}
+    lineage_macros::Vector{String}
 end
 
 function Context(
@@ -166,11 +168,12 @@ function Context(
     call_depth = 0
     prev_sibling = next_sibling = nothing
     lineage_kinds = JuliaSyntax.Kind[]
+    lineage_macros = String[]
     format_on = true
     return Context(
         src_str, src_tree, src_io, fmt_io, fmt_tree, quiet, verbose, assert, debug, check,
         diff, filemode, indent_level, call_depth, format_on, prev_sibling, next_sibling,
-        lineage_kinds
+        lineage_kinds, lineage_macros
     )
 end
 
@@ -305,6 +308,9 @@ function format_node_with_kids!(ctx::Context, node::Node)
     ctx.prev_sibling = nothing
     ctx.next_sibling = nothing
     push!(ctx.lineage_kinds, kind(node))
+    if kind(node) === K"macrocall"
+        push!(ctx.lineage_macros, macrocall_name(ctx, node))
+    end
 
     # The new node parts. `kids′` aliases `kids` and only copied below if any of the
     # nodes change ("copy-on-write").
@@ -380,6 +386,9 @@ function format_node_with_kids!(ctx::Context, node::Node)
     ctx.prev_sibling = prev_sibling
     ctx.next_sibling = next_sibling
     pop!(ctx.lineage_kinds)
+    if kind(node) === K"macrocall"
+        pop!(ctx.lineage_macros)
+    end
     ctx.call_depth -= 1
     # Return a new node if any of the kids changed
     if any_kid_changed
@@ -435,6 +444,7 @@ function format_node!(ctx::Context, node::Node)::Union{Node, Nothing, NullNode}
     @return_something no_spaces_around_colon_etc(ctx, node)
     @return_something parens_around_op_calls_in_colon(ctx, node)
     @return_something for_loop_use_in(ctx, node)
+    @return_something explicit_return(ctx, node)
     @return_something braces_around_where_rhs(ctx, node)
     @return_something indent_multiline_strings(ctx, node)
     @return_something four_space_indent(ctx, node)

src/ToggleableAsserts.jl

@@ -30,7 +30,7 @@ assert_enabled() = true
 
 macro assert(expr)
     code = macroexpand_assert(expr)
-    :(assert_enabled() ? $(code) : nothing)
+    return :(assert_enabled() ? $(code) : nothing)
 end
 
 const toggle_lock = ReentrantLock()
@@ -41,4 +41,5 @@ function enable_assert(enable::Bool)
             @eval Runic assert_enabled() = $enable
         end
     end
+    return
 end

src/chisels.jl

@@ -779,6 +779,57 @@ function kmatch(kids, kinds, i = firstindex(kids))
     return true
 end
 
+# Extract the macro name as written in the source.
+function macrocall_name(ctx, node)
+    @assert kind(node) === K"macrocall"
+    kids = verified_kids(node)
+    pred = x -> kind(x) in KSet"MacroName StringMacroName CmdMacroName core_@cmd"
+    mkind = kind(first_leaf_predicate(node, pred)::Node)
+    if kmatch(kids, KSet"@ MacroName")
+        p = position(ctx.fmt_io)
+        bytes = read(ctx.fmt_io, span(kids[1]) + span(kids[2]))
+        seek(ctx.fmt_io, p)
+        return String(bytes)
+    elseif kmatch(kids, KSet".") || kmatch(kids, KSet"CmdMacroName") ||
+            kmatch(kids, KSet"StringMacroName")
+        bytes = read_bytes(ctx, kids[1])
+        if mkind === K"CmdMacroName"
+            append!(bytes, "_cmd")
+        elseif mkind === K"StringMacroName"
+            append!(bytes, "_str")
+        end
+        return String(bytes)
+    elseif kmatch(kids, KSet"core_@cmd")
+        bytes = read_bytes(ctx, kids[1])
+        @assert length(bytes) == 0
+        return "core_@cmd"
+    else
+        # Don't bother looking in more complex expressions, just return unknown
+        return "<unknown macro>"
+    end
+end
+
+# Inserting `return` modifies the AST in a way that is visible to macros.. In general it is
+# never safe to change the AST inside a macro, but we make an exception for some common
+# "known" macros in order to be able to format functions that e.g. have an `@inline`
+# annotation in front.
+const MACROS_SAFE_TO_INSERT_RETURN = let set = Set{String}()
+    for m in (
+            "inline", "noinline", "propagate_inbounds", "generated", "eval",
+            "assume_effects", "doc",
+        )
+        push!(set, "@$m", "Base.@$m")
+    end
+    push!(set, "Core.@doc")
+    set
+end
+function safe_to_insert_return(ctx, node)
+    for m in ctx.lineage_macros
+        m in MACROS_SAFE_TO_INSERT_RETURN || return false
+    end
+    return true
+end
+
 ##########################
 # Utilities for IOBuffer #
 ##########################

src/debug.jl

@@ -20,6 +20,7 @@ function Base.showerror(io::IO, err::AssertionError)
             "please file an issue with a reproducible example at " *
             "https://github.com/fredrikekre/Runic.jl/issues/new."
     )
+    return
 end
 
 function macroexpand_assert(expr)

src/main.jl

@@ -424,6 +424,7 @@ function main(argv)
                 cmd = setenv(ignorestatus(cmd); dir = dir)
                 cmd = pipeline(cmd, stdout = stderr, stderr = stderr)
                 run_cmd(cmd)
+                return
             end
         end