Crystal: Inconsistent setter method behavior with Ruby

[icy-arctic-fox]

When using a setter method in Ruby, it always returns its assigned value, regardless of the value returned by the method. Maybe this is intentional, but is inconsistent with Ruby and can cause unexpected problems (which lead me to report this).

For instance:

class Foo
  @value = 0

  def value=(new_value)
    @value = new_value
    "Not a number"
  end
end

foo1 = Foo.new
puts foo1.value = 42 # Works

foo2 = Foo.new
foo2.value = foo1.value = 42 # Breaks, trying to assign string to Int32

https://play.crystal-lang.org/#/r/6ryq

The last line is something logical users may want to do. But because the return method of value= is used instead of new_value, it causes an unexpected compiler error. It tries to assign the string "Not a number" to foo2.value instead of 42.

With the exact code in Ruby:

class Foo
  @value = 0

  def value=(new_value)
    @value = new_value
    "Not a number"
  end
end

foo1 = Foo.new
puts foo1.value = 42 # Works

foo2 = Foo.new
foo2.value = foo1.value = 42 # Works (42) assigned to both

https://repl.it/repls/FixedJuniorFinance

I ran into this when I wanted to return an assigned value, but the setter method does some extra computation after setting the value, and that computation result was returned unexpectedly.

Crystal v0.28.0

[asterite]

Yes, this is something to improve. I recently mentioned this issue in another PR: https://github.com/crystal-lang/crystal/pull/7682#issuecomment-483791193

[asterite]

Here's a patch that makes it work:

diff --git a/spec/compiler/codegen/def_spec.cr b/spec/compiler/codegen/def_spec.cr
index c5ae5e5f0..2d6cc4abd 100644
--- a/spec/compiler/codegen/def_spec.cr
+++ b/spec/compiler/codegen/def_spec.cr
@@ -568,4 +568,72 @@ describe "Code gen: def" do
       foo { |a, b| }
       ))
   end
+
+  it "codegens setter (non-inlineable body, non-struct)" do
+    run(%(
+      class Foo
+        def x=(y)
+          'a'
+          'b'
+        end
+      end
+
+      foo = Foo.new
+      z = foo.x = 123
+      z
+      )).to_i.should eq(123)
+  end
+
+  it "codegens setter (non-inlineable body, struct)" do
+    run(%(
+      class Foo
+        def x=(y)
+          'a'
+          'b'
+        end
+      end
+
+      struct Bar
+        @x = 123
+        def x; @x; end
+      end
+
+      foo = Foo.new
+      z = foo.x = Bar.new
+      z.x
+      )).to_i.should eq(123)
+  end
+
+  it "codegens setter (inlineable body, non-struct)" do
+    run(%(
+      class Foo
+        def x=(y)
+          'a'
+        end
+      end
+
+      foo = Foo.new
+      z = foo.x = 123
+      z
+      )).to_i.should eq(123)
+  end
+
+  it "codegens setter (inlineable body, struct)" do
+    run(%(
+      class Foo
+        def x=(y)
+          'a'
+        end
+      end
+
+      struct Bar
+        @x = 123
+        def x; @x; end
+      end
+
+      foo = Foo.new
+      z = foo.x = Bar.new
+      z.x
+      )).to_i.should eq(123)
+  end
 end
diff --git a/spec/compiler/semantic/def_spec.cr b/spec/compiler/semantic/def_spec.cr
index b689e530f..fd6fa4e0b 100644
--- a/spec/compiler/semantic/def_spec.cr
+++ b/spec/compiler/semantic/def_spec.cr
@@ -486,4 +486,18 @@ describe "Semantic: def" do
       foo
     ), "there's no self in this scope"
   end
+
+  it "types setter" do
+    assert_type(%(
+      class Foo
+        def x=(y)
+          'a'
+        end
+      end
+
+      foo = Foo.new
+      z = foo.x = 1
+      z
+    )) { int32 }
+  end
 end
diff --git a/src/compiler/crystal/codegen/call.cr b/src/compiler/crystal/codegen/call.cr
index 9d39188a6..23c629d54 100644
--- a/src/compiler/crystal/codegen/call.cr
+++ b/src/compiler/crystal/codegen/call.cr
@@ -393,7 +393,7 @@ class Crystal::CodeGenVisitor
     body = target_def.body

     # Try to inline the call
-    if try_inline_call(target_def, body, self_type, call_args)
+    if try_inline_call(node, target_def, body, self_type, call_args)
       return
     end

@@ -419,7 +419,7 @@ class Crystal::CodeGenVisitor
   # to read a constant value or the value of an instance variable.
   # Additionally, not inlining instance variable getters changes the semantic
   # a program, so we must always inline these.
-  def try_inline_call(target_def, body, self_type, call_args)
+  def try_inline_call(node, target_def, body, self_type, call_args)
     return false if target_def.is_a?(External)

     case body
@@ -427,29 +427,36 @@ class Crystal::CodeGenVisitor
       return true unless @needs_value

       accept body
-      inline_call_return_value target_def, body
+      inline_call_return_value node, target_def, body, self_type, call_args
       return true
     when Var
       if body.name == "self"
         return true unless @needs_value

         @last = self_type.passed_as_self? ? call_args.first : type_id(self_type)
-        inline_call_return_value target_def, body
+        inline_call_return_value node, target_def, body, self_type, call_args
         return true
       end
     when InstanceVar
       return true unless @needs_value

       read_instance_var(body.type, self_type, body.name, call_args.first)
-      inline_call_return_value target_def, body
+      inline_call_return_value node, target_def, body, self_type, call_args
       return true
     end

     false
   end

-  def inline_call_return_value(target_def, body)
-    if target_def.type.nil_type?
+  def inline_call_return_value(node, target_def, body, self_type, call_args)
+    if node.setter?
+      # For a setter value (`foo.x = y`) the last value is `y`, which, depending
+      # on whether `foo` is passed as self or not, is the first or second codegen argument
+      @last = call_args[self_type.try(&.passed_as_self?) ? 1 : 0]
+
+      first_arg_type = node.args.first.type
+      @last = box_value(@last, first_arg_type) if first_arg_type.passed_by_value?
+    elsif target_def.type.nil_type?
       @last = llvm_nil
     else
       @last = upcast(@last, target_def.type, body.type)
@@ -510,23 +517,30 @@ class Crystal::CodeGenVisitor
         end
       end
     else
+      # For a setter value (`foo.x = y`) the last value is `y`, which, depending
+      # on whether `foo` is passed as self or not, is the first or second codegen argument
+      if node.is_a?(Call) && node.setter?
+        @last = call_args[self_type.try(&.passed_as_self?) ? 1 : 0]
+        type = node.args.first.type
+      end
+
       case type
       when .no_return?
         unreachable
       when .passed_by_value?
-        if @needs_value
-          union = alloca llvm_type(type)
-          store @last, union
-          @last = union
-        else
-          @last = llvm_nil
-        end
+        @last = @needs_value ? box_value(@last, type) : llvm_nil
       end
     end

     @last
   end

+  def box_value(value, type)
+    union = alloca llvm_type(type)
+    store value, union
+    union
+  end
+
   def set_call_attributes(node : Call, target_def, self_type, is_closure, fun_type)
     if external = target_def.c_calling_convention?
       set_call_attributes_external(node, external)
diff --git a/src/compiler/crystal/semantic/call.cr b/src/compiler/crystal/semantic/call.cr
index 5bbbf645d..5c921ad7f 100644
--- a/src/compiler/crystal/semantic/call.cr
+++ b/src/compiler/crystal/semantic/call.cr
@@ -77,8 +77,13 @@ class Crystal::Call

     @target_defs = matches

-    bind_to matches if matches
-    bind_to block.break if block
+    # For a setter like `foo.x = y` we'd like to use `y`'s type
+    if self.setter?
+      bind_to args.first
+    else
+      bind_to matches if matches
+      bind_to block.break if block
+    end

     if (parent_visitor = @parent_visitor) && matches
       if parent_visitor.typed_def? && matches.any?(&.raises?)
diff --git a/src/compiler/crystal/syntax/ast.cr b/src/compiler/crystal/syntax/ast.cr
index 6419ea861..8852d5ac0 100644
--- a/src/compiler/crystal/syntax/ast.cr
+++ b/src/compiler/crystal/syntax/ast.cr
@@ -545,6 +545,7 @@ module Crystal
     property? global : Bool
     property? expansion = false
     property? has_parentheses : Bool
+    property? setter = false

     def initialize(@obj, @name, @args = [] of ASTNode, @block = nil, @block_arg = nil, @named_args = nil, global = false, @name_column_number = 0, has_parentheses = false)
       @global = !!global
@@ -589,6 +590,7 @@ module Crystal
       clone = Call.new(@obj.clone, @name, @args.clone, @block.clone, @block_arg.clone, @named_args.clone, @global, @name_column_number, @has_parentheses)
       clone.name_size = name_size
       clone.expansion = expansion?
+      clone.setter = setter?
       clone
     end

@@ -602,6 +604,12 @@ module Crystal
       Location.new(loc.filename, loc.line_number, name_column_number + name_size)
     end

+    def setter?
+      name.ends_with?('=') &&
+        !(33 <= name.byte_at(0) <= 64) && # first letter must not be a symbol like `=` or `!`
+        args.size == 1 && !named_args && !block && !block_arg
+    end
+
     def_equals_and_hash obj, name, args, block, block_arg, named_args, global?
   end

[j8r]

Wait, what was the use case? The current behavior is totally logical, more than Ruby when using type restrictions.
This mean changing this:

class Foo
  @value = 0
  def value=(new_value) : String
    @value = new_value
    "Not a number" 
  end
end

to this?!

class Foo
  @value = 0
  def value=(new_value : T) forall T
    @value = new_value
    "Not a number" 
  end
end

[icy-arctic-fox]

The inconsistency is with what the method setter returns, not what it accepts.

The code:

foo1 = Foo.new
foo2 = Foo.new

foo2.value = foo1.value = 42

Does not work in Crystal, but works in Ruby. In Ruby, 42 is assigned to everything. In Crystal, the string "Not a number" is returned by the setter and attempts to set it to value. So Ruby always returns the argument given to the method when it's a setter. But Crystal takes the return value of the setter method instead.

The workaround is to always make sure the argument is returned from the setter method (if you want the Ruby behavior).

[ysbaddaden]

I believe Ruby got this right. Despite being a method, this is a setter; returning something else is unexpected. a = b = 42 means that a is 42, not something else, changing b to foo.b shouldn't suddenly break that.

[j8r]

To clarify this, there are two things if I've correctly understood:

• changing the behavior of setter (which is currently a macro)
• changing the behavior of the method def mymethod=(arg); end

What is your suggestion @ysbaddaden , to modify both? Only setter may be enough.

[ysbaddaden]

The workaround is to always make sure the argument is returned from the setter method

This workaround works and is what I usually do, but it must be done manually which is tedious and boring and impacts readability; it's also error prone, and it's too easy to break the contract. I believe the compiler should automate this.

@j8r Why should they be different? If so, that would introduce a confusing behavior. The setter macro is creating a setter method (i.e. def foo=(value)). Ary's patch above considers all foo= methods as setter and makes them return their argument.

[j8r]

How would you type the method then? It will become an exception that differs from other methods, that has to be documented here.
And also, how can we do to return a specific variable, even return won't work?

class Foo
  @value = 0
  def value=(new_value : Int32) : Int32
    @value = new_value
    return "Not a number" 
  end
end

Those coming from other languages will be surprised...

[konovod]

@j8r why would you need to return something else from a value=?
Of course this behaviour should be documented, but I think that if chained assignment is allowed, it should assign rightmost value to all arguments, so returning new_value is a good practice anyway. So why don't enforce it by compiler.

[j8r]

I think #[]= can be concerned too by this issue.

[straight-shoota]

I'm not sure this is something that needs fixing. The unexpected behaviour comes solely from a mistake in the method definition. The compiler shouldn't be responsible to fix that, but whoever wrote that method =)

Most setters are really simple (like def foo=(@foo); end) or even use the setter macro where this doesn't matter at all. If you have custom code in a setter method, you might have some custom considerations about what it should return.
The discussion so far completely missed the fact that the value that is passed to the setter might not be the value that is actually assigned when the setter itself applies some transformations. I imagine it would be pretty difficult to define a sound behaviour here, when you consider that the setter can mutate the argument.
I think it would be best to leave that for the individual implementation, and don't enforce any behaviour from the compiler. I'd suggest to close this issue.

[jhass]

I think this whole talk about the behavior of the setter method here is a straw-man, the behavior of the setter method is fine as is. What's not fine is the behavior of the assignment expression. That expression is what should return the value of the expression on its RHS no matter what's on its LHS.

Since there's been so much comparing to Ruby here, let's be throughout with it:

class Foo
  def bar=(v)
    @bar = v
    "x"
  end

  def []=(k, v)
    (@h ||= {})[k] = v
    "x"
  end
end

foo = Foo.new

p (x = 1) # => 1
p (foo.bar = 1) # => 1
p (foo.bar=(1)) # => 1 (this is still parsed as an assignment in Ruby)
p (foo.public_send(:bar=, 1)) # => "x"
p (foo[:bar] = 1) # => 1
p (foo[:bar]=(1)) # => 1 (still parsed as assignment)
p (foo.[]=(:bar, 1)) # => "x"
p (foo.public_send(:[]=, :bar, 1)) # => "x"

We can see the method does indeed return whatever it returns. It's just ignored in an assignment expression. And yes, that's something we should mirror.