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

Magic comment "immutable: string" makes "literal".freeze the default for that file #487

Closed
wants to merge 4 commits into from

Conversation

9 participants
@ColinDKelley
Copy link

ColinDKelley commented Dec 22, 2013

Add magic comment # -*- immutable: string -*- to Ruby that implies .freeze on every string literal in the file. To get a mutable string in a file that starts with the magic comment, use String.new or ''.dup.

For more details, background, and rationale, please see this blog post:

http://development.invoca.com/magic-comment-immutable-string-makes-ruby-2-1s-literal-freeze-optimization-the-default/

Add magic comment for immutable strings
Add a magic comment to Ruby that will implicitly call .freeze on every string in the file. If the string can be statically allocated it will, otherwise it will make the string immutable at runtime. If one does need a mutable string, they can be obtained in two ways. Either by calling String.new or invoking String#dup on a String which will reset its frozen status.
@cremno

This comment has been minimized.

Copy link

cremno commented Dec 22, 2013

It's most probably too late for 2.1.0. See also https://bugs.ruby-lang.org/issues/8976 for a very similar feature request.

parse.y Outdated
@@ -3839,6 +3840,11 @@ string1 : tSTRING_BEG string_contents tSTRING_END
{
/*%%%*/
$$ = $2;
if (parser->immutable_strings) {
$$ = NEW_CALL($2, idFreeze, 0);

This comment has been minimized.

Copy link
@charliesome

charliesome Dec 24, 2013

Member

We should probably just create a new NODE_LIT for the string here if immutable_strings is non-zero.

This comment has been minimized.

Copy link
@ColinDKelley

ColinDKelley Dec 24, 2013

Author

@charliesome Thanks for the feedback. We are adding a call to .freeze here so we get the frozen literal optimization as added by https://bugs.ruby-lang.org/issues/9042. Could NODE_LIT achieve that same effect?

This comment has been minimized.

Copy link
@tmm1

tmm1 Dec 24, 2013

Contributor

Yes, the NODE_LIT will be frozen by the compiler.

This comment has been minimized.

Copy link
@ColinDKelley
$$ = node;

if (parser->immutable_strings) {
$$ = NEW_CALL(node, idFreeze, 0);

This comment has been minimized.

Copy link
@charliesome
@charliesome

This comment has been minimized.

Copy link
Member

charliesome commented Dec 24, 2013

Patch looks good to me, bar the comments I've left above.

@ColinDKelley

This comment has been minimized.

Copy link
Author

ColinDKelley commented Dec 27, 2013

Matching ruby-lang feature/issue:
https://bugs.ruby-lang.org/issues/9278

shyouhei pushed a commit to shyouhei/ruby that referenced this pull request Jan 3, 2014

* lib/rubygems/commands/query_command.rb: Only fetch remote specs when
  showing details.  [ruby-trunk - Bug #8019]  RubyGems bug ruby#487
* lib/rubygems/remote_fetcher.rb:  ditto.
* lib/rubygems/security/policy.rb:  ditto.
* test/rubygems/test_gem_commands_query_command.rb:  Test for the
  above.

* lib/rubygems/security.rb:  Make OpenSSL optional for RubyGems.
* lib/rubygems/commands/cert_command.rb:  ditto.

* lib/rubygems/config_file.rb:  Display file with YAML error, not
  ~/.gemrc

* lib/rubygems/remote_fetcher.rb:  Only create gem subdirectories when
  installing gems.
* lib/rubygems/dependency_resolver.rb:  ditto.
* lib/rubygems/test_utilities.rb:  ditto.
* test/rubygems/test_gem_commands_fetch_command.rb:  Test for the
  above.

* lib/rubygems/spec_fetcher.rb:  Only try to upgrade
  http://rubygems.org to HTTPS
* test/rubygems/test_gem_spec_fetcher.rb:  Test for the above.

* lib/rubygems.rb:  Update win_platform? check for JRuby compatibility.

* test/rubygems/test_gem_installer.rb:  Update for Ruby 1.9.2
  compatibility


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@39606 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

tenderlove pushed a commit to tenderlove/ruby that referenced this pull request Jan 24, 2014

* lib/rubygems/commands/query_command.rb: Only fetch remote specs when
  showing details.  [ruby-trunk - Bug #8019]  RubyGems bug ruby#487
* lib/rubygems/remote_fetcher.rb:  ditto.
* lib/rubygems/security/policy.rb:  ditto.
* test/rubygems/test_gem_commands_query_command.rb:  Test for the
  above.

* lib/rubygems/security.rb:  Make OpenSSL optional for RubyGems.
* lib/rubygems/commands/cert_command.rb:  ditto.

* lib/rubygems/config_file.rb:  Display file with YAML error, not
  ~/.gemrc

* lib/rubygems/remote_fetcher.rb:  Only create gem subdirectories when
  installing gems.
* lib/rubygems/dependency_resolver.rb:  ditto.
* lib/rubygems/test_utilities.rb:  ditto.
* test/rubygems/test_gem_commands_fetch_command.rb:  Test for the
  above.

* lib/rubygems/spec_fetcher.rb:  Only try to upgrade
  http://rubygems.org to HTTPS
* test/rubygems/test_gem_spec_fetcher.rb:  Test for the above.

* lib/rubygems.rb:  Update win_platform? check for JRuby compatibility.

* test/rubygems/test_gem_installer.rb:  Update for Ruby 1.9.2
  compatibility


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/trunk@39606 b2dd03c8-39d4-4d8f-98ff-823fe69b080e

@fw42 fw42 referenced this pull request Jan 27, 2014

Merged

Freeze all the things #297

@fw42

This comment has been minimized.

Copy link

fw42 commented Feb 28, 2014

Is something like this going to happen?

@ColinDKelley

This comment has been minimized.

Copy link
Author

ColinDKelley commented Feb 28, 2014

Is something like this going to happen?

Hope so! :) As argued on our blog post, it is totally impractical and non-Ruby-like to append .freeze to every string literal. And fortunately, in our experience it is extremely rare for code to actually mutate a string literal ... and in cases where it does (buffer accumulation) it's trivial to write String.new or "".dup to get a mutable string.

@sorah sorah force-pushed the ruby:trunk branch from 5317131 to 307198a Nov 26, 2014

@collimarco

This comment has been minimized.

Copy link

collimarco commented Apr 19, 2015

+1 Really interested in this feature and totally agree with @ColinDKelley's comment. Is this feature going to be added in the near future?

@BenMorganIO

This comment has been minimized.

Copy link

BenMorganIO commented Aug 16, 2015

👍

@hsbt

This comment has been minimized.

Copy link
Member

hsbt commented Sep 12, 2015

Please continue discussion on our tracker. see https://bugs.ruby-lang.org/issues/11473

@hsbt hsbt closed this Sep 12, 2015

spatulasnout pushed a commit to spatulasnout/ruby that referenced this pull request Nov 4, 2016

merge revision(s) 39606,39609: [Backport #8019]
	* lib/rubygems/commands/query_command.rb:  Only fetch remote specs when
	  showing details.  [ruby-trunk - Bug #8019]  RubyGems bug ruby#487

	* lib/rubygems/remote_fetcher.rb:  ditto.

	* lib/rubygems/security/policy.rb:  ditto.

	* test/rubygems/test_gem_commands_query_command.rb:  Test for the
	  above.

	* lib/rubygems/security.rb:  Make OpenSSL optional for RubyGems.

	* lib/rubygems/commands/cert_command.rb:  ditto.

	* lib/rubygems/config_file.rb:  Display file with YAML error, not
	  ~/.gemrc

	* lib/rubygems/remote_fetcher.rb:  Only create gem subdirectories when
	  installing gems.

	* lib/rubygems/dependency_resolver.rb:  ditto.

	* lib/rubygems/test_utilities.rb:  ditto.

	* test/rubygems/test_gem_commands_fetch_command.rb:  Test for the
	  above.

	* lib/rubygems/spec_fetcher.rb:  Only try to upgrade
	  http://rubygems.org to HTTPS

	* test/rubygems/test_gem_spec_fetcher.rb:  Test for the above.

	* lib/rubygems.rb:  Update win_platform? check for JRuby compatibility.

	* test/rubygems/test_gem_installer.rb:  Update for Ruby 1.9.2
	  compatibility

	* test/rubygems/test_gem_spec_fetcher.rb:  Removed unused variable.


git-svn-id: svn+ssh://ci.ruby-lang.org/ruby/branches/ruby_2_0_0@39797 b2dd03c8-39d4-4d8f-98ff-823fe69b080e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.