diff --git a/crates/wright-analyzer/src/canonical/analysis.rs b/crates/wright-analyzer/src/canonical/analysis.rs index a7957093..d4b1be60 100644 --- a/crates/wright-analyzer/src/canonical/analysis.rs +++ b/crates/wright-analyzer/src/canonical/analysis.rs @@ -261,33 +261,60 @@ fn duplicate_condition_findings( rule: &Rule, value_ids: &HashMap, ) -> Vec { - let mut seen: Vec<&Value> = Vec::new(); + // A repeated condition is unreachable only as a later branch of the same + // `If`/`Else If` chain, so conditions are compared within the chain that + // owns them. Non-`If` blocks still push an entry so their `End` stays + // aligned; a `disabled` action is unwrapped only to detect a block opener. + enum Block<'a> { + IfChain(Vec<&'a Value>), + Other, + } + let mut blocks: Vec = Vec::new(); let mut findings = Vec::new(); for (action_id, action) in rule.actions.iter().enumerate() { - let condition = match action { - Action::If { condition } - | Action::ElseIf { condition } - | Action::While { condition } => condition, - _ => continue, - }; - if seen - .iter() - .any(|previous| values_equal(previous, condition)) - { - let span = program.action_argument_span(rule_id, action_id, 0); - findings.push(Finding { - code: "duplicate-condition".into(), - severity: Severity::Warning, - message: "condition is evaluated more than once in this rule; a later branch can never be taken".into(), - span: span.or_else(|| program.action_span(rule_id, action_id)), - rule: rule_id, - action: Some(action_id), - value: value_ids.get(&(condition as *const Value as usize)).copied(), - evidence: EvidenceClass::Exact, - boundedness: None, - }); - } else { - seen.push(condition); + match action { + Action::ElseIf { condition } => { + let Some(Block::IfChain(seen)) = blocks.last_mut() else { + continue; + }; + if seen + .iter() + .any(|previous| values_equal(previous, condition)) + { + let span = program.action_argument_span(rule_id, action_id, 0); + findings.push(Finding { + code: "duplicate-condition".into(), + severity: Severity::Warning, + message: "an Else If condition repeats an earlier branch's condition in the same If/Else If chain; that branch can never be taken".into(), + span: span.or_else(|| program.action_span(rule_id, action_id)), + rule: rule_id, + action: Some(action_id), + value: value_ids.get(&(condition as *const Value as usize)).copied(), + evidence: EvidenceClass::Exact, + boundedness: None, + }); + } else { + seen.push(condition); + } + } + Action::End => { + blocks.pop(); + } + _ => { + let mut current = action; + while let Action::Disabled { action } = current { + current = action; + } + match current { + Action::If { condition } => { + blocks.push(Block::IfChain(vec![condition])); + } + Action::While { .. } + | Action::ForGlobalVariable { .. } + | Action::ForPlayerVariable { .. } => blocks.push(Block::Other), + _ => {} + } + } } } findings diff --git a/crates/wright-analyzer/src/registry.rs b/crates/wright-analyzer/src/registry.rs index 0e46ef90..cd025122 100644 --- a/crates/wright-analyzer/src/registry.rs +++ b/crates/wright-analyzer/src/registry.rs @@ -304,16 +304,22 @@ impl Default for LintRegistry { id: "duplicate-condition", default_severity: Severity::Warning, evidence: EvidenceClass::Exact, - summary: "condition is evaluated more than once within one rule", + summary: "an Else If repeats a condition earlier in the same If/Else If chain", rationale: "Avoid unreachable or redundant conditional branches.", documentation: concat!( - "The same condition appears in two or more branches of the same rule. ", - "Because Workshop conditions are evaluated sequentially, a later branch ", - "with an identical condition can never be taken.", + "An `Else If` branch repeats the condition of an earlier `If` or ", + "`Else If` branch in the same chain. Workshop evaluates a chain's ", + "conditions in order until one passes, so the repeated branch is ", + "unreachable. Conditions in separate `If` statements, `While` ", + "conditions, and branches of different chains are not compared.", ), known_limits: concat!( - "Detection is structural (not value-flow) and rule-local: two ", - "structurally identical conditions in different rules are not compared.", + "Detection is structural (not value-flow) and chain-local. ", + "Structurally identical conditions built from nondeterministic ", + "values (such as the `Random *` family) are evaluated separately ", + "per branch and can differ even within one chain; Workshop ", + "semantics expose no nondeterminism fact yet, so such repeats ", + "are still reported.", ), tags: &["correctness"], }), diff --git a/crates/wright-analyzer/tests/workshop_integration.rs b/crates/wright-analyzer/tests/workshop_integration.rs index f19842e7..f3409b28 100644 --- a/crates/wright-analyzer/tests/workshop_integration.rs +++ b/crates/wright-analyzer/tests/workshop_integration.rs @@ -193,14 +193,16 @@ rule ("analysis parity") { .unwrap() .contains("condition 2 of 2") })); - assert!( - findings - .iter() - .filter(|finding| finding["code"] == "duplicate-condition") - .count() - >= 2, - "identical If/ElseIf and If/While control-flow conditions are detected" + let duplicates: Vec<_> = findings + .iter() + .filter(|finding| finding["code"] == "duplicate-condition") + .collect(); + assert_eq!( + duplicates.len(), + 1, + "only the in-chain Else If repeats an earlier branch condition" ); + assert_eq!(duplicates[0]["action"], 1); let minimum_waits: Vec<_> = findings .iter() .filter(|finding| finding["code"] == "min-wait-loop") @@ -300,6 +302,267 @@ rule ("analysis parity") { } } +fn duplicate_condition_results(source: &str) -> Vec { + let service = workshop_service_from_text(source); + let findings = query(&service, serde_json::json!({"op": "getFindings"})); + findings + .as_array() + .unwrap() + .iter() + .filter(|finding| finding["code"] == "duplicate-condition") + .cloned() + .collect() +} + +#[test] +fn duplicate_condition_ignores_independent_if_blocks() { + let source = r#" +variables { + global: + 0: index + 1: other +} +rule ("two independent ifs") { + event { + Ongoing - Global; + } + actions { + If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 1); + End; + If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 2); + End; + } +} +"#; + assert!( + duplicate_condition_results(source).is_empty(), + "a second If block is evaluated independently and is reachable" + ); +} + +#[test] +fn duplicate_condition_ignores_while_and_if_across_blocks() { + let source = r#" +variables { + global: + 0: index + 1: other +} +rule ("if and while blocks") { + event { + Ongoing - Global; + } + actions { + While(Compare(Global.index, ==, 0)); + Wait(0.016, Ignore Condition); + End; + If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 1); + End; + While(Compare(Global.index, ==, 0)); + Wait(0.016, Ignore Condition); + End; + } +} +"#; + assert!( + duplicate_condition_results(source).is_empty(), + "While conditions are re-evaluated per iteration and never compared" + ); +} + +#[test] +fn duplicate_condition_flags_repeat_within_one_chain() { + let source = r#" +variables { + global: + 0: index + 1: other +} +rule ("repeated else if") { + event { + Ongoing - Global; + } + actions { + If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 1); + Else If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 2); + End; + } +} +"#; + let findings = duplicate_condition_results(source); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0]["action"], 2); + assert_eq!(findings[0]["evidence"], "exact"); +} + +#[test] +fn duplicate_condition_flags_repeat_against_earlier_chain_branch() { + let source = r#" +variables { + global: + 0: index + 1: other +} +rule ("repeat against first branch") { + event { + Ongoing - Global; + } + actions { + If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 1); + Else If(Compare(Global.index, ==, 1)); + Set Global Variable(other, 2); + Else If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 3); + End; + } +} +"#; + let findings = duplicate_condition_results(source); + assert_eq!(findings.len(), 1); + assert_eq!(findings[0]["action"], 4); +} + +#[test] +fn duplicate_condition_does_not_compare_across_chains() { + let source = r#" +variables { + global: + 0: index + 1: other +} +rule ("same else if in two chains") { + event { + Ongoing - Global; + } + actions { + If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 1); + Else If(Compare(Global.index, ==, 1)); + Set Global Variable(other, 2); + End; + If(Compare(Global.index, ==, 2)); + Set Global Variable(other, 3); + Else If(Compare(Global.index, ==, 1)); + Set Global Variable(other, 4); + End; + } +} +"#; + assert!( + duplicate_condition_results(source).is_empty(), + "identical Else If conditions in different chains are both reachable" + ); +} + +#[test] +fn duplicate_condition_scopes_nested_chains() { + let source = r#" +variables { + global: + 0: index + 1: other +} +rule ("nested chains") { + event { + Ongoing - Global; + } + actions { + If(Compare(Global.index, ==, 0)); + If(Compare(Global.index, ==, 1)); + Set Global Variable(other, 1); + Else If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 2); + End; + Else If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 3); + End; + } +} +"#; + let findings = duplicate_condition_results(source); + assert_eq!( + findings.len(), + 1, + "the inner Else If repeats nothing in its own chain; only the outer \ + Else If repeats the outer If" + ); + assert_eq!(findings[0]["action"], 6); +} + +#[test] +fn duplicate_condition_chain_continues_past_nested_loop() { + let source = r#" +variables { + global: + 0: index + 1: other +} +rule ("loop inside chain body") { + event { + Ongoing - Global; + } + actions { + If(Compare(Global.index, ==, 0)); + While(Compare(Global.index, ==, 1)); + Wait(0.016, Ignore Condition); + End; + Else If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 1); + End; + } +} +"#; + let findings = duplicate_condition_results(source); + assert_eq!( + findings.len(), + 1, + "a loop inside the chain body must not close the chain" + ); + assert_eq!(findings[0]["action"], 4); +} + +#[test] +fn duplicate_condition_tracks_disabled_blocks() { + let source = r#" +variables { + global: + 0: index + 1: other +} +rule ("disabled blocks") { + event { + Ongoing - Global; + } + actions { + disabled If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 1); + End; + If(Compare(Global.index, ==, 0)); + disabled While(Compare(Global.index, ==, 1)); + Wait(0.016, Ignore Condition); + End; + Else If(Compare(Global.index, ==, 0)); + Set Global Variable(other, 2); + End; + } +} +"#; + let findings = duplicate_condition_results(source); + assert_eq!( + findings.len(), + 1, + "disabled blocks still occupy their `End`; only the enabled chain's \ + own repeat is reported" + ); + assert_eq!(findings[0]["action"], 7); +} + #[test] fn while_without_wait_severity_override_applies_to_each_boundedness_class() { let source = r#"