diff --git a/lib/puppet/pops/validation/checker4_0.rb b/lib/puppet/pops/validation/checker4_0.rb index 995281e242..3e18d67f99 100644 --- a/lib/puppet/pops/validation/checker4_0.rb +++ b/lib/puppet/pops/validation/checker4_0.rb @@ -144,6 +144,20 @@ def ends_with_idem(o) end end + # Returns an idempotent expression whose value is discarded when +o+ is used as a statement. + # A case expression can have side effects before its branch result, so inspect each branch tail + # even when the case expression as a whole is not idempotent. + def discarded_value_expression(o) + return o if idem(o) + return nil unless o.is_a?(Model::CaseExpression) + + o.options.each do |option| + violator = ends_with_idem(option.then_expr) + return violator if violator + end + nil + end + #---ASSIGNMENT CHECKS def assign_VariableExpression(o, via_index) @@ -269,8 +283,9 @@ def check_BlockExpression(o) acceptor.accept(Issues::RESOURCE_WITHOUT_TITLE, o, :name => o.statements[0].value) else o.statements[0..-2].each do |statement| - if idem(statement) - acceptor.accept(Issues::IDEM_EXPRESSION_NOT_LAST, statement) + violator = discarded_value_expression(statement) + if violator + acceptor.accept(Issues::IDEM_EXPRESSION_NOT_LAST, violator) break # only flag the first end end diff --git a/spec/unit/pops/validator/validator_spec.rb b/spec/unit/pops/validator/validator_spec.rb index d8a7d2e1bb..d6e5766153 100644 --- a/spec/unit/pops/validator/validator_spec.rb +++ b/spec/unit/pops/validator/validator_spec.rb @@ -499,6 +499,38 @@ def with_environment(environment, env_params = {}) end context 'for non productive expressions' do + it 'flags an ineffective conditional at the end of a discarded case branch' do + source = <<~'PUPPET' + case 'Ubuntu' { + 'Ubuntu': { + $packages = if true { ['base'] } else { [] } + if true { + $packages << 'extra' + } + } + } + $x = 1 + PUPPET + + expect(validate(parse(source))).to have_issue(Puppet::Pops::Issues::IDEM_EXPRESSION_NOT_LAST) + end + + it 'allows a final value when the case result is used' do + source = <<~PUPPET + class example { + $packages = case $os_name { + 'Ubuntu': { + $base = ['base'] + if $virtual { $base + ['vm'] } else { $base } + } + default: { [] } + } + notice($packages) + } + PUPPET + + expect(validate(parse(source))).not_to have_issue(Puppet::Pops::Issues::IDEM_EXPRESSION_NOT_LAST) + end [ '1', '3.14', "'a'",