css: with browser targets, remove an earlier duplicate rule only when its declaration block is equal

src/css/rules/mod.rs

@@ -854,6 +854,10 @@ fn minify_style_arm<R: for<'b> css::generics::DeepClone<'b>>(
                 && i < rules.len()
                 && let CssRule::Style(other) = &rules[i]
                 && (!context.css_modules || source_index == other.loc.source_index)
+                // Stricter than lightningcss: a target may not parse this rule and need `other`.
+                && let CssRule::Style(later) = &rules[idx]
+                && (context.targets.browsers.is_none()
+                    || later.declarations.eql(&other.declarations))
             {
                 rules[i] = CssRule::Ignored;
             }

test/js/bun/css/css.test.ts

@@ -2,8 +2,9 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
+import { cssInternals } from "bun:internal-for-testing";
 import { describe, expect, test } from "bun:test";
-import { bunEnv, bunExe } from "harness";
+import { bunEnv, bunExe, tempDir } from "harness";
 import { join } from "path";
 import {
   cssTest,
@@ -7672,6 +7673,211 @@ describe("css tests", () => {
     error_test("@media (prefers-color-scheme = dark) { .foo { color: chartreuse }}", "ParserError::InvalidMediaQuery");
   });
 
+  // A rule is removed when a later rule in the same list has the same selectors
+  // and declares the same properties. A browser that cannot parse a value of
+  // the later rule still applies the earlier rule.
+  describe("duplicate rules", () => {
+    // The browser targets of `bun build`. None of them parses `dvh`, `round()`,
+    // `overflow: clip` or `anchor-size()`.
+    const targets = { chrome: 80 << 16, edge: 80 << 16, firefox: 78 << 16, safari: 14 << 16, opera: 67 << 16 };
+
+    describe("browser targets: a rule with another declaration block stays", () => {
+      test.each([
+        ".a{width:100vh}.b{color:red}.a{width:100dvh}",
+        ".d{gap:15px}.e{color:red}.d{gap:round(1em,5px)}",
+        // bun has no support data for these values.
+        ".a{overflow:hidden}.b{color:red}.a{overflow:clip}",
+        ".a{width:100px}.b{color:red}.a{width:anchor-size(width)}",
+        // Every target parses the later value. The check does not ask.
+        ".a{color:red}.b{color:green}.a{color:pink}",
+        ".a{height:100vh}.b{top:0}.c{top:1px}.d{top:2px}.e{top:3px}.f{top:4px}.g{top:5px}.a{height:100dvh}",
+        // Chrome 80 has no `:where()`, so these neighbours are not merged into one rule.
+        ":where(.x){width:100vh}:where(.x){width:100dvh}",
+        "@media print{.a{width:100vh}.b{color:red}.a{width:100dvh}}",
+        "@supports (display:grid){.a{width:100vh}.b{color:red}.a{width:100dvh}}",
+        "@layer x{.a{width:100vh}.b{color:red}.a{width:100dvh}}",
+        // The blocks differ after the first declaration, or in the `!important` list.
+        ".a{color:red;overflow:hidden}.b{top:0}.a{color:red;overflow:clip}",
+        ".a{overflow:hidden!important}.b{top:0}.a{overflow:clip!important}",
+        ".a{color:red!important;overflow:hidden!important}.b{top:0}.a{color:red!important;overflow:clip!important}",
+        ".a{width:1px!important}.b{color:red}.a{width:2px}",
+        // Custom properties with other names.
+        ".a{--x:1}.b{color:red}.a{--y:2}",
+        // https://github.com/oven-sh/bun/issues/44704
+        ":where(.x){--a:1}:where(.x){--b:2}",
+        ":where(.x) .y{--a:1}:where(.x) .y{--b:2}",
+        // https://github.com/oven-sh/bun/issues/45730
+        ".tag{--h-xs:1px}.tag.xs{--h:var(--h-xs)}.tag{--h-sm:2px}",
+        // The last rule differs from the one before it. The first rule is not compared again.
+        ".a{width:1px}.b{top:0}.a{width:2px}.c{top:0}.a{width:1px}",
+        // Another url, and a radius with a second value.
+        ".a{background:url(data:,a)}.b{top:0}.a{background:url(data:,b)}",
+        ".a{border-radius:1px}.b{top:0}.a{border-radius:1px/2px}",
+      ])("%s", source => {
+        expect(cssInternals.minifyTest(source, "", targets)).toBe(source);
+      });
+
+      test.each([
+        [".p{.a{width:100vh}.b{color:red}.a{width:100dvh}}", ".p .a{width:100vh}.p .b{color:red}.p .a{width:100dvh}"],
+        [
+          "@media print{.a{width:100vh}}@media print{.b{color:red}}@media print{.a{width:100dvh}}",
+          "@media print{.a{width:100vh}.b{color:red}.a{width:100dvh}}",
+        ],
+        // https://github.com/oven-sh/bun/issues/45258
+        [":root{&{--a:2}&{--b:2}}", ":root{--a:2}:root{--b:2}"],
+      ])("%s", (source, expected) => {
+        expect(cssInternals.minifyTest(source, "", targets)).toBe(expected);
+      });
+
+      test("targets that parse the later value", () => {
+        const source = ".a{width:100vh}.b{color:red}.a{width:100dvh}";
+        expect(cssInternals.minifyTest(source, "", { chrome: 120 << 16 })).toBe(source);
+      });
+    });
+
+    describe("browser targets: a rule with an equal declaration block is removed", () => {
+      test.each([
+        [".a{color:red}.b{color:green}.a{color:#f00}", ".b{color:green}.a{color:red}"],
+        [".a{width:100dvh}.b{color:red}.a{width:100dvh}", ".b{color:red}.a{width:100dvh}"],
+        [".a{color:red;overflow:clip}.b{top:0}.a{color:red;overflow:clip}", ".b{top:0}.a{color:red;overflow:clip}"],
+        [".a{overflow:clip!important}.b{top:0}.a{overflow:clip!important}", ".b{top:0}.a{overflow:clip!important}"],
+        [".a{width:1px}.b{top:0}.a{width:2px}.c{top:0}.a{width:2px}", ".a{width:1px}.b{top:0}.c{top:0}.a{width:2px}"],
+        // A block with `url()`, a missing channel or a two-value size equals its copy.
+        [".a{background:url(data:,a)}.b{top:0}.a{background:url(data:,a)}", ".b{top:0}.a{background:url(data:,a)}"],
+        [".a{--c:oklch(98.5% 0 none)}.b{top:0}.a{--c:oklch(98.5% 0 none)}", ".b{top:0}.a{--c:oklch(98.5% 0 none)}"],
+        [".a{border-radius:1px/2px}.b{top:0}.a{border-radius:1px/2px}", ".b{top:0}.a{border-radius:1px/2px}"],
+      ])("%s", (source, expected) => {
+        expect(cssInternals.minifyTest(source, "", targets)).toBe(expected);
+      });
+    });
+
+    // bun prints a rule once for each vendor prefix that its selectors need. When bun minifies
+    // that output again, every copy from the earlier build has to go, or the sheet grows on each build.
+    describe("browser targets: output that is minified again stops changing", () => {
+      // The first build prints `:not(.b,.c)` three ways. The second build keeps the two prefixed
+      // rules and prints the `:is()` rule three ways again. Each later build prints the same.
+      test.each(["color:red", "background:url(data:,a)", "--c:oklch(98.5% 0 none)", "border-radius:1px/2px"])(
+        ".a:not(.b,.c){%s}",
+        declarations => {
+          const rule = (any: string) => `.a:not(${any}(.b,.c)){${declarations}}`;
+          const prefixed = rule(":-webkit-any") + rule(":-moz-any");
+          const first = prefixed + rule(":is");
+          const builds = [`.a:not(.b,.c){${declarations}}`];
+          for (let i = 0; i < 4; i++) builds.push(cssInternals.minifyTest(builds[i], "", targets));
+          expect(builds.slice(1)).toEqual([first, prefixed + first, prefixed + first, prefixed + first]);
+        },
+      );
+
+      // These selectors take a vendor prefix of their own. The rows hold that the output stops
+      // changing, not what it is.
+      test.each([
+        ".a:autofill{background:url(data:,a)}",
+        ".a:autofill:not(.b,.c){background:url(data:,a)}",
+        ".a:fullscreen:not(.b,.c){background:url(data:,a)}",
+      ])("%s", source => {
+        const builds = [source];
+        for (let i = 0; i < 6; i++) builds.push(cssInternals.minifyTest(builds[i], "", targets));
+        expect(builds[6]).toBe(builds[5]);
+      });
+    });
+
+    describe("no browser targets: the earlier rule is removed", () => {
+      test.each([
+        [".a{width:100vh}.b{color:red}.a{width:100dvh}", ".b{color:red}.a{width:100dvh}"],
+        [".a{color:red}.b{color:green}.a{color:pink}", ".b{color:green}.a{color:pink}"],
+        [".a{overflow:hidden}.b{color:red}.a{overflow:clip}", ".b{color:red}.a{overflow:clip}"],
+      ])("%s", (source, expected) => {
+        expect(cssInternals.minifyTest(source, "")).toBe(expected);
+      });
+    });
+
+    // The same comparison decides whether two neighbours share one rule.
+    describe("neighbours with another declaration block are not merged", () => {
+      test.each([
+        ".a{color:red;overflow:hidden}.c{color:red;overflow:clip}",
+        ".a{color:red!important}.c{color:pink!important}",
+      ])("%s", source => {
+        expect(cssInternals.minifyTest(source, "", targets)).toBe(source);
+      });
+    });
+
+    // The CSS is stdout, or the file `outfile` for a build with --outdir.
+    const build = async (cwd: string, args: string[], outfile?: string) => {
+      await using proc = Bun.spawn({
+        cmd: [bunExe(), "build", ...args],
+        env: bunEnv,
+        cwd,
+        stdout: "pipe",
+        stderr: "pipe",
+      });
+      const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+      const css = exitCode !== 0 ? "" : outfile ? await Bun.file(join(cwd, outfile)).text() : stdout;
+      return { css: css.trim(), stderr, exitCode };
+    };
+
+    test.concurrent("bun build keeps the earlier rule for a browser, and removes it for --target bun", async () => {
+      using dir = tempDir("css-duplicate-rules", {
+        "in.css": [
+          ".a{width:100vh}",
+          ".b{color:red}",
+          ".a{width:100dvh}",
+          ".d{gap:15px}",
+          ".e{color:blue}",
+          ".d{gap:round(1em,5px)}",
+          "",
+        ].join("\n"),
+        // A type selector: in a CSS module the check compares rules of one file.
+        "styles.module.css": ["div{width:100vh}", "span{color:red}", "div{width:100dvh}", ""].join("\n"),
+        "entry.js": `import "./styles.module.css";\n`,
+      });
+      const cwd = String(dir);
+
+      const [browser, bun, module, pretty] = await Promise.all([
+        build(cwd, ["./in.css", "--minify"]),
+        build(cwd, ["./in.css", "--minify", "--target", "bun"]),
+        build(cwd, ["./entry.js", "--minify", "--outdir", "out"], "out/entry.css"),
+        // The check also runs without --minify.
+        build(cwd, ["./in.css"]),
+      ]);
+      expect([browser, bun, module, { ...pretty, css: pretty.css.replace(/\/\*.*?\*\/|\s/g, "") }]).toEqual([
+        {
+          css: ".a{width:100vh}.b{color:red}.a{width:100dvh}.d{gap:15px}.e{color:#00f}.d{gap:round(1em,5px)}",
+          stderr: "",
+          exitCode: 0,
+        },
+        { css: ".b{color:red}.a{width:100dvh}.e{color:#00f}.d{gap:round(1em,5px)}", stderr: "", exitCode: 0 },
+        { css: "div{width:100vh}span{color:red}div{width:100dvh}", stderr: "", exitCode: 0 },
+        {
+          css: ".a{width:100vh;}.b{color:red;}.a{width:100dvh;}.d{gap:15px;}.e{color:#00f;}.d{gap:round(1em,5px);}",
+          stderr: "",
+          exitCode: 0,
+        },
+      ]);
+    });
+
+    test.concurrent("bun build of its own output stops changing", async () => {
+      using dir = tempDir("css-duplicate-rules-rebuild", {
+        "in.css": ".a:not(.b,.c){background:url(data:,a)}\n",
+      });
+      const cwd = String(dir);
+      const builds = [];
+      for (let i = 0; i < 4; i++) {
+        const result = await build(cwd, ["./in.css", "--minify"]);
+        builds.push(result);
+        await Bun.write(join(cwd, "in.css"), result.css);
+      }
+      const prefixed =
+        ".a:not(:-webkit-any(.b,.c)){background:url(data:,a)}.a:not(:-moz-any(.b,.c)){background:url(data:,a)}";
+      const first = prefixed + ".a:not(:is(.b,.c)){background:url(data:,a)}";
+      expect(builds).toEqual([
+        { css: first, stderr: "", exitCode: 0 },
+        { css: prefixed + first, stderr: "", exitCode: 0 },
+        { css: prefixed + first, stderr: "", exitCode: 0 },
+        { css: prefixed + first, stderr: "", exitCode: 0 },
+      ]);
+    });
+  });
+
   describe("transition", () => {
     minify_test(".foo { transition-duration: 500ms }", ".foo{transition-duration:.5s}");
     minify_test(".foo { transition-duration: .5s }", ".foo{transition-duration:.5s}");

test/js/bun/css/duplicate-declaration-merge-hang.test.ts

@@ -53,11 +53,17 @@ const script = `
       Array.from({ length: n }, (_, i) => \`.a\${i}{color:red}\`).join(""),
       { chrome: 80 << 16 },
     ],
+    "different-valued-duplicates": [
+      Array.from({ length: n }, (_, i) => \`.a{width:\${i + 1}dvh}.b\${i}{color:red}\`).join(""),
+      { chrome: 80 << 16 },
+      // Chrome 80 has no dvh, so every .a rule stays as the fallback of the next one.
+      out => ":" + (out.split(".a {").length - 1),
+    ],
   };
-  for (const [name, [src, targets]] of Object.entries(floods)) {
+  for (const [name, [src, targets, count]] of Object.entries(floods)) {
     const out = targets === undefined ? c.minifyTest(src, "") : c._test(src, "", targets);
     if (typeof out !== "string" || out.length === 0) throw new Error(name + " produced no output");
-    console.log("OK:" + name);
+    console.log("OK:" + name + (count ? count(out) : ""));
   }
 `;
 
@@ -85,6 +91,7 @@ test("duplicate declarations across merged rules minify in linear time instead o
       "OK:clamp",
       "OK:distinct-custom-props",
       "OK:distinct-selectors",
+      "OK:different-valued-duplicates:2000",
       "",
     ].join("\n"),
     stderr: "",