Feature #26561

Enable frozen string literals

Added by Pavel Rosický over 2 years ago. Updated 9 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Performance
Target version:4.1.0
Resolution:

Description

Hi, Ruby 2.3 introduced support for frozen string literals

Pros
  • less memory consumption
  • performance (less GC preasure)
  • safer code
Cons
  • mutable functions like force_encoding or gsub! can't be called on frozen strings anymore, this may cause incompatibility with some plugins, but it's very easy to fix and still maintain the backwards compatibility
  • a magic comment has to be added at the top of each rb file :-(

More information
https://www.pluralsight.com/blog/software-development/ruby-2-3--working-with-immutable-strings-

Many popular gems and libraries already did this - for example:
https://github.com/rails/rails/pull/29655
https://github.com/mikel/mail/pull/970

I attached a patch based on master that passes all tests.
There're some garbage changes like removed whitespaces, I'll clean it up if it's approved.

What do you think?

frozen_string_literals.patch Magnifier (541 KB) Pavel Rosický, 2017-07-27 22:12

frozen.patch Magnifier (86.8 KB) Pavel Rosický, 2019-03-06 20:19


Related issues

Related to Redmine - Patch #31004: Decode hexadecimal-encoded literals in order to be frozen... Closed
Related to Redmine - Defect #31052: Download unified diff throws "can't modify frozen String"... Closed
Related to Redmine - Patch #31059: Use #b shortcut instead of #force_encoding Closed
Related to Redmine - Defect #32071: FrozenError in assert_save in test/test_helper.rb Closed
Related to Redmine - Defect #31508: Add missing frozen strings and copyrights Closed

Associated revisions

Revision 17947
Added by Go MAEDA 9 months ago

Add "frozen_string_literal: false" for all files (#26561).

This will be changed to true in the future.

Revision 17974
Added by Go MAEDA 9 months ago

Fix that methods in Redmine::CodesetUtil destroy the original string (#26561).

Revision 17975
Added by Go MAEDA 9 months ago

Fix RDMwriteFormattedCell destroying an argument (#26561).

Patch by Pavel Rosický.

Revision 17977
Added by Go MAEDA 9 months ago

Enable frozen_string_literal for some files under app and lib directory (#26561).

Revision 17978
Added by Go MAEDA 9 months ago

Enable frozen_string_literal for some files under test directory (#26561).

Revision 17979
Added by Go MAEDA 9 months ago

Enable frozen_string_literal for app/views/*/*.builder (#26561).

Revision 17980
Added by Go MAEDA 9 months ago

Support frozen_string_literal in issue, journal, and query (#26561).

Patch by Pavel Rosický.

Revision 17981
Added by Go MAEDA 9 months ago

Enable frozen_string_literal for files which don't contain string literals (#26561).

Revision 17982
Added by Go MAEDA 9 months ago

Enable frozen_string_literal for all files under config directory (#26561).

Revision 17983
Added by Go MAEDA 9 months ago

Support frozen_string_literal in config/initializers/10-patches.rb (#26561).

Patch by Pavel Rosický.

Revision 17985
Added by Go MAEDA 9 months ago

Support frozen_string_literal in lib/redmine/export/*.rb (#26561).

Contributed by Pavel Rosický.

Revision 17987
Added by Go MAEDA 9 months ago

Support frozen_string_literal in app/**/*.rb (#26561).

Contributed by Pavel Rosický.

Revision 17988
Added by Go MAEDA 9 months ago

Support frozen_string_literal in lib/**/*.rb (#26561).

Contributed by Pavel Rosický.

Revision 17990
Added by Go MAEDA 9 months ago

Fix FrozenError in RepositoriesController#diff (#31052, #26561).

Patch by Mizuki ISHIKAWA.

Revision 17994
Added by Go MAEDA 9 months ago

Fix FrozenError in BazaarAdapter.branch_conf_path (#26561).

Patch by Go MAEDA.

Revision 17995
Added by Go MAEDA 9 months ago

Remove extra #dup (#26561).

Revision 17996
Added by Go MAEDA 9 months ago

Support frozen_string_literal in test/**/*.rb (#26561).

Contributed by Pavel Rosický.

Revision 17997
Added by Go MAEDA 9 months ago

Remove frozen_string_literal magic comment from files which are not used when running the application or tests (#23630).

Revision 18474
Added by Go MAEDA 3 months ago

Fix FrozenError during tests (#32071, #26561).

Patch by Go MAEDA.

History

#1 Updated by Toshi MARUYAMA over 2 years ago

  • Description updated (diff)

#2 Updated by Toshi MARUYAMA over 2 years ago

  • Target version set to 4.1.0

#3 Updated by Marius BALTEANU 10 months ago

I've added your patch to my GitLab repository in order to run the tests and to see what is missing.

Do you see any value in adding the # frozen_string_literal: true to db migration files?

#4 Updated by Pavel Rosický 10 months ago

Hi Marius,
the patch is more than a year old, so there'll be some conflicts and missing files.

Do you see any value in adding the # frozen_string_literal: true to db migration files?
There was a discussion about making it default for Ruby 3, so it was historically enforced everywhere by formatters like rubocop https://github.com/rubocop-hq/rubocop . But in terms of performance or memory usage there probably no point for adding the comment to migration files.

one more thing, if Ruby 2.2 support is dropped #30356
instead of "".dup we can use +"" . That wasn't an option a year ago when even ruby 1.x was still supported.

should I update the patch and retest it on the recent redmine version?

#5 Updated by Marius BALTEANU 10 months ago

Pavel Rosický wrote:

Hi Marius,

one more thing, if Ruby 2.2 support is dropped #30356
instead of "".dup we can use +"" . That wasn't an option a year ago when even ruby 1.x was still supported.

should I update the patch and retest it on the recent redmine version?

Yes, if you can do it, it will be very useful and I think we should have in mind that the support for Ruby 2.2 will be dropped in 4.1.

I'm in favour of this change, but this doesn't mean necessary that the patch will be committed. Most probably, it will be Jean-Philippe decision, but considering that almost all gems use frozen_string_literal, I don't see a real reason for not including it.

#6 Updated by Pavel Rosický 10 months ago

ok, rebased and pushed to the github
https://github.com/redmine/redmine/pull/113

there're many changes but most of them is just adding a magic comment

#7 Updated by Pavel Rosický 9 months ago

since ruby 2.2 support was dropped this should be ready for 4.1

let me know if there's anything else I can do

#8 Updated by Go MAEDA 9 months ago

Since the patch is too big to review for me, I would like to take a step-by-step approach. It means that I will enable frozen_string_literal in each file one after another.

Today, I confirmed that we can enable frozen_string_literal for test/unit/activity_test.rb by just adding a magic comment and for test/unit/attachment_test.rb by applying Pavel Rosický's patch.

#9 Updated by Pavel Rosický 9 months ago

https://github.com/redmine/redmine/pull/115
this is a pull request without frozen_string_literal comment - 69 vs 936 changes, it should be much easier to review

you can apply the comment afterwards using rubocop like this
> rubocop --only Style/FrozenStringLiteralComment --auto-correct -c ruboconfig.yml

ruboconfig.yml

AllCops:
  TargetRubyVersion: 2.3

#10 Updated by Go MAEDA 9 months ago

Pavel Rosický wrote:

https://github.com/redmine/redmine/pull/115
this is a pull request without frozen_string_literal comment - 69 vs 936 changes, it should be much easier to review

Thank you for updating the patch. I would like to enable frozen_string_literal in files under the ./test directory first for the following reasons:

  • Changes don't affect production environments
  • We can ensure that all code is executed with frozen_string_literals enabled by simply running bin/rails test

After that, I want to apply the rest of your patch.

Comments welcome.

#11 Updated by Pavel Rosický 9 months ago

Go MAEDA wrote:

Thank you for updating the patch. I would like to enable frozen_string_literal in files under the ./test directory first

I can do that, but there're some problematic methods
User#find_by_login
https://github.com/redmine/redmine/blob/33522bf2e88026d0ca4f3d304d016dddf5966044/app/models/user.rb#L489
Redmine::FieldFormat#parse_keyword
https://github.com/redmine/redmine/blob/4770fa5b6a12f78a5c055efc8a7bf47555496426/lib/redmine/field_format.rb#L200
and the whole codeset_utils

they're are used heavily in tests

consider this example

login = 'test'.force_encoding('ascii-8bit')
puts "before: #{login.encoding}" # => ascii-8bit
user = User.find_by_login(login)
puts "after: #{login.encoding}" # => utf-8

or in my case

login = 'test'.freeze
User.find_by_login(login)
=> FrozenError: can't modify frozen String

What is the prefered way how to deal with these issues? I would rather fix those methods to avoid side-effects, what do you think?

#12 Updated by Go MAEDA 9 months ago

Pavel Rosický wrote:

What is the prefered way how to deal with these issues? I would rather fix those methods to avoid side-effects, what do you think?

You don't have to work on splitting the patch. I will do that :)

#13 Updated by Pavel Rosický 9 months ago

great, thanks :)

#14 Updated by Go MAEDA 9 months ago

  • Related to Patch #31004: Decode hexadecimal-encoded literals in order to be frozen string literals friendly added

#15 Updated by Go MAEDA 9 months ago

  • Related to Defect #31052: Download unified diff throws "can't modify frozen String" error. added

#16 Updated by Go MAEDA 9 months ago

  • Related to Patch #31059: Use #b shortcut instead of #force_encoding added

#17 Updated by Go MAEDA 9 months ago

Thanks to Pavel Rosický, I have enabled frozen_string_literal for almost all files used when running the application or tests.

But I have not enabled frozen_string_literal for some files yet for the following reasons:

  • lib/redmine/imap.rb and lib/redmine/pop3.rb are not covered by existing tests
  • I don't have environments to test Redmine.pm or OpenId

Here is a list of files for which frozen_string_literal is still "false":

  • extra/svn/reposman.rb
  • extra/mail_handler/rdm-mailhandler.rb
  • lib/redmine/imap.rb
  • lib/redmine/pop3.rb
  • lib/plugins/open_id_authentication/init.rb
  • lib/plugins/open_id_authentication/lib/open_id_authentication/mem_cache_store.rb
  • lib/plugins/open_id_authentication/lib/open_id_authentication/request.rb
  • lib/plugins/open_id_authentication/lib/open_id_authentication/db_store.rb
  • test/extra/redmine_pm/repository_git_test_pm.rb
  • test/extra/redmine_pm/test_case.rb
  • test/extra/redmine_pm/repository_subversion_test_pm.rb

However, I think I can close this issue because frozen_string_literal is enabled for 98% of files and features implemented by the above files are not so popular.

I hope someone tests the above files and enables frozen_string_literal for those in the future.

#18 Updated by Go MAEDA 9 months ago

  • Status changed from New to Resolved

#19 Updated by Go MAEDA 9 months ago

  • Subject changed from Frozen string literals to Enable frozen string literals
  • Assignee set to Go MAEDA

#20 Updated by Go MAEDA 9 months ago

  • Status changed from Resolved to Closed

#21 Updated by Go MAEDA 3 months ago

  • Related to Defect #32071: FrozenError in assert_save in test/test_helper.rb added

#22 Updated by Marius BALTEANU about 1 month ago

  • Related to Defect #31508: Add missing frozen strings and copyrights added

Also available in: Atom PDF