Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

returns String instances when called on a subclass in Ruby 3.0.0 #851

Merged

Conversation

lxxxvi
Copy link
Contributor

@lxxxvi lxxxvi commented Oct 1, 2021

Hello 👋

I hope I am getting this right. While looking through #823 I realized that some specs are not implemented for

The following methods now return or yield String instances
instead of subclass instances when called on subclass instances:
[Bug #10845]

Here's what I think is implemented/missing:

  • String#*
  • String#capitalize
  • String#center
  • String#chomp
  • String#chop
  • String#delete
  • String#delete_prefix
  • String#delete_suffix
  • String#downcase
  • String#dump
  • String#each_char
  • String#each_grapheme_cluster
  • String#each_line
  • String#gsub
  • String#ljust
  • String#lstrip
  • String#partition
  • String#reverse
  • String#rjust
  • String#rpartition
  • String#rstrip
  • String#scrub
  • String#slice!
  • String#slice / String#[]
  • String#split
  • String#squeeze
  • String#strip
  • String#sub
  • String#succ / String#next
  • String#swapcase
  • String#tr
  • String#tr_s
  • String#upcase

This PR currently adds the tests for String#lstrip, but I'd like to proceed and add the tests for the remaining methods in the list.

Before I do that, I'd like to ask you if my list is correct or if I overlook something?

Thanks a lot for your feedback and assistance.

@eregon
Copy link
Member

eregon commented Oct 2, 2021

Yes, this looks good 👍

@lxxxvi lxxxvi force-pushed the returns-string-instances-when-called-on-subclasses branch from 1faf27d to 9c8f9ec Compare October 2, 2021 15:45
@lxxxvi lxxxvi force-pushed the returns-string-instances-when-called-on-subclasses branch from 439aaa7 to 9e6f708 Compare October 4, 2021 07:56
@lxxxvi
Copy link
Contributor Author

lxxxvi commented Oct 4, 2021

@eregon

Thank you for your feedback! I have added the tests for these methods:

  • String#lstrip
  • String#partition
  • String#reverse
  • String#rpartition
  • String#rstrip
  • String#scrub
  • String#strip

I am looking forward to receiving your review.


However... I observed a few things that I'd like to share. May be you have some comments for them.

Destructive methods (still) return subclass

All destructive methods that return String-ish values still return an instance of subclass:

# ruby-3.0.1

class MyString < String; end;

MyString.new(" hello").lstrip!.class  # => MyString
MyString.new("hello ").rstrip!.class  # => MyString
MyString.new(" hello ").strip!.class  # => MyString

MyString.new("123").slice(1, 2).class # => String

I assume this is expected, except for .slice!?

Mixed classes in return array for partition and rpartition

If the argument for partition and rpartition is an instance of sublass, the classes in the returned array are mixed.

# ruby-3.0.1

class MyString < String; end;

MyString.new("hello").partition(MyString.new("ll")).map(&:class)  # => [String, MyString, String]
MyString.new("hello").rpartition(MyString.new("ll")).map(&:class) # => [String, MyString, String]

# vice-versa in ruby-2.7.3

MyString.new("hello").partition("ll").map(&:class)                # => [MyString, String, MyString]

I assume this is expected as well, since the method signature says:

partition(sep) → [head, sep, tail]

where sep is the original input instance.

scrub in Ruby <= 2.7

I noticed that before Ruby 3, scrub only returned an instance of subclass if the input value does not contain an invalid byte sequence:

# ruby-2.7.3

class MyString < String; end;

MyString.new("foo").scrub.class          # => MyString

input = [0x81].pack('C').force_encoding('utf-8')
MyString.new(input).scrub.class          # => String
MyString.new(input).scrub("foo").class   # => String

That's why I did not add tests for the method calls that already return String in Ruby <= 2.7. Let me know if I should add them anyways.

@eregon
Copy link
Member

eregon commented Oct 4, 2021

Destructive methods typically return self or nil if no change, so of course the subclass is kept.
If there destructive methods which return something else than self but still a subclass that could be a bug.

Re partition yeah it makes sense to me at least.

Re scrub yeah sounds good as you did.

core/string/shared/partition.rb Outdated Show resolved Hide resolved
core/string/shared/strip.rb Outdated Show resolved Hide resolved
core/string/scrub_spec.rb Outdated Show resolved Hide resolved
core/string/reverse_spec.rb Outdated Show resolved Hide resolved
@eregon
Copy link
Member

eregon commented Oct 4, 2021

Looks great, could you adapt the guards to all use 3.0 and not 3.0.0 so they are "matched" with each other?
The meaning is the same but by convention the third digit is only added if necessary.

@eregon eregon merged commit 9631c5a into ruby:master Oct 4, 2021
@eregon
Copy link
Member

eregon commented Oct 4, 2021

Thank you for the PR!

@lxxxvi lxxxvi deleted the returns-string-instances-when-called-on-subclasses branch October 4, 2021 13:23
@lxxxvi
Copy link
Contributor Author

lxxxvi commented Oct 4, 2021

@eregon Thank you a lot, I will keep your feedback about the version guards in mind for future PR! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants