-
-
Notifications
You must be signed in to change notification settings - Fork 941
Make Symbol#to_s return frozen strings #5868
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
Conversation
|
There are at least a few places in rails where this breaks already. For example: https://github.com/rails/rails/blob/ae2cbf40449c270feec6fa0aa82b3d5fc2350be7/activesupport/lib/active_support/ordered_options.rb#L43 |
|
Passed all the tests we run 😮 |
|
Should I go ahead with implementing the flag and add some tests around it? It won't be helpful for rails apps yet but anybody that is using it outside of rails may benefit from it. If so, what flag should it be and can you point me at somewhere that you check a flag like this already so I can get an idea of how to do it? Should I expand it to all the "primitives"? |
|
@headius @rdubya yeah I am game to try this as it can be used to demonstrate what problems would exist if MRI decided to make this change. I don't think they will but if there is any chance it will be needed with a ton of empirical evidence (and this flag could help provide that). If we add this and realize it doesn't work with lots of stuff we can remove it too. |
|
Any thoughts on naming or an example I can look at? Also, should I keep it symbols only for now or expand to some of the other primitives? Seems like the other primitives are probably safer than symbols, can't imagine too many people are trying to call |
|
fyi, it was approved as an experimental 2.7 feature |
What do you mean? matz agreed to experiment with it: https://bugs.ruby-lang.org/issues/16150#note-18 |
|
@eregon you are aking @headius but I think the whole twitter thread last week on not having frozen strings by default in Ruby 3 has taken away our optimism. Historically, changing Ruby has been much less likely than adding to it. I do agree though that we should accept Matz moving forward with the experiment as a positive sign. |
|
@eregon Thank you for pushing the issue forward...I have not had time to circle back to it in the past week or so. My frustration above is that we still seem no closer to frozen strings in 3.0 and even freezing My interpretation of "experimental" was that it wouldn't be enabled by default in the 2.7 release...but I guess this is not the case? I'm very confused about calling it an "experimental" feature if everyone will have to adapt to it anyway (I'm glad they'll have to adapt to it, but I question what "experimental" means here). |
|
@headius experimental for MRI just means it might be removed before the Ruby 2.7 if it's too problematic, but otherwise it'll be part of the release. Some experimental features have a warning when they are used, e.g. Pattern matching. |
|
@rdubya Given that this will be part of a larger push to freeze strings for a few core singleton objects, let's go with a property name that reflects that. Perhaps "frozen.core.to_s" since this applies to the |
|
@rdubya Oh I'd also say go ahead and do it for the other items marked as "experimental" in the ruby-lang issue, so that's nil, true, false, and Module#name. |
|
Closing in favor of #6052 (and because it seems like this feature will not be accepted in CRuby). |
This is a PR to explore the possibility of adding support for returning frozen strings. We'll probably want to have a JRuby flag until the spec is hammered out for the ruby language in general.
This relates to https://bugs.ruby-lang.org/issues/16150