fix(opt-report): a rule spanning two tiers reports two buckets instead of panicking (#7234)

changelog.d/7808-opt-report-two-tiers.md

@@ -0,0 +1,11 @@
+**`--opt-report` no longer aborts the compiler when one rule spans two actionability tiers** (#7234).
+
+`rule_buckets` keyed on `(analysis, rule)` and `debug_assert`ed that a rule never carries two tiers, reasoning that "rule-to-tier is 1:1 because both come from one `ShapeDenial` constant". That reads the relation backwards: **many constants share one rule NAME**, and they do not agree on tier. `rule 2 (containment)` alone spans both — `ESC_REASSIGNED` and `ESC_CLOSURE_CAPTURE` are `Fixable`; `ESC_CALL_ARGUMENT`, `ESC_RETURN` and `ESC_ELEMENT` are `CompilerLimitation`, each carrying its own `#7034` reference. So the invariant was false by construction, and any module denying through both halves of rule 2 — ordinary dependency JS — panicked in a debug-assertions build.
+
+The assertion's *concern* was right: a scheduler-facing bucket must not mean two things at once. Keying the bucket on the tier resolves that instead of asserting it away. A rule spanning two tiers now reports two buckets, each meaning exactly one thing, with counts kept separate rather than one tier's total silently absorbed into the other's label.
+
+The #7176 S20 guard test is updated rather than deleted, and it is worth being precise about what it used to prove. It was `#[should_panic(expected = "carries two tiers")]` plus `#[cfg(debug_assertions)]` — so it passed, permanently and by design, while the behaviour it pinned was aborting the compiler on real input. It now asserts the reporting behaviour (two buckets, one per tier, `denied` counts intact) and runs in every profile, because what it pins no longer depends on whether assertions are compiled in.
+
+Verified by sabotage rather than assertion: dropping the tier from the bucket key makes the updated test fail with `one bucket per (rule, tier): [… "tier": Null …]`, and restoring it passes. `cargo test -p perry-codegen --lib` is 851 passed / 0 failed.
+
+One honest limitation: I could not get `--opt-report=json` to render at all from a debug binary on this host — the report simply did not print, cache-disabled or not — so the before/after is demonstrated at the unit level, on the exact `rule_buckets` path, rather than as a live end-to-end panic. The old test asserting the panic is itself the "before" evidence.

crates/perry-codegen/src/opt_report/render.rs

@@ -404,25 +404,31 @@ struct JsonSummary<'a> {
 }
 
 fn rule_buckets(entries: &[Entry]) -> Vec<JsonRuleBucket<'_>> {
-    let mut out: BTreeMap<(&str, &str), (Option<Tier>, usize)> = BTreeMap::new();
+    // #7234: the key carries the TIER.
+    //
+    // This used to be keyed on `(analysis, rule)` and `debug_assert`ed that a
+    // rule never carries two tiers, on the reasoning that "rule-to-tier is 1:1
+    // because both come from one `ShapeDenial` constant". That reads the
+    // relation backwards: MANY constants share one rule NAME, and they do not
+    // agree on tier. `rule 2 (containment)` alone spans both — `ESC_REASSIGNED`
+    // and `ESC_CLOSURE_CAPTURE` are `Fixable`, while `ESC_CALL_ARGUMENT`,
+    // `ESC_RETURN` and `ESC_ELEMENT` are `CompilerLimitation`, each with its
+    // own `#7034` issue reference. So the invariant was false by construction,
+    // and any module denying through both halves of rule 2 — ordinary
+    // dependency JS — aborted the compiler in a debug-assertions build.
+    //
+    // The assertion's CONCERN was right: a scheduler-facing bucket must not
+    // mean two things at once. Keying on the tier resolves that instead of
+    // asserting it away — a rule spanning two tiers now reports two buckets,
+    // each meaning exactly one thing, and no count is silently merged into a
+    // tier it does not belong to.
+    let mut out: BTreeMap<(&str, &str, Option<Tier>), usize> = BTreeMap::new();
     for e in entries.iter().filter(|e| e.outcome == Outcome::Denied) {
         let rule = e.rule.as_deref().unwrap_or("<unnamed>");
-        let slot = out
-            .entry((e.analysis.as_str(), rule))
-            .or_insert((e.tier, 0));
-        // Rule-to-tier is 1:1 because both come from one `ShapeDenial`
-        // constant — but nothing HERE enforces that, and silently keeping the
-        // first tier seen would let a scheduler-facing bucket mean two things
-        // at once, which is the defect this PR exists to remove. Assert the
-        // invariant rather than resolving it.
-        debug_assert_eq!(
-            slot.0, e.tier,
-            "rule {rule} carries two tiers; the bucket would report only one"
-        );
-        slot.1 += 1;
+        *out.entry((e.analysis.as_str(), rule, e.tier)).or_insert(0) += 1;
     }
     out.into_iter()
-        .map(|((analysis, rule), (tier, denied))| JsonRuleBucket {
+        .map(|((analysis, rule, tier), denied)| JsonRuleBucket {
             analysis,
             rule,
             tier,
@@ -1030,20 +1036,39 @@ mod r0_bucket_tests {
         assert_eq!(json["summary"]["masked_by_dedup"], 7);
     }
 
-    /// The rule-to-tier invariant is asserted, not resolved silently — and the
-    /// assertion is exercised, so it is not a guard nobody has watched fire.
+    /// #7234: a rule that carries two tiers is REPORTED, not asserted away.
     ///
-    /// `debug_assert` is compiled out in release, which is the right trade for
-    /// a report-only invariant: the cost of being wrong is one mislabelled
-    /// bucket, not a miscompile. This test runs in the debug profile where the
-    /// assertion is live.
+    /// This test used to be `#[should_panic(expected = "carries two tiers")]`
+    /// and `#[cfg(debug_assertions)]`, pinning an invariant the shipped denial
+    /// table does not satisfy — so it passed while the compiler aborted on
+    /// ordinary dependency JS. It now runs in every profile, because the
+    /// behaviour it pins no longer depends on whether assertions are compiled
+    /// in.
     #[test]
-    #[should_panic(expected = "carries two tiers")]
-    #[cfg(debug_assertions)]
-    fn one_rule_carrying_two_tiers_is_a_hard_error() {
+    fn one_rule_carrying_two_tiers_reports_two_buckets() {
+        // #7234: this used to assert a PANIC. `rule 2 (containment)` carries
+        // both tiers in the shipped denial table, so the panic fired on real
+        // dependency JS in any debug-assertions build. Two tiers is a fact
+        // about the table, not a defect; reporting them as one bucket would
+        // be. Each tier gets its own bucket, and the counts stay separate.
         let a = alloc_site("return", RULE1, Tier::CompilerLimitation, 0);
         let b = alloc_site("return", RULE1, Tier::Fixable, 1);
-        let _ = render_json_with(&[a, b], 0);
+        let json: serde_json::Value = serde_json::from_str(&render_json_with(&[a, b], 0)).unwrap();
+        let buckets = json["summary"]["by_rule"].as_array().unwrap();
+        assert_eq!(buckets.len(), 2, "one bucket per (rule, tier): {buckets:?}");
+        let mut tiers: Vec<&str> = buckets
+            .iter()
+            .map(|b| b["tier"].as_str().unwrap_or("<none>"))
+            .collect();
+        tiers.sort_unstable();
+        assert_eq!(tiers, vec!["compiler-limitation", "fixable"]);
+        for bucket in buckets {
+            assert_eq!(
+                bucket["denied"].as_u64(),
+                Some(1),
+                "each tier keeps its own count: {bucket:?}"
+            );
+        }
     }
 
     /// An entry that is not an allocation site contributes to `by_rule` and to