Add `Style/RedundantFreeze` to remove redudant `.freeze` #32031

yahonda · 2018-02-17T09:21:22Z

Summary

Since Rails 6.0 will support Ruby 2.4.1 or higher # frozen_string_literal: true magic comment enabled by Style/FrozenStringLiteralComment cop is enough to make string object frozen.

Other Information

I'd like to hear from Rails committers for this change since Rails 5.2 will supports Ruby 2.2, this change itself should not be backported but also it could make other backports complex.

rails-bot · 2018-02-17T09:21:23Z

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

matthewd · 2018-02-17T10:23:04Z

I wonder how many of these are extremely performance-critical. For most, it seems fine to backport to 5.2, just to make future maintenance easier: things being slightly slower on 5.2 + 2.2 doesn't sound like a big deal... I'm just not sure how we'd identify any where it makes a catastrophic difference.

Otherwise, we can wait a while and then merge this in a few months, when 5.2 backports have settled down.

I think we might want to keep the explicit freeze on the tests that specifically want a frozen string, for clarity. (Maybe?)

yahonda · 2018-02-17T23:08:04Z

Thanks for the review. I did not expect that backporting this change to 5-2-stable is possible.

I think we might want to keep the explicit freeze on the tests that specifically want a frozen string, for clarity. (Maybe?)

Agreed. I have excluded 3 test files which have tests expecting a frozen string in case of backporting and respecting the intention of the tests.

jeremy · 2018-02-17T23:12:57Z

Backporting to 5.2 is a nice idea. We could say Rails 5.2 supports Ruby 2.2, with this caveat, but prefers 2.3 or newer.

On the other hand, it's strange for our API to return differently mutable/frozen strings depending on the version of Ruby you're running.

matthewd · 2018-02-18T07:12:12Z

On the other hand, it's strange for our API to return differently mutable/frozen strings depending on the version of Ruby you're running.

We're already doing that on any other literals where we don't explicitly call .freeze, so it doesn't seem terrible... I guess there is a distinction between the performance-based freezings, and the ones where we actually care about making the value immutable (because it's being stored and reused in a constant, say).

I do think it's reasonable for us to strongly recommend a newer ruby than the oldest we support, and to bump that recommendation more freely: we want 5.2 to work on almost-already-EOL 2.2, but I don't see myself feeling bad it if doesn't work well.

yahonda · 2018-02-18T13:02:20Z

Excluded two more files changed by this commit. 8454aee

yahonda · 2018-02-23T13:04:19Z

Addressed conflicts. Since this pull request has been conflicting other commits
By separating commits by rubocop -a it is easy to rebase.

Will squash two commits when it is ready to merge.

yahonda · 2018-02-23T22:31:33Z

It has been approved then I have squashed two commits.

yahonda · 2018-09-24T01:01:05Z

As #32971 has been merged to master I wanted this pull request reviewed again.

kamipo · 2018-09-24T15:32:34Z

.rubocop.yml

@@ -130,6 +130,15 @@ Style/FrozenStringLiteralComment:
    - 'actionpack/test/**/*.ruby'
    - 'activestorage/db/migrate/**/*.rb'

+Style/RedundantFreeze:
+  Enabled: true
+  Exclude:


Why exclude these test files?

https://github.com/rails/rails/blob/master/actionpack/test/controller/test_case_test.rb
https://github.com/rails/rails/blob/master/activemodel/test/cases/type/string_test.rb
https://github.com/rails/rails/blob/master/activesupport/test/core_ext/string_ext_test.rb
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/core_ext/string/strip.rb
https://github.com/rails/rails/blob/master/railties/test/generators/actions_test.rb

In my 5 second test, Rubocop didn't seem to complain about redundant -"foo". If that's the case (and deliberate?), maybe we can consider switching to that for the first two. string_ext_test.rb seems more like it's missing a test for the unfrozen case (though I'd really prefer to see all those wrapped in an assert_deprecated anyway).

Yeah, I also confirmed about redundant -"foo" is no offence for the cop when I commented.
Since we already replaced all String#dup to String#+@, replacing redundant String#freeze to String#-@ is fine to me.

Thanks for the review. I have removed these files from the exclude list and replaced String#freeze with String#-@ where explicit string objects are necessary.

kamipo · 2018-09-24T15:41:58Z

activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb

@@ -539,7 +539,7 @@ def translate_exception(exception, message)
          end
        end

-        COLLATE_REGEX = /.*\"(\w+)\".*collate\s+\"(\w+)\".*/i.freeze
+        COLLATE_REGEX = /.*\"(\w+)\".*collate\s+\"(\w+)\".*/i


/.*\"(\w+)\".*collate\s+\"(\w+)\".*/i is not a string so this .freeze is not redundant.

Thanks for the review. Let me confirm this review first. I think you mean regular expressions is not a string. It looks like Ruby itself does not support frozen regular expressions yet.
https://bugs.ruby-lang.org/issues/8948 I'm wondering if this .freeze is actually valid or not

Looks like that https://bugs.ruby-lang.org/issues/8948 is a proposal for 3.times { /foo/f } returning the same regex object like a frozen string literal.
At least removing .freeze has an effect that changing the result of .frozen?.

irb(main):001:0> RUBY_VERSION => "2.5.1" irb(main):002:0> /foo/.frozen? => false irb(main):003:0> /foo/.freeze.frozen? => true irb(main):004:0> /foo/.instance_variable_set(:@foo, "foo") => "foo" irb(main):005:0> /foo/.freeze.instance_variable_set(:@foo, "foo") Traceback (most recent call last): 3: from /Users/kamipo/.rbenv/versions/2.5.1/bin/irb:11:in `<main>' 2: from (irb):7 1: from (irb):7:in `instance_variable_set' FrozenError (can't modify frozen Regexp)

While it does sound like Rubocop's being over-eager in declaring it redundant, I think this instance is fine in practice. It's not a value we return anywhere, so someone would have to be trying pretty hard to even find it... and at that point a const_set is just as easy.

Yeah, this instance is fine in practice, and this is an only place where RuboCop's auto-correct affects in I reviewed all diff in this PR.
I suppose it is an issue for the cop, but I'm not sure whether RuboCop intentionally regard a regex literal like a string literal or not.

I have opened a RuboCop issue rubocop/rubocop#6331

Thank you for opening the RuboCop issue. I think this is a false positive of RuboCop. I opened a PR rubocop/rubocop#6333 to fix, but it may take some time to the next new release of RuboCop (maybe 0.59.3 or 0.60.0) and subsequent Code Climate support. Until then it will depend on hand craft.

kamipo · 2018-09-24T15:55:45Z

.rubocop.yml

+    - 'actionpack/test/controller/test_case_test.rb'
+    - 'activemodel/test/cases/type/string_test.rb'
+    - 'activesupport/test/core_ext/string_ext_test.rb'
+    - 'activesupport/lib/active_support/core_ext/string/strip.rb'


This is not redundant freeze.

rails/activesupport/lib/active_support/core_ext/string/strip.rb

Line 24 in d03cc17

stripped.freeze if frozen?

This "".freeze is obviously redundant, there is no reason to exclude it.

rails/activesupport/lib/active_support/core_ext/string/strip.rb

Line 23 in d03cc17

gsub(/^#{scan(/^[ \t]*(?=\S)/).min}/, "".freeze).tap do |stripped|

yahonda · 2018-09-29T06:13:37Z

.rubocop.yml

@@ -130,6 +130,15 @@ Style/FrozenStringLiteralComment:
    - 'actionpack/test/**/*.ruby'
    - 'activestorage/db/migrate/**/*.rb'

+Style/RedundantFreeze:
+  Enabled: true
+  Exclude:


Thanks for the review. I have removed these files from the exclude list and replaced String#freeze with String#-@ where explicit string objects are necessary.

.rubocop.yml

kamipo · 2018-09-29T06:49:49Z

actionpack/lib/action_dispatch/journey/router/utils.rb

-          PATH     = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/]/.freeze
+          FRAGMENT = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/\?]/
+          SEGMENT  = /[^#{UNRESERVED}#{SUB_DELIMS}:@]/
+          PATH     = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/]/


I re-searched by "/.freeze" in the diff and found extra affected regexs.

Thank you for finding other false positives. Addressed and exclude 'actionpack/lib/action_dispatch/journey/router/utils.rb' also.

Since Rails 6.0 will support Ruby 2.4.1 or higher `# frozen_string_literal: true` magic comment is enough to make string object frozen. This magic comment is enabled by `Style/FrozenStringLiteralComment` cop. * Exclude these files not to auto correct false positive `Regexp#freeze` - 'actionpack/lib/action_dispatch/journey/router/utils.rb' - 'activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb' It has been fixed by rubocop/rubocop#6333 Once the newer version of RuboCop released and available at Code Climate these exclude entries should be removed. * Replace `String#freeze` with `String#-@` manually if explicit frozen string objects are required - 'actionpack/test/controller/test_case_test.rb' - 'activemodel/test/cases/type/string_test.rb' - 'activesupport/lib/active_support/core_ext/string/strip.rb' - 'activesupport/test/core_ext/string_ext_test.rb' - 'railties/test/generators/actions_test.rb'

Refer rails/rails#32031.

ref. rails/rails#32031

rails-bot assigned kaspth Feb 17, 2018

yahonda force-pushed the remove_redundant_freeze branch from 7eb1da2 to 1606bce Compare February 17, 2018 23:03

yahonda force-pushed the remove_redundant_freeze branch from 1606bce to 4631e3d Compare February 18, 2018 13:01

yahonda force-pushed the remove_redundant_freeze branch 2 times, most recently from ee9f04d to c4c1da0 Compare February 23, 2018 13:04

rafaelfranca approved these changes Feb 23, 2018

View reviewed changes

rafaelfranca assigned matthewd and unassigned kaspth Feb 23, 2018

yahonda force-pushed the remove_redundant_freeze branch from c4c1da0 to e4412ac Compare February 23, 2018 22:26

yahonda force-pushed the remove_redundant_freeze branch 4 times, most recently from a7e4fe4 to a210082 Compare March 2, 2018 10:04

yahonda mentioned this pull request Mar 2, 2018

Add Style/RedundantFreeze to remove redudant .freeze rsim/oracle-enhanced#1662

Merged

yahonda force-pushed the remove_redundant_freeze branch from a210082 to 921c15f Compare March 4, 2018 11:24

pixeltrix mentioned this pull request Mar 16, 2018

Remove all unnecessary freeze on strings #32267

Closed

yahonda force-pushed the remove_redundant_freeze branch from 921c15f to a6ce3e6 Compare March 18, 2018 10:08

yahonda force-pushed the remove_redundant_freeze branch from a6ce3e6 to bc680e1 Compare April 16, 2018 15:34

yahonda force-pushed the remove_redundant_freeze branch from bc680e1 to 1955e67 Compare April 24, 2018 01:01

yahonda force-pushed the remove_redundant_freeze branch 3 times, most recently from 9e2aef9 to 597ebf3 Compare May 5, 2018 06:07

yahonda force-pushed the remove_redundant_freeze branch 3 times, most recently from 8c1cccb to 4fa3ba4 Compare August 20, 2018 07:53

yahonda force-pushed the remove_redundant_freeze branch from 4fa3ba4 to 490c7ec Compare August 23, 2018 12:20

yahonda force-pushed the remove_redundant_freeze branch from 490c7ec to 405e03a Compare September 14, 2018 04:54

yahonda force-pushed the remove_redundant_freeze branch from 405e03a to a61f38d Compare September 23, 2018 06:06

kamipo reviewed Sep 24, 2018

View reviewed changes

yahonda mentioned this pull request Sep 25, 2018

Style/RedundantFreeze find offenses for Regexp#freeze rubocop/rubocop#6331

Closed

koic mentioned this pull request Sep 26, 2018

Fix a false negative for Style/RedundantFreeze rubocop/rubocop#6334

Merged

8 tasks

yahonda force-pushed the remove_redundant_freeze branch from a61f38d to 30a2d5f Compare September 29, 2018 06:08

rails-bot bot added actioncable actionmailer actionpack routing security labels Sep 29, 2018

yahonda commented Sep 29, 2018

View reviewed changes

kamipo reviewed Sep 29, 2018

View reviewed changes

yahonda force-pushed the remove_redundant_freeze branch from 30a2d5f to c0d1721 Compare September 29, 2018 06:57

kamipo approved these changes Sep 29, 2018

View reviewed changes

yahonda force-pushed the remove_redundant_freeze branch from c0d1721 to aa3dcab Compare September 29, 2018 07:20

kamipo merged commit b13a5cb into rails:master Oct 1, 2018

koic added a commit to koic/rubocop-rails_config that referenced this pull request Oct 1, 2018

Enable Style/RedundantFreeze cop

18a4767

Refer rails/rails#32031.

koic mentioned this pull request Oct 1, 2018

Enable Style/RedundantFreeze cop toshimaru/rubocop-rails_config#15

Merged

yahonda deleted the remove_redundant_freeze branch October 27, 2018 00:38

toshimaru added a commit to toshimaru/rubocop-rails_config that referenced this pull request Oct 30, 2018

Same exclude settings with official

aa0d60c

ref. rails/rails#32031

yahonda mentioned this pull request Nov 8, 2018

Use RuboCop 0.60.0 and remove exclude files for Style/RedundantFreeze #34406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Style/RedundantFreeze` to remove redudant `.freeze` #32031

Add `Style/RedundantFreeze` to remove redudant `.freeze` #32031

yahonda commented Feb 17, 2018 •

edited

rails-bot commented Feb 17, 2018

matthewd commented Feb 17, 2018

yahonda commented Feb 17, 2018

jeremy commented Feb 17, 2018

matthewd commented Feb 18, 2018

yahonda commented Feb 18, 2018 •

edited

yahonda commented Feb 23, 2018

yahonda commented Feb 23, 2018

yahonda commented Sep 24, 2018

kamipo Sep 24, 2018

matthewd Sep 25, 2018

kamipo Sep 25, 2018

yahonda Sep 29, 2018 •

edited

kamipo Sep 24, 2018

yahonda Sep 24, 2018

kamipo Sep 25, 2018 •

edited

matthewd Sep 25, 2018

kamipo Sep 25, 2018

yahonda Sep 25, 2018

koic Sep 27, 2018

kamipo Sep 24, 2018

kamipo Sep 24, 2018

yahonda Sep 29, 2018 •

edited

kamipo Sep 29, 2018

yahonda Sep 29, 2018

Add Style/RedundantFreeze to remove redudant .freeze #32031

Add Style/RedundantFreeze to remove redudant .freeze #32031

Conversation

yahonda commented Feb 17, 2018 • edited

Summary

Other Information

rails-bot commented Feb 17, 2018

matthewd commented Feb 17, 2018

yahonda commented Feb 17, 2018

jeremy commented Feb 17, 2018

matthewd commented Feb 18, 2018

yahonda commented Feb 18, 2018 • edited

yahonda commented Feb 23, 2018

yahonda commented Feb 23, 2018

yahonda commented Sep 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yahonda Sep 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamipo Sep 25, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yahonda Sep 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `Style/RedundantFreeze` to remove redudant `.freeze` #32031

Add `Style/RedundantFreeze` to remove redudant `.freeze` #32031

yahonda commented Feb 17, 2018 •

edited

yahonda commented Feb 18, 2018 •

edited

yahonda Sep 29, 2018 •

edited

kamipo Sep 25, 2018 •

edited

yahonda Sep 29, 2018 •

edited