Project

General

Profile

Bug #13312

String#casecmp raises TypeError instead of returning nil

Added by stomar (Marcus Stollsteimer) over 2 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Normal
Target version:
-
[ruby-core:80145]

Description

String#casecmp and String#casecmp? behave differently from other comparison methods: for incomparable values they raise a TypeError, while Symbol#{casecmp,casecmp?} and the #<=> methods (also for other classes) return nil:

"abc" <=> 1       # => nil
"abc".casecmp 1   # TypeError: no implicit conversion of Integer into String
"abc".casecmp? 1  # TypeError: no implicit conversion of Integer into String

:abc <=> 1        # => nil
:abc.casecmp 1    # => nil
:abc.casecmp? 1   # => nil

1  <=> Time.now   # => nil
[] <=> :foo       # => nil

This is surprising, since String#casecmp is essentially a case-insensitive version of String#<=>, which seems to imply that they should behave in a similar way. Also, the different behavior for String and Symbol might be an indication that this is a bug and not intentional.


Files

patch.diff (2.5 KB) patch.diff shingo (shingo morita), 03/29/2017 09:57 AM

Associated revisions

Revision 40bc846b
Added by stomar (Marcus Stollsteimer) over 2 years ago

string.c: fix String#{casecmp,casecmp?} for non-string arguments

  • string.c: make String#{casecmp,casecmp?} return nil for
    non-string arguments instead of raising a TypeError.

  • test/ruby/test_string.rb: add tests.

Reported by Marcus Stollsteimer. Based on a patch by Shingo Morita.
[ruby-core:80145] [Bug #13312]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@58837 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 58837
Added by stomar (Marcus Stollsteimer) over 2 years ago

string.c: fix String#{casecmp,casecmp?} for non-string arguments

  • string.c: make String#{casecmp,casecmp?} return nil for
    non-string arguments instead of raising a TypeError.

  • test/ruby/test_string.rb: add tests.

Reported by Marcus Stollsteimer. Based on a patch by Shingo Morita.
[ruby-core:80145] [Bug #13312]

Revision 58837
Added by stomar (Marcus Stollsteimer) over 2 years ago

string.c: fix String#{casecmp,casecmp?} for non-string arguments

  • string.c: make String#{casecmp,casecmp?} return nil for
    non-string arguments instead of raising a TypeError.

  • test/ruby/test_string.rb: add tests.

Reported by Marcus Stollsteimer. Based on a patch by Shingo Morita.
[ruby-core:80145] [Bug #13312]

Revision 58837
Added by stomar (Marcus Stollsteimer) over 2 years ago

string.c: fix String#{casecmp,casecmp?} for non-string arguments

  • string.c: make String#{casecmp,casecmp?} return nil for
    non-string arguments instead of raising a TypeError.

  • test/ruby/test_string.rb: add tests.

Reported by Marcus Stollsteimer. Based on a patch by Shingo Morita.
[ruby-core:80145] [Bug #13312]

Revision 102ec7f4
Added by stomar (Marcus Stollsteimer) over 2 years ago

NEWS: String#{casecmp,casecmp?} [Bug #13312]

git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@58838 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

Revision 58838
Added by stomar (Marcus Stollsteimer) over 2 years ago

NEWS: String#{casecmp,casecmp?} [Bug #13312]

Revision 58838
Added by stomar (Marcus Stollsteimer) over 2 years ago

NEWS: String#{casecmp,casecmp?} [Bug #13312]

Revision 58838
Added by stomar (Marcus Stollsteimer) over 2 years ago

NEWS: String#{casecmp,casecmp?} [Bug #13312]

History

#1

Updated by stomar (Marcus Stollsteimer) over 2 years ago

  • Description updated (diff)

Updated by shingo (shingo morita) over 2 years ago

I agree this is not expecting behavior. then I made a patch for this.
Added test codes below.

diff --git a/test/ruby/test_string.rb b/test/ruby/test_string.rb
index 7dbf27e..3b72d25 100644
--- a/test/ruby/test_string.rb
+++ b/test/ruby/test_string.rb
@@ -2318,6 +2318,12 @@ def test_compare_different_encoding_string

   def test_casecmp
     assert_equal(1, "\u3042B".casecmp("\u3042a"))
+    assert_nil("Foo".casecmp(:Foo))
+    assert_nil("1".casecmp(1))
+
+    o = Object.new
+    def o.to_str; "abcde"; end
+    assert_equal(1, "abcdef".casecmp(o))
   end

   def test_casecmp?
@@ -2325,6 +2331,12 @@ def test_casecmp?
     assert_equal(false, 'FoO'.casecmp?('BaR'))
     assert_equal(false, 'baR'.casecmp?('FoO'))
     assert_equal(true, 'äöü'.casecmp?('ÄÖÜ'))
+    assert_nil("Foo".casecmp?(:Foo))
+    assert_nil("1".casecmp?(1))
+
+    o = Object.new
+    def o.to_str; "foo"; end
+    assert_equal(true, "Foo".casecmp?(o))
   end

   def test_upcase2

Updated by hsbt (Hiroshi SHIBATA) over 2 years ago

  • Assignee set to stomar (Marcus Stollsteimer)
  • Status changed from Open to Assigned

stomar (Marcus Stollsteimer)

It should return nil same as other comparison methods. Can you handle this?

Updated by stomar (Marcus Stollsteimer) over 2 years ago

Can you handle this?

Ok.

#5

Updated by stomar (Marcus Stollsteimer) over 2 years ago

  • Status changed from Assigned to Closed

Applied in changeset trunk|r58837.


string.c: fix String#{casecmp,casecmp?} for non-string arguments

  • string.c: make String#{casecmp,casecmp?} return nil for
    non-string arguments instead of raising a TypeError.

  • test/ruby/test_string.rb: add tests.

Reported by Marcus Stollsteimer. Based on a patch by Shingo Morita.
[ruby-core:80145] [Bug #13312]

Updated by stomar (Marcus Stollsteimer) over 2 years ago

I did not include the type check in str_casecmp, which seems superfluous to me. I also adjusted the tests, docs, and examples to those of the corresponding Symbol methods, and fixed the code formatting.

hsbt (Hiroshi SHIBATA)

I also added a NEWS entry and updated the specs with a version guard for 2.5 (I do not know how this would be handled in case the change should be backported).

Updated by Eregon (Benoit Daloze) about 2 years ago

stomar (Marcus Stollsteimer) wrote:

I also added a NEWS entry and updated the specs with a version guard for 2.5 (I do not know how this would be handled in case the change should be backported).

Thanks, r58839 looks great.

In case of backport we can adjust the version guard, but only after a release of 2.x as we need to test against releases in ruby/spec Travis and locally.

Updated by usa (Usaku NAKAMURA) about 2 years ago

  • Backport changed from 2.2: UNKNOWN, 2.3: UNKNOWN, 2.4: UNKNOWN to 2.2: WONTFIX, 2.3: WONTFIX, 2.4: UNKNOWN

I think this change may not cause any compatibility issue with real-world scripts, but it means that nobody hits this problem, too.
Then I decided not to backport to ruby_2_3.

Also available in: Atom PDF