Revert "Insert explicit return expressions in functions/macros/do-blocks (fredrikekre#48)"

This reverts commit cca294f.

Author: tecosaur

README.md

@@ -460,7 +460,6 @@ 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)
@@ -620,58 +619,6 @@ x = a + b *
     d
 ```
 
-### Explicit `return`
-
-Explicit `return` statements are ensured in function and macro definitions 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.
- - No `return` is added in short form functions (`f(...) = ...`), short form anonymous
-   functions (`(...) -> ...`), and `do`-blocks (`f(...) do ...; ...; end`).
- - 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
-```
-
-#### 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,5 +48,4 @@ function _show_green_node(io, node, indent, pos, str, show_trivia)
             p += x.span
         end
     end
-    return
 end

src/Runic.jl

@@ -70,7 +70,6 @@ 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)
@@ -148,7 +147,6 @@ mutable struct Context
     next_sibling::Union{Node, Nothing}
     # parent::Union{Node, Nothing}
     lineage_kinds::Vector{JuliaSyntax.Kind}
-    lineage_macros::Vector{String}
 end
 
 const RANGE_FORMATTING_BEGIN = "#= RUNIC RANGE FORMATTING " * "BEGIN =#"
@@ -272,12 +270,11 @@ 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, line_ranges, indent_level, call_depth, format_on, prev_sibling, next_sibling,
-        lineage_kinds, lineage_macros
+        lineage_kinds
     )
 end
 
@@ -412,9 +409,6 @@ 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").
@@ -490,9 +484,6 @@ 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
@@ -549,7 +540,6 @@ 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/chisels.jl

@@ -822,57 +822,6 @@ 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

@@ -24,7 +24,6 @@ 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
 
 macro assert(expr)

src/main.jl

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

src/runestone.jl

@@ -3453,186 +3453,3 @@ function spaces_around_comments(ctx::Context, node::Node)
         return make_node(node, kids′)
     end
 end
-
-function return_node(ctx::Context, ret::Node)
-    ws = Node(JuliaSyntax.SyntaxHead(K"Whitespace", JuliaSyntax.TRIVIA_FLAG), 1)
-    kids = [
-        Node(JuliaSyntax.SyntaxHead(K"return", 0), 6),
-    ]
-    if is_leaf(ret)
-        replace_bytes!(ctx, "return ", 0)
-        push!(kids, ws)
-        push!(kids, ret)
-    else
-        @assert !(kind(first_leaf(ret)) in KSet"NewlineWs Whitespace")
-        replace_bytes!(ctx, "return ", 0)
-        push!(kids, ws)
-        push!(kids, ret)
-    end
-    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"macrocall"
-        # Check direct kids but also recurse into blocks to catch e.g. `@foo begin ... end`.
-        idx = findfirst(x -> kind(x) === K"return", kids)
-        idx === nothing || return true
-        return any(kids) do x
-            return !is_leaf(x) && kind(x) in KSet"let try if block macrocall" && has_return(x)
-        end
-    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
-        unreachable()
-    end
-end
-
-function explicit_return_block(ctx, node)
-    @assert kind(node) === K"block"
-    if has_return(node)
-        # If the block already has a return node (anywhere) we accept it and move on.
-        return nothing
-    end
-    kids = verified_kids(node)
-    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]
-    @assert kind(rexpr) !== K"return" # Should have been caught by has_return
-    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) ||
-            (kind(rexpr) in KSet"let if try" && !has_return(rexpr)) ||
-            (kind(rexpr) === K"macrocall" && !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])
-        end
-        # If this is a call node, and the call the function name contains `throw` or `error` we
-        # bail because `return throw(...)` looks kinda stupid.
-        if kind(rexpr) === K"call"
-            call_kids = verified_kids(rexpr)
-            fname_idx = findfirst(!JuliaSyntax.is_whitespace, call_kids)::Int
-            @assert fname_idx == firstindex(call_kids)
-            local fname
-            let p = position(ctx.fmt_io)
-                fname = String(read_bytes(ctx, call_kids[fname_idx]))
-                seek(ctx.fmt_io, p)
-            end
-            if contains(fname, "throw") || contains(fname, "error")
-                return nothing
-            end
-        end
-        # We will make changes so copy
-        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
-            @assert kind(first_leaf(rexpr)) !== K"NewlineWs"
-            # Can it happen that there are whitespace hidden in the previous node?
-            # Let's see if this assert ever fire.
-            if rexpr_idx > 1
-                prev = kids′[rexpr_idx - 1]
-                if !is_leaf(prev)
-                    @assert !(kind(last_leaf(prev)) in KSet"Whitespace NewlineWs")
-                end
-            end
-            # Check whether there are whitespace we need to overwrite
-            if kmatch(kids′, KSet"Whitespace", rexpr_idx - 1)
-                # The previous node is whitespace
-                spn = span(popat!(kids′, rexpr_idx - 1))
-                seek(ctx.fmt_io, position(ctx.fmt_io) - spn)
-                rexpr_idx -= 1
-                @assert kind(first_leaf(rexpr)) !== K"Whitespace"
-            end
-            @assert kind(first_leaf(rexpr)) !== K"Whitespace"
-            nl = "\n" * " "^(4 * ctx.indent_level)
-            nlnode = Node(JuliaSyntax.SyntaxHead(K"NewlineWs", JuliaSyntax.TRIVIA_FLAG), sizeof(nl))
-            insert!(kids′, rexpr_idx, nlnode)
-            rexpr_idx += 1
-            replace_bytes!(ctx, nl, spn)
-            accept_node!(ctx, nlnode)
-        end
-        ret = return_node(ctx, rexpr)
-        kids′[rexpr_idx] = ret
-        return make_node(node, kids′)
-    elseif kind(rexpr) in KSet"for while"
-        # For `for` and `while` loops we add `return` after the block.
-        @assert kind(kids′[end]) === K"NewlineWs"
-        @assert kind(last_leaf(rexpr)) === K"end"
-        insert_idx = lastindex(kids)
-        kids′ = kids′ === kids ? copy(kids) : kids′
-        for i in 1:(insert_idx - 1)
-            accept_node!(ctx, kids′[i])
-        end
-        # Insert newline
-        nl = "\n" * " "^(4 * ctx.indent_level)
-        nlnode = Node(JuliaSyntax.SyntaxHead(K"NewlineWs", JuliaSyntax.TRIVIA_FLAG), sizeof(nl))
-        insert!(kids′, insert_idx, nlnode)
-        replace_bytes!(ctx, nl, 0)
-        accept_node!(ctx, nlnode)
-        # Insert `return`
-        replace_bytes!(ctx, "return", 0)
-        retnode = Node(
-            JuliaSyntax.SyntaxHead(K"return", 0), [
-                Node(JuliaSyntax.SyntaxHead(K"return", 0), 6),
-            ]
-        )
-        insert!(kids′, insert_idx + 1, retnode)
-        return make_node(node, kids′)
-    else
-        # error("Unhandled node in explicit_return_block: $(kind(rexpr))")
-    end
-    return nothing
-end
-
-function explicit_return(ctx::Context, node::Node)
-    if !(!is_leaf(node) && kind(node) in KSet"function macro")
-        return nothing
-    end
-    if !safe_to_insert_return(ctx, node)
-        return nothing
-    end
-    kids = verified_kids(node)
-    pos = position(ctx.fmt_io)
-    block_idx = findlast(x -> kind(x) === K"block", verified_kids(node))
-    block_idx === nothing && return nothing
-    for i in 1:(block_idx - 1)
-        accept_node!(ctx, kids[i])
-    end
-    block′ = explicit_return_block(ctx, kids[block_idx])
-    seek(ctx.fmt_io, pos)
-    block′ === nothing && return nothing
-    kids′ = copy(kids)
-    kids′[block_idx] = block′
-    return make_node(node, kids′)
-end