Style/FrozenStringLiteralComment always on

[will]

I think the frozen string literal comment should always be true, especially as ruby 3 comes down the line, and I've been manually going through my projects and adding the comment as I go. But it's hard to remember to always do it, so it'd be nice if standard added it for me.

However I couldn't figure out how to actually get the auto correct to work for this, which is the test failure :/

links:
docs
disable enforcing no comment
invert rules

[timriley]

This is certainly something I've missed since moving to standardrb (so much so that it's a TODO of mine to figure out how to add this auto-correction locally somehow). I'd be in favour of adding this in.

[searls]

Hi Will & Tim. Could either of you point me to the tangible benefits that frozen string brings? I've heard conflicting things both about any performance benefits as well about any meaningful changes actually landing in Ruby 3.

Firm confirmation of either would be helpful in understanding the value of this, because "add two lines of cruft to the top of every file" is a very high bar in my book

[will]

I think the problem with getting real-world benchmarks on this, is that so many other gems don’t work with the setting on, that you can't turn it on globally. Then if more time is spent in external libraries (which feels common?) rather than code you can control with this setting file by file, you're admittedly not going to see a lot of benefit.

However, for me it's more of, I want to know I'm doing what I can with my stuff, and know I'm not accidentally writing code that will fail once we can turn the stetting on globally.

And you can do synthetic benchmarks that show improvement:

require "benchmark/ips"

@existing_hash = {"key" => "value"}

def meth(arg)
  arg
end

Benchmark.ips do |x|
  x.report("using existing hash") do
    @existing_hash["key"]
  end

  x.report("using new hash") do
    new_hash = {"key" => "value"}
    new_hash["key"]
  end

  x.report("calling a method") do
    meth "hello"
  end
end

without the comment

 using existing hash     17.020M (± 3.1%) i/s -     86.396M in   5.081253s
      using new hash      7.437M (± 2.5%) i/s -     37.211M in   5.006543s
    calling a method     14.175M (± 2.8%) i/s -     70.846M in   5.002084s

with it

 using existing hash     18.336M (± 2.2%) i/s -     91.740M in   5.005601s
      using new hash     10.579M (± 2.5%) i/s -     53.396M in   5.050854s
    calling a method     22.679M (± 2.3%) i/s -    113.976M in   5.028322s

[searls]

Thank you for sharing that, @will

Every time this comes up, I forget about where things stand with ruby-core, but I was just reminded Matz closed the door on this ever becoming the default due to compatibility concerns (Rubocop discussion here). As a result, I don't think Frozen is going to happen so I'm 👎 on trying to use Standard to force it.

[will]

Oh I missed that he came down that way on the issue. That's a shame, but understandable.

Thanks for considering this, and also good work on becoming a co-op by the way :)

[searls]

Thanks @will! And thanks for being understanding

[tenderlove]

I don’t have any good benchmarks, but I can tell you from experience that they really helped us at GitHub. The reason it’s hard to get good benchmarks is because they impact thoughput and not just small benchmarks. We had a benchmark where switching to frozen strings massively improved performance, but the reason was because eliminating the runtime allocations prevented a GC from happening. Of course the GC would happen at some point, but we were able to perform more computations before the GC would kick off.

I don’t know how to reproduce this behavior in a micro bench. You have to create an environment where a GC is expensive (it usually isn’t that expensive), and then somehow trigger the GC in a benchmark. It’s not easy.

Anyway, I am very pro frozen strings everywhere mainly because of that experience.

[tenderlove]

btw I used to be in the “I’ve never seen frozen stings help anything” camp until I actually saw them help.

[ioquatix]

OpenSSL (gem) had a line of code which was literally generating thousands of strings (one per read/write operation IIRC). It was a single " " character. I recently applied frozen string literals and it wiped out over 300,000 string allocations in some of my longer benchmarks. Yes, it should be the default.

[natematykiewicz]

I was able to remove about 2,600 string allocations from https://rubyapi.org/2.7/o/string simply by adding the magic comment to the entire project (which was no easy task, since the project uses StandardRb, which provides no access to this cop).
rubyapi/rubyapi#344

[mperham]

A good portion of the "garbage created" reduction from Sidekiq 3.5.1 to 4.0.0 was adding the magic comment to the top of all Sidekiq code. It had a huge effect:

https://github.com/mperham/sidekiq#performance

[nateberkopec]

You have to create an environment where a GC is expensive (it usually isn’t that expensive), and then somehow trigger the GC in a benchmark. It’s not easy.

Create a heap with ~5 million live objects, then each iteration of the benchmark, create ~2.5 million objects? I think that would do it. But I'm not sure what the "control" and what the "experiment" is in that case.

Personally, I just use frozen strings wherever practical. It's usually not a big effort to accomodate them. You can freeze all strings globally: ruby --enable=frozen-string-literal -e "'foo'.concat'bar'". I think you can do that with RUBYOPT as well.

[searls]

Thanks for everyone for chiming in with their anecdotal experiences. A lot of the people in this thread are folks whose opinions on running Ruby at scale I already trust so I'm just about convinced we turn this on (even though I cringe at the 2 line tax at the top of every file—maybe a hangup from package statements at the top of java files or something)

The only practical question I have is what @nateberkopec just mentioned: what's gained by encoding this in file listings versus using the runtime --enable flag? Is it just the case that trying to use that flag in practice is fraught because a lot of gems/code will break, so working file-by-file gives greater control / odds of success?

[natematykiewicz]

I personally have no idea how to use that command flag in a real-world example. When you’re used to running commands like rails s or sidekiq, how do you add that flag? From what I understand, it’s an argument to the ruby command.

But also, as you alluded to, the “magic comment” only applies to the current file. The flag (I assume) applies to all gems you have installed, and you can’t assume that all gems you have installed are okay with frozen string literals (meaning they don’t mutate any strings for files that they didn’t add the comment to themselves). Hopefully Nate can shed more light on that.

[nateberkopec]

The flag (I assume) applies to all gems you have installed

Exactly, it applies to everything in the ObjectSpace, regardless of source, and would probably be incompatible with most apps.

When you’re used to running commands like rails s or sidekiq, how do you add that flag?

RUBYOPT="--enable=frozen-string-literal" ruby -e "'foo'.concat('bar')" -> can't modify frozen String (FrozenError)

[natematykiewicz]

Maybe the question is irrelevant at this point, but if I were to want to run Rails (Puma, you need a specific server) and Sidekiq using this flag, how would you do that? That’s what I was intending to ask.

I doubt RUBYOPT="--enable=frozen-string-literal" rails s works. Does it? I’m now realizing it’s an environment variable, not a command argument.

[nateberkopec]

I doubt RUBYOPT="--enable=frozen-string-literal" rails s works.

It works. RUBYOPT="--enable=frozen-string-literal" bin/rails runner "'foo'.concat('bar')". I just tried it on CodeTriage and mustermann blows up on a frozen String while trying to construct it's AST.

[mgk]

While I'm in the pro frozen camp, the reasons for Standard not enforcing frozen make sense. I wanted Standard + frozen on a project so I added a RuboCop config that enables only the frozen rule and have rake run both RuboCop and Standard. It feels a little like it is going against the premise of Standard, but it is working out pretty well. Having rake fix handle frozen + Standard is handy.

[searls]

While I'm in the pro frozen camp, the reasons for Standard not enforcing frozen make sense. I wanted Standard + frozen on a project so I added a RuboCop config that enables only the frozen rule and have rake run both RuboCop and Standard. It feels a little like it is going against the premise of Standard, but it is working out pretty well. Having rake fix handle frozen + Standard is handy.

Indeed. Ruby is used in enough contexts outside long-running web applications that it's a bit heavy-handed for Standard to go through and either add or remove all of your frozen strings comments. I was thinking it'd be nice to just write a gem that did nothing but add frozen strings to ruby file listings, which could be quite fast and pair nicely with standard as two tasks in the same default rakefile

[natematykiewicz]

@searls https://github.com/Invoca/magic_frozen_string_literal This has worked nicely for me in Ruby API.

The problem I'm attempting to solve is that Ruby API uses Standard. I was able to gain significant performance improvements (allocation reductions) by adding frozen string literals to the whole app. But, right now there's nothing that enforces people use them in the code -- and I would like to enforce that people keep using them (lest other people that are not me subtly slow down the app over time). So how do I enforce this, while also using Standard?

it's a bit heavy-handed for Standard to go through and either add or remove all of your frozen strings comments

I know you don't like settings, but what about a setting? Presumably, frozen string literals are the future (that's what we've been told anyways). So not only are they better for performance, using them helps ease the migration into Ruby 3 (which seems like a thing Standard would like to help with -- standardizing Ruby code between versions 2 and 3).

[searls]

FWIW as I mentioned in an earlier reply, Matz had ruled out frozen string literals in Ruby as the default #181 (comment)

[juanmanuelramallo]

@searls would it be possible for Standard to allow users to enable this cop if they wanted it to?

I can draft a PR if this is a feasible middle ground.

[searls]

Given that the point of standard is that the rules are not configurable and that every project styled by standard should be very nearly the same, I don't think adding an option for this (or nearly any) rule would be appropriate (it'd just open the door to other people wanting options added). If something isn't a good idea in nearly all cases, I'm generally in favor of excluding it from standard, even if it's a very good idea in some cases.

So since not all projects need this feature, but some do, if I were in your shoes I'd use a git hook to enforce this (since it can be detected so easily with a shell script). This (not officially supported) technique might also work if you want to enable the cop via configuration

[zubin]

I was thinking it'd be nice to just write a gem that did nothing but add frozen strings to ruby file listings, which could be quite fast and pair nicely with standard as two tasks in the same default rakefile

Like this? https://github.com/Invoca/magic_frozen_string_literal

[searls]

Haha that's great. We should work a link to that into the README