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

Mark Style/FrozenStringLiteralComment as Safe, but with unsafe auto-correction. #8529

Merged
merged 1 commit into from Aug 14, 2020

Conversation

marcandre
Copy link
Contributor

@marcandre marcandre commented Aug 12, 2020

@bbatsov wrote:

I think the cop is actually unsafe (as it doesn't actually check the source to figure out if it's ok to add the magic comment). Unsafe for a cop means that it yields false positives or the suggestions can change the meaning of the code. Unsafe for auto-correct means the cop the cop suggestions are safe, but the auto-correct can't apply those reliably.

I would propose a different definition. Unsafe for a cop means that it yields false positives and that's it. (e.g. "don't use dig with single argument" is unsafe, as it could be a custom method in a gem dependency and thus perfectly o.k. and without another alternative). The Lint/FrozenStringLiteral cop is a good example where the fact that we can't reliably auto-correct a source doesn't mean it can't or shouldn't be done. Since any source can modified to have a frozen string literal comment, that all sources should be modified, and that the cop will always correctly detect if there is a frozen string literal comment, I believe this cop is safe, only its auto-correction is potentially unsafe.

@marcandre marcandre requested a review from bbatsov August 12, 2020 16:36
@marcandre marcandre self-assigned this Aug 12, 2020
@marcandre marcandre force-pushed the safety_first branch 2 times, most recently from 2ccf43a to b352926 Compare August 12, 2020 16:54
@koic
Copy link
Member

koic commented Aug 12, 2020

First of all thank you for opening a good question. In conclusion, I think this cop will be Safe: false.

As far as I understand, the following could be Safe: false. For example...

  • Not safe if false positives is not avoidable because the type of instance assigned to variable is unknown
  • Not safe if (potentially) incompatible behaviours are proposed

I think that Lint/FrozenStringLiteralComment cop is unsafe (Safe: false) in the latter respect.

On the other hand, it is about unsafe auto-correction (e.g.: Safe: true and SafeAutoCorrect: false) .
For example, replacing obj =~ /\Afoo/ with obj.start_with?('foo') is unsafe. Because nil error occurs if obj is nil. So, I think it will be set to SafeAutoCorrect: false if it is unsafe to "auto"-correction.

If users want to safely correct will have to manually handcraft. In this case, users may change using safe navigation operator if needed.

So, Lint/FrozenStringLiteralComment cop will behave (potentially) incompatibly with proposed corrrection, either manually or automatically. So I understand that this case is Safe: false.

Anyway, I'm also interested in @bbatsov's opinion :-) Thank you!

@marcandre
Copy link
Contributor Author

marcandre commented Aug 12, 2020

So, Lint/FrozenStringLiteralComment cop will behave (potentially) incompatibly with proposed corrrection, either manually
If users want to safely correct will have to manually handcraft.

I'm sorry if I missed your point, but manually one can prefix all needed string literals in the source with a + and get 100% compatible behavior.

# before

str = 'hello'
str << ' world'
def name
  'Marc-André'
end

# after

# frozen_string_literal: true
str = +'hello'
str << ' world'
def name
  +'Marc-André'
end

That is my whole point...

@koic
Copy link
Member

koic commented Aug 12, 2020

Thank you for understanding the explanation! Surely both are SafeAutoCorrect: false. So, I get the definition of SafeAutoCorrect: false is done.

On the other hand, I thought again about Safe.

  • Does Safe: false have false positives? => I think this is definitely yes.
  • Does Safe: false have incompatible behaviour changes? => I'm not sure about the current focus.

It's difficult to say which is user-friendly definition of Safe 😅

@bbatsov
Copy link
Collaborator

bbatsov commented Aug 14, 2020

Btw, after our original conversation I've documented the safety definitions here https://docs.rubocop.org/rubocop/0.89/usage/auto_correct.html#safe-auto-correct I wouldn't make any changes that are not aligned with those (unless we also update the definition of safety).

I believe this cop is safe, only its auto-correction is potentially unsafe.

I agree.

CHANGELOG.md Outdated
@@ -7,6 +7,7 @@
* [#8362](https://github.com/rubocop-hq/rubocop/issues/8362): Add numbers of correctable offenses to summary. ([@nguyenquangminh0711][])
* [#8513](https://github.com/rubocop-hq/rubocop/pull/8513): Clarify the ruby warning mentioned in the `Lint/ShadowingOuterLocalVariable` documentation. ([@chocolateboy][])
* [#8517](https://github.com/rubocop-hq/rubocop/pull/8517): Make `Style/HashTransformKeys` and `Style/HashTransformValues` aware of `to_h` with block. ([@eugeneius][])
* [#x](https://github.com/rubocop-hq/rubocop/pulls/x): Mark `Lint/FrozenStringLiteralComment` as `Safe`, but with unsafe auto-correction. ([@marcandre][])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you forgot to update the links. :-)

@koic
Copy link
Member

koic commented Aug 14, 2020

I've documented the safety definitions here

Ah... I overlooked this nice document. I understand the safety definition, thank you!

@mergify mergify bot merged commit 1d2659a into rubocop:master Aug 14, 2020
koic added a commit to koic/rubocop-rails that referenced this pull request Aug 17, 2020
Follow rubocop/rubocop#8529 and rubocop#320.

This PR changes the mark of `Rails/UniqBeforePluck` from `Safe: false`
to `SafeAutoCorrect: false`.

False positives to detection are not a concern. Only worry about
incompatibilities with auto-correction.

Below is an excerpt from the official document.

> * Safe (true/false) - indicates whether the cop can yield false
> positives (by design) or not.
>
> *SafeAutoCorrect (true/false) - indicates whether the auto-correct a cop
> does is safe (equivalent) by design. If a cop is unsafe its auto-correct
> automatically becomes unsafe as well.

https://docs.rubocop.org/rubocop/0.89/usage/auto_correct.html#safe-auto-correct
koic added a commit to koic/rubocop-rails that referenced this pull request Aug 17, 2020
Follow rubocop#320 and rubocop/rubocop#8529.

This PR changes the mark of `Rails/UniqBeforePluck` from `Safe: false`
to `SafeAutoCorrect: false`.

False positives to detection are not a concern. Only worry about
incompatibilities with auto-correction.

Below is an excerpt from the official document.

> * Safe (true/false) - indicates whether the cop can yield false
> positives (by design) or not.
>
> * SafeAutoCorrect (true/false) - indicates whether the auto-correct a cop
> does is safe (equivalent) by design. If a cop is unsafe its auto-correct
> automatically becomes unsafe as well.

https://docs.rubocop.org/rubocop/0.89/usage/auto_correct.html#safe-auto-correct
@andyw8
Copy link
Contributor

andyw8 commented Oct 1, 2020

Note: This PR title appears be incorrect, it's Style/FrozenStringLiteralComment, not Lint/FrozenStringLiteralComment

@marcandre marcandre changed the title Mark Lint/FrozenStringLiteralComment as Safe, but with unsafe auto-correction. Mark Style/FrozenStringLiteralComment as Safe, but with unsafe auto-correction. Oct 1, 2020
@marcandre
Copy link
Contributor Author

Good catch @andyw8, Changelog & PR title fixed, thanks 👍

@marcandre marcandre deleted the safety_first branch October 1, 2020 21:09
koic added a commit to koic/rubocop that referenced this pull request Jan 4, 2021
Follow rubocop#8529.

`Style/MutableConstant` is the same as `Style/FrozenStringLiteralComment`,
freezing object is unsafe auto-correction.
@koic koic mentioned this pull request Jan 4, 2021
8 tasks
bbatsov pushed a commit that referenced this pull request Jan 4, 2021
Follow #8529.

`Style/MutableConstant` is the same as `Style/FrozenStringLiteralComment`,
freezing object is unsafe auto-correction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants