Skip to content

fix(canonicalize): prevent collapse cache pollution across calls#19675

Open
camc314 wants to merge 1 commit intotailwindlabs:mainfrom
camc314:c/fix-canonicalizeCandidates
Open

fix(canonicalize): prevent collapse cache pollution across calls#19675
camc314 wants to merge 1 commit intotailwindlabs:mainfrom
camc314:c/fix-canonicalizeCandidates

Conversation

@camc314
Copy link

@camc314 camc314 commented Feb 14, 2026

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 example h-4 + w-4 -> size-4) after unrelated prior calls.

To reproduce this issue:

# checkout this branch
$ git checkout c/fix-canonicalizeCandidates
# Revert the fix to the current `main` branch
$ git checkout main ./packages/tailwindcss/src/canonicalize-candidates.ts
# Run the tests
$ pnpm run test

This should produce a failure like so:


 FAIL   tailwindcss  src/canonicalize-candidates.test.ts > regressions > collapse canonicalization is not affected by previous calls
AssertionError: expected [ 'underline', 'h-4', 'w-4' ] to deeply equal [ 'underline', 'size-4' ]

- Expected
+ Received

  [
    "underline",
-   "size-4",
+   "h-4",
+   "w-4",
  ]

 ❯ src/canonicalize-candidates.test.ts:1167:66
    1165|     designSystem.canonicalizeCandidates(['underline', 'mb-4'], options)
    1166| 
    1167|     expect(designSystem.canonicalizeCandidates(target, options)).toEqual(['underline', 'size-4'])
       |                                                                  ^
    1168|   })
    1169| })
# reset all changes on this branch
git reset --hard
# run the tests again (they should now pass)
pnpm run test

The cause of this bug is that the canonicalization caches used DefaultMap in places where lookups were expected to be read-only. DefaultMap.get inserts missing entried, which mutated shared cache state during intermediate lookups and made later canonicalization results depend on prior calls.

By replacing the use of DefaultMap with a plain Map, it avoids inserting into the map on lookup paths. I've polyfilled Map#getOrInsert as it is not widely available yet, and used that where appropriate.

Test plan

I wrote a test that fails on main branch, I then fixed the issue, and validated that the test now passes.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 14, 2026

Walkthrough

This pull request introduces a new Map.prototype.getOrInsert utility method and refactors the storage layer in canonicalize-candidates.ts to use Map-based caches instead of nested DefaultMap structures. Storage keys for static utilities and utility properties are updated to use Map<SignatureOptions, Map<string, Map<string, Set<string>>>>. New helper functions encapsulate lookup patterns for static utilities and utility properties. Function signatures are updated, including createUtilityPropertiesCache() which now takes no parameters. Regression tests validate collapse canonicalization consistency across repeated calls.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and concisely describes the main issue being fixed: preventing cache pollution in collapse canonicalization across multiple calls.
Description check ✅ Passed The description comprehensively explains the order-sensitive canonicalization bug, root cause (DefaultMap inserting on lookup), the fix (replacing with Map and polyfilling Map#getOrInsert), and includes reproduction steps and test validation.
Linked Issues check ✅ Passed The PR fully addresses issue #321 by ensuring canonicalizeCandidates produces consistent order-independent results across multiple calls, fixing the cache pollution that prevented valid canonicalizations like h-4 + w-4 -> size-4.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the cache pollution bug: replacing DefaultMap with Map in canonicalization caches, adding Map#getOrInsert polyfill, and adding regression tests demonstrating the fix.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into main

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
packages/tailwindcss/src/canonicalize-candidates.ts (1)

2323-2374: getUtilityPropertiesLookup correctly encapsulates the compute-on-miss logic.

The function handles prefix resolution, candidate parsing, AST walking, and populating both the utility-properties cache and the static-utilities lookup in a single pass. The early return on line 2341 for unparseable candidates is a good guard.

One minor observation: lines 2354-2357 use a get + getOrInsert pattern inside the walk callback, which could be called many times per invocation. Since getOrInsert creates a new Set<string>() only when the key is missing, this is fine — but you could simplify to just getOrInsert directly since the allocation only happens on miss anyway:

Simplification (optional)
-      let values = localPropertyValueLookup.get(node.property)
-      if (!values) {
-        values = localPropertyValueLookup.getOrInsert(node.property, new Set<string>())
-      }
+      let values = localPropertyValueLookup.getOrInsert(node.property, new Set<string>())

Same for lines 2360-2363 and 2365-2368.

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 with Map to avoid mutation on lookup paths.
  • Added a Map#getOrInsert polyfill 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.

Comment on lines +1 to +21
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)!
},
})
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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)!

Copilot uses AI. Check for mistakes.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

plugin does not work correctly with oxlint

1 participant