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

Use String#+@ before mutating the result of Symbol#to_s #37303

Merged
merged 1 commit into from
Sep 26, 2019

Conversation

eregon
Copy link
Contributor

@eregon eregon commented Sep 26, 2019

Summary

Ruby 2.7 will return a frozen String for Symbol#to_s to avoid extra allocations:
ruby/ruby#2437
https://bugs.ruby-lang.org/issues/16150#note-18

That is currently an experimental change, but I think it would make a lot of sense and I would expect only few places need to be adapted to handle a frozen Symbol#to_s.

* String#+@ is available since Ruby 2.3.
* See the upstream Ruby change making Symbol#to_s return a frozen String
  for efficiency: ruby/ruby#2437
@eregon
Copy link
Contributor Author

eregon commented Sep 26, 2019

FWIW, I also tried to use only immutable Strings, but the change is then larger:

diff --git a/activesupport/lib/active_support/ordered_options.rb b/activesupport/lib/active_support/ordered_options.rb
index c4e419f546..6d61f9a38d 100644
--- a/activesupport/lib/active_support/ordered_options.rb
+++ b/activesupport/lib/active_support/ordered_options.rb
@@ -40,13 +40,11 @@ def [](key)
 
     def method_missing(name, *args)
       name_string = name.to_s
-      if name_string.chomp!("=")
-        self[name_string] = args.first
+      if name_string.end_with?('=')
+        self[name_string[0..-2]] = args.first
       else
-        bangs = name_string.chomp!("!")
-
-        if bangs
-          self[name_string].presence || raise(KeyError.new(":#{name_string} is blank"))
+        if name_string.end_with?("!")
+          self[name_string[0..-2]].presence || raise(KeyError.new("#{name.inspect} is blank"))
         else
           self[name_string]
         end

@kamipo
Copy link
Member

kamipo commented Sep 26, 2019

Related ruby/ruby#2487.

@byroot
Copy link
Member

byroot commented Sep 26, 2019

I would expect only few places need to be adapted to handle a frozen Symbol#to_s.

That's the only change I had to do when I tested ruby/ruby#2175 against Redmine.

There's definitely some stuff that will break out there, but it's both relatively rare and extremely easy fixes.

@casperisfine
Copy link
Contributor

👍 to this.

cc @rafaelfranca

@kamipo kamipo merged commit 192df82 into rails:master Sep 26, 2019
@eregon
Copy link
Contributor Author

eregon commented Sep 26, 2019

Thanks for the quick merge!

@byroot
Copy link
Member

byroot commented Sep 26, 2019

Can we also backport it?

Assuming this experiment isn't reverted (🤞) MRI 2.7 will be released in about 3 months, so It would be better if people didn't hit that issue with the supported versions of Rails.

@kamipo
Copy link
Member

kamipo commented Sep 26, 2019

Yeah, I will do that later today (l’m out of home now).

@eregon
Copy link
Contributor Author

eregon commented Sep 26, 2019

Could we backport this change to both Rails 6.0 and 5.2 so people can use Ruby 2.7 on those Rails versions? For 5.2 we can use name.to_s.dup or #37303 (comment)

@rafaelfranca
Copy link
Member

Backported.

rafaelfranca pushed a commit that referenced this pull request Sep 26, 2019
Use String#+@ before mutating the result of Symbol#to_s
rafaelfranca pushed a commit that referenced this pull request Sep 26, 2019
Use String#+@ before mutating the result of Symbol#to_s
@eregon
Copy link
Contributor Author

eregon commented Sep 26, 2019

Thank you!
I added a comment on the 5.2 backport, I am not sure we can use String#+@ there for Ruby 2.2: ed81031#commitcomment-35254852

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

Successfully merging this pull request may close these issues.

None yet

5 participants