[Fix #7287] Auto-correction of FrozenStringLiteralComment is unsafe

[buehmann]

Auto-correction of Style/FrozenStringLiteralComment changes the state of
string objects in a program and may thus break it.

This closes #7287.

[koic]

IMHO, SafeAutoCorrect: false may not be a bad choice. For example, it causes a breaking change when a string passed from a dependent gem changes from a mutable string to a frozen string.

However, it may be better to warn users to use String#dup or String#+@ for destructive operations on frozen string literals. AFAIK, matz has not given up the possibility of defaulting to a frozen string in the future. In Ruby 3.0, matz was abandant for changing it because there were not enough tools to prevent destructive changes (I heard that at RubyConf Taiwan 2019).

Therefore, I couldn't conclude and was pending this configuration.

[bbatsov]

However, it may be better to warn users to use String#dup or String#+@ for destructive operations on frozen string literals.

I guess that should be a different cop, though, right?

[bbatsov]

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.

[koic]

I get it! That makes sense to me. Thanks for the explanation.

[buehmann]

I think the cop is actually unsafe

Makes sense. Should we add Safe: false then as well?

Having looked into how --safe and --safe-auto-correct work I am a bit confused, though:

Is it correct that currently rubocop --safe-auto-correct applies "safe" autocorrections of unsafe cops (i.e. it also fixes false positives reliably)? And that you need to use rubocop --safe-auto-correct --safe to really not make any changes to the meaning of the code? 😕

[bbatsov]

Is it correct that currently rubocop --safe-auto-correct applies "safe" autocorrections of unsafe cops (i.e. it also fixes false positives reliably)?

There are no such cases - if a cop is unsafe, its auto-correct is unsafe as well.

And that you need to use rubocop --safe-auto-correct --safe to really not make any changes to the meaning of the code?

Yep - basically you get the safe report, but not all of those offences would have safe fixes, so you need to specify this further. I guess that's somewhat confusing, but we couldn't come up with a better way to express it.

[bbatsov]

Makes sense. Should we add Safe: false then as well?

Only it - this will affect the auto-correct as well.

[bbatsov]

PreferredHashMethod is inherently unsafe, as you can never be certain if the receiver is a hash or not. I guess it's simply not marked as unsafe.

[buehmann]

That's my point. The cop is marked as unsafe, but its auto-correction is not marked as unsafe. You claimed above that setting Safe: false for a cop implies SafeAutoCorrect: false (if I understood you correctly). My example shows that this is not true.

So we either have a bug here or I still have a misconception about how these two levels of safeness are supposed to work.

[bbatsov]

Or I do. 😆 Since I don't spend as much time on the code as I used to, I'm starting to forget some of my original reasoning. But in general I'm struggling to see how the cop's suggestions can be unsafe, but it corrections can be somehow safe. Safe always meant preserving the semantics on the code intact.

[marcandre]

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 FrozenStringLiteral cop is a good example where the fact that we can't reliably autocorrect 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 unsafe. How does that sound @bbatsov ?

[marcandre]

ping @bbatsov

[koic]

I guess that should be a different cop, though, right?

Yes. I think it should be a different cop's responsibility.

[bbatsov]

@buehmann It'd be nice if we wrapped this PR at some point. Going back to https://github.com/rubocop-hq/rubocop/pull/7307/files#r318753133 - it seems to me that marking some cop as unsafe should imply that it's auto-correct is unsafe as well.

[buehmann]

Thanks for the reminder. Then RuboCop does not behave as expected at the moment. I will probably create a separate issue for that and propose a fix. After that we can come back to this one.

I opened #7611

[buehmann]

@bbatsov Now that we have #7611 I changed this PR to declare the cop as Safe: false (only).

[bbatsov]

Thanks!