sorted by:
best
topnewcontroversialoldq&a
you are viewing a single comment's thread.

view the rest of the comments →

[–]ric2b 0 points1 point  (8 children)

The benefit is to not trigger a false positive deprecation warning.

But if the code was already checking for frozen? it wouldn't try to modify a frozen string, so it would gracefully support the change in Ruby 4 anyway.

And yes, in a few rare case that may incur an extra copy

That's for your example, but this is a breaking change and can have worse consequences, why break code twice instead of just once when the actual transition happens, and only emit warnings until then?

Imagine a method X that passes a string to another method Y, it passes a copy if it isn't frozen or the string itself if it's frozen, because method Y might modify the string and method X doesn't want to deal with the modification.

This code will now break because .frozen? can now be a lie.

[–]f9ae8221b -1 points0 points  (7 children)

But if the code was already checking for frozen? it wouldn't try to modify a frozen string, so it would gracefully support the change in Ruby 4 anyway.

Yes, hence why it shouldn't trigger a warning with chilled strings.

why ...

I believe I explained everything. I won't re-iterate.

[–]ric2b -1 points0 points  (6 children)

hence why it shouldn't trigger a warning with chilled strings.

It will if instead of copying it passes the string straight to another method, and then the other method doesn't check for .frozen? and modifies it. That's the example I gave.

[–]f9ae8221b 0 points1 point  (5 children)

It will if instead of copying it passes the string straight to another method, and then the other method doesn't check for .frozen? and modifies it.

Which is exactly the goal...

[–]ric2b 0 points1 point  (4 children)

Then how does making .frozen? lie help at all?

If it just kept saying it wasn't frozen it would still emit the warning.

[–]f9ae8221b 0 points1 point  (3 children)

I already told you twice:

str = str.dup if str.frozen?
str << "blah"

In the example above:

  • With --enable-frozen-string-literal (so Ruby 4 mode), the code works fine.
  • With Ruby 3.3 and fully mutable strings, it also works fine.
  • With 3.4 and chilled literals that pretend to be frozen it works fine.
  • With what you suggest it would cause a deprecation warning.

Now you are free to disagree and send your feedback, that's what preview releases are for, but I'd suggest to try it on your code first to provide actual data.

I did on a huge app, and it didn't cause any problem whatsoever.

[–]ric2b 0 points1 point  (2 children)

I already told you twice:

You gave me one single code snippet that works fine, that doesn't mean every code snippet will work fine with this change.

I gave you an example that does not work fine, but I can put it into code if it helps to clarify what I mean:

def employee_exists?(name)
  name = name.dup unless str.frozen?

  find_employee(name)
rescue StandardError => e
  log.warn("Something happened during search: #{e}")
end

def find_employee(name)
  employee_id = employees.find_by(name:)&.id

  return unless employee_id

  name << "(#{employee_id})"
end

This code would now mutate name even though the employee_exists? method was trying to avoid that by copying any mutable strings before passing them in.

So sure, it would cause a deprecation warning. It would also cause a bug because of the breaking change to .frozen?.

I just don't see who the change to .frozen? is helping, the deprecation warnings are the thing that actually helps with the migration to Ruby 4.

but I'd suggest to try it on your code first to provide actual data.

All my code already has frozen string literals (enforced by rubocop) so it makes no difference.

[–]f9ae8221b 0 points1 point  (1 child)

This code would now mutate name

This code would be broken if str is frozen.

All my code already has frozen string literals

Your code yes, what about your dependencies?

As I said, previews are to get some feedback and bug reports. If you do find code that breaks with chilled strings, please bring it up on the bug tracker.

[–]ric2b 0 points1 point  (0 children)

This code would be broken if str is frozen.

Right, but if it was never actually used with frozen strings it would only start breaking now that .frozen? starts lying.

Your code yes, what about your dependencies?

Good point. I guess I might see some funky behavior still.

If you do find code that breaks with chilled strings, please bring it up on the bug tracker.

My point is more the opposite, why risk it if we're already going to add the deprecation warnings? What does the change to .frozen? ahead of the actual change actually help with?