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

Refactor frozen_string_literal check during compilation #10263

Merged
merged 1 commit into from Mar 15, 2024

Conversation

casperisfine
Copy link
Contributor

@casperisfine casperisfine commented Mar 15, 2024

In preparation for https://bugs.ruby-lang.org/issues/20205.

The frozen_string_literal compilation option will no longer be a boolean but a tri-state: on/off/default.

Blocked on ruby/prism#2603 (cc @kddnewton)

Also fix a few discrepancies with how prism compiler handle these options.

Copy link
Contributor

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right. Will rb_iseq_opt_frozen_string_literal take into account --enable-frozen-string-literal and --disable-frozen-string-literal?

@casperisfine
Copy link
Contributor Author

Will rb_iseq_opt_frozen_string_literal take into account --enable-frozen-string-literal and --disable-frozen-string-literal

Yes, right now it's a boolean so it's easy. Once I change it for a tri-state, if it's at "unset" I just won't call pm_options_frozen_string_literal_set at all.

@casperisfine casperisfine force-pushed the refactor-compile-frozen-string-literal branch from 6b60f6f to 818afcf Compare March 15, 2024 13:39
In preparation for https://bugs.ruby-lang.org/issues/20205.

The `frozen_string_literal` compilation option will no longer
be a boolean but a tri-state: `on/off/default`.
@casperisfine
Copy link
Contributor Author

Alright, now the only failure left is with prism, but it's already an issue on master:

    1) Failure:
  TestRubyLiteral#test_hash_duplicated_key [/home/runner/work/ruby/ruby/src/test/ruby/test_literal.rb:495]:
  duplicated literal key.
  
  1. [11/12] Assertion for "__FILE__"
     | expected: /key "\(eval\ at\ \/home\/runner\/work\/ruby\/ruby\/src\/test\/ruby\/test_literal\.rb:510\)" is duplicated/
     | actual: "".

From my understanding, now that __FILE__ is no longer a pm_static_literal_value the duplicated key warning no longer works.

Not too sure how to fix that.

@byroot byroot merged commit 91bf7eb into ruby:master Mar 15, 2024
97 of 98 checks passed
@casperisfine
Copy link
Contributor Author

The way the current compiler does it is that strings used as literal hash keys are always considered literals:

      case NODE_STR:
      case NODE_FILE:
        return hash_key || frozen_string_literal_p(iseq);

So I guess prism would need something similar.

@kddnewton kddnewton deleted the refactor-compile-frozen-string-literal branch March 15, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants