Repository: tailwindlabs/tailwindcss

Problem statement:
Using important: true leads to duplicate rules in output
This issue supersedes #9203 which was a bit of a false start.
V3.1.18
Tailwind CLI
Node v14.18.1
Chrome
Window 10 (No Docker or WSL)
Command line : npx tailwindcss --output twout.css --watch -i twbase.css

Reproduction URL:  https://github.com/willdean/TwRepro1

**Describe your issue**

When the important option is set to true, modifications to the source file cause TW to emit duplicate rules into the output for each update.

Repro steps:

1. Start the CLI, using the command line above (or tw.bat on Windows) 
2. Observe that the output file `twout.css` contains just two rules (after the boilerplate):
```
.ml-2 {
  margin-left: 0.5rem !important
}
.ml-4 {
  margin-left: 1rem !important
}
```
3. Modify the index.html file so that the `ml-4` selector on the second div becomes `ml-6` and save the file
4. Verify that the CLI regenerates the output file `twout.css`

The output file now contains
```
.ml-2 {
  margin-left: 0.5rem !important
}
.ml-4 {
  margin-left: 1rem !important
}
.ml-2 {
  margin-left: 0.5rem !important
}
.ml-6 {
  margin-left: 1.5rem !important
}
```
i.e. the `.ml-2` rule has been duplicated.

This will happen each time a modification to the source file is made - there will be an additional `.ml-2` rule generated.

**Some commentary:**

I have spent some time trying to understand this, and I think there may be two separate problems, though I'm not a JS developer by trade, and I don't really understand TW's architecture, so please forgive me if I'm completely wrong here.

**Possible Issue 1** -  Each time a file is modified, TW adds duplicates of all the rules related to that specific file to `context.stylesheetCache`.  This is nothing to do with whether the `important` configuration option is set or not.   However, this does not normally cause duplication in the output, because the code which later converts the list of rules into css code elides all the duplicates.   This may however be a part of the slow-down/leak problem in #7449.

**Possible Issue 2**- The code which converts the rules list into CSS (is this Postcss?) is apparently able to elide duplicate rules, and that normally deals with the duplicates which build-up in the cache.   However, this elision features partly breaks (not completely - some rules get repeated 'n' times, whereas some only see one duplicate) when the `important` option is set.

If issue 1 is dealt with then issue 2 possibly wouldn't matter.  In #9203 I posted some very crude work-around code which stops the duplicates in the cache.

Edit: This issue doesn't seem to repro in https://play.tailwindcss.com/



Hints:
In case this raises questions about PostCSS:
```
>npm -g list postcss
C:\Users\will\AppData\Roaming\npm
`-- tailwindcss@3.1.8
  +-- postcss-import@14.1.0
  | `-- postcss@8.4.16 deduped
  +-- postcss-js@4.0.0
  | `-- postcss@8.4.16 deduped
  +-- postcss-load-config@3.1.4
  | `-- postcss@8.4.16 deduped
  +-- postcss-nested@5.0.6
  | `-- postcss@8.4.16 deduped
  `-- postcss@8.4.16
```
I don't think this is likely to be related to PostCSS. We do some internal cache clearing stuff on Tailwind Play. Since there's only a single content input source we have it set up to act essentially as if it were a fresh build.
Thanks Jordan - I don't properly comprehend the flow of:

`(List-Of-Rules-with-lots-of-duplicates) => [A miracle happens here] => (CSS file with fewer duplicates)`

 and thought that the miracle might be PostCSS.

Anyway, the duplicates have been annoying us for months and months, so it was nice to get to *some* level of understanding.   
Yeah I'm not sure why the duplicates some time disappear and sometimes don't but I can absolutely reproduce it in the CLI so I'm taking a look. I've also been able to create a script that makes the rule cache grow significantly which reproduces the slowdown issue. That's a small but significantly helpful data point as well!
The point at which the duplicate is created in the cache is `returnValue.utilities.add(rule);` in `buildStylesheet`.    I wondered if there had originally been the intent that, being made from a `Set` object, the cache would inherently not hold duplicates.  Of course, from the PoV of a `Set`, the duplicates are not the same, so that doesn't work.
I wouldn’t be surprised if it did used to be the same rule instance but in fixing another bug we’ve introduced a regression (probably by cloning the rule somewhere) that is causing duplicates to appear. Candidates (like `ml-4`) should be pulling their corresponding rule from a cache if they have been generated already, so should have been the same rule instance originally I think.

Patch:
diff --git a/src/lib/expandTailwindAtRules.js b/src/lib/expandTailwindAtRules.js
--- a/src/lib/expandTailwindAtRules.js
+++ b/src/lib/expandTailwindAtRules.js
@@ -177,16 +177,12 @@ export default function expandTailwindAtRules(context) {
     let classCacheCount = context.classCache.size
 
     env.DEBUG && console.time('Generate rules')
-    let rules = generateRules(candidates, context)
+    generateRules(candidates, context)
     env.DEBUG && console.timeEnd('Generate rules')
 
     // We only ever add to the classCache, so if it didn't grow, there is nothing new.
     env.DEBUG && console.time('Build stylesheet')
     if (context.stylesheetCache === null || context.classCache.size !== classCacheCount) {
-      for (let rule of rules) {
-        context.ruleCache.add(rule)
-      }
-
       context.stylesheetCache = buildStylesheet([...context.ruleCache], context)
     }
     env.DEBUG && console.timeEnd('Build stylesheet')
diff --git a/src/lib/generateRules.js b/src/lib/generateRules.js
--- a/src/lib/generateRules.js
+++ b/src/lib/generateRules.js
@@ -649,8 +649,37 @@ function inKeyframes(rule) {
   return rule.parent && rule.parent.type === 'atrule' && rule.parent.name === 'keyframes'
 }
 
+function getImportantStrategy(important) {
+  if (important === true) {
+    return (rule) => {
+      if (inKeyframes(rule)) {
+        return
+      }
+
+      rule.walkDecls((d) => {
+        if (d.parent.type === 'rule' && !inKeyframes(d.parent)) {
+          d.important = true
+        }
+      })
+    }
+  }
+
+  if (typeof important === 'string') {
+    return (rule) => {
+      if (inKeyframes(rule)) {
+        return
+      }
+
+      rule.selectors = rule.selectors.map((selector) => {
+        return `${important} ${selector}`
+      })
+    }
+  }
+}
+
 function generateRules(candidates, context) {
   let allRules = []
+  let strategy = getImportantStrategy(context.tailwindConfig.important)
 
   for (let candidate of candidates) {
     if (context.notClassCache.has(candidate)) {
@@ -658,7 +687,11 @@ function generateRules(candidates, context) {
     }
 
     if (context.classCache.has(candidate)) {
-      allRules.push(context.classCache.get(candidate))
+      continue
+    }
+
+    if (context.candidateRuleCache.has(candidate)) {
+      allRules = allRules.concat(Array.from(context.candidateRuleCache.get(candidate)))
       continue
     }
 
@@ -670,47 +703,27 @@ function generateRules(candidates, context) {
     }
 
     context.classCache.set(candidate, matches)
-    allRules.push(matches)
-  }
 
-  // Strategy based on `tailwindConfig.important`
-  let strategy = ((important) => {
-    if (important === true) {
-      return (rule) => {
-        rule.walkDecls((d) => {
-          if (d.parent.type === 'rule' && !inKeyframes(d.parent)) {
-            d.important = true
-          }
-        })
-      }
-    }
+    let rules = context.candidateRuleCache.get(candidate) ?? new Set()
+    context.candidateRuleCache.set(candidate, rules)
 
-    if (typeof important === 'string') {
-      return (rule) => {
-        rule.selectors = rule.selectors.map((selector) => {
-          return `${important} ${selector}`
-        })
-      }
-    }
-  })(context.tailwindConfig.important)
+    for (const match of matches) {
+      let [{ sort, layer, options }, rule] = match
 
-  return allRules.flat(1).map(([{ sort, layer, options }, rule]) => {
-    if (options.respectImportant) {
-      if (strategy) {
+      if (options.respectImportant && strategy) {
         let container = postcss.root({ nodes: [rule.clone()] })
-        container.walkRules((r) => {
-          if (inKeyframes(r)) {
-            return
-          }
-
-          strategy(r)
-        })
+        container.walkRules(strategy)
         rule = container.nodes[0]
       }
+
+      let newEntry = [sort | context.layerOrder[layer], rule]
+      rules.add(newEntry)
+      context.ruleCache.add(newEntry)
+      allRules.push(newEntry)
     }
+  }
 
-    return [sort | context.layerOrder[layer], rule]
-  })
+  return allRules
 }
 
 function isArbitraryValue(input) {
diff --git a/src/lib/setupContextUtils.js b/src/lib/setupContextUtils.js
--- a/src/lib/setupContextUtils.js
+++ b/src/lib/setupContextUtils.js
@@ -873,6 +873,7 @@ export function createContext(tailwindConfig, changedContent = [], root = postcs
   let context = {
     disposables: [],
     ruleCache: new Set(),
+    candidateRuleCache: new Map(),
     classCache: new Map(),
     applyClassCache: new Map(),
     notClassCache: new Set(),


Test patch:
diff --git a/tests/important-boolean.test.js b/tests/important-boolean.test.js
--- a/tests/important-boolean.test.js
+++ b/tests/important-boolean.test.js
@@ -1,7 +1,8 @@
 import fs from 'fs'
 import path from 'path'
+import * as sharedState from '../src/lib/sharedState'
 
-import { run, css } from './util/run'
+import { run, css, html } from './util/run'
 
 test('important boolean', () => {
   let config = {
@@ -63,3 +64,74 @@ test('important boolean', () => {
     expect(result.css).toMatchFormattedCss(expected)
   })
 })
+
+// This is in a describe block so we can use `afterEach` :)
+describe('duplicate elision', () => {
+  let filePath = path.resolve(__dirname, './important-boolean-duplicates.test.html')
+
+  afterEach(async () => await fs.promises.unlink(filePath))
+
+  test('important rules are not duplicated when rebuilding', async () => {
+    let config = {
+      important: true,
+      content: [filePath],
+    }
+
+    await fs.promises.writeFile(
+      config.content[0],
+      html`
+        <div class="ml-2"></div>
+        <div class="ml-4"></div>
+      `
+    )
+
+    let input = css`
+      @tailwind utilities;
+    `
+
+    let result = await run(input, config)
+    let allContexts = Array.from(sharedState.contextMap.values())
+
+    let context = allContexts[allContexts.length - 1]
+
+    let ruleCacheSize1 = context.ruleCache.size
+
+    expect(result.css).toMatchFormattedCss(css`
+      .ml-2 {
+        margin-left: 0.5rem !important;
+      }
+      .ml-4 {
+        margin-left: 1rem !important;
+      }
+    `)
+
+    await fs.promises.writeFile(
+      config.content[0],
+      html`
+        <div class="ml-2"></div>
+        <div class="ml-6"></div>
+      `
+    )
+
+    result = await run(input, config)
+
+    let ruleCacheSize2 = context.ruleCache.size
+
+    expect(result.css).toMatchFormattedCss(css`
+      .ml-2 {
+        margin-left: 0.5rem !important;
+      }
+      .ml-4 {
+        margin-left: 1rem !important;
+      }
+      .ml-6 {
+        margin-left: 1.5rem !important;
+      }
+    `)
+
+    // The rule cache was effectively doubling in size previously
+    // because the rule cache was never de-duped
+    // This ensures this behavior doesn't return
+    expect(ruleCacheSize2 - ruleCacheSize1).toBeLessThan(10)
+  })
+})