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

Frozen String Literal Behavior is Confusing #7426

Closed
gfyoung opened this issue Oct 13, 2019 · 4 comments
Closed

Frozen String Literal Behavior is Confusing #7426

gfyoung opened this issue Oct 13, 2019 · 4 comments

Comments

@gfyoung
Copy link
Contributor

gfyoung commented Oct 13, 2019

According to the docs, my understanding was that in order for the cop to be satisfied, we had to add # frozen_string_literal: true to the top of the file.

However, after finding what appeared to be a false negative in one of my projects, I found #7241 and its corresponding fix in #7251 that highlighted that adding # frozen_string_literal: false to the top can still pass this cop. Two points:

  • I think the docs should be updated to reflect that false is accepted in the magic comment
  • Is there a way to configure the cop so that it enforces # frozen_string_literal: true only?

Rubocop Info

0.75.0 (using Parser 2.6.5.0, running on ruby 2.6.5 x86_64-linux)
@koic
Copy link
Member

koic commented Oct 13, 2019

Good points.

I think the docs should be updated to reflect that false is accepted in the magic comment

Yeah, it will be easy to understand if it is documented.

Is there a way to configure the cop so that it enforces # frozen_string_literal: true only?

AFAIK, there is no such configure option, so enhancement will be necessary.

@gfyoung
Copy link
Contributor Author

gfyoung commented Oct 13, 2019

@koic : Thanks for your reply!

I imagine a docs change is something people wouldn't object to.

The enhancement would be nice. Would be curious to know if people have any objections.

@olliebennett
Copy link
Contributor

I assumed the Style/FrozenStringLiteralComment cop already checked for true, to be honest.

For reference, current RuboCop default configuration options are;

  EnforcedStyle: always
  SupportedStyles:
    # `always` will always add the frozen string literal comment to a file
    # regardless of the Ruby version or if `freeze` or `<<` are called on a
    # string literal. If you run code against multiple versions of Ruby, it is
    # possible that this will create errors in Ruby 2.3.0+.
    - always
    # `never` will enforce that the frozen string literal comment does not
    # exist in a file.
    - never

Would it make sense to add a new SupportedStyle of alwaystrue?

    # `alwaystrue` will enforce that the frozen string literal comment exists
    # and is set to `true` (i.e. enforcing that string literals _are_ frozen). 
    - alwaystrue

I think defaulting to always (the current default) is most intuitive. If someone explicitly uses frozen_string_literal: false they are intentionally not freezing strings, rather than accidentally not freezing strings. An alwaystrue configuration option hopefully solves everyone's use-cases, while maintaining backwards compatibility.

@gfyoung
Copy link
Contributor Author

gfyoung commented Oct 15, 2019

@olliebennett : My thoughts exactly. I wanted to see if there were any maintainer objections though on this before proceeding with an enhancement.

parkerfinch added a commit to parkerfinch/rubocop that referenced this issue Dec 2, 2019
This adds another style to Style/FrozenStringLiteralComment that
enforces that the comment is always enabled, that is, it requires that
string literals are always frozen. The default style, `always`,
enforces that the comment exists but does not enforce that the comment
is enabled.

This also differentiates the offense message for the `always` and
`always_true` styles.

It changes the offense message when using the `always` style to be
less specific, since the old version of the message implied that the
only acceptable value of the frozen_string_literal comment was
`true`. In fact, in the `always` style, either `true` or `false` is
acceptable. The new `always_true` style uses that preexisting message,
since when using that style it is required that the value of the
frozen_string_literal comment is `true`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants