Project

General

Profile

Actions

Bug #14194

closed

--enable-frozen-string-literal ruby runner.rb cgi

Added by MSP-Greg (Greg L) over 4 years ago. Updated about 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Target version:
-
ruby -v:
ruby 2.5.0dev (2017-12-17 trunk 61304) [x64-mingw32]
[ruby-core:84313]

Description

Assuming --enable-frozen-string-literal, ruby runner.rb cgi

Running without patch:

  1) Failure:
CGIMultipartTest#test_cgi_multipart_without_tempfile [E:/GitHub/ruby/test/cgi/test_cgi_multipart.rb:353]:

1. [2/2] Assertion for "stderr"
   | <[]> expected but was
   | <["-:20:in `gsub!': can't modify frozen String, created at -:5 (FrozenError)",
   |  "\tfrom -:20:in `<main>'"]>.


Finished tests in 3.042000s, 143.3268 tests/s, 333.3333 assertions/s.
436 tests, 1014 assertions, 1 failures, 0 errors, 0 skips

With patch:

Finished tests in 2.620800s, 166.3614 tests/s, 386.9048 assertions/s.
436 tests, 1014 assertions, 0 failures, 0 errors, 0 skips

ruby -v: ruby 2.5.0dev (2017-12-17 trunk 61304) [x64-mingw32]

Files

frozen_cgi.patch (740 Bytes) frozen_cgi.patch MSP-Greg (Greg L), 12/17/2017 05:34 PM
test_all_err.log (2.04 KB) test_all_err.log MSP-Greg (Greg L), 06/22/2019 03:28 PM
test_mspec_err.log (832 Bytes) test_mspec_err.log MSP-Greg (Greg L), 06/22/2019 03:28 PM

Updated by jeremyevans0 (Jeremy Evans) about 3 years ago

  • Status changed from Open to Feedback

Now that it has been decided that Ruby 3 will not use frozen string literals by default, I think we should not assume that code that breaks with --enable-frozen-string-literal is buggy, just as we should not assume that code that breaks with --disable-gems is buggy. Do you still consider this a bug that we should fix?

Updated by MSP-Greg (Greg L) about 3 years ago

@jeremyevans0 (Jeremy Evans)

Sorry for the tardy response, I had hoped to run a CI build with the flag set, but busy with other things...

I was unaware that the decision had been made re Ruby 3. If you have any links to discussions, etc, I'd be interested.

I have seen repos (Nokogiri comes to mind) testing with the flag set, so whatever parts of Ruby they're using are working.

In a perfect world, and given that I mostly recall seeing postive comments about using frozen strings, I think Ruby should pass CI with it.

I'll run a build with it and see how many tests/specs fail. This isn't a perfect world...

Thanks, Greg

Updated by nobu (Nobuyoshi Nakada) about 3 years ago

It doesn’t seem harmful, and that test should be independent from the condition.

Updated by nobu (Nobuyoshi Nakada) about 3 years ago

sorry, should be independent

Updated by Eregon (Benoit Daloze) about 3 years ago

nobu (Nobuyoshi Nakada) wrote:

It doesn’t seem harmful, and tests should be independent from the condition.

I'm unsure, it forces existing code to add # frozen_string_literal: false, which seems not valuable if anyway the default is not going to change.
E.g., I don't think it's a valuable change to add # frozen_string_literal: false at the top of every ruby/spec file for now (I'm rather against it).

--enable-frozen-string-literal can break existing code, so I think we should document that in the man page/--help, if we don't remove the flag.

Updated by MSP-Greg (Greg L) about 3 years ago

FYI,

Just ran ruby-loco with frozen string. Both test-all & spec are run parallel, and both crashed.

test-all failures/errors before crash:

ruby 2.7.0dev (2019-06-22T14:38:07Z master f738eeabc2) [x64-mingw32]
CRASHED?

 3 CGIMultipartTest#test_cgi_multipart_without_tempfile = 0.06 s = F
   /ruby/test/cgi/test_cgi_multipart.rb:353
   
   1. [2/2] Assertion for "stderr"
      | <[]> expected but was
      | <["-:20:in `gsub!': can't modify frozen String: \"--foobar1234\\\\nContent-Disposition: form-data: name=\\\\\"name1\\\\\"\\\\n\\\\nvalue1\\\\n--foobar1234\\\\nContent-Disposition: form-data: name=\\\\\"file1\\\\\"; filename=\\\\\"file1.html\\\\\"\\\\nContent-Type: text/html\\\\n\\\\n<html>\\\\n<body><p>Hello</p></body>\\\\n</html>\\\\n\\\\n--foobar1234--\\\\n\" (FrozenError)",
      |  "\tfrom -:20:in `<main>'"]>.


 2 TestObjSpace#test_dump_all = 0.04 s = F
   /ruby/test/objspace/test_objspace.rb:382
   Expected /"bytesize":11, "value":"TEST STRING", "encoding":"UTF-8", "file":"-", "line":4, "method":"dump_my_heap_please", "generation":/ to match "".


 4 TestObjSpace#test_reachable_objects_from = 0.14 s = F
   /ruby/test/objspace/test_objspace.rb:120
   <[Array, "a", "a", "a"]> expected but was
   <[Array, "a"]>.


 0 Test_String_Fstring#test_singleton_class = 0.00 s = E
   /ruby/test/-ext-/string/test_fstring.rb:61
   can't modify frozen String: "_206aw_cd7_464323"

I can't look at this today, but I attached both STDERR logs which show the output from the crashes.

Thanks, Greg

Actions #7

Updated by MSP-Greg (Greg L) about 3 years ago

  • Status changed from Feedback to Closed

Applied in changeset git|2ad7a7f801c07f18150116b819530840eeefebf8.


Get rid of error with frozen string literal

[Bug #14194]

Actions

Also available in: Atom PDF