RuboCop: Performance

[skip-stages=Flakey]

auto-corrected

Change-Id: I3c37291f9cc0d104b90c4a4e6023d6069954bc37
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/278347
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Jacob Burroughs <jburroughs@instructure.com>
QA-Review: Cody Cutrer <cody@instructure.com>
Product-Review: Cody Cutrer <cody@instructure.com>

.rubocop.common.yml

@@ -73,35 +73,14 @@ Naming/FileName:
   Exclude:
     - "**/Gemfile.d/~after.rb"
 
+Performance:
+  Severity: error
 Performance/Casecmp:
-  Severity: error
   AutoCorrect: false # prefer using casecmp?(other) instead of casecmp(other).zero?; it handles unicode better
-Performance/Count:
-  Severity: error
-Performance/Detect:
-  Severity: error
-Performance/InefficientHashSearch:
-  Severity: error
-Performance/MapCompact:
-  Severity: error
-Performance/RedundantBlockCall:
-  Severity: error
-Performance/RedundantMerge:
-  Severity: error
-Performance/RedundantSplitRegexpArgument:
-  Severity: error
-Performance/RegexpMatch:
-  Severity: error
-Performance/StartWith:
-  Severity: error
-Performance/StringInclude:
-  Severity: error
-Performance/StringReplacement:
-  Severity: error
-Performance/Sum:
-  Severity: error
-Performance/TimesMap:
-  Severity: error
+Performance/CollectionLiteralInLoop:
+  Severity: info # not auto-correctable; can be a pain to fix and isn't necessarily performance critical
+Performance/MethodObjectAsBlock:
+  Enabled: false # decreases expressiveness
 
 Rails/ActiveRecordAliases:
   Severity: error

app/controllers/conditional_release/rules_controller.rb

@@ -121,7 +121,7 @@ module ConditionalRelease
       attrs
     end
 
-    def arrange_items(items, &_block)
+    def arrange_items(items)
       if items.present?
         items.map.with_index(1) do |item, position|
           item[:position] = position if item.present?

app/controllers/external_content_controller.rb

@@ -43,7 +43,7 @@ class ExternalContentController < ApplicationController
     if params[:service] == 'equella'
       params.each do |key, value|
         if key.to_s.start_with?('eq_')
-          @retrieved_data[key.to_s.gsub(/\Aeq_/, "")] = value
+          @retrieved_data[key.to_s.delete_prefix('eq_')] = value
         end
       end
     elsif params[:return_type] == 'oembed'

app/models/appointment_group_sub_context.rb

@@ -24,10 +24,10 @@ class AppointmentGroupSubContext < ActiveRecord::Base
 
   validates_each :sub_context do |record, attr, value|
     if record.participant_type == 'User'
-      record.errors.add(attr, t('errors.invalid_course_section', 'Invalid course section')) unless value.blank? || (value.is_a?(CourseSection) && record.appointment_group.contexts.any? { |c| c == value.course })
+      record.errors.add(attr, t('errors.invalid_course_section', 'Invalid course section')) unless value.blank? || (value.is_a?(CourseSection) && record.appointment_group.contexts.any?(value.course))
     else
       record.errors.add(attr, t('errors.missing_group_category', 'Group appointments must have a group category')) unless value.present? && value.is_a?(GroupCategory)
-      record.errors.add(attr, t('errors.invalid_group_category', 'Invalid group category')) unless value && record.appointment_group.contexts.any? { |c| c == value.context }
+      record.errors.add(attr, t('errors.invalid_group_category', 'Invalid group category')) unless value && record.appointment_group.contexts.any?(value.context)
     end
   end
 

app/models/assignment.rb

@@ -546,7 +546,7 @@ class Assignment < ActiveRecord::Base
     new_value = new_value.split(/[\s,]+/) if new_value.is_a?(String)
 
     # remove the . if they put it on, and extra whitespace
-    new_value.map! { |v| v.strip.gsub(/\A\./, '').downcase } if new_value.is_a?(Array)
+    new_value.map! { |v| v.strip.delete_prefix('.').downcase } if new_value.is_a?(Array)
 
     write_attribute(:allowed_extensions, new_value)
   end

app/models/attachment.rb

@@ -941,7 +941,7 @@ class Attachment < ActiveRecord::Base
   # created if necessary.
   def open(opts = {}, &block)
     if instfs_hosted?
-      if block_given?
+      if block
         streaming_download(&block)
       else
         create_tempfile(opts) do |tempfile|

app/models/conversation_participant.rb

@@ -98,7 +98,7 @@ class ConversationParticipant < ActiveRecord::Base
       scope_shard = s
     end
     scope_shard ||= Shard.current
-    exterior_user_ids = tags.map { |t| t.sub(/\Auser_/, '').to_i }
+    exterior_user_ids = tags.map { |t| t.delete_prefix('user_').to_i }
 
     # which users have conversations on which shards?
     users_by_conversation_shard =
@@ -442,7 +442,7 @@ class ConversationParticipant < ActiveRecord::Base
                                # closest one after it)
                                times = messages.map(&:created_at)
                                older = times.reject! { |t| t <= last_message_at } || []
-                               older.first || times.reverse.first
+                               older.first || times.last
                              end
       self.has_attachments = messages.with_attachments.exists?
       self.has_media_objects = messages.with_media_comments.exists?

app/models/message.rb

@@ -496,7 +496,7 @@ class Message < ActiveRecord::Base
 
     instance_variable_set(:"@message_content_#{name}",
                           @output_buffer.to_s.strip)
-    @output_buffer = old_output_buffer.sub(/\n\z/, '')
+    @output_buffer = old_output_buffer.delete_suffix("\n")
 
     if old_output_buffer.is_a?(ActiveSupport::SafeBuffer) && old_output_buffer.html_safe?
       @output_buffer = old_output_buffer.class.new(@output_buffer)
@@ -536,7 +536,7 @@ class Message < ActiveRecord::Base
       path = Canvas::MessageHelper.find_message_path(filename)
     end
 
-    @i18n_scope = "messages." + filename.sub(/\.erb\z/, '')
+    @i18n_scope = "messages." + filename.delete_suffix('.erb')
 
     if (File.exist?(path) rescue false)
       File.read(path)

app/models/profile.rb

@@ -86,7 +86,7 @@ class Profile < ActiveRecord::Base
   # some tricks to make it behave like STI with a type column
   def self.inherited(klass)
     super
-    context_type = klass.name.sub(/Profile\z/, '')
+    context_type = klass.name.delete_suffix('Profile')
     klass.class_eval { alias_method context_type.downcase.underscore, :context }
   end
 

app/models/pseudonym.rb

@@ -445,7 +445,7 @@ class Pseudonym < ActiveRecord::Base
   def valid_ssha?(plaintext_password)
     return false if plaintext_password.blank? || self.sis_ssha.blank?
 
-    decoded = Base64.decode64(self.sis_ssha.sub(/\A\{SSHA\}/, ""))
+    decoded = Base64.decode64(self.sis_ssha.delete_prefix('{SSHA}'))
     digest = decoded[0, 40]
     salt = decoded[40..]
     return false unless digest && salt

app/models/user.rb

@@ -2270,7 +2270,7 @@ class User < ActiveRecord::Base
     if filter_after_db
       original_count = events.count
       if events.any? { |e| e.context_code.start_with?("course_section_") }
-        section_ids = events.map(&:context_code).grep(/\Acourse_section_\d+\z/).map { |s| s.sub(/\Acourse_section_/, '').to_i }
+        section_ids = events.map(&:context_code).grep(/\Acourse_section_\d+\z/).map { |s| s.delete_prefix('course_section_').to_i }
         section_course_codes = Course.joins(:course_sections).where(:course_sections => { :id => section_ids })
                                      .pluck(:id).map { |id| "course_#{id}" }
         visible_section_codes = self.section_context_codes(section_course_codes)
@@ -2498,7 +2498,7 @@ class User < ActiveRecord::Base
   end
 
   def section_context_codes(context_codes, skip_visibility_filter = false)
-    course_ids = context_codes.grep(/\Acourse_\d+\z/).map { |s| s.sub(/\Acourse_/, '').to_i }
+    course_ids = context_codes.grep(/\Acourse_\d+\z/).map { |s| s.delete_prefix('course_').to_i }
     return [] unless course_ids.present?
 
     section_ids = []

app/models/wimba_conference.rb

@@ -30,7 +30,7 @@ class WimbaConference < WebConference
   def archive_external_url(user, url_id)
     urls = []
     if (res = send_request('listClass', { 'filter00' => 'archive_of', 'filter00value' => wimba_id, 'attribute' => 'longname' }))
-      res.sub(/\A100 OK\n/, '').split(/\n=END RECORD\n?/).each do |match|
+      res.delete_prefix("100 OK\n").split(/\n=END RECORD\n?/).each do |match|
         data = match.split("\n").inject({}) { |hash, line|
           key, hash[key.to_sym] = line.split(/=/, 2)
           hash

app/presenters/authentication_providers_presenter.rb

@@ -35,14 +35,14 @@ class AuthenticationProvidersPresenter
     AuthenticationProvider.valid_auth_types.filter_map do |auth_type|
       klass = AuthenticationProvider.find_sti_class(auth_type)
       next unless klass.enabled?(account)
-      next if klass.singleton? && configs.any? { |aac| aac.is_a?(klass) }
+      next if klass.singleton? && configs.any?(klass)
 
       klass
     end
   end
 
   def needs_unknown_user_url?
-    configs.any? { |c| c.is_a?(AuthenticationProvider::Delegated) }
+    configs.any?(AuthenticationProvider::Delegated)
   end
 
   def login_url_options(aac)

build/dockerfile_writer.rb

@@ -68,7 +68,7 @@ class DockerfileWriter
 
     def <<(obj)
       if @contents[parent.out_file_suffix].nil?
-        @contents[parent.out_file_suffix] = String.new
+        @contents[parent.out_file_suffix] = +''
       end
 
       @contents[parent.out_file_suffix] << obj

config/initializers/active_record.rb

@@ -749,7 +749,7 @@ module UsefulFindInBatches
   end
 
   def in_batches(strategy: nil, start: nil, finish: nil, **kwargs, &block)
-    unless block_given?
+    unless block
       return ActiveRecord::Batches::BatchEnumerator.new(strategy: strategy, start: start, relation: self, **kwargs)
     end
 
@@ -1923,11 +1923,11 @@ ActiveRecord::ConnectionAdapters::Transaction.prepend(PreserveShardAfterTransact
 module ConnectionWithMaxRuntime
   def initialize(*)
     super
-    @created_at = Concurrent.monotonic_time
+    @created_at = Process.clock_gettime(Process::CLOCK_MONOTONIC)
   end
 
   def runtime
-    Concurrent.monotonic_time - @created_at
+    Process.clock_gettime(Process::CLOCK_MONOTONIC) - @created_at
   end
 end
 ActiveRecord::ConnectionAdapters::AbstractAdapter.prepend(ConnectionWithMaxRuntime)

config/initializers/template_streaming.rb

@@ -51,7 +51,7 @@ module StreamingViewExtensions
   end
 
   def provide(name, content = nil, &block)
-    if block_given?
+    if block
       content = capture(&block) || '' # still carry on even if the block doesn't return anything
       provide(name, content)
     else

gems/dynamic_settings/lib/dynamic_settings.rb

@@ -151,7 +151,7 @@ module DynamicSettings
     alias_method :kv_proxy, :find
 
     def reset_cache!
-      cache.delete_matched(/^#{CACHE_KEY_PREFIX}/)
+      cache.delete_matched(/^#{CACHE_KEY_PREFIX}/o)
     end
   end
 end

gems/plugins/qti_exporter/lib/qti/associate_interaction.rb

@@ -314,7 +314,7 @@ module Qti
     def check_for_meta_matches
       if (long_matches = @doc.search('instructureMetadata matchingMatch'))
         @question[:matches].each_with_index do |match, i|
-          match[:text] = long_matches[i].text.strip.gsub(/ +/, " ") if long_matches[i]
+          match[:text] = long_matches[i].text.strip.squeeze(' ') if long_matches[i]
         end
         if long_matches.size > 0 && long_matches.size != @question[:matches].size
           @question[:qti_warning] = "The matching options for this question may have been incorrectly imported."

gems/rubocop-canvas/lib/rubocop_canvas/cops/specs/no_skip_without_ticket.rb

@@ -51,7 +51,7 @@ module RuboCop
         end
 
         def refs_ticket?(reason)
-          reason =~ /#{JiraRefParser::IssueIdRegex}/
+          reason =~ /#{JiraRefParser::IssueIdRegex}/o
         end
       end
     end

lib/address_book/caching.rb

@@ -68,17 +68,17 @@ module AddressBook
       private
 
       def globalize(role_hash)
-        Hash[*role_hash.map do |id, roles|
+        Hash[*role_hash.flat_map do |id, roles|
           id = Shard.global_id_for(id) if id > 0
           [id, roles]
-        end.flatten(1)]
+        end]
       end
 
       def relativize(role_hash)
-        Hash[*role_hash.map do |id, roles|
+        Hash[*role_hash.flat_map do |id, roles|
           id = Shard.relative_id_for(id, Shard.current, Shard.current) if id > 0
           [id, roles]
-        end.flatten(1)]
+        end]
       end
     end
 

lib/api.rb

@@ -260,7 +260,7 @@ module Api
           raise ArgumentError, "missing scope for collection" unless sis_mapping[:scope]
 
           ids = columns[column]
-          if ids.any? { |id| id.is_a?(Array) }
+          if ids.any?(Array)
             ids_hash = {}
             ids.each do |id|
               id = Array(id)

lib/canvas.rb

@@ -24,7 +24,7 @@ module Canvas
   mattr_accessor :protected_attribute_error
 
   def self.active_record_foreign_key_check(name, type, options)
-    if name.to_s =~ /_id\z/ && type.to_s == 'integer' && options[:limit].to_i < 8
+    if name.to_s.end_with?('_id') && type.to_s == 'integer' && options[:limit].to_i < 8
       raise ArgumentError, <<~TEXT
         All foreign keys need to be at least 8-byte integers. #{name}
         looks like a foreign key, please add this option: `:limit => 8`

lib/cc/exporter/epub/converters/files_converter.rb

@@ -129,7 +129,7 @@ module CC::Exporter::Epub::Converters
           file_basename = CGI.escape(file_basename) unless @export_type == :web_zip
           path_args = [
             CC::Exporter::Epub::FILE_PATH,
-            File.dirname(original_path.match(/#{WEB_RESOURCES_FOLDER}\/(.+)$/)[1]),
+            File.dirname(original_path.match(/#{WEB_RESOURCES_FOLDER}\/(.+)$/o)[1]),
             file_basename
           ].reject do |path_part|
             path_part.match(/^\.$/)

lib/messageable_user/calculator.rb

@@ -390,7 +390,7 @@ class MessageableUser
     def messageable_users_in_context_scope(asset_string_or_asset, options = {})
       if asset_string_or_asset.is_a?(String)
         asset_string = asset_string_or_asset
-        return unless asset_string.sub(/_all\z/, '') =~ CONTEXT_RECIPIENT
+        return unless asset_string.delete_suffix('_all') =~ CONTEXT_RECIPIENT
 
         context_type = $1
         context_id_or_object = $2.to_i

lib/progress_runner.rb

@@ -39,7 +39,7 @@ class ProgressRunner
   # @param process_element A block that performs the actually processing on each element.
   #   Passed an individual element as a parameter.
   def do_batch_update(elements, &process_element)
-    raise 'block required' unless block_given?
+    raise 'block required' unless process_element
 
     @progress.start!
     update_every = [elements.size / 20, 4].max
@@ -58,7 +58,7 @@ class ProgressRunner
   # @param block The block to call to format the completed message.
   # @see #default_completed_message
   def completed_message(&block)
-    raise 'block required' unless block_given?
+    raise 'block required' unless block
 
     @completed_message = block
     self
@@ -67,7 +67,7 @@ class ProgressRunner
   # Provide a custom error message formatter. The provided block overrides the default
   # @param block The block to call to format an error message. See #default_error_message
   def error_message(&block)
-    raise 'block required' unless block_given?
+    raise 'block required' unless block
 
     @error_message = block
     self

lib/turnitin.rb

@@ -139,7 +139,7 @@ module Turnitin
         settings[:exclude_value] = case settings[:exclude_type]
                                    when '0'; ''
                                    when '1'; [exclude_value, 1].max.to_s
-                                   when '2'; (0..100).include?(exclude_value) ? exclude_value.to_s : '0'
+                                   when '2'; (0..100).cover?(exclude_value) ? exclude_value.to_s : '0'
                                    end
       end
       settings

spec/feature_flag_helper.rb

@@ -41,7 +41,7 @@ module FeatureFlagHelper
     [Account, Course, User].each do |model|
       original_method = model.instance_method(:lookup_feature_flag)
       allow_any_instance_of(model).to receive(:lookup_feature_flag) do |m, feature, **kwargs|
-        original_method.bind(m).call(feature, **kwargs)
+        original_method.bind_call(m, feature, **kwargs)
       rescue
         nil
       end

spec/lib/microsoft_sync/state_machine_job_spec.rb

@@ -116,8 +116,7 @@ module MicrosoftSync
 
     # Used as a helper to enqueue actual delayed jobs
     def direct_enqueue_run(run_at, step, initial_mem_state)
-      StateMachineJob.instance_method(:delay).bind(self)
-                     .call(sender: self, strand: strand, run_at: run_at)
+      StateMachineJob.instance_method(:delay).bind_call(self, sender: self, strand: strand, run_at: run_at)
                      .run(step, initial_mem_state)
     end
 

spec/models/active_record_base_spec.rb

@@ -900,23 +900,23 @@ describe ActiveRecord::ConnectionAdapters::ConnectionPool do
   end
 
   it "evicts connections on checkout" do
-    allow(Concurrent).to receive(:monotonic_time).and_return(0)
+    allow(Process).to receive(:clock_gettime).and_return(0)
 
     conn1 = pool.connection
     pool.checkin(conn1)
 
-    allow(Concurrent).to receive(:monotonic_time).and_return(60)
+    allow(Process).to receive(:clock_gettime).and_return(60)
     conn2 = pool.connection
     expect(conn2).not_to eql conn1
   end
 
   it "evicts connections on checkin" do
-    allow(Concurrent).to receive(:monotonic_time).and_return(0)
+    allow(Process).to receive(:clock_gettime).and_return(0)
 
     conn1 = pool.connection
     expect(conn1.runtime).to eq 0
 
-    allow(Concurrent).to receive(:monotonic_time).and_return(60)
+    allow(Process).to receive(:clock_gettime).and_return(60)
 
     expect(conn1.runtime).to eq 60
     pool.checkin(conn1)
@@ -925,12 +925,12 @@ describe ActiveRecord::ConnectionAdapters::ConnectionPool do
   end
 
   it "evicts connections if you call flush" do
-    allow(Concurrent).to receive(:monotonic_time).and_return(0)
+    allow(Process).to receive(:clock_gettime).and_return(0)
 
     conn1 = pool.connection
     pool.checkin(conn1)
 
-    allow(Concurrent).to receive(:monotonic_time).and_return(60)
+    allow(Process).to receive(:clock_gettime).and_return(60)
 
     pool.flush
 

spec/selenium/test_setup/common_helper_methods/custom_date_helpers.rb

@@ -36,11 +36,11 @@ module CustomDateHelpers
       "#{date} at #{time_string(time)}"
     else
       datetime_string(time, :no_words)
-    end.gsub(/ +/, ' ')
+    end.squeeze(' ')
   end
 
   def calendar_time_string(time)
-    time_string(time).sub(/m\z/, "").strip
+    time_string(time).delete_suffix('m').strip
   end
 
   # this is for a datepicker that uses Intl.DateTimeFormat to format the field.

spec/spec_helper.rb

@@ -671,7 +671,7 @@ RSpec.configure do |config|
       end
 
       def method_missing(sym, *args, &blk)
-        @ancestor.instance_method(sym).bind(@subject).call(*args, &blk)
+        @ancestor.instance_method(sym).bind_call(@subject, *args, &blk)
       end
     end
 

spec/support/spec_time_limit.rb

@@ -83,7 +83,7 @@ module SpecTimeLimit
     # changes to things it depends on. but that's where the status_quo stuff
     # helps us out
     def commit_modifies_spec?(example)
-      commit_files.include?(example.metadata[:file_path].sub(/\A\.\//, ''))
+      commit_files.include?(example.metadata[:file_path].delete_prefix('./'))
     end
 
     def commit_files