Add frozen_string_literal magic comments #858

technicalpickles · 2023-08-22T01:07:09Z

I spotted this while profiling my app with memory_profiler. InheritedResouces::UrlHelpers#define_params_helper showed up, where it allocated some 12320 blank strings ('').

It isn't much relatively by itself, but it adds up.

codecov · 2023-08-22T01:23:55Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (63c53a7) 98.19% compared to head (708ed61) 98.19%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #858   +/-   ##
=======================================
  Coverage   98.19%   98.19%           
=======================================
  Files          14       14           
  Lines         555      555           
=======================================
  Hits          545      545           
  Misses         10       10

Files	Coverage Δ
lib/inherited_resources.rb	`95.83% <ø> (ø)`
lib/inherited_resources/actions.rb	`100.00% <ø> (ø)`
lib/inherited_resources/base_helpers.rb	`98.16% <ø> (ø)`
lib/inherited_resources/belongs_to_helpers.rb	`92.30% <ø> (ø)`
lib/inherited_resources/blank_slate.rb	`100.00% <ø> (ø)`
lib/inherited_resources/class_methods.rb	`98.15% <ø> (ø)`
lib/inherited_resources/dsl.rb	`100.00% <ø> (ø)`
lib/inherited_resources/engine.rb	`83.33% <ø> (ø)`
lib/inherited_resources/polymorphic_helpers.rb	`96.87% <ø> (ø)`
lib/inherited_resources/responder.rb	`100.00% <ø> (ø)`
... and 3 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/inherited_resources/url_helpers.rb

javierjulio · 2023-08-23T14:41:40Z

lib/inherited_resources/version.rb

 module InheritedResources
-  VERSION = '1.13.1'.freeze
+  VERSION = '1.13.1'


This should remain as is because frozen_string_literal is opt-in even with Ruby 3.0 where it was no longer decided to make it the default.

Suggested change

VERSION = '1.13.1'

VERSION = '1.13.1'.freeze

Hi, can I have a clarification?

Does it mean that even if the # frozen_string_literal: true magic comment is present in this file, there are chances that VERSION is not frozen?

Sure. Yes because you have to run Ruby with the option for enabling frozen string literals, otherwise the comment does nothing. It's opt-in. Originally, it was planned to make the option enabled by default in Ruby 3.0 but that was retracted. I don't know if they still intend to enable it by default in future major versions.

So let's say that I have

test-frozen.rb

# frozen_string_literal: true puts 'hello'.frozen?

How I can run this file to get false?

I can see that there as option to opt-in and to allow a file without the magic comment to automatically freeze the strings, but I cannot see a "disable" option to override the magic comment

To clarify a bit, frozen_string_literal has been around for awhile (initial googling suggests 2.3). It was proposed to turn that on by default for 3.0, as in, that is the behavior you get without the magic comment. But that didn't end up happening.

I haven't found a way that you can disable using frozen_string_literal for turning ON frozen strings, just enabling/disabling using frozen string literals globally without needing to use the magic comment.

@javierjulio I still do not get why is that needed. It also triggers a RuboCop offense:

$ cat version.rb # frozen_string_literal: true module InheritedResources VERSION = '1.13.1'.freeze end

$ rubocop version.rb Inspecting 1 file C Offenses: version.rb:4:13: C: [Correctable] Style/RedundantFreeze: Do not freeze immutable objects, as freezing them has no effect. VERSION = '1.13.1'.freeze ^^^^^^^^^^^^^^^ 1 file inspected, 1 offense detected, 1 offense autocorrectable

I'm more interested in landing the rest of this PR than the specifics of this one file 😅 I made the change to use .freeze, but I agree with @tagliala .

I'm happy to do any other changes to get this accepted, but wanted to "Allow edits by maintianers" is checked if that is easier.

Then why not disable it in the file and leave the .freeze call in place? This would satisfy both sides. The point I was trying to make is that the change here assumes its only for those the are enabling the feature with the --enable=frozen-string-literal flag but if you are not, then this would be unfrozen now. Why not just disable the global setting in this single file using the magic comment and leave the .freeze call here? Am I missing something?

https://stackoverflow.com/a/37799399

Sure, that works for me. I pushed up that change.

Hello,

sorry for the silence here

The point I was trying to make is that the change here assumes its only for those the are enabling the feature with the --enable=frozen-string-literal flag but if you are not, then this would be unfrozen now

My interpretation for the above sentence is: "instead of adding a magic comment to each file, it is possible to frozen string literals with --enable=frozen-string-literal command line option".

At that point, the magic comment can work in the opposite way, also mentioned in the linked answer, by specifying # frozen_string_literal: false

So, I would say that if # frozen_string_literal: true is present, all literals present in the file are frozen, without the possibility for the developer to unfreeze them (*)

Moreover, there isn't a command line option --disable=frozen-string-literal, which would invalidate the magic comment, but even in those cases, if a user is deliberately adding that command line option, that should be honored so an unfrozen string would not be a problem

Case 1: no freeze, no magic comment

ELSA = 'Elsa' puts ELSA.frozen?

$ ruby elsa.rb false $ ruby --enable=frozen-string-literal elsa.rb true

Case 2: no freeze, magic comment with true

# frozen_string_literal: true ELSA = 'Elsa' puts ELSA.frozen?

$ ruby elsa_fsl_true.rb true $ ruby --enable=frozen-string-literal elsa_fsl_true.rb true

Case 3: no freeze, magic comment with false

# frozen_string_literal: false ELSA = 'Elsa' puts ELSA.frozen?

$ ruby elsa_fsl_false.rb false $ ruby --enable=frozen-string-literal elsa_fsl_false.rb false

Case 4: freeze, no magic comment

ELSA = 'Elsa'.freeze puts ELSA.frozen?

$ ruby elsa_freeze.rb true $ ruby --enable=frozen-string-literal elsa_freeze.rb true

Case 5: freeze, magic comment true

Skipping example because it's trivial (all true)

Case 6: freeze, magic comment false

# frozen_string_literal: false ELSA = 'Elsa'.freeze puts ELSA.frozen?

$ ruby elsa_freeze_fsl_false.rb true $ ruby --enable=frozen-string-literal elsa_freeze_fsl_false.rb true

Since we are in Case 2 (no freeze, magic comment true) it should be guaranteed that the string is frozen.

This should be the reason why RuboCop triggers an auto-correctable safe offense when developers try to add freeze on Case 2 scenarios

$ cat elsa_fsl_true_freeze.rb # frozen_string_literal: true ELSA = 'Elsa'.freeze puts ELSA.frozen?

$ rubocop elsa_fsl_true_freeze.rb Inspecting 1 file C Offenses: elsa_fsl_true_freeze.rb:3:8: C: [Correctable] Style/RedundantFreeze: Do not freeze immutable objects, as freezing them has no effect. ELSA = 'Elsa'.freeze ^^^^^^^^^^^^^ 1 file inspected, 1 offense detected, 1 offense autocorrectable

$ rubocop -a elsa_fsl_true_freeze.rb Inspecting 1 file C Offenses: elsa_fsl_true_freeze.rb:3:8: C: [Corrected] Style/RedundantFreeze: Do not freeze immutable objects, as freezing them has no effect. ELSA = 'Elsa'.freeze ^^^^^^^^^^^^^ 1 file inspected, 1 offense detected, 1 offense corrected

$ cat elsa_fsl_true_freeze.rb # frozen_string_literal: true ELSA = 'Elsa' puts ELSA.frozen?

(*) The only possibility for the developer to unfreeze would be to edit the file and remove the magic comment. If we fear such a possibility, then we should also freeze all other literals in all other files

I spotted this while profiling my app with memory_profiler. InheritedResouces::UrlHelpers#define_params_helper showed up, where it allocated some 12320 blank strings (`''`). It isn't much relatively by itself, but it adds up. Update lib/inherited_resources/url_helpers.rb Co-authored-by: Javier Julio <[email protected]>

javierjulio

Thanks!

javierjulio reviewed Aug 23, 2023

View reviewed changes

javierjulio force-pushed the frozen-string-literal branch from caf9e47 to 01f48d6 Compare October 6, 2023 22:29

javierjulio changed the title ~~Use frozen_string_literal~~ Add frozen_string_literal magic comments Oct 13, 2023

javierjulio force-pushed the frozen-string-literal branch from 78d3ec3 to 708ed61 Compare October 13, 2023 22:40

javierjulio approved these changes Oct 13, 2023

View reviewed changes

javierjulio merged commit f5be55b into activeadmin:master Oct 13, 2023

technicalpickles deleted the frozen-string-literal branch October 16, 2023 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add frozen_string_literal magic comments #858

Add frozen_string_literal magic comments #858

Uh oh!

technicalpickles commented Aug 22, 2023

Uh oh!

codecov bot commented Aug 22, 2023 •

edited

Loading

Uh oh!

Uh oh!

javierjulio Aug 23, 2023

Uh oh!

tagliala Aug 23, 2023 •

edited

Loading

Uh oh!

javierjulio Aug 23, 2023

Uh oh!

tagliala Aug 23, 2023

Uh oh!

technicalpickles Aug 23, 2023

Uh oh!

tagliala Oct 7, 2023 •

edited

Loading

Uh oh!

technicalpickles Oct 10, 2023

Uh oh!

javierjulio Oct 10, 2023

Uh oh!

technicalpickles Oct 13, 2023

Uh oh!

tagliala Nov 1, 2023 •

edited

Loading

Uh oh!

javierjulio left a comment

Uh oh!

Uh oh!

Uh oh!

Add frozen_string_literal magic comments #858

Add frozen_string_literal magic comments #858

Uh oh!

Conversation

technicalpickles commented Aug 22, 2023

Uh oh!

codecov bot commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

javierjulio Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

tagliala Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javierjulio Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

tagliala Aug 23, 2023

Choose a reason for hiding this comment

test-frozen.rb

Uh oh!

technicalpickles Aug 23, 2023

Choose a reason for hiding this comment

Uh oh!

tagliala Oct 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technicalpickles Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

javierjulio Oct 10, 2023

Choose a reason for hiding this comment

Uh oh!

technicalpickles Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

tagliala Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Case 1: no freeze, no magic comment

Case 2: no freeze, magic comment with true

Case 3: no freeze, magic comment with false

Case 4: freeze, no magic comment

Case 5: freeze, magic comment true

Case 6: freeze, magic comment false

Uh oh!

javierjulio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Aug 22, 2023 •

edited

Loading

tagliala Aug 23, 2023 •

edited

Loading

tagliala Oct 7, 2023 •

edited

Loading

tagliala Nov 1, 2023 •

edited

Loading

Case 1: no `freeze`, no magic comment

Case 2: no `freeze`, magic comment with `true`

Case 3: no `freeze`, magic comment with `false`

Case 4: `freeze`, no magic comment

Case 5: `freeze`, magic comment `true`

Case 6: `freeze`, magic comment `false`