Merge pull request #9295 from joshcooper/deprecate_nonliteral_expressions

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

Author: AriaXLi

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
 
@@ -88,11 +88,10 @@ def extract_default(structure, p)
     structure
   end
 
-  def typeexpr_to_string(type_expr)
+  def typeexpr_to_string(name, type_expr)
     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
 
   def value_as_literal(value_expr)

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,12 +518,15 @@ class oops_2(Optional[[String]] $double_brackets) { }
      it "errors 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) }
        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

@@ -211,13 +211,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
@@ -269,13 +262,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
@@ -306,13 +292,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
@@ -343,6 +322,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