Bug #7564: r38175 introduces incompatibility

• Status: Closed
• Assignee: nobu (Nobuyoshi Nakada)
• Target version: 2.0.0
• ruby -v: ruby 2.0.0dev (2012-12-15 trunk 38381) [x86_64-darwin12.2.1]

[tenderlovemaking (Aaron Patterson)]

r38175 introduces incompatibility with 1.9.3. Before r38175, when looking for _dump, Marshal would not call method_missing. Now marshal calls method_missing when trying to dump.

The following example exits with no error on 1.9.3, but on trunk it raises an exception (_dump() must return string (TypeError)):

class TR
def initialize calls = []
@calls = calls
end

def method_missing name, *args
@calls << [name, args]
end
end

Marshal.dump TR.new

I've attached a test case.

[ko1 (Koichi Sasada)]

• Category set to core
• Assignee set to mame (Yusuke Endoh)
• Target version set to 2.0.0

mame-san, how about this ticket?

[tenderlovemaking (Aaron Patterson)]

Bump. /cc nobu

[zenspider (Ryan Davis)]

I'm getting bit by this in my multi-version CI on Flay and any other project that uses the sexp gem and calls my deep_clone:

def deep_clone
  Marshal.load(Marshal.dump(self))
end

[usa (Usaku NAKAMURA)]

• Status changed from Open to Assigned

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Rejected

If method_missing does not deal with a method call, it should raise NoMethodError.

def method_missing name, *args
  @calls << [name, args]
  super
end 

On Sun, Dec 23, 2012 at 10:01:33AM +0900, nobu (Nobuyoshi Nakada) wrote:

Issue #7564 has been updated by nobu (Nobuyoshi Nakada).

Status changed from Assigned to Rejected

If method_missing does not deal with a method call, it should raise NoMethodError.

def method_missing name, *args
  @calls << [name, args]
  super
end 

Before this changeset, method_missing did deal with all method
calls. That's why this is not backwards compatible.

--
Aaron Patterson
http://tenderlovemaking.com/

[zenspider (Ryan Davis)]

I still think this is a bug, as shown by needing a respond_to? that does nothing more than call super:

class Sexp < Array
  def inspect
    "s(#{map(&:inspect).join ', '})"
  end

  def respond_to? meth
    super
  end if ENV["WHY_DO_I_NEED_THIS"]

  def method_missing meth, delete = false
    raise "shouldn't be here: #{meth.inspect}"
  end
end

def s *args
  Sexp.new args
end

p Marshal.load Marshal.dump s(1, 2, 3)
puts

# % WHY_DO_I_NEED_THIS=1 ruby20 trunk_bug.rb && ruby20 trunk_bug.rb
# s(1, 2, 3)
#
# trunk_bug.rb:11:in `method_missing': shouldn't be here: :marshal_dump (RuntimeError)
#       from trunk_bug.rb:19:in `dump'
#       from trunk_bug.rb:19:in `<main>'

[zenspider (Ryan Davis)]

• Status changed from Rejected to Open

No, really. This is a bug that needs more eyeballs.

A respond_to? with just a super should be equivalent to no code at all.

Can we get Matz and Mame to weigh in?

[nobu (Nobuyoshi Nakada)]

Anonymous wrote:

Before this changeset, method_missing diddeal with all method
calls.

No, previously respond_to? was called so method_missing did not get called.

That's why this is not backwards compatible.

It depended on the internal behavior too much.
And the bug that overriding method_missing without respond_to_missing? has been revealed.
Just like overriding hash without eql?.

[zenspider (Ryan Davis)]

This seems highly inconsistent to me. Specifically, MM + RT is the only working solution, but RT implementation is entirely meaningless. It doesn't make sense to me that I need a method table entry that does nothing but super. That should be the same as not existing in the method table.

Notes:

Runtime	Passes	Failures
(none)		1.8 1.9 2.0 fails #blah (expected)
MM	1.8 1.9	2.0 raises w/ marshal_dump
MM + RT	1.8 1.9 2.0
MM + RTM	1.8 1.9	2.0 raises w/ marshal_dump (doesn't use RTM)
MM + RT + RTM	1.8 1.9 2.0
MM + ARTM	1.8	1.9 SSE, 2.0 raises
MM + RT + ARTM	1.8	1.9 SSE, 2.0 SSE (SystemStackError)
RT		1.8 1.9 2.0 fails #blah (expected)
RTM		1.8 1.9 2.0 fails #blah (expected)
ARTM		1.8 2.0 fails #blah (expected), 1.9 SSE

class Sexp < Array
  def inspect
    "s(#{map(&:inspect).join ', '})"
  end

  def method_missing meth, delete = false
    x = nil
    return x if x = find { |o| Sexp === o && o.first == meth }
    raise "shouldn't be here: #{inspect}.#{meth}"
  end if ENV["MM"]

  def respond_to? meth, private = false
    p :respond_to? => meth if ENV["V"]
    super
  end if ENV["RT"]

  def respond_to_missing? meth, private = false
    p :respond_to_missing? => meth if ENV["V"]
    super
  end if ENV["RTM"]

  alias respond_to_missing? respond_to? if ENV["ARTM"]
end

def s *args
  Sexp.new args
end

END { puts }
p Marshal.load Marshal.dump s(1, 2, 3)
p s(1, 2, s(:blah)).blah

[ko1 (Koichi Sasada)]

• Priority changed from Normal to 7

[matz (Yukihiro Matsumoto)]

Since this is incompatibility, I propose to get back to the old behavior, for 2.0.
We have to discuss the issue that @zenspider (Ryan Davis) mentioned in separated thread.

Matz.

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned
• Assignee changed from mame (Yusuke Endoh) to nobu (Nobuyoshi Nakada)

I agree with Matz. Nobu, please handle this.

[nobu (Nobuyoshi Nakada)]

• Status changed from Assigned to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r38888.
Aaron, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

marshal.c: get back to the old behavior

• marshal.c (w_object, r_object0): separate respond_to checks and
  calling, and get back to the old behavior for 2.0. [Bug #7564]