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

Bug in Style/FrozenStringLiteralComment where comment can exist anywhere in file #7929

Closed
jeffcarbs opened this issue May 4, 2020 · 3 comments · Fixed by #7939
Closed

Bug in Style/FrozenStringLiteralComment where comment can exist anywhere in file #7929

jeffcarbs opened this issue May 4, 2020 · 3 comments · Fixed by #7939
Labels

Comments

@jeffcarbs
Copy link
Contributor

jeffcarbs commented May 4, 2020

Looking at this history of the Style/FrozenStringLiteralComment cop it looks like it:

frozen_string_literal actually must appear before any code. Here's an example that should raise an exception at runtime but doesn't:

1 + 2
# frozen_string_literal: true

K = "foo"
K.slice!(4)

However, since the cop is checking the first three comments they can appear anywhere in the source. For example, according to the cop these are OK:

class Something
end

# frozen_string_literal: true
# One

class Something
  # Two
end

# frozen_string_literal: true

This is actually invalid but "accidentally" because the cop has logic about this needing to be one of the first three comments:

# One

class Something
  # Two
end

# Three
# frozen_string_literal: true

There's actually nothing special about the first three lines/comments either. This should be valid as well:

# something else that's important
# I love comments
# comments commenty comments
# Oh boy, more comments

# frozen_string_literal: true
q = 1 + 2
K = "foo"
K.slice!(4)

The reason this cop cares about "first three" is because it has special logic dealing with the shebang and encoding comments.


Expected behavior

This cop would enforce the string literal comment is somewhere in the leading comments (i.e. comments appearing before any ruby code), not necessarily just the first three.

Actual behavior

The cop checks the first three comments in source, regardless of their location in the file

Steps to reproduce the problem

See test cases above.

RuboCop version

$ [bundle exec] rubocop -V
0.82.0 (using Parser 2.5.1.2, running on ruby 2.5.1 x86_64-linux)
@jeffcarbs jeffcarbs changed the title Potential bug in Style/FrozenStringLiteralComment where comment can exist anywhere in file Bug in Style/FrozenStringLiteralComment where comment can exist anywhere in file May 4, 2020
@koic koic added the bug label May 5, 2020
@zhustec
Copy link

zhustec commented May 6, 2020

Consider a executable ruby file with Unix shebang

would it be

#!/usr/bin/env ruby
# frozen_string_literal: true

or

# frozen_string_literal: true
#!/usr/bin/env ruby

@koic
Copy link
Member

koic commented May 6, 2020

Shebang would always have to be on the first line.

% cat example1.rb
#!/usr/bin/env ruby
# frozen_string_literal: true
p 'str'.frozen?

% example1.rb
true

% cat example2.rb
# frozen_string_literal: true
#!/usr/bin/env ruby
p 'str'.frozen?

% example2.rb
example2.rb: line 3: p: command not found

See also: https://docs.rubocop.org/en/stable/cops_lint/#lintorderedmagiccomments

jeffcarbs added a commit to jeffcarbs/rubocop that referenced this issue May 7, 2020
…omment lines

The frozen_string_literal comment can exist anywhere in the
"leading comment lines" (i.e. before the first line of ruby
code). If it appears after any ruby code it is ignored which
does not cause an errors but could cause the absence of an
expected error which is bad.

The previous implementation was checking if this comment was
one of the first three comments which had two issues:
- The comments it was checking could be anywhere, even after
  ruby code
- Since it was only checking three comments, it's possible
  that you can have an appropriate frozen_string_literal but
  this cop would show an error.

This commit updates the logic to look at all leading comment
lines, which may be more or less than three depending on where
the first non-comment token is.
@jeffcarbs
Copy link
Contributor Author

jeffcarbs commented May 7, 2020

Have a fix out in #7939 🙂

@koic koic closed this as completed in #7939 May 8, 2020
koic added a commit that referenced this issue May 8, 2020
…ment

[Fix #7929] Accept frozen_string_literal anywhere in leading comment lines
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants