Action View: compile ERB templates with `# frozen_string_literal: true` by caspe...

Copy link

Contributor

casperisfine commented 17 days ago

Hum, it causes lots of warnings:

test/fixtures/test/_local_inspector.html.erb:1: warning: `frozen_string_literal' is ignored after any tokens

And breaks a few tests, I'll have to dig into this.

rafaelfranca commented 17 days ago

The idea makes sense. I'd be surprised if any application code was relying in the fact that the strings generated by the ERB compilation are not frozen, so maybe let's try in a few applications we know to see if any of them fall in that problem?

matthewd commented 17 days ago

I thought ERB-generated strings were already frozen (meaning this would only affect regular string literals inside ERB tags). Is that not true? Can we make it true? Seems a lot less likely to run into compatibility issues. <<-driven construction of something like a CSS class list doesn't seem sufficiently rare for us to just break it by default, IMO.

casperisfine commented 17 days ago

I thought ERB-generated strings were already frozen (meaning this would only affect regular string literals inside ERB tags)

Yes that's the case.

<<-driven construction of something like a CSS class list doesn't seem sufficiently rare for us to just break it by default, IMO.

Yes that's the kind of cases that worry me. Adding a flag is easy I'll do that.

But first I need to understand what's going on with these warning, I wonder if we're concatenating the generated code somehow.

casperisfine commented 17 days ago

Ok, so it's because we do add some prelude in ActionView::Template, so we can't directly use the Erubi option, but it's super easy to do ourself.

One problem though is that is does shift all the backtrace line numbers by one:

    assert_equal "1", e.line_number

--- expected
+++ actual
@@ -1 +1,3 @@
-"1"
+"2"

And I'm not too sure how this could be fixed. Since # frozen_string_literals: true is a comment, we can't just shove the first line after it.

So we might need to rewrite the backtraces which isn't cheap.

casperisfine commented 17 days ago

Nevermind, I found a simple solution:

mod.module_eval("# frozen_string_literal: true\n#{source}", identifier, -1)

casperisfine commented 17 days ago

Ok, this should be good to go. I added a config flag and made it the default for new 7.0 apps.

matthewd commented 15 days ago

Ah, I'm very reluctant to change the behaviour for actual string literals in views... we don't even generate models etc with frozen strings enabled.

I'm dubious that the marginal performance improvement is going to make enough difference to warrant forcing users to understand the different behaviour -- at least for all but a tiny handful of applications.

How about adding the option but keeping it false by default for new applications?

(For the record, I wish Ruby 3 had kept the once-planned transition to frozen-by-default... but that's not the world we live in, and it feels like overreach for us to push that decision ourselves, especially in just a subset of files.)

Or, actually, can we arrange to respect a magic comment at the top of the view? That way people could choose to lint-enforce it in whatever way they do for standard .rb files, plus it would be easier to enable incrementally, and we can push the question of whether we should generate it by default onto a broader question of whether we should do that across the board.

casperisfine commented 14 days ago

How about adding the option but keeping it false by default for new applications?

Fine by me.

I wish Ruby 3 had kept the once-planned transition to frozen-by-default

Me too...

but that's not the world we live in

We do however live in world where rubocop is extremely common and it enable frozen string literals by default.

For the anecdote, what prompted me to look at this was a YJIT-bench for ERubi giving pretty bad results, and lots of de-optimization:

Top-20 most frequent exit ops (100.0% of exits):
    opt_aref_with:   17263742 (100.0)
      invokeblock:         26 (0.0)
....

opt_aref_with being somehash["somestring"] in a file without frozen_string_literal: true.

Or, actually, can we arrange to respect a magic comment at the top of the view?

Sounds doable. But I still think a global flag has value, what about I ship with turned off by default and look at a followup?

byroot commented 11 days ago

@matthewd any further concerns? Can I merge this as a purely optional feature?

matthewd commented 11 days ago

We do however live in world where rubocop is extremely common and it enable frozen string literals by default.

True. My thinking is just that given how widely prevalent that comment is, often via linter enforcement, it seems reasonable to infer/assume that when it's not present, strings are not frozen.

No objections to adding it as an optional feature. (And then we can revisit defaulting it to true in time, if it seems like many [less mega-scale] apps do report choosing to enable it.)