fix(canonicalize): prevent collapse cache pollution across calls#19675
fix(canonicalize): prevent collapse cache pollution across calls#19675RobinMalfait merged 8 commits intotailwindlabs:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 WalkthroughThe PR adds two unit tests for canonicalization: one verifying canonical output for a subset of classes and another exercising repeated collapse canonicalization to ensure idempotence. It updates 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes an order-sensitive bug in canonicalizeCandidates where collapse-related caches could be unintentionally mutated during read-only lookups, causing later canonicalization results to depend on earlier calls (important for non-deterministic ESLint lint order).
Changes:
- Replaced
DefaultMap-backed collapse caches withMapto avoid mutation on lookup paths. - Added a
Map#getOrInsertpolyfill and refactored cache access to use explicit “insert-on-miss” behavior. - Added a regression test ensuring collapse canonicalization is stable across repeated calls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/tailwindcss/src/utils/map-get-or-insert.ts | Introduces a Map#getOrInsert polyfill used for explicit insert-on-miss cache behavior. |
| packages/tailwindcss/src/canonicalize-candidates.ts | Refactors static-utility and utility-property caches from DefaultMap to Map, and updates collapse logic to use explicit cache population. |
| packages/tailwindcss/src/canonicalize-candidates.test.ts | Adds regression coverage for the order-sensitivity/cross-call cache pollution bug. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export {} | ||
|
|
||
| declare global { | ||
| interface Map<K, V> { | ||
| getOrInsert(key: K, defaultValue: V): V | ||
| } | ||
| } | ||
|
|
||
| if (typeof Map.prototype.getOrInsert !== 'function') { | ||
| Object.defineProperty(Map.prototype, 'getOrInsert', { | ||
| configurable: true, | ||
| enumerable: false, | ||
| writable: true, | ||
| value: function <K, V>(this: Map<K, V>, key: K, defaultValue: V): V { | ||
| if (!this.has(key)) { | ||
| this.set(key, defaultValue) | ||
| } | ||
|
|
||
| return this.get(key)! | ||
| }, | ||
| }) |
There was a problem hiding this comment.
This module both augments the global Map TypeScript interface and patches Map.prototype at runtime. Since tailwindcss is a published library, this leaks a non-standard API into consumers’ global types/runtime and can lead to conflicts with other tooling or future platform implementations. Prefer a local helper (e.g. getOrInsert(map, key, createValue)), or a small wrapper class used only for these caches, to avoid global/prototype mutation.
| export {} | |
| declare global { | |
| interface Map<K, V> { | |
| getOrInsert(key: K, defaultValue: V): V | |
| } | |
| } | |
| if (typeof Map.prototype.getOrInsert !== 'function') { | |
| Object.defineProperty(Map.prototype, 'getOrInsert', { | |
| configurable: true, | |
| enumerable: false, | |
| writable: true, | |
| value: function <K, V>(this: Map<K, V>, key: K, defaultValue: V): V { | |
| if (!this.has(key)) { | |
| this.set(key, defaultValue) | |
| } | |
| return this.get(key)! | |
| }, | |
| }) | |
| export function getOrInsert<K, V>(map: Map<K, V>, key: K, defaultValue: V): V { | |
| if (!map.has(key)) { | |
| map.set(key, defaultValue) | |
| } | |
| return map.get(key)! |
There was a problem hiding this comment.
this is a fair point, I didn't think about the fact this was a library vs runtime code.
I'll leave this up to a maintainer to decide if this should be fixed or not - please let me know and i'll change it
RobinMalfait
force-pushed
the
c/fix-canonicalizeCandidates
branch
from
ec8049a to
2c5a71f
Compare
It does mean that the `result` can be empty now, so in that case we fallback to an empty Set. While it's true that consecutive calls to `designSystem.canonicalizeCandidates(candidates, options)` will update some shared caches for this design system, it's done on purpose to reduce the amount of work necessary to compute canonicalizations. However, this resulted in the fact that some caches become polluted by previous runs. This on its own is not the issue, but we made some assumptions as-if it's a fresh run. Ignoring unrelated values should do the trick here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/canonicalize-candidates.ts (1)
2604-2618: Pre-existing bug in theintersectionpolyfill — returns a copy ofainstead of the true intersection on Node.js < 22.The loop iterates over
band callsresult.delete(item)only when!result.has(item)— but ifitemis not inresult(a copy ofa), the delete is a no-op. Items inathat are absent frombare never removed. The function effectively returnsnew Set(a).This does not affect correctness — the signature comparison at line 399 gates every potential replacement, so only genuinely valid candidates pass through. It does, however, degrade performance in polyfill environments:
potentialReplacementsis the fullotherUtilities[0]set rather than the narrower intersection, so the innerfor…ofloop iterates over more candidates than necessary.♻️ Corrected polyfill
let result = new Set<T>(a) - for (let item of b) { - if (!result.has(item)) { - result.delete(item) - } - } + for (let item of result) { + if (!b.has(item)) { + result.delete(item) + } + } return result🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tailwindcss/src/canonicalize-candidates.ts` around lines 2604 - 2618, The polyfill in function intersection<T>(a: Set<T>, b: Set<T>) is wrong: it creates result = new Set(a) then iterates b but deletes only when !result.has(item), which never removes elements of a that are not in b; fix by computing the true intersection — either create an empty Set and add items from a that are present in b, or iterate over result (a's copy) and delete any item not present in b — ensuring the polyfill returns only items common to both a and b for Node.js <22 environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tailwindcss/src/canonicalize-candidates.ts`:
- Around line 2604-2618: The polyfill in function intersection<T>(a: Set<T>, b:
Set<T>) is wrong: it creates result = new Set(a) then iterates b but deletes
only when !result.has(item), which never removes elements of a that are not in
b; fix by computing the true intersection — either create an empty Set and add
items from a that are present in b, or iterate over result (a's copy) and delete
any item not present in b — ensuring the polyfill returns only items common to
both a and b for Node.js <22 environments.
Alright silly me. The DefaultMap of course caches the moment you use `.get(…)` which results in the cache pollution. We _only_ want to do that if the properties of the current candidate already contains the line-height or font-size properties. If we do it unconditionally, then `['underline', 'text-sm', 'w-4', 'h-4']` will have the `line-height` and `font-size` properties set for every candidate in this list but we only want it for the `text-sm` one. If we don't do this, the `h-4` will get a `line-height: new Set()` which is empty and therefore an intersection won't work anymore.
|
Hey, thanks for the PR! We track some caches on a per designSystem instance to reduce the amount of computation that we have to do. You are correct that subsequent calls influence the caches. While looking at the tests and the implementation, I noticed something where your current test has: designSystem.canonicalizeCandidates(['underline', 'h-4', 'w-4'], options)Which correctly becomes designSystem.canonicalizeCandidates(['underline', 'h-4', 'w-4', 'text-sm'], options)Turns out that this on its own also doesn't work. I added an additional test for this test case, and turns out while doing some debugging fixing this also means that your original test passes. We do collect properties/values so we can use some intersections later. However, we did do something silly where we added some properties of some text utilities to the property/value cache of unrelated candidates (classes). If we only do that for the relevant candidates then we should be good. |
2 participants
Summary
fixes schoero/eslint-plugin-better-tailwindcss#321
This PR fixes an order-sensitive canonicalization bug. This bug caused issues when running eslint-plugin-better-tailwindcss as the order in which files are linted in is not consistent. This caused, in some scenarios,
canonicalizeCandidates(..., { collapse: true, logicalToPhysical: true, rem: 16 })to stop collapsing valid combinations (for exampleh-4 + w-4 -> size-4) after unrelated prior calls.To reproduce this issue:
This should produce a failure like so:
The cause of this bug is that the canonicalization caches used
DefaultMapin places where lookups were expected to be read-only.DefaultMap.getinserts missing entried, which mutated shared cache state during intermediate lookups and made later canonicalization results depend on prior calls.By replacing the use of
DefaultMapwith a plainMap, it avoids inserting into the map on lookup paths. I've polyfilledMap#getOrInsertas it is not widely available yet, and used that where appropriate.Test plan
I wrote a test that fails on
mainbranch, I then fixed the issue, and validated that the test now passes.