Repository navigation
fix: scope W1028 to enclosing Fn::If pins - #4690
Merged
Merged
Conversation
W1028 reported both branches of an Fn::If as unreachable when a condition was used at two independent places in one resource and the resource schema forced condition-scenario enumeration (e.g. an if/then sub-schema). During enumeration the subtree is walked once per scenario with the condition already pinned, so evolving that condition to the other value in fn_if always contradicted the pin. The check became a tautology rather than a reachability question, and every enumerated scenario flagged the branch it was not exploring. Only a branch that contradicts a condition pinned by an *enclosing* Fn::If (structural nesting) is truly dead. Track those pins on the context via fn_if_conditions and gate the W1028 report on membership, so a condition pinned solely by scenario enumeration no longer fires. Verified against real deployments: both condition branches of the reported ECS TaskDefinition template create successfully, confirming neither branch is unreachable. aws-cloudformation#4673
The first cut of the aws-cloudformation#4673 fix keyed W1028 off conditions pinned by an enclosing Fn::If. That wrongly suppressed legitimate findings whenever the condition was forced by another real source -- a resource/output Condition, or a condition that logically implies it (e.g. an And condition). test_cfn_json's vpc.json regressed from 5 findings to 1. Separate forced facts from enumeration assumptions in the conditions context. Conditions.evolve gains assumed=; scenario enumeration (_build_conditions) pins with assumed=True, recording the pin only in cnf (used to prune impossible scenarios) and not in the new hard_cnf. fn_if now reports W1028 only when Conditions.is_reachable -- evaluated against hard_cnf -- says the branch value is impossible, so implication via And/Or conditions is handled correctly while pure enumeration hypotheses are ignored. aws-cloudformation#4673
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4690 +/- ##
=======================================
Coverage 94.64% 94.64%
=======================================
Files 434 434
Lines 15530 15543 +13
Branches 3012 3015 +3
=======================================
+ Hits 14698 14711 +13
Misses 454 454
Partials 378 378
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available
Fixes #4673
Description of changes
W1028reported both branches of anFn::Ifas unreachable when a condition is used at two independent places in one resource and the resource schema forces condition-scenario enumeration. In the reported template,RequiresCompatibilities: [FARGATE]selects a conditional (if/then) sub-schema onAWS::ECS::TaskDefinition, which walks the property subtree once per condition scenario with the condition already pinned.fn_ifchecks each branch's reachability by evolving the condition toTrue/Falseand reportingW1028if that raisesUnsatisfiable. When the enclosing scenario enumeration has already pinned that same condition, evolving it to the other value always contradicts the pin, so the check becomes a tautology rather than a reachability question — and each enumerated scenario flags the branch it is not currently exploring.A branch is only truly unreachable when the condition was pinned by an enclosing
Fn::If(structural nesting), not by ambient scenario enumeration. This change:Context.fn_if_conditions, a frozenset recording conditions pinned by an enclosingFn::Ifas we descend into its branch.W1028report on membership in that set, so a condition pinned solely by scenario enumeration no longer produces a false positive.The existing nested-same-condition case (where an inner
Fn::Ifreuses a condition already fixed by an outerFn::If) still correctly reportsW1028.Validated against real deployments: both condition branches of the reported ECS
TaskDefinitiontemplate create successfully (CREATE_COMPLETE), confirming neither branch is unreachable.Related PRs / Issues
N/A
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT-0 license.