From 9b9df8478bc5568eed92bc7cc754b18657480d1c Mon Sep 17 00:00:00 2001 From: JSup Date: Thu, 16 Jul 2026 07:17:24 +0900 Subject: [PATCH 1/2] Fix discarded case branch validation Inspect the final expression of each case branch when the case result is discarded, so an ineffective conditional is reported even when earlier branch statements have side effects. Signed-off-by: JSup --- lib/puppet/pops/validation/checker4_0.rb | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) 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 From ac0c92640717b670afe1a7c4bd1811ea35376af5 Mon Sep 17 00:00:00 2001 From: JSup Date: Thu, 16 Jul 2026 07:18:39 +0900 Subject: [PATCH 2/2] Add coverage for discarded case branch values Signed-off-by: JSup --- spec/unit/pops/validator/validator_spec.rb | 32 ++++++++++++++++++++++ 1 file changed, 32 insertions(+) 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'",