Project

General

Profile

Actions

Bug #7564

closed

r38175 introduces incompatibility

Added by tenderlovemaking (Aaron Patterson) over 11 years ago. Updated about 11 years ago.

Status:
Closed
Target version:
ruby -v:
ruby 2.0.0dev (2012-12-15 trunk 38381) [x86_64-darwin12.2.1]
Backport:
[ruby-core:50900]

Description

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.


Files

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

Related issues 1 (0 open1 closed)

Related to Ruby master - Bug #7638: trunk で rails の activesupport のテストが失敗してしまうClosednobu (Nobuyoshi Nakada)12/30/2012Actions

Updated by ko1 (Koichi Sasada) over 11 years ago

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

mame-san, how about this ticket?

Updated by zenspider (Ryan Davis) over 11 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

Updated by usa (Usaku NAKAMURA) over 11 years ago

  • Status changed from Open to Assigned

Updated by nobu (Nobuyoshi Nakada) over 11 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 

Updated by Anonymous over 11 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/

Updated by zenspider (Ryan Davis) about 11 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>'

Updated by zenspider (Ryan Davis) about 11 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?

Updated by nobu (Nobuyoshi Nakada) about 11 years ago

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

Updated by zenspider (Ryan Davis) about 11 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

Updated by ko1 (Koichi Sasada) about 11 years ago

  • Priority changed from Normal to 7

Updated by matz (Yukihiro Matsumoto) about 11 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 (Ryan Davis) mentioned in separated thread.

Matz.

Updated by mame (Yusuke Endoh) about 11 years ago

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

I agree with Matz. Nobu, please handle this.

Actions #14

Updated by nobu (Nobuyoshi Nakada) about 11 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]
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0