diff: skip selector-list factoring in the canonical projection

CHANGES.md

@@ -1,5 +1,15 @@
 ## Unreleased
 
+### Diffing
+
+- `--diff=canonical` skips selector-list factoring. Factoring is input-shape
+  dependent: whether a shared declaration can be lifted depends on how the
+  input grouped its selectors, and once lifted an intervening rule writing the
+  same property can make putting it back unsafe. The same stylesheet written
+  factored and inline therefore canonicalised to different forms, and the
+  difference was reported as a missing declaration (#215)
+- `Css.optimize` takes `?factor` to turn that pass off (#215)
+
 ### Parsing
 
 - Read `perspective: none` and `text-underline-offset: auto`, and allow a

lib/css.ml

@@ -878,10 +878,11 @@ let inline_style_of_declarations ?(optimize = false) ?minify ?mode declarations
   in
   inline_style_of_declarations ?minify ?mode declarations
 
-let optimize ?scope ?flatten_nesting ?lossless ?enforce_spec ?aggressive
+let optimize ?scope ?flatten_nesting ?lossless ?enforce_spec ?aggressive ?factor
     ?closed_world ?objective ?prune_unused_custom_props stylesheet =
   Optimize.stylesheet ?scope ?flatten_nesting ?lossless ?enforce_spec
-    ?aggressive ?closed_world ?objective ?prune_unused_custom_props stylesheet
+    ?aggressive ?factor ?closed_world ?objective ?prune_unused_custom_props
+    stylesheet
 
 let flatten_nesting = Optimize.flatten_nesting
 let canonicalize_rule_order = Rule_order.canonicalize

lib/css.mli

@@ -7667,6 +7667,7 @@ val optimize :
   ?lossless:bool ->
   ?enforce_spec:bool ->
   ?aggressive:bool ->
+  ?factor:bool ->
   ?closed_world:bool ->
   ?objective:[ `Raw | `Transfer ] ->
   ?prune_unused_custom_props:bool ->

lib/ctx.ml

@@ -6,6 +6,9 @@ type t = {
   registered : string -> bool;
   lossless : bool;
   aggressive : bool;
+  factor : bool;
+      (** Selector-list factoring is input-shape dependent, so a canonical
+          projection turns it off to stay confluent. *)
   extend_lists : bool;
   closed_world : bool;
   objective : objective;
@@ -22,21 +25,23 @@ let fragment =
     registered = (fun _ -> false);
     lossless = false;
     aggressive = false;
+    factor = true;
     extend_lists = false;
     closed_world = false;
     objective = `Transfer;
     enforce_spec = false;
   }
 
-let of_scope ?(lossless = false) ?(aggressive = false) ?(extend_lists = false)
-    ?(closed_world = false) ?(objective = `Transfer) ?(enforce_spec = false) =
-  function
+let of_scope ?(lossless = false) ?(aggressive = false) ?(factor = true)
+    ?(extend_lists = false) ?(closed_world = false) ?(objective = `Transfer)
+    ?(enforce_spec = false) = function
   | Some scope ->
       {
         fragment with
         scope;
         lossless;
         aggressive;
+        factor;
         extend_lists;
         closed_world;
         objective;
@@ -47,20 +52,22 @@ let of_scope ?(lossless = false) ?(aggressive = false) ?(extend_lists = false)
         fragment with
         lossless;
         aggressive;
+        factor;
         extend_lists;
         closed_world;
         objective;
         enforce_spec;
       }
 
-let v ?(lossless = false) ?(aggressive = false) ?(extend_lists = false)
-    ?(closed_world = false) ?(objective = `Transfer) ?(enforce_spec = false)
-    ?(registered = fun _ -> false) scope =
+let v ?(lossless = false) ?(aggressive = false) ?(factor = true)
+    ?(extend_lists = false) ?(closed_world = false) ?(objective = `Transfer)
+    ?(enforce_spec = false) ?(registered = fun _ -> false) scope =
   {
     scope;
     registered;
     lossless;
     aggressive;
+    factor;
     extend_lists;
     closed_world;
     objective;
@@ -71,6 +78,7 @@ let scope t = t.scope
 let registered t = t.registered
 let lossless t = t.lossless
 let aggressive t = t.aggressive
+let factor t = t.factor
 let extend_lists t = t.extend_lists
 let closed_world t = t.closed_world
 let objective t = t.objective

lib/ctx.mli

@@ -16,6 +16,7 @@ val fragment : t
 val of_scope :
   ?lossless:bool ->
   ?aggressive:bool ->
+  ?factor:bool ->
   ?extend_lists:bool ->
   ?closed_world:bool ->
   ?objective:objective ->
@@ -27,6 +28,7 @@ val of_scope :
 val v :
   ?lossless:bool ->
   ?aggressive:bool ->
+  ?factor:bool ->
   ?extend_lists:bool ->
   ?closed_world:bool ->
   ?objective:objective ->
@@ -45,6 +47,11 @@ val registered : t -> string -> bool
 val lossless : t -> bool
 (** Whether lossless value optimization is enabled. *)
 
+val factor : t -> bool
+(** Whether selector-list factoring runs. It is input-shape dependent - whether
+    a shared declaration can be lifted depends on how the input grouped its
+    selectors - so a canonical projection turns it off to stay confluent. *)
+
 val aggressive : t -> bool
 (** Whether expensive optimization passes (notably the global factoring
     fixpoint) run regardless of the preflight's byte-gain estimate. *)

lib/diff/css_compare.ml

@@ -230,7 +230,11 @@ let canonical_semantic_css ~strict ~lossless
       try
         Some
           (stylesheet
-          |> Css.optimize ~lossless ~prune_unused_custom_props
+          (* Selector-list factoring depends on how the input grouped its
+             selectors, so it is not confluent: the same sheet factored and
+             unfactored would canonicalise differently. The projection skips
+             it. *)
+          |> Css.optimize ~lossless ~factor:false ~prune_unused_custom_props
           |> Css.canonicalize_rule_order
           |> Css.to_string ~minify:true ~lossless)
       with Invalid_argument _ -> None)

lib/optimize.ml

@@ -418,7 +418,11 @@ and rules_aux ?factor_cache ~ctx ~enforce_spec (rules : rule list) : rule list =
      optimizations above always run; this incremental gate only decides whether
      the expensive global factoring fixpoint is likely to buy enough bytes to
      justify the full indexed scheduler walk. *)
-  let factored = factor_rules_incremental ?cache:factor_cache ~ctx prepared in
+  let factored =
+    if Ctx.factor ctx then
+      factor_rules_incremental ?cache:factor_cache ~ctx prepared
+    else prepared
+  in
   (* After factoring so the greedy scheduler sees the unconstrained input (a
      local pre-merge can lock a pair together and hide a larger group): this
      only picks up the merges factoring did not make because its preflight
@@ -1150,9 +1154,9 @@ let drop_unused_custom_props (stmts : statement list) : statement list =
   list_map_preserve prune stmts
 
 let stylesheet ?scope ?(flatten_nesting = false) ?(lossless = false)
-    ?(enforce_spec = false) ?(aggressive = false) ?(closed_world = false)
-    ?(objective = `Transfer) ?(prune_unused_custom_props = false)
-    (stylesheet : t) : t =
+    ?(enforce_spec = false) ?(aggressive = false) ?(factor = true)
+    ?(closed_world = false) ?(objective = `Transfer)
+    ?(prune_unused_custom_props = false) (stylesheet : t) : t =
   Selector_summary.clear_memo ();
   reset_counters ();
   let scope = Option.value scope ~default:`Fragment in
@@ -1165,7 +1169,7 @@ let stylesheet ?scope ?(flatten_nesting = false) ?(lossless = false)
   let registered = registered_foldable stylesheet in
   let stylesheet = prune_position_try_fallbacks ~scope stylesheet in
   let ctx =
-    Ctx.v ~lossless ~aggressive ~closed_world ~objective ~enforce_spec
+    Ctx.v ~lossless ~aggressive ~factor ~closed_world ~objective ~enforce_spec
       ~registered scope
   in
   let result =

lib/optimize.mli

@@ -47,6 +47,7 @@ type ctx
 val ctx_of_scope :
   ?lossless:bool ->
   ?aggressive:bool ->
+  ?factor:bool ->
   ?extend_lists:bool ->
   ?closed_world:bool ->
   ?objective:Ctx.objective ->
@@ -148,6 +149,7 @@ val stylesheet :
   ?lossless:bool ->
   ?enforce_spec:bool ->
   ?aggressive:bool ->
+  ?factor:bool ->
   ?closed_world:bool ->
   ?objective:Ctx.objective ->
   ?prune_unused_custom_props:bool ->

test/test_optimize.ml

@@ -877,8 +877,30 @@ let test_lossless_declaration_order () =
     ".a{border-top:1px solid red;border-color:#00f}"
     (opt ~lossless:true ".a{border-top:1px solid red;border-color:blue}")
 
+(* Selector-list factoring is input-shape dependent: whether a shared
+   declaration can be lifted depends on how the input grouped its selectors, and
+   once lifted an intervening rule writing the same property can make putting it
+   back unsafe. A canonical projection therefore turns factoring off, so the
+   same stylesheet written factored and inline maps to one form. *)
+let factoring_can_be_disabled () =
+  let src =
+    ".text-xs{font-size:var(--text-xs);line-height:1}.text-xs\\/4{font-size:var(--text-xs);line-height:4}.text-xs\\/5{font-size:var(--text-xs);line-height:5}.text-xs\\/6{font-size:var(--text-xs);line-height:6}.text-xs\\/7{font-size:var(--text-xs);line-height:7}"
+  in
+  let sheet = Css.of_string_exn src in
+  let out ?factor () =
+    Css.to_string ~minify:true (Css.optimize ?factor sheet)
+  in
+  Alcotest.(check bool)
+    "factoring on lifts the shared declaration" true
+    (Astring.String.is_infix ~affix:".text-xs,.text-xs\\/4" (out ()));
+  Alcotest.(check bool)
+    "factoring off leaves each rule alone" false
+    (Astring.String.is_infix ~affix:".text-xs,.text-xs\\/4"
+       (out ~factor:false ()))
+
 let optimize_tests =
   [
+    ("factoring can be disabled", `Quick, factoring_can_be_disabled);
     ("vendor prefix strip", `Quick, test_vendor_prefix_strip);
     ("lossless declaration order", `Quick, test_lossless_declaration_order);
     ( "var() colour functions preserved",