r? @eileencodes

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

@kirs just to note that #29506 already covers off ActiveRecord, ActiveSupport, ActionMailer and ActionView (plus ActionJob didn't, when I last checked, require any changes). Glad to have ActionPack covered though :)

Guess that means just Railties is left!

@pat this PR fixes half of the failures that are happening on current master with #29506 merged and frozen_string_literal enabled in all files. I guess there were more of them?

Ah, must be things that aren't covered by tests? That's how I was checking things - running the tests with RUBYOPT="--enable-frozen-string-literal"

Fixed a failing test.

@pat those can be reproduced in master with:

cd actionpack
env RUBYOPT="--enable-frozen-string-literal" bundle exec rake test
cd ../activerecord
env RUBYOPT="--enable-frozen-string-literal" bundle exec rake test:postgresql

upd: RUBYOPT="--enable-frozen-string-literal" seems to have different behaviour than # frozen_string_literal: true on top of .rb file

These dup in AR is necessary? I tried env RUBYOPT="--enable-frozen-string-literal" bundle exec rake test:postgresql but only failed the test_to_xml. It looks like an issue for builder gem.

% RUBYOPT="--enable-frozen-string-literal" ARCONN=postgresql be ruby -w -Itest test/cases/relations_test.rb -n test_to_xml               
Using postgresql
Run options: -n test_to_xml --seed 55082

# Running:

E

Finished in 0.255228s, 3.9181 runs/s, 0.0000 assertions/s.

  1) Error:
RelationTest#test_to_xml:
RuntimeError: can't modify frozen String
    /Users/kamipo/src/github.com/rails/rails/vendor/bundle/ruby/2.4.0/gems/builder-3.2.3/lib/builder/xmlmarkup.rb:287:in `_special'
    /Users/kamipo/src/github.com/rails/rails/vendor/bundle/ruby/2.4.0/gems/builder-3.2.3/lib/builder/xmlmarkup.rb:254:in `instruct!'
    /Users/kamipo/src/github.com/rails/rails/activesupport/lib/active_support/core_ext/array/conversions.rb:196:in `to_xml'
    /Users/kamipo/src/github.com/rails/rails/activerecord/lib/active_record/relation/delegation.rb:39:in `to_xml'
    test/cases/relations_test.rb:83:in `block in test_to_xml'
    /Users/kamipo/src/github.com/rails/rails/activesupport/lib/active_support/testing/assertions.rb:30:in `assert_nothing_raised'
    test/cases/relations_test.rb:83:in `test_to_xml'

1 runs, 0 assertions, 0 failures, 1 errors, 0 skips
RUBYOPT="--enable-frozen-string-literal" ARCONN=postgresql bundle exec ruby -  2.30s user 0.98s system 34% cpu 9.580 total

@kamipo I think that happens because RUBYOPT="--enable-frozen-string-literal" and # frozen_string_literal: true in the top of every ActiveRecord file have different effect. The first one enforces frozen strings across all files and gems and the latter enforces it only in ActiveRecord.

Our goal is to enforce it in Rails codebase (#29540) so I think we should rely on # frozen_string_literal: true and not RUBYOPT to find all the spots that we want to fix. To reproduce the failures, I used the change set from #29540.

@kamipo you were right, that one wasn't necessary. Thanks for 👀 it! I updated the code.

Could you add the magic comments to the changed files?

Thanks for feedback! I addressed those.

If you think it's a good idea, I can also add these lines to .rubocop.yml:

Style/FrozenStringLiteralComment:
  Enabled: true
  EnforcedStyle: always
  Include:
    - 'actionpack/**/*'
    - 'activerecord/**/*'

However that would made the PR diff quite big to 👀 . My initial plan was to do it in a separate PR.

Yeah, separate PR.

@matthewd please let me know if there's anything else I can improve in this PR.