Add `Safety` section to documentation #575

koic · 2021-10-11T15:39:59Z

This PR adds safety section to documentation for all cops that are Safe: false or SafeAutoCorrect: false.

Before submitting the PR make sure the following are checked:

Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.
The PR relates to only one subject with a clear title
and description in grammatically correct, complete sentences.
Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.
If this is a new cop, consider making a corresponding update to the Rails Style Guide.

andyw8 · 2021-10-11T17:27:07Z

lib/rubocop/cop/rails/save_bang.rb

+      #   This cop's autocorrection is unsafe because custom `update` method call was changed to `update!`
+      #   but the method name remained same in the method definition.


Suggested change

# This cop's autocorrection is unsafe because custom `update` method call was changed to `update!`

# but the method name remained same in the method definition.

# This cop's autocorrection is unsafe because a custom `update` method call would be changed to `update!`,

# but the method name in the definition would be unchanged.

andyw8 · 2021-10-11T17:45:41Z

lib/rubocop/cop/rails/application_controller.rb

@@ -5,6 +5,10 @@ module Cop
    module Rails
      # This cop checks that controllers subclass ApplicationController.
      #
+      # @safety
+      #   This cop's autocorrection is unsafe because a controller class which inherits


I'm having trouble understanding what this means.

@pirj's comment text (#575 (comment)) is excellent and I updated it.

andyw8 · 2021-10-11T17:46:25Z

lib/rubocop/cop/rails/dynamic_find_by.rb

@@ -7,6 +7,10 @@ module Rails
      # Use `find_by` instead of dynamic method.
      # See. https://rails.rubystyle.guide#find_by
      #
+      # @safety
+      #   This cop is unsafe because it may detect user-defined `find_by_xxx` method.


Suggested change

# This cop is unsafe because it may detect user-defined `find_by_xxx` method.

# This cop is unsafe because it may detect a user-defined `find_by_xxx` method.

andyw8 · 2021-10-11T17:46:45Z

lib/rubocop/cop/rails/mailer_name.rb

@@ -8,6 +8,10 @@ module Rails
      # Without the `Mailer` suffix it isn't immediately apparent what's a mailer
      # and which views are related to the mailer.
      #
+      # @safety
+      #   This cop's autocorrection is unsafe because Renaming a constant is


Suggested change

# This cop's autocorrection is unsafe because Renaming a constant is

# This cop's autocorrection is unsafe because renaming a constant is

andyw8 · 2021-10-11T17:47:21Z

lib/rubocop/cop/rails/negate_include.rb

-      # a receiver object that do not have `exclude?` method. (e.g. `IPAddr`)
+      # @safety
+      #   This cop is unsafe because false positive will occur for
+      #   a receiver object that do not have `exclude?` method. (e.g. `IPAddr`)


Suggested change

# a receiver object that do not have `exclude?` method. (e.g. `IPAddr`)

# receiver objects that do not have an `exclude?` method. (e.g. `IPAddr`)

andyw8 · 2021-10-11T17:49:42Z

lib/rubocop/cop/rails/pick.rb

+      #   This cop is unsafe because pluck is defined on both `ActiveRecord::Relation` and `Enumerable`,
+      #   whereas `pick` is only defined on `ActiveRecord::Relation` in Rails 6.0. This will be addressed
+      #   in Rails 6.1 via rails/rails#38760, at which point the cop will be safe.


Suggested change

# This cop is unsafe because pluck is defined on both `ActiveRecord::Relation` and `Enumerable`,

# whereas `pick` is only defined on `ActiveRecord::Relation` in Rails 6.0. This will be addressed

# in Rails 6.1 via rails/rails#38760, at which point the cop will be safe.

# This cop is unsafe because `pluck` is defined on both `ActiveRecord::Relation` and `Enumerable`,

# whereas `pick` is only defined on `ActiveRecord::Relation` in Rails 6.0. This was addressed

# in Rails 6.1 via rails/rails#38760, at which point the cop is safe.

andyw8 · 2021-10-11T17:51:10Z

lib/rubocop/cop/rails/rake_environment.rb

@@ -14,6 +14,10 @@ module Rails
      # * The task does not need application code.
      # * The task invokes the `:environment` task.
      #
+      # @safety
+      #   Probably not a problem in most cases, but it is likely that calling `:environment` task


Suggested change

# Probably not a problem in most cases, but it is likely that calling `:environment` task

# Probably not a problem in most cases, but it is possible that calling `:environment` task

(otherwise it seems like the sentence is a contradiction).

pirj

Incredible job!

pirj · 2021-10-11T16:30:10Z

lib/rubocop/cop/rails/active_record_aliases.rb

+      # @safety
+      #   This cop is unsafe because custom `update_attributes` method call was changed to
+      #   `update` but the method name remained same in the method definition.
+      #
      # @example
      #   #bad
      #   Book.update_attributes!(author: 'Alice')


Unrelated: I believe there's a typo here, should be lowercase book.

pirj · 2021-10-11T17:13:23Z

lib/rubocop/cop/rails/arel_star.rb

+      #   [source,ruby]
+      #   ---
+      #   hash = { "*" => true }
+      #   hash["*"] # This would get corrected to `hash[Arel.star]`


This code is from a "does not register an offense" example.

Here is a comment that explains why we've kept the cop unsafe.

WDYT of:

This cop's autocorrection is unsafe because it turns a quoted * into an SQL *, unquoted. * is a valid column name in certain databases supported by Rails, and even though it is usually a mistake, it might denote legitimate access to a column named *.

pirj · 2021-10-11T17:27:14Z

lib/rubocop/cop/rails/application_controller.rb

+      #   This cop's autocorrection is unsafe because a controller class which inherits
+      #   `ActionController::Base` directly.


This (and similar job/mailer/model)'s descriptions are hard to understand.
Maybe something like:

This cop's autocorrection is unsafe because it may let the logic from ApplicationContoller sneak into a controller that is not purposed to inherit logic common among other controllers.

pirj · 2021-10-11T17:30:47Z

lib/rubocop/cop/rails/dynamic_find_by.rb

@@ -7,6 +7,10 @@ module Rails
      # Use `find_by` instead of dynamic method.
      # See. https://rails.rubystyle.guide#find_by
      #
+      # @safety
+      #   This cop is unsafe because it may detect user-defined `find_by_xxx` method.


I would argue that the cop is unsafe. It is certainly unsafe when not configured properly, i.e. user-defined find_by_xxx method is not added to cop's AllowedMethods.

pirj · 2021-10-11T17:33:48Z

lib/rubocop/cop/rails/mailer_name.rb

@@ -8,6 +8,10 @@ module Rails
      # Without the `Mailer` suffix it isn't immediately apparent what's a mailer
      # and which views are related to the mailer.
      #
+      # @safety
+      #   This cop's autocorrection is unsafe because Renaming a constant is


Cosmetic: lowercase "Renaming"

pirj · 2021-10-11T17:41:33Z

lib/rubocop/cop/rails/rake_environment.rb

+      #   Probably not a problem in most cases, but it is likely that calling `:environment` task
+      #   will break a behavior.


It's also slower. E.g. some task that only needs one gem to be loaded to run will run significantly faster without loading the whole application.

pirj · 2021-10-11T17:43:53Z

lib/rubocop/cop/rails/reflection_class_name.rb

+      #   This cop is unsafe because it cannot be determined whether
+      #   constant or method return value specified to `class_name` is a string.


Like in MODEL_CLASS_REGISTRY.names['account']?

Is there anything I need to add about this?

pirj · 2021-10-11T17:47:56Z

lib/rubocop/cop/rails/safe_navigation_with_blank.rb

+      #
+      #   [source,ruby]
+      #   ----
+      #   foo&.blank? #=> nil


Is it possible that someone would write this intentionally?

I would argue that the cop is unsafe for flagging this.

This is based on the following.
https://github.com/rubocop/rubocop-rails/blob/v2.12.3/config/default.yml#L712-L718

Makes total sense to me to draw attention, but not autocorrect 👍

My apologies, I've missed the point - thought that it's Safe: false, not SafeAutoCorrect: false.

(I lack permissions to resolve this thread).

that it's Safe: false, not SafeAutoCorrect: false.

Sure! I've opened #577.

Sorry for the confusion, I meant that its current config is fine. I mistakenly thought it is Safe: false, so I was worried that those running rubocop --safe would miss its offences.

pirj · 2021-10-11T17:49:32Z

lib/rubocop/cop/rails/save_bang.rb

+      #   This cop's autocorrection is unsafe because custom `update` method call was changed to `update!`
+      #   but the method name remained same in the method definition.


Can you please explain in a little more detail?

I added the example code of #98.

pirj · 2021-10-11T18:24:32Z

lib/rubocop/cop/rails/time_zone.rb

+      # @safety
+      #   This cop is unsafe because it may change handling time.
+      #


Oh. It's a pity it's marked as unsafe. Do you think it's possible to make it autocorrect-unsafe instead?

Good catch! It makes sense to me! This does not seem to cause false positives. I look rubocop/rubocop#6966, which should have used SafeAutoCorrect: false correctly. So I've opened PR #576 (I adjust the text of the @safety for the next release) .

Follow up to #575 (comment).

Follow up to rubocop#575 (comment) This PR marks `Rails/TimeZone` as unsafe auto-correction from unsafe.

koic · 2021-10-12T01:57:26Z

@andyw8 @pirj Thank you for the many great reviews! I've updated most of it except for #575 (comment) and #575 (comment).

Follow up to rubocop/rubocop#10094. This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.

Follow up to rubocop#575 (comment).

pirj

Looks great, thank you!

koic force-pushed the add_safety_section_to_docs branch 2 times, most recently from 2035d0b to e059e3f Compare October 11, 2021 16:32

andyw8 reviewed Oct 11, 2021

View reviewed changes

pirj reviewed Oct 11, 2021

View reviewed changes

koic added a commit that referenced this pull request Oct 12, 2021

Fix a typo

e0b4fce

Follow up to #575 (comment).

koic added a commit to koic/rubocop-rails that referenced this pull request Oct 12, 2021

Mark Rails/TimeZone as unsafe auto-correction from unsafe

82c068f

Follow up to rubocop#575 (comment) This PR marks `Rails/TimeZone` as unsafe auto-correction from unsafe.

koic mentioned this pull request Oct 12, 2021

Mark Rails/TimeZone as unsafe auto-correction from unsafe #576

Merged

9 tasks

koic force-pushed the add_safety_section_to_docs branch 2 times, most recently from e9ac684 to a1e8986 Compare October 12, 2021 01:58

Add Safety section to documentation

26d71af

Follow up to rubocop/rubocop#10094. This PR adds safety section to documentation for all cops that are `Safe: false` or `SafeAutoCorrect: false`.

koic force-pushed the add_safety_section_to_docs branch from a1e8986 to 26d71af Compare October 12, 2021 02:00

koic added a commit to koic/rubocop-rails that referenced this pull request Oct 13, 2021

Mark Rails/SafeNavigationWithBlank as unsafe`

b47193d

Follow up to rubocop#575 (comment).

koic mentioned this pull request Oct 13, 2021

Mark Rails/SafeNavigationWithBlank as unsafe` #577

Closed

9 tasks

pirj approved these changes Oct 14, 2021

View reviewed changes

koic merged commit cf584e5 into rubocop:master Oct 16, 2021

koic deleted the add_safety_section_to_docs branch October 16, 2021 18:24

koic mentioned this pull request Jul 20, 2022

Add Rails/ToSWithArgument cop #747

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Safety` section to documentation #575

Add `Safety` section to documentation #575

koic commented Oct 11, 2021 •

edited

andyw8 Oct 11, 2021

andyw8 Oct 11, 2021

koic Oct 12, 2021

andyw8 Oct 11, 2021

andyw8 Oct 11, 2021

andyw8 Oct 11, 2021

andyw8 Oct 11, 2021

andyw8 Oct 11, 2021

pirj left a comment

pirj Oct 11, 2021

pirj Oct 11, 2021

pirj Oct 11, 2021

pirj Oct 11, 2021

pirj Oct 11, 2021

pirj Oct 11, 2021

pirj Oct 11, 2021

koic Oct 12, 2021

pirj Oct 11, 2021

koic Oct 12, 2021

pirj Oct 12, 2021

koic Oct 13, 2021

pirj Oct 13, 2021

pirj Oct 11, 2021

koic Oct 12, 2021

pirj Oct 11, 2021

koic Oct 12, 2021

koic commented Oct 12, 2021 •

edited

pirj left a comment

		# This cop's autocorrection is unsafe because custom `update` method call was changed to `update!`
		# but the method name remained same in the method definition.

	# This cop is unsafe because it may detect user-defined `find_by_xxx` method.
	# This cop is unsafe because it may detect a user-defined `find_by_xxx` method.

	# a receiver object that do not have `exclude?` method. (e.g. `IPAddr`)
	# receiver objects that do not have an `exclude?` method. (e.g. `IPAddr`)

	# Probably not a problem in most cases, but it is likely that calling `:environment` task
	# Probably not a problem in most cases, but it is possible that calling `:environment` task

		# This cop's autocorrection is unsafe because a controller class which inherits
		# `ActionController::Base` directly.

		# Probably not a problem in most cases, but it is likely that calling `:environment` task
		# will break a behavior.

		# This cop is unsafe because it cannot be determined whether
		# constant or method return value specified to `class_name` is a string.

Add Safety section to documentation #575

Add Safety section to documentation #575

Conversation

koic commented Oct 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pirj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

koic commented Oct 12, 2021 • edited

pirj left a comment

Choose a reason for hiding this comment

Add `Safety` section to documentation #575

Add `Safety` section to documentation #575

koic commented Oct 11, 2021 •

edited

koic commented Oct 12, 2021 •

edited