Resolve remaining Lint cops

.rubocop.yml

@@ -16,6 +16,7 @@ AllCops:
     - 'ext/suse/puppet.spec'
     - 'lib/puppet/vendor/**/*'
     - 'lib/puppet/pops/parser/eparser.rb'
+    - 'util/**/*'
 
 # puppet uses symbol booleans in types and providers to work around long standing
 # bugs when trying to manage falsey pararameters and properties
@@ -28,6 +29,31 @@ Lint/BooleanSymbol:
     - 'lib/puppet/reference/providers.rb'
     - 'lib/puppet/parameter/value.rb'
 
+# This breaks the ruby convention of passing &block and using block_given?
+Lint/UnusedMethodArgument:
+  Enabled: false
+
+# Nested methods are bad because the nested method is defined in the outer scope
+# and the nested method is redefined each time the outer method is called.
+# However, `Puppet::Functions.create_loaded_function` calls `class_eval` to
+# evaluate the function and that avoids the first issue. Secondly, the type
+# system always memoizes the result, so the nested method is only called once.
+Lint/NestedMethodDefinition:
+  Enabled: true
+  Exclude:
+    - 'lib/puppet/pops/types/p_binary_type.rb'
+    - 'lib/puppet/pops/types/p_init_type.rb'
+    - 'lib/puppet/pops/types/p_object_type.rb'
+    - 'lib/puppet/pops/types/p_sem_ver_range_type.rb'
+    - 'lib/puppet/pops/types/p_sem_ver_type.rb'
+    - 'lib/puppet/pops/types/p_sensitive_type.rb'
+    - 'lib/puppet/pops/types/p_timespan_type.rb'
+    - 'lib/puppet/pops/types/p_timestamp_type.rb'
+    - 'lib/puppet/pops/types/p_uri_type.rb'
+    - 'lib/puppet/pops/types/types.rb'
+  # AllowedMethods: this requires rubocop 1.37.0
+  #  - 'create_loaded_function'
+
 Metrics/AbcSize:
   Enabled: false
 

.rubocop_todo.yml

@@ -580,69 +580,6 @@ Layout/TrailingEmptyLines:
 Layout/TrailingWhitespace:
   Enabled: false
 
-# Configuration parameters: AllowedMethods.
-# AllowedMethods: enums
-Lint/ConstantDefinitionInBlock:
-  Enabled: false
-
-Lint/MissingSuper:
-  Enabled: false
-
-Lint/NestedMethodDefinition:
-  Exclude:
-    - 'lib/puppet/pops/types/p_binary_type.rb'
-    - 'lib/puppet/pops/types/p_init_type.rb'
-    - 'lib/puppet/pops/types/p_object_type.rb'
-    - 'lib/puppet/pops/types/p_sem_ver_range_type.rb'
-    - 'lib/puppet/pops/types/p_sem_ver_type.rb'
-    - 'lib/puppet/pops/types/p_sensitive_type.rb'
-    - 'lib/puppet/pops/types/p_timespan_type.rb'
-    - 'lib/puppet/pops/types/p_timestamp_type.rb'
-    - 'lib/puppet/pops/types/p_uri_type.rb'
-    - 'lib/puppet/pops/types/types.rb'
-    - 'lib/puppet/type.rb'
-
-Lint/RescueException:
-  Exclude:
-    - 'ext/windows/service/daemon.rb'
-    - 'lib/puppet/configurer/fact_handler.rb'
-    - 'lib/puppet/generate/type.rb'
-    - 'lib/puppet/settings.rb'
-    - 'lib/puppet/transaction/resource_harness.rb'
-    - 'lib/puppet/util.rb'
-    - 'lib/puppet/util/autoload.rb'
-    - 'lib/puppet/util/command_line/trollop.rb'
-    - 'util/rspec_grouper'
-
-# Configuration parameters: AllowComments, AllowNil.
-Lint/SuppressedException:
-  Exclude:
-    - 'lib/puppet/application/face_base.rb'
-    - 'lib/puppet/ffi/windows/functions.rb'
-    - 'lib/puppet/forge/errors.rb'
-    - 'lib/puppet/functions/each.rb'
-    - 'lib/puppet/functions/filter.rb'
-    - 'lib/puppet/functions/map.rb'
-    - 'lib/puppet/functions/slice.rb'
-    - 'lib/puppet/pops/time/timespan.rb'
-    - 'lib/puppet/pops/types/iterable.rb'
-    - 'lib/puppet/pops/types/p_runtime_type.rb'
-    - 'lib/puppet/util/command_line.rb'
-    - 'lib/puppet/util/execution.rb'
-    - 'util/rspec_grouper'
-
-# This cop supports safe auto-correction (--auto-correct).
-Lint/ToJSON:
-  Exclude:
-    - 'lib/puppet/module_tool/metadata.rb'
-    - 'lib/puppet/network/http/error.rb'
-    - 'lib/puppet/pops/serialization/json.rb'
-
-# This cop supports safe auto-correction (--auto-correct).
-# Configuration parameters: AllowUnusedKeywordArguments, IgnoreEmptyMethods, IgnoreNotImplementedMethods.
-Lint/UnusedMethodArgument:
-  Enabled: false
-
 Naming/AccessorMethodName:
   Enabled: false
 

ext/windows/service/daemon.rb

@@ -91,13 +91,13 @@ def service_main(*argsv)
           sleep(runinterval)
           service.log_debug('Service worker thread woken up')
         end
-      rescue Exception => e
+      rescue Exception => e # rubocop:disable Lint/RescueException
         service.log_exception(e)
       end
     end
     @run_thread.join
 
-  rescue Exception => e
+  rescue Exception => e # rubocop:disable Lint/RescueException
     log_exception(e)
   ensure
     log_notice('Service stopped')
@@ -151,7 +151,7 @@ def report_windows_event(type,id,message)
         :event_id    => id,     # 0x01 or 0x02, 0x03 etc.
         :data        => message # "the message"
       )
-    rescue Exception
+    rescue Exception # rubocop:disable Lint/RescueException
       # Ignore all errors
     ensure
       if (!eventlog.nil?)
@@ -167,7 +167,7 @@ def parse_runinterval(puppet_path)
         runinterval = 1800
         log_err("Failed to determine runinterval, defaulting to #{runinterval} seconds")
       end
-    rescue Exception => e
+    rescue Exception => e # rubocop:disable Lint/RescueException
       log_exception(e)
       runinterval = 1800
     end
@@ -182,7 +182,7 @@ def parse_log_level(puppet_path,cmdline_debug)
         loglevel = :notice
         log_err("Failed to determine loglevel, defaulting to #{loglevel}")
       end
-    rescue Exception => e
+    rescue Exception => e # rubocop:disable Lint/RescueException
       log_exception(e)
       loglevel = :notice
     end

lib/puppet/application/face_base.rb

@@ -130,6 +130,7 @@ def parse_options
         begin
           super
         rescue OptionParser::InvalidOption
+          # fall through
         end
 
         face   = @face.name

lib/puppet/configurer/fact_handler.rb

@@ -21,7 +21,7 @@ def find_facts
       facts
     rescue SystemExit,NoMemoryError
       raise
-    rescue Exception => detail
+    rescue Exception => detail # rubocop:disable Lint/RescueException
       message = _("Could not retrieve local facts: %{detail}") % { detail: detail }
       Puppet.log_exception(detail, message)
       raise Puppet::Error, message, detail.backtrace

lib/puppet/confine.rb

@@ -13,6 +13,7 @@ class << self
   end
 
   def self.inherited(klass)
+    super(klass)
     name = klass.to_s.split("::").pop.downcase.to_sym
     raise "Test #{name} is already defined" if @tests.include?(name)
 

lib/puppet/face/config.rb

@@ -2,6 +2,11 @@
 require_relative '../../puppet/face'
 require_relative '../../puppet/settings/ini_file'
 
+module Puppet::Face::Config
+  DEFAULT_SECTION_MARKER = Object.new
+  DEFAULT_SECTION = "main"
+end
+
 Puppet::Face.define(:config, '0.0.1') do
   extend Puppet::Util::Colors
   copyright "Puppet Inc.", 2011
@@ -13,10 +18,14 @@
     'puppet.conf' configuration file. For documentation about individual settings,
     see https://puppet.com/docs/puppet/latest/configuration.html."
 
-  DEFAULT_SECTION_MARKER = Object.new
-  DEFAULT_SECTION = "main"
+  # deprecate top-level constants
+  DEFAULT_SECTION_MARKER = Puppet::Face::Config::DEFAULT_SECTION_MARKER # rubocop:disable Lint/ConstantDefinitionInBlock
+  Object.deprecate_constant(:DEFAULT_SECTION_MARKER)
+  DEFAULT_SECTION = Puppet::Face::Config::DEFAULT_SECTION # rubocop:disable Lint/ConstantDefinitionInBlock
+  Object.deprecate_constant(:DEFAULT_SECTION)
+
   option "--section " + _("SECTION_NAME") do
-    default_to { DEFAULT_SECTION_MARKER } #Sentinel object for default detection during commands
+    default_to { Puppet::Face::Config::DEFAULT_SECTION_MARKER } # Sentinel object for default detection during commands
     summary _("The section of the configuration file to interact with.")
     description <<-EOT
       The section of the puppet.conf configuration file to interact with.
@@ -62,8 +71,8 @@
       options = args.pop
 
       @default_section = false
-      if options[:section] == DEFAULT_SECTION_MARKER
-        options[:section] = DEFAULT_SECTION
+      if options[:section] == Puppet::Face::Config::DEFAULT_SECTION_MARKER
+        options[:section] = Puppet::Face::Config::DEFAULT_SECTION
         @default_section = true
       end
 
@@ -138,8 +147,8 @@ def report_section_and_environment(section_name, environment_name)
     when_invoked do |name, value, options|
 
       @default_section = false
-      if options[:section] == DEFAULT_SECTION_MARKER
-        options[:section] = DEFAULT_SECTION
+      if options[:section] == Puppet::Face::Config::DEFAULT_SECTION_MARKER
+        options[:section] = Puppet::Face::Config::DEFAULT_SECTION
         @default_section = true
       end
 
@@ -221,8 +230,8 @@ def report_section_and_environment(section_name, environment_name)
     when_invoked do |name, options|
 
       @default_section = false
-      if options[:section] == DEFAULT_SECTION_MARKER
-        options[:section] = DEFAULT_SECTION
+      if options[:section] == Puppet::Face::Config::DEFAULT_SECTION_MARKER
+        options[:section] = Puppet::Face::Config::DEFAULT_SECTION
         @default_section = true
       end
 

lib/puppet/face/help.rb

@@ -5,6 +5,12 @@
 require 'pathname'
 require 'erb'
 
+module Puppet::Face::Help
+  COMMON = 'Common:'
+  SPECIALIZED = 'Specialized:'
+  BLANK = "\n"
+end
+
 Puppet::Face.define(:help, '0.0.1') do
   copyright "Puppet Inc.", 2011
   license   _("Apache 2 license; see COPYING")
@@ -162,7 +168,7 @@ def all_application_summaries()
     available_application_names_special_sort().inject([]) do |result, appname|
       next result if exclude_from_docs?(appname)
 
-      if (appname == COMMON || appname == SPECIALIZED || appname == BLANK)
+      if (appname == Puppet::Face::Help::COMMON || appname == Puppet::Face::Help::SPECIALIZED || appname == Puppet::Face::Help::BLANK)
         result << appname
       elsif (is_face_app?(appname))
         begin
@@ -191,16 +197,21 @@ def all_application_summaries()
     end
   end
 
-  COMMON = 'Common:'
-  SPECIALIZED = 'Specialized:'
-  BLANK = "\n"
+  # Deprecate top-level constants
+  COMMON = Puppet::Face::Help::COMMON # rubocop:disable Lint/ConstantDefinitionInBlock
+  Object.deprecate_constant(:COMMON)
+  SPECIALIZED = Puppet::Face::Help::SPECIALIZED # rubocop:disable Lint/ConstantDefinitionInBlock
+  Object.deprecate_constant(:SPECIALIZED)
+  BLANK = Puppet::Face::Help::BLANK # rubocop:disable Lint/ConstantDefinitionInBlock
+  Object.deprecate_constant(:BLANK)
+
   def available_application_names_special_sort()
     full_list = Puppet::Application.available_application_names
     a_list = full_list & %w{apply agent config help lookup module resource}
     a_list = a_list.sort
     also_ran = full_list - a_list
     also_ran = also_ran.sort
-    [[COMMON], a_list, [BLANK], [SPECIALIZED], also_ran].flatten(1)
+    [[Puppet::Face::Help::COMMON], a_list, [Puppet::Face::Help::BLANK], [Puppet::Face::Help::SPECIALIZED], also_ran].flatten(1)
   end
 
   def horribly_extract_summary_from(appname)

lib/puppet/face/node/clean.rb

@@ -47,28 +47,10 @@ def cleanup(node)
     clean_reports(node)
   end
 
-  class LoggerIO
-    def debug(message)
-      Puppet.debug(message)
-    end
-
-    def warn(message)
-      Puppet.warning(message) unless message =~ /cadir is currently configured to be inside/
-    end
-
-    def err(message)
-      Puppet.err(message) unless message =~ /^\s*Error:\s*/
-    end
-
-    def inform(message)
-      Puppet.notice(message)
-    end
-  end
-
   # clean signed cert for +host+
   def clean_cert(node)
     if Puppet.features.puppetserver_ca?
-      Puppetserver::Ca::Action::Clean.new(LoggerIO.new).run({ 'certnames' => [node] })
+      Puppetserver::Ca::Action::Clean.new(Puppet::Face::Node::LoggerIO.new).run({ 'certnames' => [node] })
     else
       Puppet.info _("Not managing %{node} certs as this host is not a CA") % { node: node }
     end
@@ -106,3 +88,23 @@ def type_is_ensurable(resource)
     return false
   end
 end
+
+module Puppet::Face::Node
+  class LoggerIO
+    def debug(message)
+      Puppet.debug(message)
+    end
+
+    def warn(message)
+      Puppet.warning(message) unless message =~ /cadir is currently configured to be inside/
+    end
+
+    def err(message)
+      Puppet.err(message) unless message =~ /^\s*Error:\s*/
+    end
+
+    def inform(message)
+      Puppet.notice(message)
+    end
+  end
+end

lib/puppet/ffi/windows/functions.rb

@@ -477,7 +477,6 @@ module Functions
       ffi_lib :kernel32
       attach_function_private :CreateSymbolicLinkW,
         [:lpwstr, :lpwstr, :dword], :boolean
-    rescue LoadError
     end
 
     # https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcurrentdirectory

lib/puppet/forge.rb

@@ -24,6 +24,7 @@ class Puppet::Forge < SemanticPuppet::Dependency::Source
   attr_reader :host, :repository
 
   def initialize(host = Puppet[:module_repository])
+    super()
     @host = host
     @repository = Puppet::Forge::Repository.new(host, USER_AGENT)
   end

lib/puppet/forge/errors.rb

@@ -88,6 +88,7 @@ def initialize(options)
           @message ||= body['message'].strip
         end
       rescue Puppet::Util::Json::ParseError
+        # fall through
       end
 
       message = if @message