Skip to content

Conversation

@rdubya
Copy link

@rdubya rdubya commented Sep 10, 2019

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

@rdubya
Copy link
Author

rdubya commented Sep 10, 2019

@headius
Copy link
Member

headius commented Sep 11, 2019

Passed all the tests we run 😮

@rdubya
Copy link
Author

rdubya commented Sep 12, 2019

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
Copy link
Member

headius commented Sep 25, 2019

@rdubya I'm game to add the flag. I'm starting to think we're not going to get ANY help from ruby-core on this though. They seem to be afraid of breaking one line out of a million. @enebo thoughts?

@enebo
Copy link
Member

enebo commented Sep 26, 2019

@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.

@rdubya
Copy link
Author

rdubya commented Sep 26, 2019

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 false.to_s.gsub!(...)

@ahorek
Copy link
Contributor

ahorek commented Sep 26, 2019

fyi, it was approved as an experimental 2.7 feature
ruby/ruby#2437
see also
ruby/ruby#2487
ruby/ruby#2490

@eregon
Copy link
Member

eregon commented Sep 26, 2019

@headius

@rdubya I'm game to add the flag. I'm starting to think we're not going to get ANY help from ruby-core on this though. They seem to be afraid of breaking one line out of a million. @enebo thoughts?

What do you mean? matz agreed to experiment with it: https://bugs.ruby-lang.org/issues/16150#note-18
(and I spent some time to push for this on your own feature request)

@enebo
Copy link
Member

enebo commented Sep 26, 2019

@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.

@headius
Copy link
Member

headius commented Sep 26, 2019

@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 to_s strings for the "obvious" cases (Symbol, NilClass, True/False) is only being added as "experimental".

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).

@eregon
Copy link
Member

eregon commented Sep 26, 2019

@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.

@headius
Copy link
Member

headius commented Sep 26, 2019

@eregon Experimental for us means if it gets into a release it will be guarded by an option, which is what @rdubya and I were discussing above. We do releases more frequently than CRuby, and would like to get this feature out there for people to test against ASAP (i.e. in JRuby 9.2.9).

@headius
Copy link
Member

headius commented Sep 26, 2019

@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 to_s result of several "core" types?

@headius
Copy link
Member

headius commented Sep 26, 2019

@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.

@nomadium
Copy link
Contributor

nomadium commented Feb 8, 2020

@headius I continued this PR at: #6052

@headius
Copy link
Member

headius commented Feb 10, 2020

Closing in favor of #6052 (and because it seems like this feature will not be accepted in CRuby).

@headius headius closed this Feb 10, 2020
@enebo enebo added this to the Invalid or Duplicate milestone Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants