Bug #14193
closed--enable-frozen-string-literal and rubygems, erb, & rdoc
Description
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...
Files
Updated by k0kubun (Takashi Kokubun) over 6 years ago
- 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.
Updated by MSP-Greg (Greg L) over 6 years ago
- 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
Updated by k0kubun (Takashi Kokubun) over 6 years ago
- Status changed from Feedback to Open
Updated by k0kubun (Takashi Kokubun) over 6 years ago
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.
Updated by znz (Kazuhiro NISHIYAMA) over 6 years ago
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)
Updated by MSP-Greg (Greg L) over 6 years ago
znz (Kazuhiro NISHIYAMA) wrote:
I think
''.dup
is better thanString.new
in most cases. BecauseString.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
Updated by aycabta (aycabta .) over 6 years ago
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.
Updated by MSP-Greg (Greg L) over 6 years ago
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
Updated by hsbt (Hiroshi SHIBATA) over 6 years ago
- 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.