Bug #7564

r38175 introduces incompatibility

Added by Aaron Patterson about 2 years ago. Updated about 2 years ago.

[ruby-core:50900]
Status:Closed
Priority:Immediate
Assignee:Nobuyoshi Nakada
ruby -v:ruby 2.0.0dev (2012-12-15 trunk 38381) [x86_64-darwin12.2.1] Backport:

Description

r38175 introduces incompatibility with 1.9.3. Before r38175, when looking for dump, Marshal would not call method_missing. Now marshal calls methodmissing 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.

bug.patch Magnifier (675 Bytes) Aaron Patterson, 12/15/2012 03:02 AM


Related issues

Related to Ruby trunk - Bug #7638: trunk で rails の activesupport のテストが失敗してしまう Closed 12/30/2012

Associated revisions

Revision 38888
Added by Nobuyoshi Nakada about 2 years ago

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]

Revision 38888
Added by Nobuyoshi Nakada about 2 years ago

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]

History

#1 Updated by Koichi Sasada about 2 years ago

  • Category set to core
  • Assignee set to Yusuke Endoh
  • Target version set to 2.0.0

mame-san, how about this ticket?

#2 Updated by Aaron Patterson about 2 years ago

Bump. /cc nobu

#3 Updated by Ryan Davis about 2 years ago

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

#4 Updated by Usaku NAKAMURA about 2 years ago

  • Status changed from Open to Assigned

#5 Updated by Nobuyoshi Nakada about 2 years ago

  • 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 

#6 Updated by Anonymous about 2 years ago

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/

#7 Updated by Ryan Davis about 2 years ago

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>'

#8 Updated by Ryan Davis about 2 years ago

  • 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?

#9 Updated by Nobuyoshi Nakada about 2 years ago

Anonymous wrote:

Before this changeset, method_missing *did*deal 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?.

#10 Updated by Ryan Davis about 2 years ago

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

#11 Updated by Koichi Sasada about 2 years ago

  • Priority changed from Normal to Immediate

#12 Updated by Yukihiro Matsumoto about 2 years ago

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

Matz.

#13 Updated by Yusuke Endoh about 2 years ago

  • Status changed from Open to Assigned
  • Assignee changed from Yusuke Endoh to Nobuyoshi Nakada

I agree with Matz. Nobu, please handle this.

#14 Updated by Nobuyoshi Nakada about 2 years ago

  • 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]

Also available in: Atom PDF