From 7066e9dbc61f371b8d966062b465fa5044cc89f4 Mon Sep 17 00:00:00 2001 From: Sam Bostock Date: Sat, 18 Feb 2023 10:46:52 -0500 Subject: [PATCH] Add `Sorbet/FetchWhenMust` cop Checks for `T.must(object[key])` and recommends `object.fetch(key)` instead. While usually safe, false positives are possible if `object` does not respond to `fetch`, or if `Hash#default_proc` (or similar) is being used. --- config/default.yml | 11 ++++ lib/rubocop/cop/sorbet/fetch_when_must.rb | 65 +++++++++++++++++++ lib/rubocop/cop/sorbet_cops.rb | 1 + manual/cops.md | 1 + manual/cops_sorbet.md | 25 +++++++ .../cop/sorbet/fetch_when_must_spec.rb | 32 +++++++++ 6 files changed, 135 insertions(+) create mode 100644 lib/rubocop/cop/sorbet/fetch_when_must.rb create mode 100644 spec/rubocop/cop/sorbet/fetch_when_must_spec.rb diff --git a/config/default.yml b/config/default.yml index d94a62d6..2e97b4c8 100644 --- a/config/default.yml +++ b/config/default.yml @@ -66,6 +66,17 @@ Sorbet/FalseSigil: - db/**/*.rb - script/**/* +Sorbet/FetchWhenMust: + Description: >- + Checks for `T.must(object[key])` and recommends `object.fetch(key)` instead. + + While usually safe, false positives are possible if `object` does not respond to `fetch`, + or if `Hash#default_proc` (or similar) is being used. + Enabled: pending + VersionAdded: '<>' + Safe: false + SafeAutoCorrect: false + Sorbet/ForbidExtendTSigHelpersInShims: Description: 'Forbid the use of `extend T::Sig` and `extend T::Helpers` in RBI shims' Enabled: true diff --git a/lib/rubocop/cop/sorbet/fetch_when_must.rb b/lib/rubocop/cop/sorbet/fetch_when_must.rb new file mode 100644 index 00000000..b63a0e98 --- /dev/null +++ b/lib/rubocop/cop/sorbet/fetch_when_must.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Sorbet + # rubocop:disable Lint/RedundantCopDisableDirective + + # Checks for `T.must(object[key])` and recommends `object.fetch(key)` instead. + # + # @safety + # False positives are possible if `object` does not respond to `fetch`, + # or if `Hash#default_proc` (or similar) is being used. + # + # @example + # # bad + # T.must(object[key]) + # + # # good + # object.fetch(key) + # + # # good + # object[key] + # + # # good + # # If `object` does not `respond_to? :fetch`, or if using `Hash` `default_proc` + # T.must(object[key]) # rubocop:disable Sorbet/FetchWhenMust + # + class FetchWhenMust < RuboCop::Cop::Base + # rubocop:enable Lint/RedundantCopDisableDirective + + extend AutoCorrector + + MSG = "Use `%s` instead of `%s` when value must always be found, and receiver supports it." + RESTRICT_ON_SEND = [:must].freeze + + # @!method t_must_on_index_result(node) + def_node_matcher :t_must_on_index_result, <<~PATTERN + (send + (const { nil? cbase } :T) :must + { + (index $_ $_) + (send $_ :[] $_) + } + ) + PATTERN + + def on_send(node) + t_must_on_index_result(node) do |receiver, key| + message = format(MSG, expected: expected(receiver, key), actual: node.source) + + add_offense(node, message: message) do |corrector| + corrector.replace(node, expected(receiver, key)) + end + end + end + + private + + def expected(receiver, key) + "#{receiver.source}.fetch(#{key.source})" + end + end + end + end +end diff --git a/lib/rubocop/cop/sorbet_cops.rb b/lib/rubocop/cop/sorbet_cops.rb index 373c954a..e23ecbd4 100644 --- a/lib/rubocop/cop/sorbet_cops.rb +++ b/lib/rubocop/cop/sorbet_cops.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require_relative "sorbet/binding_constant_without_type_alias" require_relative "sorbet/constants_from_strings" +require_relative "sorbet/fetch_when_must" require_relative "sorbet/forbid_superclass_const_literal" require_relative "sorbet/forbid_include_const_literal" require_relative "sorbet/forbid_untyped_struct_props" diff --git a/manual/cops.md b/manual/cops.md index b8f213ea..2528b513 100644 --- a/manual/cops.md +++ b/manual/cops.md @@ -15,6 +15,7 @@ In the following section you find all available cops: * [Sorbet/EnforceSignatures](cops_sorbet.md#sorbetenforcesignatures) * [Sorbet/EnforceSingleSigil](cops_sorbet.md#sorbetenforcesinglesigil) * [Sorbet/FalseSigil](cops_sorbet.md#sorbetfalsesigil) +* [Sorbet/FetchWhenMust](cops_sorbet.md#sorbetfetchwhenmust) * [Sorbet/ForbidExtendTSigHelpersInShims](cops_sorbet.md#sorbetforbidextendtsighelpersinshims) * [Sorbet/ForbidIncludeConstLiteral](cops_sorbet.md#sorbetforbidincludeconstliteral) * [Sorbet/ForbidRBIOutsideOfAllowedPaths](cops_sorbet.md#sorbetforbidrbioutsideofallowedpaths) diff --git a/manual/cops_sorbet.md b/manual/cops_sorbet.md index 6285988e..25ee2cc7 100644 --- a/manual/cops_sorbet.md +++ b/manual/cops_sorbet.md @@ -259,6 +259,31 @@ SuggestedStrictness | `false` | String Include | `**/*.{rb,rbi,rake,ru}` | Array Exclude | `bin/**/*`, `db/**/*.rb`, `script/**/*` | Array +## Sorbet/FetchWhenMust + +Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged +--- | --- | --- | --- | --- +Enabled | No | Yes (Unsafe) | <> | - + +Checks for `T.must(object[key])` and recommends `object.fetch(key)` instead. + +### Examples + +```ruby +# bad +T.must(object[key]) + +# good +object.fetch(key) + +# good +object[key] + +# good +# If `object` does not `respond_to? :fetch`, or if using `Hash` `default_proc` +T.must(object[key]) # rubocop:disable Sorbet/FetchWhenMust +``` + ## Sorbet/ForbidExtendTSigHelpersInShims Enabled by default | Safe | Supports autocorrection | VersionAdded | VersionChanged diff --git a/spec/rubocop/cop/sorbet/fetch_when_must_spec.rb b/spec/rubocop/cop/sorbet/fetch_when_must_spec.rb new file mode 100644 index 00000000..0fac39b9 --- /dev/null +++ b/spec/rubocop/cop/sorbet/fetch_when_must_spec.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +RSpec.describe(RuboCop::Cop::Sorbet::FetchWhenMust, :config) do + it "registers an offense when using `T.must(object[key])`" do + expect_offense(<<~RUBY) + T.must(object[key]) + ^^^^^^^^^^^^^^^^^^^ Use `object.fetch(key)` instead of `T.must(object[key])` when value must always be found, and receiver supports it. + RUBY + + expect_correction(<<~RUBY) + object.fetch(key) + RUBY + end + + it "does not register an offense when using `object[key]` without `T.must`" do + expect_no_offenses(<<~RUBY) + object[key] + RUBY + end + + it "does not register an offense when using `object[key]` without `T.must`" do + expect_no_offenses(<<~RUBY) + T.must(object.fetch(key)) + RUBY + end + + it "does not register an offense when using `T.must(object[key1, key2])`" do + expect_no_offenses(<<~RUBY) + T.must(object[key1, key2]) + RUBY + end +end