From 4653a5f9f09ca831c24d92a61acee88c1949e368 Mon Sep 17 00:00:00 2001 From: Ian Kronquist Date: Wed, 8 Jul 2015 16:12:04 -0700 Subject: [PATCH] (PDOC-21) Typecheck documented parameters The documented parameter types should be compared with the actual types when possible. --- .../yard/code_objects/defined_type_object.rb | 2 + .../yard/code_objects/host_class_object.rb | 2 + .../code_objects/puppet_namespace_object.rb | 2 + .../yard/handlers/host_class_handler.rb | 17 +++++ .../handlers/puppet_4x_function_handler.rb | 48 +++++++++----- .../yard/templates/default/hostclass/setup.rb | 4 ++ .../default/puppetnamespace/setup.rb | 1 + .../yard/templates/default/template_helper.rb | 57 ++++++++++++++++ .../yard/puppet_4x_function_handler_spec.rb | 8 +-- .../strings/yard/template_helper_spec.rb | 66 +++++++++++++++++++ 10 files changed, 185 insertions(+), 22 deletions(-) diff --git a/lib/puppet_x/puppetlabs/strings/yard/code_objects/defined_type_object.rb b/lib/puppet_x/puppetlabs/strings/yard/code_objects/defined_type_object.rb index 24f6894..fba8cef 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/code_objects/defined_type_object.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/code_objects/defined_type_object.rb @@ -2,4 +2,6 @@ class PuppetX::PuppetLabs::Strings::YARD::CodeObjects::DefinedTypeObject < Puppe # A list of parameters attached to this class. # @return [Array] attr_accessor :parameters + attr_accessor :type_info + end diff --git a/lib/puppet_x/puppetlabs/strings/yard/code_objects/host_class_object.rb b/lib/puppet_x/puppetlabs/strings/yard/code_objects/host_class_object.rb index 0fc1c53..4b7e0ec 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/code_objects/host_class_object.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/code_objects/host_class_object.rb @@ -2,6 +2,8 @@ class PuppetX::PuppetLabs::Strings::YARD::CodeObjects::HostClassObject < PuppetX # The {HostClassObject} that this class inherits from, if any. # @return [HostClassObject, Proxy, nil] attr_accessor :parent_class + attr_accessor :type_info + # NOTE: `include_mods` is never used as it makes no sense for Puppet, but # this is called by `YARD::Registry` and it will pass a parameter. diff --git a/lib/puppet_x/puppetlabs/strings/yard/code_objects/puppet_namespace_object.rb b/lib/puppet_x/puppetlabs/strings/yard/code_objects/puppet_namespace_object.rb index 0497269..8fe2329 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/code_objects/puppet_namespace_object.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/code_objects/puppet_namespace_object.rb @@ -5,6 +5,8 @@ class PuppetX::PuppetLabs::Strings::YARD::CodeObjects::PuppetNamespaceObject < Y [self] end + attr_accessor :type_info + # FIXME: We used to override `self.new` to ensure no YARD proxies were # created for namespaces segments that did not map to a host class or # defined type. Fighting the system in this way turned out to be diff --git a/lib/puppet_x/puppetlabs/strings/yard/handlers/host_class_handler.rb b/lib/puppet_x/puppetlabs/strings/yard/handlers/host_class_handler.rb index af7a4ef..08bd6f9 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/handlers/host_class_handler.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/handlers/host_class_handler.rb @@ -8,6 +8,23 @@ class PuppetX::PuppetLabs::Strings::YARD::Handlers::HostClassHandler < PuppetX:: param_tuple << ( a[1].nil? ? nil : a[1].source ) end end + tp = Puppet::Pops::Types::TypeParser.new + param_type_info = {} + statement.pops_obj.parameters.each do |pop_param| + # If the parameter's type expression is nil, default to Any + if pop_param.type_expr == nil + param_type_info[pop_param.name] = Puppet::Pops::Types::TypeFactory.any() + else + begin + param_type_info[pop_param.name] = tp.interpret_any(pop_param.type_expr) + rescue Puppet::ParseError + # If the type could not be interpreted, default to type Any + param_type_info[pop_param.name] = "TypeError - " + + "#{pop_param.type_expr} isn't a valid type." + end + end + end + obj.type_info = [param_type_info] statement.pops_obj.tap do |o| if o.parent_class diff --git a/lib/puppet_x/puppetlabs/strings/yard/handlers/puppet_4x_function_handler.rb b/lib/puppet_x/puppetlabs/strings/yard/handlers/puppet_4x_function_handler.rb index cd373d9..14fd54c 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/handlers/puppet_4x_function_handler.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/handlers/puppet_4x_function_handler.rb @@ -52,19 +52,24 @@ class Puppet4xFunctionHandler < YARD::Handlers::Ruby::Base obj = MethodObject.new(function_namespace, name) do |o| end - # overload_signatures is a array of arrays of tuples or empty: + # The data structure for overload_signatures is an array of hashes. Each + # hash represents the arguments a single function dispatch (aka overload) + # can take. # overload_signatures = [ - # [ # First function dispatch arguments + # { # First function dispatch arguments # # argument name, argument type - # ['arg0', 'Variant[String,Array[String]]' - # ['arg1', 'Optional[Type]'] - # ], - # [ # Second function dispatch arguments - # ['arg0', 'Variant[String,Array[String]]' - # ['arg1', 'Optional[Type]'] - # ['arg2', 'Any'] - # ] + # 'arg0': 'Variant[String,Array[String]]', + # 'arg1': 'Optional[Type]' + # }, + # { # Second function dispatch arguments + # 'arg0': 'Variant[String,Array[String]]', + # 'arg1': 'Optional[Type]', + # 'arg2': 'Any' + # } # ] + # Note that the order for arguments to a function doesn't actually matter + # because we allow users flexibility when listing their arguments in the + # comments. overload_signatures = [] statement.traverse do |node| # Find all of the dispatch methods @@ -75,16 +80,23 @@ class Puppet4xFunctionHandler < YARD::Handlers::Ruby::Base if do_block == command next end + signature = {} # Iterate through each of the children of the do block and build # tuples of parameter names and parameter type signatures do_block.children.first.children.each do |child| - overload_signatures <<= [extract_type_from_command(child)] + name, type = extract_type_from_command(child) + # This can happen if there is a function or something we aren't + # expecting. + if name != nil and type != nil + signature[name] = type + end end + overload_signatures <<= signature end end - # If the overload_signatures array is empty because we couldn't find any - # dispatch blocks, then there must be one method named the same as the + # If the overload_signatures list is empty because we couldn't find any + # dispatch blocks, then there must be one function named the same as the # name of the function being created. if overload_signatures.length == 0 statement.traverse do |node| @@ -92,7 +104,7 @@ class Puppet4xFunctionHandler < YARD::Handlers::Ruby::Base # function being created. if (node.type == :def and node.children.first.type == :ident and node.children.first.source == obj.name.to_s) - signature = [] + signature = {} # Find its parameters. If they don't exist, fine params = node.jump :params break if params == node @@ -100,7 +112,7 @@ class Puppet4xFunctionHandler < YARD::Handlers::Ruby::Base if param.type == :ident # The parameters of Puppet functions with no defined dispatch are # as though they are Any type. - signature << [param[0], 'Any'] + signature[param[0]] = 'Any' end end overload_signatures <<= signature @@ -114,9 +126,9 @@ class Puppet4xFunctionHandler < YARD::Handlers::Ruby::Base # at the docstring. obj.type_info = overload_signatures - # The yard docstring parser expects a array of arrays, not a array of - # arrays of arrays. - obj.parameters = overload_signatures.flatten(1) + # The yard docstring parser expects a list of lists, not a list of lists of + # lists. + obj.parameters = overload_signatures.map { |sig| sig.to_a }.flatten(1) obj['puppet_4x_function'] = true diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/hostclass/setup.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/hostclass/setup.rb index f640d5b..b88dbff 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/hostclass/setup.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/hostclass/setup.rb @@ -6,6 +6,10 @@ def init @template_helper = TemplateHelper.new @template_helper.check_parameters_match_docs object + params = object.parameters.map { |param| param.first } + param_tags = object.tags.find_all{ |tag| tag.tag_name == "param"} + param_details = @template_helper.extract_param_details(params, param_tags) unless params.nil? + @template_helper.check_types_match_docs object, param_details end def subclasses diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb index 054cee2..b3e7e9a 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/puppetnamespace/setup.rb @@ -76,6 +76,7 @@ def method_details_list params = parameters.nil? ? nil : parameters[1].split(/\s*,\s*/) param_details = @template_helper.extract_param_details(params, param_tags) unless params.nil? + @template_helper.check_types_match_docs object, param_details else param_details = @template_helper.comment_only_param_details(param_tags) end diff --git a/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb b/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb index 64b236c..ac4a920 100644 --- a/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb +++ b/lib/puppet_x/puppetlabs/strings/yard/templates/default/template_helper.rb @@ -105,6 +105,63 @@ class TemplateHelper parameter_info end + # Check that any types specified in the docstrings match the actual method + # types. This is used by puppet 4x functions and defined types. + # @param object the code object to examine for parameters names + def check_types_match_docs(object, params_hash) + # We'll need this to extract type info from the type specified by the + # docstring. + type_parser = Puppet::Pops::Types::TypeParser.new + type_calculator = Puppet::Pops::Types::TypeCalculator.new + + object.type_info.each do |function| + function.keys.each do |key| + if function[key].class == String + begin + instantiated = type_parser.parse function[key].gsub(/'/, '').gsub(/"/, "") + rescue Puppet::ParseError + # Likely the result of a malformed type + next + end + else + instantiated = function[key] + end + params_hash.each do |param| + if param[:name] == key and param[:types] != nil + param[:types].each do |type| + param_instantiated = type_parser.parse type + if not type_calculator.assignable? instantiated, param_instantiated + actual_types = object.type_info.map do |sig| + sig[key] + end + # Get the locations where the object can be found. We only care about + # the first one. + locations = object.files + # If the locations aren't in the shape we expect then report that + # the file number couldn't be determined. + if locations.length >= 1 and locations[0].length == 2 + file = locations[0][0] + line = locations[0][1] + warning = "@param tag types do not match the code. The " + + "#{param[:name]} parameter is declared as types #{param[:types]} in " + + "the docstring, but the code specifies the types " + + "#{actual_types.inspect} in file #{file} near line #{line}" + else + warning = "@param tag types do not match the code. The " + + "#{param[:name]} parameter is declared as types #{param[:types]} in " + + "the docstring, but the code specifies the types " + + "#{actual_types.inspect} Sorry, the file and line number could" + + "not be determined." + end + log.warn warning + end + end + end + end + end + end + end + # Check that the actual function parameters match what is stated in the docs. # If there is a mismatch, print a warning to stderr. # This is necessary for puppet classes and defined types. This type of diff --git a/spec/unit/puppet_x/puppetlabs/strings/yard/puppet_4x_function_handler_spec.rb b/spec/unit/puppet_x/puppetlabs/strings/yard/puppet_4x_function_handler_spec.rb index f6adb15..51f5f5f 100644 --- a/spec/unit/puppet_x/puppetlabs/strings/yard/puppet_4x_function_handler_spec.rb +++ b/spec/unit/puppet_x/puppetlabs/strings/yard/puppet_4x_function_handler_spec.rb @@ -129,8 +129,8 @@ describe PuppetX::PuppetLabs::Strings::YARD::Handlers::Puppet4xFunctionHandler d "parameter name: also_not_a_param \n in file `(stdin)' near line 3" expect { parse <<-RUBY - # @param not_a_param Integer the first number to be compared - # @param also_not_a_param Integer the second number to be compared + # @param not_a_param [Integer] the first number to be compared + # @param also_not_a_param [Integer] the second number to be compared Puppet::Functions.create_function(:max) do dispatch max_1 do param 'Integer[1,2]', :num_a @@ -158,8 +158,8 @@ describe PuppetX::PuppetLabs::Strings::YARD::Handlers::Puppet4xFunctionHandler d "docstring in dispatch method" do expect { parse <<-RUBY - # @param num_a Integer the first number to be compared - # @param num_b Integer the second number to be compared + # @param [Integer] num_a the first number to be compared + # @param num_b [Integer] the second number to be compared Puppet::Functions.create_function(:max) do dispatch max_1 do param 'Integer', :num_a diff --git a/spec/unit/puppet_x/puppetlabs/strings/yard/template_helper_spec.rb b/spec/unit/puppet_x/puppetlabs/strings/yard/template_helper_spec.rb index 1f3a4b4..db02849 100644 --- a/spec/unit/puppet_x/puppetlabs/strings/yard/template_helper_spec.rb +++ b/spec/unit/puppet_x/puppetlabs/strings/yard/template_helper_spec.rb @@ -56,4 +56,70 @@ describe TemplateHelper do obj2.docstring.instance_variable_set("@tags", []) end + it "should issue a warning if the parameter types do not match the docstring in dispatch method" do + expected_output_not_a_param = "[warn]: @param tag types do not match the" + + " code. The arg1 parameter is declared as types [\"Integer\"] in the " + + "docstring, but the code specifies the types [\"Optional[String]\"] " + + "in file test near line 0\n" + object = PuppetX::PuppetLabs::Strings::YARD::CodeObjects::PuppetNamespaceObject.new(:root, 'Puppet4xFunctions') + object.files = [['test', 0]] + object.type_info = [{ + 'arg0' => 'Variant[String,Array[String]]', + 'arg1' => 'Optional[String]' + }] + param_details = [{ + :name => 'arg0', + :types => ['Variant[String,Array[String]]'] + }, + { + :name => 'arg1', + :types => ['Integer'] + }] + template_helper = TemplateHelper.new + expect { + template_helper.check_types_match_docs(object, param_details) + }.to output(expected_output_not_a_param).to_stdout_from_any_process + end + + it "should not issue a warning if the parameter types do match the docstring in dispatch method" do + object = PuppetX::PuppetLabs::Strings::YARD::CodeObjects::PuppetNamespaceObject.new(:root, 'Puppet4xFunctions') + object.files = [['test', 0]] + object.type_info = [{ + 'arg0' => 'Variant[String,Array[String]]', + 'arg1' => 'Optional[String]' + }] + param_details = [{ + :name => 'arg0', + :types => ['Variant[String,Array[String]]'] + }, + { + :name => 'arg1', + :types => ['Optional[String]'] + }] + template_helper = TemplateHelper.new + expect { + template_helper.check_types_match_docs(object, param_details) + }.to output("").to_stdout_from_any_process + end + + it "should not issue a warning if the types in the docstring in dispatch method are assignable to parameter types" do + object = PuppetX::PuppetLabs::Strings::YARD::CodeObjects::PuppetNamespaceObject.new(:root, 'Puppet4xFunctions') + object.files = [['test', 0]] + object.type_info = [{ + 'arg0' => 'Variant[String,Array[String]]', + 'arg1' => 'Optional[String]' + }] + param_details = [{ + :name => 'arg0', + :types => ['Variant[String,Array[String]]'] + }, + { + :name => 'arg1', + :types => ['String'] + }] + template_helper = TemplateHelper.new + expect { + template_helper.check_types_match_docs(object, param_details) + }.to output("").to_stdout_from_any_process + end end