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

Require right-hand side of one-to-many assignments to be Indexable #11545

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ static ?= ## Enable static linking
O := .build
SOURCES := $(shell find src -name '*.cr')
SPEC_SOURCES := $(shell find spec -name '*.cr')
override FLAGS += -D preview_multi_assign $(if $(release),--release )$(if $(stats),--stats )$(if $(progress),--progress )$(if $(threads),--threads $(threads) )$(if $(debug),-d )$(if $(static),--static )$(if $(LDFLAGS),--link-flags="$(LDFLAGS)" )$(if $(target),--cross-compile --target $(target) )
override FLAGS += -D strict_multi_assign $(if $(release),--release )$(if $(stats),--stats )$(if $(progress),--progress )$(if $(threads),--threads $(threads) )$(if $(debug),-d )$(if $(static),--static )$(if $(LDFLAGS),--link-flags="$(LDFLAGS)" )$(if $(target),--cross-compile --target $(target) )
SPEC_WARNINGS_OFF := --exclude-warnings spec/std --exclude-warnings spec/compiler --exclude-warnings spec/primitives
SPEC_FLAGS := $(if $(verbose),-v )$(if $(junit_output),--junit_output $(junit_output) )
CRYSTAL_CONFIG_LIBRARY_PATH := '$$ORIGIN/../lib/crystal'
Expand Down
2 changes: 1 addition & 1 deletion spec/compiler/codegen/array_literal_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ end

private def enumerable_element_type
%(
struct Enumerable(T)
module Enumerable(T)
def self.element_type(x)
x.each { |elem| return elem }
ret = uninitialized NoReturn
Expand Down
59 changes: 46 additions & 13 deletions spec/compiler/codegen/multi_assign_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,20 @@ describe "Code gen: multi assign" do
CR
end

it "supports 1 to n assignment" do
run(<<-CR).to_i.should eq(123)
class Foo
def [](index)
index &+ 1
context "without strict_multi_assign" do
it "supports 1 to n assignment" do
run(<<-CR).to_i.should eq(123)
class Foo
def [](index)
index &+ 1
end
end
end

a, b, c = Foo.new
a &* 100 &+ b &* 10 &+ c
CR
end
a, b, c = Foo.new
a &* 100 &+ b &* 10 &+ c
CR
end

context "without strict_multi_assign" do
it "doesn't raise if value size in 1 to n assignment doesn't match target count" do
run(<<-CR).to_i.should eq(4)
require "prelude"
Expand All @@ -38,6 +38,27 @@ describe "Code gen: multi assign" do
end

context "strict_multi_assign" do
it "supports 1 to n assignment" do
run(<<-CR, flags: %w(strict_multi_assign)).to_i.should eq(123)
require "prelude"

class Foo
include Indexable(Int32)

def unsafe_fetch(index)
index &+ 1
end

def size
3
end
end

a, b, c = Foo.new
a &* 100 &+ b &* 10 &+ c
CR
end

it "raises if value size in 1 to n assignment doesn't match target count" do
run(<<-CR, flags: %w(strict_multi_assign)).to_i.should eq(5)
require "prelude"
Expand Down Expand Up @@ -119,6 +140,7 @@ describe "Code gen: multi assign" do
it "supports 1 to n assignment, with splat on left-hand side (2)" do
run(<<-CR).to_i.should eq(12345)
#{range_new}
#{include_indexable}

*a, b, c = {1, 2, 3, 4, 5}
a[0] &* 10000 &+ a[1] &* 1000 &+ a[2] &* 100 &+ b &* 10 &+ c
Expand All @@ -128,6 +150,7 @@ describe "Code gen: multi assign" do
it "supports 1 to n assignment, with splat on left-hand side (3)" do
run(<<-CR).to_i.should eq(12345)
#{range_new}
#{include_indexable}

a, b, *c = {1, 2, 3, 4, 5}
a &* 10000 &+ b &* 1000 &+ c[0] &* 100 &+ c[1] &* 10 &+ c[2]
Expand All @@ -147,6 +170,7 @@ describe "Code gen: multi assign" do
run(<<-CR).to_b.should be_true
#{tuple_new}
#{range_new}
#{include_indexable}

*x, _, _ = {1, 2}
x.is_a?(Tuple(*typeof(Tuple.new)))
Expand All @@ -157,6 +181,7 @@ describe "Code gen: multi assign" do
run(<<-CR).to_b.should be_true
#{tuple_new}
#{range_new}
#{include_indexable}

_, _, *x = {1, 2}
x.is_a?(Tuple(*typeof(Tuple.new)))
Expand Down Expand Up @@ -210,7 +235,7 @@ private def tuple_new
args
end
end
CR
CR
end

private def range_new
Expand All @@ -219,5 +244,13 @@ private def range_new
def initialize(@begin : B, @end : E, @exclusive : Bool = false)
end
end
CR
CR
end

private def include_indexable
<<-CR
struct Tuple(*T)
include Indexable(Union(*T))
end
CR
end
66 changes: 66 additions & 0 deletions spec/compiler/semantic/multi_assign_spec.cr
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,38 @@ describe "Semantic: multi assign" do
{a, b}
)) { tuple_of [int32, int32] }
end

it "doesn't error if assigning non-Indexable (#11414)" do
assert_no_errors <<-CR
class Foo
def [](index)
end

def size
3
end
end

a, b, c = Foo.new
CR
end

it "errors if assigning non-Indexable to splat (#11414)" do
assert_error <<-CR, "right-hand side of one-to-many assignment must be an Indexable, not Foo"
require "prelude"

class Foo
def [](index)
end

def size
3
end
end

a, *b, c = Foo.new
CR
end
end

context "strict_multi_assign" do
Expand Down Expand Up @@ -60,5 +92,39 @@ describe "Semantic: multi assign" do
{a, b}
), flags: "strict_multi_assign") { tuple_of [int32, union_of(int32, string)] }
end

it "errors if assigning non-Indexable (#11414)" do
assert_error <<-CR, "right-hand side of one-to-many assignment must be an Indexable, not Foo", flags: "strict_multi_assign"
require "prelude"

class Foo
def [](index)
end

def size
3
end
end

a, b, c = Foo.new
CR
end

it "errors if assigning non-Indexable to splat (#11414)" do
assert_error <<-CR, "right-hand side of one-to-many assignment must be an Indexable, not Foo", flags: "strict_multi_assign"
require "prelude"

class Foo
def [](index)
end

def size
3
end
end

a, *b, c = Foo.new
CR
end
end
end
7 changes: 5 additions & 2 deletions src/compiler/crystal/program.cr
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ module Crystal
types["Struct"] = struct_t = @struct_t = NonGenericClassType.new self, self, "Struct", value
abstract_value_type(struct_t)

types["Enumerable"] = @enumerable = GenericModuleType.new self, self, "Enumerable", ["T"]
types["Indexable"] = @indexable = GenericModuleType.new self, self, "Indexable", ["T"]

types["Array"] = @array = GenericClassType.new self, self, "Array", reference, ["T"]
types["Hash"] = @hash_type = GenericClassType.new self, self, "Hash", reference, ["K", "V"]
types["Regex"] = @regex = NonGenericClassType.new self, self, "Regex", reference
Expand Down Expand Up @@ -450,8 +453,8 @@ module Crystal
end

{% for name in %w(object no_return value number reference void nil bool char int int8 int16 int32 int64 int128
uint8 uint16 uint32 uint64 uint128 float float32 float64 string symbol pointer array static_array
exception tuple named_tuple proc union enum range regex crystal
uint8 uint16 uint32 uint64 uint128 float float32 float64 string symbol pointer enumerable indexable
array static_array exception tuple named_tuple proc union enum range regex crystal
packed_annotation thread_local_annotation no_inline_annotation
always_inline_annotation naked_annotation returns_twice_annotation
raises_annotation primitive_annotation call_convention_annotation
Expand Down
33 changes: 23 additions & 10 deletions src/compiler/crystal/semantic/cleanup_transformer.cr
Original file line number Diff line number Diff line change
Expand Up @@ -307,15 +307,28 @@ module Crystal
end

def transform(node : MultiAssign)
if @program.has_flag?("strict_multi_assign")
if node.values.size == 1
# the expanded node always starts with `temp = {{ node.values[0] }}`;
# this is the whole Assign node and its deduced type is equal to the
# original RHS's type
temp_assign = node.expanded.as(Expressions).expressions.first
target_count = node.targets.size

case type = temp_assign.type
expanded = node.expanded

if node.values.size == 1 && expanded
# the expanded node always starts with `temp = {{ node.values[0] }}`;
# `temp_assign` is this whole Assign node and its deduced type is same
# as the original RHS's type
temp_assign = expanded.as(Expressions).expressions.first
type = temp_assign.type
target_count = node.targets.size
has_strict_multi_assign = @program.has_flag?("strict_multi_assign")

# disallows `a, *b, c = {0 => "x", 1 => "y", 2 => "z"}`, as `Hash` is
# not `Indexable`
# also disallows `a, b, c = ...` if the strict flag is set
if node.targets.any?(Splat) || has_strict_multi_assign
unless type.implements?(@program.indexable)
node.values.first.raise "right-hand side of one-to-many assignment must be an Indexable, not #{type}"
end
end

if has_strict_multi_assign
case type
when UnionType
sizes = type.union_types.map { |union_type| constant_size(union_type) }
if sizes.none? &.in?(target_count, nil)
Expand All @@ -331,7 +344,7 @@ module Crystal
end
end

if expanded = node.expanded
if expanded
return expanded.transform self
end

Expand Down
2 changes: 1 addition & 1 deletion src/compiler/crystal/semantic/type_guess_visitor.cr
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,7 @@ module Crystal
end

def guess_type(node : RegexLiteral)
program.types["Regex"]
program.regex
end

def guess_type(node : TupleLiteral)
Expand Down