Merge pull request puppetlabs#6998 from kris-bosland/fix/5.5.x/pup-9020

(PUP-9020) Issue deprecation for top level constructs in module files.

lib/puppet/pops/issues.rb

@@ -490,6 +490,14 @@ def self.hard_issue(issue_code, *args, &block)
  _("Unacceptable location. The name '%{name}' is unacceptable in file '%{file}'") % { name: name, file: file }
  end
 
+ ILLEGAL_TOP_CONSTRUCT_LOCATION = issue :ILLEGAL_TOP_CONSTRUCT_LOCATION do
+ if semantic.is_a?(Puppet::Pops::Model::NamedDefinition)
+ _("The %{value} '%{name}' is unacceptable as a top level construct in this location") % { name: semantic.name, value: label(semantic) }
+ else
+ _("This %{value} is unacceptable as a top level construct in this location") % { value: label(semantic) }
+ end
+ end
+
  CAPTURES_REST_NOT_LAST = hard_issue :CAPTURES_REST_NOT_LAST, :param_name do
  _("Parameter $%{param} is not last, and has 'captures rest'") % { param: param_name }
  end

lib/puppet/pops/validation/checker4_0.rb

@@ -48,6 +48,7 @@ def validate(model)
  # tree iterate the model, and call check for each element
  @path = []
  check(model)
+ internal_check_top_construct_in_module(model)
  model._pcore_all_contents(@path) { |element| check(element) }
  end
 
@@ -396,7 +397,7 @@ def check_NamedDefinition(o)
  acceptor.accept(Issues::ILLEGAL_DEFINITION_NAME, o, {:name=>o.name})
  end
 
- internal_check_file_namespace(o, o.name, o.locator.file)
+ internal_check_file_namespace(o)
  internal_check_reserved_type_name(o, o.name)
  internal_check_future_reserved_word(o, o.name)
  end
@@ -535,18 +536,40 @@ def internal_check_future_reserved_word(o, name)
  NO_NAMESPACE = :no_namespace
  NO_PATH = :no_path
  BAD_MODULE_FILE = :bad_module_file
- def internal_check_file_namespace(o, name, file)
+
+ def internal_check_file_namespace(o)
+ file = o.locator.file
  return if file.nil? || file == '' #e.g. puppet apply -e '...'
 
  file_namespace = namespace_for_file(file)
  return if file_namespace == NO_NAMESPACE
 
  # Downcasing here because check is case-insensitive
- if file_namespace == BAD_MODULE_FILE || !name.downcase.start_with?(file_namespace)
- acceptor.accept(Issues::ILLEGAL_DEFINITION_LOCATION, o, {:name => name, :file => file})
+ if file_namespace == BAD_MODULE_FILE || !o.name.downcase.start_with?(file_namespace)
+ acceptor.accept(Issues::ILLEGAL_DEFINITION_LOCATION, o, {:name => o.name, :file => file})
  end
  end
 
+ def internal_check_top_construct_in_module(prog)
+ return unless prog.is_a?(Model::Program) && !prog.body.nil?
+
+ #Check that this is a module autoloaded file
+ file = prog.locator.file
+ return if file.nil?
+ return if namespace_for_file(file) == NO_NAMESPACE
+
+ body = prog.body
+ if(body.is_a?(Model::BlockExpression))
+ body.statements.each { |s| acceptor.accept(Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION, s) unless valid_top_construct?(s) }
+ else
+ acceptor.accept(Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION, body) unless valid_top_construct?(body)
+ end
+ end
+
+ def valid_top_construct?(o)
+ o.is_a?(Model::Definition) && !o.is_a?(Model::NodeDefinition)
+ end
+
  # @api private
  class Puppet::Util::FileNamespaceAdapter < Puppet::Pops::Adaptable::Adapter
  attr_accessor :file_to_namespace

lib/puppet/pops/validation/validator_factory_4_0.rb

@@ -37,6 +37,7 @@ def severity_producer
  p[Issues::EMPTY_RESOURCE_SPECIALIZATION] = :ignore
  p[Issues::CLASS_NOT_VIRTUALIZABLE] = Puppet[:strict] == :off ? :warning : Puppet[:strict]
  p[Issues::ILLEGAL_DEFINITION_LOCATION] = :deprecation
+ p[Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION] = :deprecation
  p
  end
 end

spec/unit/pops/validator/validator_spec.rb

@@ -145,6 +145,16 @@ def with_environment(environment, env_params = {})
  expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
  end
  end
+
+ it 'produces a deprecation for illegal top level constructs' do
+ with_environment(environment) do
+ acceptor = validate(parse('$foo = 1', 'path/aaa/manifests/bbb.pp'))
+ expect(deprecation_count(acceptor)).to eql(1)
+ expect(acceptor.warning_count).to eql(1)
+ expect(acceptor.error_count).to eql(0)
+ expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
+ end
+ end
  end
 
  context 'with --strict set to warning' do
@@ -179,6 +189,16 @@ def with_environment(environment, env_params = {})
  expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
  end
  end
+
+ it 'produces a deprecation for illegal top level constructs' do
+ with_environment(environment) do
+ acceptor = validate(parse('$foo = 1', 'path/aaa/manifests/bbb.pp'))
+ expect(deprecation_count(acceptor)).to eql(1)
+ expect(acceptor.warning_count).to eql(1)
+ expect(acceptor.error_count).to eql(0)
+ expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
+ end
+ end
  end
 
  context 'with --strict set to error' do
@@ -220,6 +240,16 @@ def with_environment(environment, env_params = {})
  expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
  end
  end
+
+ it 'produces a deprecation for illegal top level constructs' do
+ with_environment(environment) do
+ acceptor = validate(parse('$foo = 1', 'path/aaa/manifests/bbb.pp'))
+ expect(deprecation_count(acceptor)).to eql(1)
+ expect(acceptor.warning_count).to eql(1)
+ expect(acceptor.error_count).to eql(0)
+ expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
+ end
+ end
  end
 
  context 'with --strict set to off' do
@@ -240,6 +270,16 @@ def with_environment(environment, env_params = {})
  expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_DEFINITION_LOCATION)
  end
  end
+
+ it 'produces a deprecation for illegal top level constructs' do
+ with_environment(environment) do
+ acceptor = validate(parse('$foo = 1', 'path/aaa/manifests/bbb.pp'))
+ expect(deprecation_count(acceptor)).to eql(1)
+ expect(acceptor.warning_count).to eql(1)
+ expect(acceptor.error_count).to eql(0)
+ expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
+ end
+ end
  end
 
  context 'irrespective of --strict' do
@@ -760,6 +800,71 @@ def issue(at_top)
  end
  end
 
+ context 'top level constructs' do
+ {
+ 'a class' => 'class x{}',
+ 'a define' => 'define x{}',
+ 'a function' => 'function x() {}',
+ 'a type alias' => 'type A = Data',
+ 'a type alias for a complex type' => 'type C = Hash[String[1],Integer]',
+ 'a type definition' => 'type A {}',
+ }.each_pair do |word, source|
+ it "will not have an issue with #{word} at the top level in a module" do
+ with_environment(environment) do
+ expect(validate(parse(source, 'path/x/manifests/init.pp'))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
+ end
+ end
+ end
+ end
+
+ context 'non-top level constructs' do
+ {
+ 'an assignment' => '$foo = 1',
+ 'a resource' => 'notify { nope: }',
+ 'a resource default' => "Notify { message => 'yo' }",
+ 'a function call' => "include 'foo'",
+ 'a node definition' => 'node default {}',
+ 'an expression' => '1+1',
+ 'a conditional' => 'if true {42}',
+ 'a literal value' => '42',
+ 'a virtual collector' => 'User <| tag == web |>',
+ 'an exported collector' => 'Sshkey <<| |>>',
+ }.each_pair do |word, source|
+ it "will have an issue with #{word} at the top level in a module" do
+ with_environment(environment) do
+ expect(validate(parse(source, 'path/x/manifests/init.pp'))).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
+ end
+ end
+
+ it "will not have an issue with #{word} at top level not in a module" do
+ with_environment(environment) do
+ expect(validate(parse(source))).not_to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
+ end
+ end
+ end
+
+ it "will give multiple errors in one file with multiple issues" do
+ source = <<-SOURCE
+ class foo {}
+ notify { nope: }
+ node bar {}
+
+ $a = 7
+ SOURCE
+
+ with_environment(environment) do
+ acceptor = validate(parse(source, 'path/foo/manifests/init.pp'))
+ expect(deprecation_count(acceptor)).to eql(3)
+ expect(acceptor.warning_count).to eql(3)
+ expect(acceptor.error_count).to eql(0)
+
+ expect(acceptor.warnings[0].source_pos.line).to eql(2)
+ expect(acceptor.warnings[1].source_pos.line).to eql(3)
+ expect(acceptor.warnings[2].source_pos.line).to eql(5)
+ end
+ end
+ end
+
  context "capability annotations" do
  ['produces', 'consumes'].each do |word|
  it "rejects illegal resource types in #{word} clauses" do