(maint) Enable rubocop checking for shadowed variables

Previously, we allowed variables to be shadowed by variables
of the same name at local scope. This can make for confusing
reading to someone new to the code (and can potentially indicate
logic errors).

So this commit enables rubocop checking for shadow variables. It
also fixes (by renaming) all extant shadowed variables. Naming
is hard, but a best effort was made.

.rubocop.yml

@@ -37,9 +37,8 @@ Lint/HandleExceptions:
 Lint/LiteralInCondition:
   Enabled: false
 
-# MAYBE useful - but too many instances
 Lint/ShadowingOuterLocalVariable:
-  Enabled: false
+  Enabled: true
 
 # Can catch complicated strings.
 Lint/LiteralInInterpolation:

ext/nagios/check_puppet.rb

@@ -16,7 +16,7 @@ class CheckPuppet
     :interval  => 30,
   }
 
-  o = OptionParser.new do |o|
+  OptionParser.new do |o|
     o.set_summary_indent('  ')
     o.banner =    "Usage: #{script_name} [OPTIONS]"
     o.define_head "The check_puppet Nagios plug-in checks that specified Puppet process is running and the state file is no older than specified interval."

lib/puppet/application/face_base.rb

@@ -121,9 +121,9 @@ def parse_options
 
     # Now we can interact with the default option code to build behaviour
     # around the full set of options we now know we support.
-    @action.options.each do |option|
-      option = @action.get_option(option) # make it the object.
-      self.class.option(*option.optparse) # ...and make the CLI parse it.
+    @action.options.each do |o|
+      o = @action.get_option(o) # make it the object.
+      self.class.option(*o.optparse) # ...and make the CLI parse it.
     end
 
     # ...and invoke our parent to parse all the command line options.

lib/puppet/external/nagios/base.rb

@@ -283,8 +283,8 @@ def to_ldif
     oc.sub!(/Nagios/,'nagios')
     oc.sub!(/::/,'')
     ocs.push oc
-    ocs.each { |oc|
-      str += "objectclass: #{oc}\n"
+    ocs.each { |objclass|
+      str += "objectclass: #{objclass}\n"
     }
     @parameters.each { |name,value|
       next if self.class.suppress.include?(name)

lib/puppet/file_bucket/dipper.rb

@@ -94,10 +94,9 @@ def restore(file, sum)
         end
         ::File.open(file, ::File::WRONLY|::File::TRUNC|::File::CREAT) { |of|
           of.binmode
-          source_stream = newcontents.stream do |source_stream|
+          newcontents.stream do |source_stream|
             FileUtils.copy_stream(source_stream, of)
           end
-          #of.print(newcontents)
         }
         ::File.chmod(changed, file) if changed
       else

lib/puppet/file_serving/mount/pluginfacts.rb

@@ -6,7 +6,7 @@
 class Puppet::FileServing::Mount::PluginFacts < Puppet::FileServing::Mount
   # Return an instance of the appropriate class.
   def find(relative_path, request)
-    return nil unless mod = request.environment.modules.find { |mod|  mod.pluginfact(relative_path) }
+    return nil unless mod = request.environment.modules.find { |m|  m.pluginfact(relative_path) }
 
     path = mod.pluginfact(relative_path)
 

lib/puppet/file_serving/mount/plugins.rb

@@ -6,7 +6,7 @@
 class Puppet::FileServing::Mount::Plugins < Puppet::FileServing::Mount
   # Return an instance of the appropriate class.
   def find(relative_path, request)
-    return nil unless mod = request.environment.modules.find { |mod|  mod.plugin(relative_path) }
+    return nil unless mod = request.environment.modules.find { |m|  m.plugin(relative_path) }
 
     path = mod.plugin(relative_path)
 

lib/puppet/graph/relationship_graph.rb

@@ -126,9 +126,9 @@ def traverse(options = {}, &block)
         if made_progress
           enqueue(*deferred_resources)
         else
-          deferred_resources.each do |resource|
-            overly_deferred_resource_handler.call(resource)
-            finish(resource)
+          deferred_resources.each do |res|
+            overly_deferred_resource_handler.call(res)
+            finish(res)
           end
         end
 

lib/puppet/indirector/catalog/static_compiler.rb

@@ -140,10 +140,10 @@ def get_child_resources(host, catalog, resource, file)
     source = resource[:source]
 
     # This is largely a copy of recurse_remote in File
-    total = file[:source].collect do |source|
-      next unless result = file.perform_recursion(source)
+    total = file[:source].collect do |src|
+      next unless result = file.perform_recursion(src)
       return if top = result.find { |r| r.relative_path == "." } and top.ftype != "directory"
-      result.each { |data| data.source = "#{source}/#{data.relative_path}" }
+      result.each { |data| data.source = "#{src}/#{data.relative_path}" }
       break result if result and ! result.empty? and sourceselect == :first
       result
     end.flatten.compact

lib/puppet/indirector/rest.rb

@@ -85,13 +85,13 @@ def find(request)
     uri, body = Puppet::Network::HTTP::API::V1.request_to_uri_and_body(request)
     uri_with_query_string = "#{uri}?#{body}"
 
-    response = do_request(request) do |request|
+    response = do_request(request) do |req|
       # WEBrick in Ruby 1.9.1 only supports up to 1024 character lines in an HTTP request
       # http://redmine.ruby-lang.org/issues/show/3991
       if "GET #{uri_with_query_string} HTTP/1.1\r\n".length > 1024
-        http_post(request, uri, body, headers)
+        http_post(req, uri, body, headers)
       else
-        http_get(request, uri_with_query_string, headers)
+        http_get(req, uri_with_query_string, headers)
       end
     end
 
@@ -119,8 +119,8 @@ def find(request)
   end
 
   def head(request)
-    response = do_request(request) do |request|
-      http_head(request, Puppet::Network::HTTP::API::V1.request_to_uri(request), headers)
+    response = do_request(request) do |req|
+      http_head(req, Puppet::Network::HTTP::API::V1.request_to_uri(req), headers)
     end
 
     if is_http_200?(response)
@@ -131,8 +131,8 @@ def head(request)
   end
 
   def search(request)
-    response = do_request(request) do |request|
-      http_get(request, Puppet::Network::HTTP::API::V1.request_to_uri(request), headers)
+    response = do_request(request) do |req|
+      http_get(req, Puppet::Network::HTTP::API::V1.request_to_uri(req), headers)
     end
 
     if is_http_200?(response)
@@ -146,8 +146,8 @@ def search(request)
   def destroy(request)
     raise ArgumentError, "DELETE does not accept options" unless request.options.empty?
 
-    response = do_request(request) do |request|
-      http_delete(request, Puppet::Network::HTTP::API::V1.request_to_uri(request), headers)
+    response = do_request(request) do |req|
+      http_delete(req, Puppet::Network::HTTP::API::V1.request_to_uri(req), headers)
     end
 
     if is_http_200?(response)
@@ -161,8 +161,8 @@ def destroy(request)
   def save(request)
     raise ArgumentError, "PUT does not accept options" unless request.options.empty?
 
-    response = do_request(request) do |request|
-      http_put(request, Puppet::Network::HTTP::API::V1.request_to_uri(request), request.instance.render, headers.merge({ "Content-Type" => request.instance.mime }))
+    response = do_request(request) do |req|
+      http_put(req, Puppet::Network::HTTP::API::V1.request_to_uri(req), req.instance.render, headers.merge({ "Content-Type" => req.instance.mime }))
     end
 
     if is_http_200?(response)
@@ -180,7 +180,7 @@ def save(request)
   # to request.do_request from here, thus if we change what we pass or how we
   # get it, we only need to change it here.
   def do_request(request)
-    request.do_request(self.class.srv_service, self.class.server, self.class.port) { |request| yield(request) }
+    request.do_request(self.class.srv_service, self.class.server, self.class.port) { |req| yield(req) }
   end
 
   def validate_key(request)

lib/puppet/interface/face_collection.rb

@@ -27,8 +27,8 @@ def self.get_action_for_face(name, action_name, version)
       # ...we need to search for it bound to an o{lder,ther} version.  Since
       # we load all actions when the face is first references, this will be in
       # memory in the known set of versions of the face.
-      (@faces[name].keys - [ :current ]).sort.reverse.each do |version|
-        break if action = @faces[name][version].get_action(action_name)
+      (@faces[name].keys - [ :current ]).sort.reverse.each do |vers|
+        break if action = @faces[name][vers].get_action(action_name)
       end
     end
 

lib/puppet/module_tool/applications/installer.rb

@@ -57,11 +57,11 @@ def run
         results = { :action => :install, :module_name => name, :module_version => version }
 
         begin
-          if mod = installed_modules[name]
+          if installed_module = installed_modules[name]
             unless forced?
-              if Semantic::VersionRange.parse(version).include? mod.version
+              if Semantic::VersionRange.parse(version).include? installed_module.version
                 results[:result] = :noop
-                results[:version] = mod.version
+                results[:version] = installed_module.version
                 return results
               else
                 changes = Checksummer.run(installed_modules[name].mod.path) rescue []
@@ -135,8 +135,8 @@ def run
           unless forced?
             # Check for module name conflicts.
             releases.each do |rel|
-              if mod = installed_modules_source.by_name[rel.name.split('-').last]
-                next if mod.has_metadata? && mod.forge_name.tr('/', '-') == rel.name
+              if installed_module = installed_modules_source.by_name[rel.name.split('-').last]
+                next if installed_module.has_metadata? && installed_module.forge_name.tr('/', '-') == rel.name
 
                 if rel.name != name
                   dependency = {
@@ -149,8 +149,8 @@ def run
                   :requested_module  => name,
                   :requested_version => options[:version] || 'latest',
                   :dependency        => dependency,
-                  :directory         => mod.path,
-                  :metadata          => mod.metadata
+                  :directory         => installed_module.path,
+                  :metadata          => installed_module.metadata
               end
             end
           end

lib/puppet/module_tool/applications/upgrader.rb

@@ -107,9 +107,9 @@ def installed_release.priority
             add_module_name_constraints_to_graph(graph)
           end
 
-          installed_modules.each do |mod, release|
-            mod = mod.tr('/', '-')
-            next if mod == name
+          installed_modules.each do |installed_module, release|
+            installed_module = installed_module.tr('/', '-')
+            next if installed_module == name
 
             version = release.version
 
@@ -118,7 +118,7 @@ def installed_release.priority
               # we'll place constraints on the graph for each installed
               # module, locking it to upgrades within the same major version.
               ">=#{version} #{version.major}.x".tap do |range|
-                graph.add_constraint('installed', mod, range) do |node|
+                graph.add_constraint('installed', installed_module, range) do |node|
                   Semantic::VersionRange.parse(range).include? node.version
                 end
               end
@@ -127,7 +127,7 @@ def installed_release.priority
                 dep_name = dep['name'].tr('/', '-')
 
                 dep['version_requirement'].tap do |range|
-                  graph.add_constraint("#{mod} constraint", dep_name, range) do |node|
+                  graph.add_constraint("#{installed_module} constraint", dep_name, range) do |node|
                     Semantic::VersionRange.parse(range).include? node.version
                   end
                 end

lib/puppet/module_tool/contents_description.rb

@@ -42,17 +42,17 @@ def data
           end
         end
 
-        type_names.each do |type_name|
-          if type = Puppet::Type.type(type_name.to_sym)
-            type_hash = {:name => type_name, :doc => type.doc}
+        type_names.each do |name|
+          if type = Puppet::Type.type(name.to_sym)
+            type_hash = {:name => name, :doc => type.doc}
             type_hash[:properties] = attr_doc(type, :property)
             type_hash[:parameters] = attr_doc(type, :param)
             if type.providers.size > 0
               type_hash[:providers] = provider_doc(type)
             end
             @data << type_hash
           else
-            Puppet.warning "Could not find/load type: #{type_name}"
+            Puppet.warning "Could not find/load type: #{name}"
           end
         end
       end

lib/puppet/module_tool/installed_modules.rb

@@ -69,11 +69,11 @@ def initialize(source, mod)
         super(source, name, version, {})
 
         if mod.dependencies
-          mod.dependencies.each do |dep|
-            results = Puppet::ModuleTool.parse_module_dependency(release, dep)
+          mod.dependencies.each do |dependency|
+            results = Puppet::ModuleTool.parse_module_dependency(release, dependency)
             dep_name, parsed_range, range = results
 
-            dep.tap do |dep|
+            dependency.tap do |dep|
               add_constraint('initialize', dep_name, range.to_s) do |node|
                 parsed_range === node.version
               end

lib/puppet/network/auth_config_parser.rb

@@ -40,8 +40,8 @@ def parse_rights
     # Verify each of the rights are valid.
     # We let the check raise an error, so that it can raise an error
     # pointing to the specific problem.
-    rights.each { |name, right|
-      right.valid?
+    rights.each { |name, r|
+      r.valid?
     }
     rights
   end

lib/puppet/network/format_handler.rb

@@ -77,16 +77,16 @@ def self.format_to_canonical_name(format)
   # @return [Puppet::Network::Format, nil] the most suitable format
   # @api private
   def self.most_suitable_format_for(accepted, supported)
-    format_name = accepted.collect do |accepted|
-      accepted.to_s.sub(/;q=.*$/, '')
-    end.collect do |accepted|
-      if accepted == ALL_MEDIA_TYPES
+    format_name = accepted.collect do |format|
+      format.to_s.sub(/;q=.*$/, '')
+    end.collect do |format|
+      if format == ALL_MEDIA_TYPES
         supported.first
       else
-        format_to_canonical_name_or_nil(accepted)
+        format_to_canonical_name_or_nil(format)
       end
-    end.find do |accepted|
-      supported.include?(accepted)
+    end.find do |format|
+      supported.include?(format)
     end
 
     if format_name

lib/puppet/network/http/handler.rb

@@ -57,7 +57,7 @@ def process(request, response)
     profiler = configure_profiler(request_headers, request_params)
 
     Puppet::Util::Profiler.profile("Processed request #{request_method} #{request_path}", [:http, request_method, request_path]) do
-      if route = @routes.find { |route| route.matches?(new_request) }
+      if route = @routes.find { |r| r.matches?(new_request) }
         route.process(new_request, new_response)
       else
         raise Puppet::Network::HTTP::Error::HTTPNotFoundError.new("No route for #{new_request.method} #{new_request.path}", HANDLER_NOT_FOUND)

lib/puppet/network/resolver.rb

@@ -31,8 +31,8 @@ def self.each_srv_record(domain, service_name = :puppet, &block)
       # for the specific service.
       each_srv_record(domain, :puppet, &block)
     else
-      each_priority(records) do |priority, records|
-        while next_rr = records.delete(find_weighted_server(records))
+      each_priority(records) do |priority, recs|
+        while next_rr = recs.delete(find_weighted_server(recs))
           Puppet.debug "Yielding next server of #{next_rr.target.to_s}:#{next_rr.port}"
           yield next_rr.target.to_s, next_rr.port
         end

lib/puppet/network/rights.rb

@@ -20,8 +20,8 @@ def is_request_forbidden_and_why?(method, path, params)
                        else
                          [method]
                        end
-    authorization_failure_exceptions = methods_to_check.map do |method|
-      is_forbidden_and_why?(path, params.merge({:method => method}))
+    authorization_failure_exceptions = methods_to_check.map do |m|
+      is_forbidden_and_why?(path, params.merge({:method => m}))
     end
     if authorization_failure_exceptions.include? nil
       # One of the methods we checked is ok, therefore this request is ok.

lib/puppet/parameter.rb

@@ -108,8 +108,8 @@ def doc
           @doc << "\n\n#{vals}"
         end
 
-        if f = self.required_features
-          @doc << "\n\nRequires features #{f.flatten.collect { |f| f.to_s }.join(" ")}."
+        if features = self.required_features
+          @doc << "\n\nRequires features #{features.flatten.collect { |f| f.to_s }.join(" ")}."
         end
         @addeddocvals = true
       end
@@ -542,7 +542,7 @@ def to_s
   #
   def self.format_value_for_display(value)
     if value.is_a? Array
-      formatted_values = value.collect {|value| format_value_for_display(value)}.join(', ')
+      formatted_values = value.collect {|v| format_value_for_display(v)}.join(', ')
       "[#{formatted_values}]"
     elsif value.is_a? Hash
       # Sorting the hash keys for display is largely for having stable

lib/puppet/pops/evaluator/access_operator.rb

@@ -89,11 +89,11 @@ def access_Array(o, scope, keys)
     when 0
       fail(Puppet::Pops::Issues::BAD_ARRAY_SLICE_ARITY, @semantic.left_expr, {:actual => keys.size})
     when 1
-      k = coerce_numeric(keys[0], @semantic.keys[0], scope)
-      unless k.is_a?(Integer)
-        bad_access_key_type(o, 0, k, Integer)
+      key = coerce_numeric(keys[0], @semantic.keys[0], scope)
+      unless key.is_a?(Integer)
+        bad_access_key_type(o, 0, key, Integer)
       end
-      o[k]
+      o[key]
     when 2
       # A slice [from, to] with support for -1 to mean start, or end respectively.
       k1 = coerce_numeric(keys[0], @semantic.keys[0], scope)