Bug #7564

r38175 introduces incompatibility

Added by Aaron Patterson over 1 year ago. Updated over 1 year ago.

[ruby-core:50900]
Status:Closed
Priority:Immediate
Assignee:Nobuyoshi Nakada
Category:core
Target version:2.0.0
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 methodmissing. 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.

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 over 1 year ago

marshal.c: get back to the old behavior

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

History

#1 Updated by Koichi Sasada over 1 year 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 over 1 year ago

Bump. /cc nobu

#3 Updated by Ryan Davis over 1 year 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 over 1 year ago

  • Status changed from Open to Assigned

#5 Updated by Nobuyoshi Nakada over 1 year ago

  • Status changed from Assigned to Rejected

=begin
If (({method_missing})) does not deal with a method call, it should raise (({NoMethodError})).

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

#6 Updated by Anonymous over 1 year 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 over 1 year 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 respondto? meth
super
end if ENV["WHY
DOINEED_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

% WHYDOINEEDTHIS=1 ruby20 trunkbug.rb && ruby20 trunkbug.rb

s(1, 2, 3)

#

trunkbug.rb:11:in `methodmissing': shouldn't be here: :marshal_dump (RuntimeError)

from trunk_bug.rb:19:in `dump'

from trunk_bug.rb:19:in `'

#8 Updated by Ryan Davis over 1 year 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 over 1 year ago

=begin
:Anonymous wrote:
Before this changeset, (({method_missing})) ((did)) deal with all method
calls.

No, previously (({respondto?})) was called so (({methodmissing})) did ((not)) get called.

:Anonymous wrote:
That's why this is not backwards compatible.

It depended on the internal behavior too much.
And the bug that overriding (({methodmissing})) without (({respondto_missing?})) has been revealed.
Just like overriding (({hash})) without (({eql?})).
=end

#10 Updated by Ryan Davis over 1 year ago

=begin
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/ marshaldump
# 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

=end

#11 Updated by Koichi Sasada over 1 year ago

  • Priority changed from Normal to Immediate

#12 Updated by Yukihiro Matsumoto over 1 year 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 over 1 year 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 over 1 year 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 (wobject, robject0): separate respond_to checks and calling, and get back to the old behavior for 2.0. [Bug #7564]

Also available in: Atom PDF