Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incompatibilities with --enable-frozen-string-literal #851

Closed
strayer opened this issue May 29, 2020 · 3 comments
Closed

Incompatibilities with --enable-frozen-string-literal #851

strayer opened this issue May 29, 2020 · 3 comments

Comments

@strayer
Copy link

strayer commented May 29, 2020

Slim has various issues with frozen string literals. Some are pretty easy to fix:

diff --git a/lib/slim/embedded.rb b/lib/slim/embedded.rb
index 316809c..adb6e57 100644
--- a/lib/slim/embedded.rb
+++ b/lib/slim/embedded.rb
@@ -8,7 +8,7 @@ module Slim
     end
 
     def on_slim_interpolate(text)
-      @collected << text
+      @collected += text
       nil
     end
   end
@@ -36,12 +36,12 @@ module Slim
     end
 
     def on_static(text)
-      @collected << text
+      @collected += text
       nil
     end
 
     def on_slim_output(escape, text, content)
-      @collected << @tag
+      @collected += @tag
       @protect << [:slim, :output, escape, text, content]
       nil
     end
diff --git a/lib/slim/parser.rb b/lib/slim/parser.rb
index 810779c..6f68cd4 100644
--- a/lib/slim/parser.rb
+++ b/lib/slim/parser.rb
@@ -84,7 +84,7 @@ module Slim
       @code_attr_re = /#{@attr_name}\s*=(=?)\s*/
 
       splat_prefix = Regexp.escape(options[:splat_prefix])
-      splat_regexp_source = '\A\s*' << splat_prefix << '(?=[^\s]+)'
+      splat_regexp_source = '\A\s*' + splat_prefix + '(?=[^\s]+)'
       @splat_attrs_regexp = Regexp.new(splat_regexp_source)
     end
 
@@ -478,7 +478,7 @@ module Slim
 
       until @line.empty? || (count == 0 && @line =~ end_re)
         if @line =~ /\A[,\\]\Z/
-          code << @line << "\n"
+          code += @line + "\n"
           expect_next_line
         else
           if count > 0
@@ -491,7 +491,7 @@ module Slim
             count = 1
             delimiter, close_delimiter = $&, @code_attr_delims[$&]
           end
-          code << @line.slice!(0)
+          code += @line.slice!(0)
         end
       end
       syntax_error!("Expected closing delimiter #{close_delimiter}") if count != 0
@@ -503,7 +503,7 @@ module Slim
 
       until count == 0 && @line[0] == quote[0]
         if @line =~ /\A(\\)?\Z/
-          value << ($1 ? ' ' : "\n")
+          value += ($1 ? ' ' : "\n")
           expect_next_line
         else
           if @line[0] == ?{
@@ -511,7 +511,7 @@ module Slim
           elsif @line[0] == ?}
             count -= 1
           end
-          value << @line.slice!(0)
+          value += @line.slice!(0)
         end
       end
 
diff --git a/test/core/test_encoding.rb b/test/core/test_encoding.rb
index b69ce5f..14fc99c 100644
--- a/test/core/test_encoding.rb
+++ b/test/core/test_encoding.rb
@@ -9,13 +9,13 @@ class TestSlimEncoding < TestSlim
 
   def test_binary
     source = "| \xFF\xFF"
-    source.force_encoding(Encoding::BINARY)
+    source = source.dup.force_encoding(Encoding::BINARY)
 
     result = "\xFF\xFF"
-    result.force_encoding(Encoding::BINARY)
+    result = result.dup.force_encoding(Encoding::BINARY)
 
     out = render(source, default_encoding: 'binary')
-    out.force_encoding(Encoding::BINARY)
+    out = out.dup.force_encoding(Encoding::BINARY)
 
     assert_equal result, out
   end

Note: there are possible performance improvements with using "".dup instead of +=, see Friendly Frozen String Literals for reference. Just wanted to start working on this issue and see quick results.

But then some cryptic issues happen within tilt, which I couldn't yet figure out:

254) Error:
TestSlimCodeEvaluation#test_render_with_call_to_set_custom_attributes:
FrozenError: can't modify frozen String: ""
    (__TEMPLATE__):2:in `__tilt_1880'
    /Users/strayer/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/tilt-2.0.10/lib/tilt/template.rb:170:in `call'
    /Users/strayer/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/tilt-2.0.10/lib/tilt/template.rb:170:in `evaluate'
    /Users/strayer/.asdf/installs/ruby/2.7.0/lib/ruby/gems/2.7.0/gems/tilt-2.0.10/lib/tilt/template.rb:109:in `render'
    /Users/strayer/.../slim/test/core/helper.rb:22:in `render'
    /Users/strayer/.../slim/test/core/helper.rb:45:in `assert_html'
    /Users/strayer/.../slim/test/core/test_code_evaluation.rb:18:in `test_render_with_call_to_set_custom_attributes'

Any idea how to tackle this? I'd love to investigate this further and provide a PR, but I am really at a loss what happens here.

@tainoe
Copy link
Contributor

tainoe commented Jul 10, 2020

The error in that test is caused by Temple, not Slim.
Temple should be fixed like this:

diff --git a/lib/temple/generators/string_buffer.rb b/lib/temple/generators/string_buffer.rb
index c22646c..673a991 100644
--- a/lib/temple/generators/string_buffer.rb
+++ b/lib/temple/generators/string_buffer.rb
@@ -10,7 +10,7 @@ module Temple
     # @api public
     class StringBuffer < ArrayBuffer
       def create_buffer
-        "#{buffer} = ''"
+        "#{buffer} = ''.dup"
       end

       def return_buffer

And codes you suggested did not work even in no --enable-frozen-string-literal environment.
Fixing variable initialization is more compatible like this:

diff --git a/lib/slim/embedded.rb b/lib/slim/embedded.rb
index 316809c..9dd7ec3 100644
--- a/lib/slim/embedded.rb
+++ b/lib/slim/embedded.rb
@@ -2,7 +2,7 @@ module Slim
   # @api private
   class TextCollector < Filter
     def call(exp)
-      @collected = ''
+      @collected = ''.dup
       super(exp)
       @collected
     end
@@ -30,7 +30,7 @@ module Slim
   # @api private
   class OutputProtector < Filter
     def call(exp)
-      @protect, @collected, @tag = [], '', "%#{object_id.abs.to_s(36)}%"
+      @protect, @collected, @tag = [], ''.dup, "%#{object_id.abs.to_s(36)}%"
       super(exp)
       @collected
     end
diff --git a/lib/slim/parser.rb b/lib/slim/parser.rb
index 810779c..3c449dc 100644
--- a/lib/slim/parser.rb
+++ b/lib/slim/parser.rb
@@ -84,7 +84,7 @@ module Slim
       @code_attr_re = /#{@attr_name}\s*=(=?)\s*/

       splat_prefix = Regexp.escape(options[:splat_prefix])
-      splat_regexp_source = '\A\s*' << splat_prefix << '(?=[^\s]+)'
+      splat_regexp_source = '\A\s*' + splat_prefix + '(?=[^\s]+)'
       @splat_attrs_regexp = Regexp.new(splat_regexp_source)
     end

@@ -471,7 +471,7 @@ module Slim
     end

     def parse_ruby_code(outer_delimiter)
-      code, count, delimiter, close_delimiter = '', 0, nil, nil
+      code, count, delimiter, close_delimiter = ''.dup, 0, nil, nil

       # Attribute ends with space or attribute delimiter
       end_re = /\A[\s#{Regexp.escape outer_delimiter.to_s}]/
@@ -499,7 +499,7 @@ module Slim
     end

     def parse_quoted_attribute(quote)
-      value, count = '', 0
+      value, count = ''.dup, 0

       until count == 0 && @line[0] == quote[0]
         if @line =~ /\A(\\)?\Z/

@minad
Copy link
Member

minad commented Mar 19, 2023

If I understood correctly Ruby is moving to frozen literals by default? We should make this change slowly by adding frozen_string_literal headers to every file in Slim and Temple. PRs welcome!

@minad
Copy link
Member

minad commented Apr 23, 2023

Implemented in Temple and Slim. See e1b66cc and judofyr/temple@0173e5e. Please confirm that it works for you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants