Bug #14193: --enable-frozen-string-literal and rubygems, erb, & rdoc

• Status: Third Party's Issue
• Assignee: hsbt (Hiroshi SHIBATA)
• Target version: -
• ruby -v: ruby 2.5.0dev (2017-12-16 trunk 61295) [x64-mingw32]
• Backport: 2.3: UNKNOWN, 2.4: UNKNOWN

[MSP-Greg (Greg L)]

A popular gem that I use for my doc site doesn’t currently test against MinGW trunk, but I’ve got a PR there for that. Noticed that the current CI’s were failing, and took a look.

One could debate the practicality of doing so, but for 2.4 and trunk, --enable-frozen-string-literal was set in RUBYOPT. The builds were failing in RubyGems while setting up the test environment. That took me to my fork of rubygems/rubygems, which led me back to trunk.

Attached are three patch files that are required for ‘runner.rb’ testing to pass on rdoc & rubygems folders when --enable-frozen-string-literal is used.

Notes:

RubyGems - I believe its tests run against the files in the src directory, but it needs the other patches in the app directory to pass.

RDoc - I used “”.dup for an unfrozen string, not sure if String.new is preferred or better.

ERB - I haven’t worked with or debugged bindings that much, but I started looking at the bindings passed from RDoc, and when I added a begin/rescue to look at the string variables in them, all the tests for RDoc passed. Seems odd...

Thanks, Greg

PS - I use markdown a lot, may look odd in email. Don't know how many people use email vs browser...

[k0kubun (Takashi Kokubun)]

• Status changed from Open to Feedback

frozen_erb.patch

Thank you for your work to fix it, but I'm strongly against silently suppressing all StandardError exceptions. That makes very hard to find bugs other than ones you expected.

Also, note that frozen_string_literal is already enabled in erb.rb https://github.com/ruby/ruby/blob/70d07d7a85ee13ba9bc2d7881910898cab59aed7/lib/erb.rb#L2.
While it's hard to understand the actual cause of the failure from this report (it should be "ruby --enable-frozen-string-literal test/erb/xxx_test.rb" fails), it's very likely that a template given to ERB is wrong and ERB shouldn't be modified.

[MSP-Greg (Greg L)]

• File deleted (frozen_erb.patch)

[MSP-Greg (Greg L)]

• File deleted (frozen_rdoc.patch)

[MSP-Greg (Greg L)]

• File frozen_rdoc.patch frozen_rdoc.patch added

k0kubun (Takashi Kokubun) wrote:

frozen_erb.patch

Thank you for your work to fix it, but I'm strongly against silently suppressing all StandardError exceptions. That makes very hard to find bugs other than ones you expected.

You're right; I was a bit frustrated while working on it, as some work has been done in Rdoc with frozen strings.

While it's hard to understand the actual cause of the failure from this report (it should be "ruby --enable-frozen-string-literal test/erb/xxx_test.rb" fails)

Sorry if the layout, format or scope of this bothers you. As mentioned, to get RubyGems to pass its tests with --enable-frozen-string-literal requires patches to RDoc.

I've removed the erb patch file and moved the patch to RDoc. I still haven't located the source of the frozen string issue, but all the tests pass (in trunk) using runner.rb rdoc. I've worked quite a bit with YARD's binding to ERB, but not RDoc. Also, I have not (yet) run up YARD with --enable-frozen-string-literal.

I'd certainly appreciate it if someone familiar with RDoc had a look, as it seems that both its code and its tests may need changes/additions.

Since it has been stated that future versions of Ruby will have strings frozen by default, at a minimum, the test suites should pass as such. If gem owners/maintainers cannot set the option and have Ruby load their CI environment...

Note that most of those environments would only require the RubyGems patch, not the RDoc patch.

I should be posting more fixes/patches for tests using --enable-frozen-string-literal.

Thanks, Greg

[k0kubun (Takashi Kokubun)]

• Status changed from Feedback to Open

[k0kubun (Takashi Kokubun)]

I'd certainly appreciate it if someone familiar with RDoc had a look, as it seems that both its code and its tests may need changes/additions.

I believe RDoc upstream is https://github.com/ruby/rdoc and it's just copied from that place to ruby core. Thus I think it's better to post an issue or create PR to https://github.com/ruby/rdoc too, for RDoc changes.

[znz (Kazuhiro NISHIYAMA)]

I think ''.dup is better than String.new in most cases. Because String.new.encoding is ASCII-8BIT, so it may be incompatible.
String.new is better in some cases. For example: https://github.com/ruby/ruby/blob/373babeaac8c3e663e1ded74a9f06ac94a671ed9/lib/fileutils.rb#L745-L746 (buffers)

[MSP-Greg (Greg L)]

znz (Kazuhiro NISHIYAMA) wrote:

I think ''.dup is better than String.new in most cases. Because String.new.encoding is ASCII-8BIT, so it may be incompatible.
String.new is better in some cases. For example: https://github.com/ruby/ruby/blob/373babeaac8c3e663e1ded74a9f06ac94a671ed9/lib/fileutils.rb#L745-L746 (buffers)

Thanks. I did not know about the encoding difference.

Also, I wasn't previously aware of it, and haven't investigated, but what about the + character, as in

a = +''
b = +""

Greg

[aycabta (aycabta .)]

k0kubun (Takashi Kokubun) wrote:

I believe RDoc upstream is https://github.com/ruby/rdoc and it's just copied from that place to ruby core. Thus I think it's better to post an issue or create PR to https://github.com/ruby/rdoc too, for RDoc changes.

I think so too.

Then, lib/rdoc/rd/block_parser.rb and lib/rdoc/rd/inline_parser.rb are generated from *.ry by Racc, so it's Racc's issue.

[MSP-Greg (Greg L)]

aycabta (aycabta .) wrote:

Then, lib/rdoc/rd/block_parser.rb and lib/rdoc/rd/inline_parser.rb are generated from *.ry by Racc, so it's Racc's issue.

Thanks for pointing that out.

The rubygems patch was accepted there, I'll patch any future changes to rdoc.

OK to close, thanks, Greg

[hsbt (Hiroshi SHIBATA)]

• Status changed from Open to Third Party's Issue
• Assignee set to hsbt (Hiroshi SHIBATA)

A patch of rubygems was merged by the upstream repository.