Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support splat expansions inside tuple and array literals #10429

Conversation

HertzDevil
Copy link
Contributor

@HertzDevil HertzDevil commented Feb 20, 2021

Implements #3718. Does not implement double splats and #3718 (comment) (#to_array_splat). The codegen portion for Tuple splats is copied directly from #10193's head (with the codegen optimization).

Tuple and array literals have slightly different requirements on the splatted value's interface:

# `exp` must be a `Tuple` instance, it cannot be a union of `Tuple`s (of different arities)
{*exp}

# `exp` must respond to `#each(& : _ -> _)`
[*exp]
[*exp] of T
T{*exp}
T(U, ...){*exp}

Very few array-like types implement #each but do not include Enumerable, so the requirement on Enumerable shouldn't be a pressing issue. (CSV and Regex::MatchData are notable exceptions in the standard library.)

src/compiler/crystal/semantic/literal_expander.cr Outdated Show resolved Hide resolved
src/compiler/crystal/semantic/literal_expander.cr Outdated Show resolved Hide resolved
src/compiler/crystal/semantic/literal_expander.cr Outdated Show resolved Hide resolved
src/compiler/crystal/syntax/parser.cr Outdated Show resolved Hide resolved
@HertzDevil
Copy link
Contributor Author

Instead of __crystal_first there is now Enumerable.element_type, other kinds of literal expansions refer to class methods in the prelude so this shouldn't be a problem. It might replace the first_internal helper I added in some other PRs too.

#
# will end up calling `typeof(1, ::Enumerable.element_type({2, 3.5}), 4)`.
#
# NOTE: there should never be a need to call this method outside the standard library.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# NOTE: there should never be a need to call this method outside the standard library.
# WARNING: there should never be a need to call this method outside the standard library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should be a warning.
We could consider making this method nodoc, though. It's only internal API.

@HertzDevil
Copy link
Contributor Author

The PR currently doesn't allow non-Enumerable splats inside Arrays because of Array#concat(other : Enumerable)'s type restriction (array literals expand to concat, not the more verbose each loop). Should we remove that type restriction?

@straight-shoota
Copy link
Member

I suppose this restriction is not really necessary. However, what type would implement a compatible #each interface an not include Enumerable? Practically, I don't see much impact.

@HertzDevil
Copy link
Contributor Author

HertzDevil commented Jun 2, 2021

CSV, Enum (flags enums only), LLVM::FunctionCollection, and YAML::Nodes::Mapping.

For comparison Set#concat does not have the same restriction, so it is possible to write:

@[Flags]
enum Foo
  X
  Y
  Z
end

set = Set(Foo).new
set.concat(Foo::All) # => Set{X, Y, Z}

accept exp
{exp.type, @last}

if node.elements.any?(Splat)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great use of this variant of any?! It actually reads really nice too :-)

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this new feature! ❤️

@asterite asterite added this to the 1.1.0 milestone Jun 4, 2021
@asterite asterite merged commit 84a202c into crystal-lang:master Jun 4, 2021
@HertzDevil HertzDevil deleted the feature/container-literal-single-splat branch June 5, 2021 02:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants