Splat values correctly inside return/break/next statements

[HertzDevil]

Return/break/next statements in Crystal are parsed as calls, and therefore accept single splats, but nothing is done to those splats. This is okay for returning a single tuple, since multiple return values are combined into a tuple expression by the parser (c.f. #10181 (comment)):

def foo; return 1, 2; end; foo    # => {1, 2}
def foo; return *{1, 2}; end; foo # => {1, 2}
def foo; return {1, 2}; end; foo  # => {1, 2}

But if there are non-splat return values, the results become different:

def foo; return 1, 2, 3; end; foo    # => {1, 2, 3}
def foo; return *{1, 2}, 3; end; foo # => {{1, 2}, 3}
def foo; return {1, 2}, 3; end; foo  # => {{1, 2}, 3}

Contrast this to Ruby where splats in return statements are also permitted:

def foo; return 1, 2, 3; end; foo    # => [1, 2, 3]
def foo; return *[1, 2], 3; end; foo # => [1, 2, 3]
def foo; return [1, 2], 3; end; foo  # => [[1, 2], 3]

This PR fixes this so that *{1, 2}, 3 in Crystal returns a flattened 3-tuple. It also rejects statements such as return *1 that were previously silently accepted (since the splats did nothing after all). Most notably, this brings the splat behaviour of return statements on par with yield:

def foo(*x)
  return x, *x, x
end

def foo(*x, &)
  yield x, *x, x
end

foo(1, 'a')            # => {{1, 'a'}, 1, 'a', {1, 'a'}}
foo(1, 'a') { |*v| v } # => {{1, 'a'}, 1, 'a', {1, 'a'}}

Internally, Crystal wraps the multiple return values inside a TupleLiteral, which means there is already a way to get splats inside them; if we extend the parser to also accept splats inside tuple literals in actual code, then #3718 could be implemented on top of this PR (without delegating {...} to Tuple.new(...)). However, the code paths between tuple literal parsing and return argument parsing seem considerably different, so that issue is not pursued here.

For simplicity, returning a single splat will be considered multiple return values after this PR, which may affect macros and formatters:

def foo; return *{1}; end
# Before this PR: bare splat in return expression
# After this PR: equivalent to `return {*{1}}`

[HertzDevil]

I am starting to think if we could do the following instead, and remove similar checks in other places:

module Crystal
  class MainVisitor
    def end_visit(node : Splat)
      node.bind_to node.exp

      if (type = node.exp.type?) && !type.is_a?(TupleInstanceType)
        node.raise "argument to splat must be a tuple, not #{type}"
      end
    end
  end
end

Running all the semantic tests locally reveals exactly two failing test cases which only affect error messages. The first one is a slight wording change:

it "errors if splatting a non-tuple (#9853)" do
  assert_error %(
    Array(*Int32)
    ),
    "argument to splat must be a tuple type, not Int32"
  # got: "argument to splat must be a tuple, not Int32"
end

The second is

it "errors if splatting union" do
  assert_error %(
    a = {1} || {1, 2}
    foo *a
    ),
    "not yet supported"
  # got: "argument to splat must be a tuple, not (Tuple(Int32) | Tuple(Int32, Int32))"
end

This is also probably acceptable since Crystal already merges unions of tuples of the same arity (not that I agree with it), so the unions that aren't reduced to tuples are precisely ones containing tuples of different arities.

[asterite]

This can probably be done in CleanUpTransformer (or something like that) where all types are known

[asterite]

Since this is a syntax change we also need to add specs for the formatter to verify these still format well.

[bcardiff]

I rebase locally on master and compiling a 2nd generator of the compiler in release mode seems to lead to almost double time in the Codegen (bc+obj) (14min vs 24 min).

Can someone verify/confirm?

The memory didn't increase. I am not sure if there are many occurrences of what I mention in the inline comment.

[HertzDevil]

Anyone care to test the build time again?

[oprypin]

I am not seeing a difference in build times, either with the latest change or not.

$ c=9ed19b90976e03a25728a8bc7dc3d4bd871791f4; git fetch origin $c; for commit in $(git merge-base $c origin/master) $c~ $c; do git checkout $commit && make clean && make -j release=1 && touch src/compiler/crystal.cr && time make release=1; done

From https://github.com/crystal-lang/crystal
 * branch                9ed19b90976e03a25728a8bc7dc3d4bd871791f4 -> FETCH_HEAD
HEAD is now at 015c5687b Update NOTICE's copyright year to 2021 (#10166)
Using /usr/bin/llvm-config [version=10.0.1]
rm -rf .build
rm -rf ./docs
rm -rf src/llvm/ext/llvm_ext.o
rm -rf src/ext/sigfault.o src/ext/libcrystal.a
Using /usr/bin/llvm-config [version=10.0.1]
g++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc -I/usr/include -std=c++14   -fno-exceptions -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
cc -fPIC    -c -o src/ext/sigfault.o src/ext/sigfault.c
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
CRYSTAL_CONFIG_LIBRARY_PATH="" CRYSTAL_CONFIG_BUILD_COMMIT="015c5687b" SOURCE_DATE_EPOCH="1609531932" ./bin/crystal build --release  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
Using /usr/bin/llvm-config [version=10.0.1]
CRYSTAL_CONFIG_LIBRARY_PATH="" CRYSTAL_CONFIG_BUILD_COMMIT="015c5687b" SOURCE_DATE_EPOCH="1609531932" ./bin/crystal build --release  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
Using compiled compiler at .build/crystal
make release=1  479.11s user 3.30s system 99% cpu 8:02.43 total
Previous HEAD position was 015c5687b Update NOTICE's copyright year to 2021 (#10166)
HEAD is now at e5df064fb Add formatter tests
Using /usr/bin/llvm-config [version=10.0.1]
rm -rf .build
rm -rf ./docs
rm -rf src/llvm/ext/llvm_ext.o
rm -rf src/ext/sigfault.o src/ext/libcrystal.a
Using /usr/bin/llvm-config [version=10.0.1]
g++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc -I/usr/include -std=c++14   -fno-exceptions -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
cc -fPIC    -c -o src/ext/sigfault.o src/ext/sigfault.c
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
CRYSTAL_CONFIG_LIBRARY_PATH="" CRYSTAL_CONFIG_BUILD_COMMIT="e5df064fb" SOURCE_DATE_EPOCH="1610643544" ./bin/crystal build --release  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
Using /usr/bin/llvm-config [version=10.0.1]
CRYSTAL_CONFIG_LIBRARY_PATH="" CRYSTAL_CONFIG_BUILD_COMMIT="e5df064fb" SOURCE_DATE_EPOCH="1610643544" ./bin/crystal build --release  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
Using compiled compiler at .build/crystal
make release=1  483.28s user 4.16s system 99% cpu 8:09.94 total
Previous HEAD position was e5df064fb Add formatter tests
HEAD is now at 9ed19b909 Don't use exp_values when there are no splats
Using /usr/bin/llvm-config [version=10.0.1]
rm -rf .build
rm -rf ./docs
rm -rf src/llvm/ext/llvm_ext.o
rm -rf src/ext/sigfault.o src/ext/libcrystal.a
Using /usr/bin/llvm-config [version=10.0.1]
g++ -c  -o src/llvm/ext/llvm_ext.o src/llvm/ext/llvm_ext.cc -I/usr/include -std=c++14   -fno-exceptions -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS
cc -fPIC    -c -o src/ext/sigfault.o src/ext/sigfault.c
ar -rcs src/ext/libcrystal.a src/ext/sigfault.o
CRYSTAL_CONFIG_LIBRARY_PATH="" CRYSTAL_CONFIG_BUILD_COMMIT="9ed19b909" SOURCE_DATE_EPOCH="1613701138" ./bin/crystal build --release  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
Using /usr/bin/llvm-config [version=10.0.1]
CRYSTAL_CONFIG_LIBRARY_PATH="" CRYSTAL_CONFIG_BUILD_COMMIT="9ed19b909" SOURCE_DATE_EPOCH="1613701138" ./bin/crystal build --release  -o .build/crystal src/compiler/crystal.cr -D without_openssl -D without_zlib
Using compiled compiler at .build/crystal
make release=1  490.38s user 4.69s system 99% cpu 8:17.48 total

$ crystal --version

Crystal 0.35.1 (2020-10-17)

My system specs

[beta-ziliani]

This one falls in the obscure middle ground between bug and breaking change, but I'd like to see it merged. Thanks @HertzDevil