(PUP-12026) Change non-literal type check to deprecation warning

Previously, the ILLEGAL_NONLITERAL_PARAMETER_TYPE issue was under control of the
`strict` setting. Since `strict` defaults to `error`, `puppet parser validate`
logged an error message and exited non-zero for non-literal types such as
`Integer[0, 60*24]`

In order to gradually phase out non-literal parameter types, this commit changes
the issue to be a deprecation. When parsing and validating puppet code a warning
is now logged, regardless of `strict`, and it exits with 0:

    Warning: The parameter '$i' must be a literal type, not a ...
    (file: /etc/puppetlabs/code/environments/production/manifests/site.pp,
    line: 2, column: 19)

As a result of this change, the ClassInfoService error does not include locator
information in the error hash:

    {"error":"The parmeter '$i' is invalid: The expression <60*24> is not a valid type specification"}

But the location information is included in the deprecation warning logged on
the server.

All language deprecation warnings can already be disabled via the
`disable_warnings=deprecations` setting. In a future major release, the
non-literal type issue will be converted to an error.

This pattern follows how we handled the most recent language checks:

ILLEGAL_DEFINITION_LOCATION: see 6e7835b, 44c4ea0 and 81e61de
ILLEGAL_TOP_CONSTRUCT_LOCATION: see 5661509 and 81e61de

(cherry picked from commit 86a8c14)

Author: joshcooper

lib/puppet/info_service/class_information_service.rb

@@ -74,7 +74,7 @@ def extract_param(p)
 
   def extract_type(structure, p)
     return structure if p.type_expr.nil?
-    structure[:type] = typeexpr_to_string(p.type_expr)
+    structure[:type] = typeexpr_to_string(p.name, p.type_expr)
     structure
   end
 
@@ -87,12 +87,11 @@ def extract_default(structure, p)
     structure
   end
 
-  def typeexpr_to_string(type_expr)
+  def typeexpr_to_string(name, type_expr)
     begin
       type_parser.interpret_any(type_expr, nil).to_s
-    rescue Puppet::ParseError
-      # type is to complex - contains expressions that are not literal
-      nil
+    rescue Puppet::ParseError => e
+      raise Puppet::Error, "The parameter '$#{name}' is invalid: #{e.message}", e.backtrace
     end
   end
 

lib/puppet/pops/validation/validator_factory_4_0.rb

@@ -37,7 +37,7 @@ def severity_producer
     p[Issues::EMPTY_RESOURCE_SPECIALIZATION]  = :ignore
     p[Issues::CLASS_NOT_VIRTUALIZABLE]        = :error
 
-    p[Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE] = Puppet[:strict] == :off ? :ignore : Puppet[:strict]
+    p[Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE] = :deprecation
     p
   end
 end

spec/lib/puppet_spec/matchers.rb

@@ -14,6 +14,12 @@
   end
 end
 
+RSpec::Matchers.define :have_matching_log_with_source do |expected, file, line, pos|
+  match do |actual|
+    actual.any? { |item| item.message =~ expected && item.file == file && item.line == line && item.pos == pos }
+  end
+end
+
 RSpec::Matchers.define :exit_with do |expected|
   actual = nil
   match do |block|

spec/unit/info_service_spec.rb

@@ -518,20 +518,23 @@ class oops_2(Optional[[String]] $double_brackets) { }
      it "warns with a descriptive message if non-literal class parameter is given" do
        files = ['non_literal.pp', 'non_literal_2.pp'].map {|f| File.join(code_dir, f) }
        Puppet::InfoService.classes_per_environment({'production' => files })
-       expect(@logs).to include(an_object_having_attributes(message: "The parameter '$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression"))
-       expect(@logs).to include(an_object_having_attributes(message: "The parameter '$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression"))
+       expect(@logs).to have_matching_log_with_source(/The parameter '\$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression/, "#{code_dir}/non_literal.pp", 1, 37)
+       expect(@logs).to have_matching_log_with_source(/The parameter '\$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression/, "#{code_dir}/non_literal_2.pp", 1, 44)
      end
 
      it "errors in strict mode if non-literal class parameter is given" do
        Puppet[:strict] = "error"
        files = ['non_literal.pp', 'non_literal_2.pp'].map {|f| File.join(code_dir, f) }
        result = Puppet::InfoService.classes_per_environment({'production' => files })
+       expect(@logs).to have_matching_log_with_source(/The parameter '\$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression/, "#{code_dir}/non_literal.pp", 1, 37)
+       expect(@logs).to have_matching_log_with_source(/The parameter '\$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression/, "#{code_dir}/non_literal_2.pp", 1, 44)
+
        expect(result).to eq({
         "production"=>{
            "#{code_dir}/non_literal.pp" =>
-           {:error=> "The parameter '$bad_int' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: #{code_dir}/non_literal.pp, line: 1, column: 37)"},
+           {:error=> "The parameter '\$bad_int' is invalid: The expression <1-3> is not a valid type specification."},
            "#{code_dir}/non_literal_2.pp" =>
-           {:error=> "The parameter '$double_brackets' must be a literal type, not a Puppet::Pops::Model::AccessExpression (file: #{code_dir}/non_literal_2.pp, line: 1, column: 44)"}
+           {:error=> "The parameter '\$double_brackets' is invalid: The expression <Optional[[String]]> is not a valid type specification."}
           } # end production env
         })
      end

spec/unit/pops/evaluator/literal_evaluator_spec.rb

@@ -21,9 +21,9 @@
     'Integer[-1]' => [-1],
     'Integer[-5, -1]' => [-5, -1],
     'Integer[-5, 5]'  => [-5, 5],
-    # we can't actually represent MIN_INTEGER because it's glexed as
+    # we can't actually represent MIN_INTEGER below, because it's lexed as
     # UnaryMinusExpression containing a positive LiteralInteger and the integer
-    # must be <= MAX_INTEGER
+    # must be <= MAX_INTEGER. Therefore, the effective minimum is one greater.
     "Integer[#{Puppet::Pops::MIN_INTEGER + 1}]" => [-0x7FFFFFFFFFFFFFFF],
     "Integer[0, #{Puppet::Pops::MAX_INTEGER}]"  => [0, 0x7FFFFFFFFFFFFFFF],
     'Integer[0, default]'         => [0, :default],

spec/unit/pops/validator/validator_spec.rb

@@ -212,13 +212,6 @@ def with_environment(environment, env_params = {})
         expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
       end
     end
-
-    it 'produces a warning for non-literal class parameters' do
-      acceptor = validate(parse('class test(Integer[2-1] $port) {}'))
-      expect(acceptor.warning_count).to eql(1)
-      expect(acceptor.error_count).to eql(0)
-      expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE)
-    end
   end
 
   context 'with --strict set to error' do
@@ -270,13 +263,6 @@ def with_environment(environment, env_params = {})
         expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
       end
     end
-
-    it 'produces an error for non-literal class parameters' do
-      acceptor = validate(parse('class test(Integer[2-1] $port) {}'))
-      expect(acceptor.warning_count).to eql(0)
-      expect(acceptor.error_count).to eql(1)
-      expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE)
-    end
   end
 
   context 'with --strict set to off' do
@@ -307,13 +293,6 @@ def with_environment(environment, env_params = {})
         expect(acceptor).to have_issue(Puppet::Pops::Issues::ILLEGAL_TOP_CONSTRUCT_LOCATION)
       end
     end
-
-    it 'does not produce an error or warning for non-literal class parameters' do
-      acceptor = validate(parse('class test(Integer[2-1] $port) {}'))
-      expect(acceptor.warning_count).to eql(0)
-      expect(acceptor.error_count).to eql(0)
-      expect(acceptor).to_not have_issue(Puppet::Pops::Issues::ILLEGAL_NONLITERAL_PARAMETER_TYPE)
-    end
   end
 
   context 'irrespective of --strict' do
@@ -344,6 +323,14 @@ def with_environment(environment, env_params = {})
       expect(acceptor.error_count).to eql(1)
       expect(acceptor).to have_issue(Puppet::Pops::Issues::CLASS_NOT_VIRTUALIZABLE)
     end
+
+    it 'produces a deprecation warning for non-literal class parameters' do
+      acceptor = validate(parse('class test(Integer[2-1] $port) {}'))
+      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_NONLITERAL_PARAMETER_TYPE)
+    end
   end
 
   context 'with --tasks set' do