improve converting algorithm

packages/@tailwindcss-upgrade/src/template/codemods/theme-to-var.test.ts

@@ -3,8 +3,12 @@ import { expect, test } from 'vitest'
 import { themeToVar } from './theme-to-var'
 
 test.each([
+  // Keep candidates that don't contain `theme(…)` or `theme(…, …)`
+  ['[color:red]', '[color:red]'],
+
   // Convert to `var(…)` if we can resolve the path
   ['[color:theme(colors.red.500)]', '[color:var(--color-red-500)]'], // Arbitrary property
+  ['[color:theme(colors.red.500)]/50', '[color:var(--color-red-500)]/50'], // Arbitrary property + modifier
   ['bg-[theme(colors.red.500)]', 'bg-[var(--color-red-500)]'], // Arbitrary value
   ['bg-[size:theme(spacing.4)]', 'bg-[size:var(--spacing-4)]'], // Arbitrary value + data type hint
 
@@ -77,6 +81,20 @@ test.each([
 
   // `theme(…)` calls valid in v3, but not in v4 should still be converted.
   ['[--foo:theme(fontWeight.semibold)]', '[--foo:theme(fontWeight.semibold)]'],
+
+  // Invalid cases
+  ['[--foo:theme(colors.red.500/50/50)]', '[--foo:theme(colors.red.500/50/50)]'],
+  ['[--foo:theme(colors.red.500/50/50)]/50', '[--foo:theme(colors.red.500/50/50)]/50'],
+
+  // Partially invalid cases
+  [
+    '[--foo:theme(colors.red.500/50/50)_theme(colors.blue.200)]',
+    '[--foo:theme(colors.red.500/50/50)_var(--color-blue-200)]',
+  ],
+  [
+    '[--foo:theme(colors.red.500/50/50)_theme(colors.blue.200)]/50',
+    '[--foo:theme(colors.red.500/50/50)_var(--color-blue-200)]/50',
+  ],
 ])('%s => %s', async (candidate, result) => {
   let designSystem = await __unstable__loadDesignSystem('@import "tailwindcss";', {
     base: __dirname,

packages/@tailwindcss-upgrade/src/template/codemods/theme-to-var.ts

@@ -12,147 +12,201 @@ import { toKeyPath } from '../../../../tailwindcss/src/utils/to-key-path'
 import * as ValueParser from '../../../../tailwindcss/src/value-parser'
 import { printCandidate } from '../candidates'
 
+enum Convert {
+  None = 0,
+  MigrateModifier = 1 << 0,
+  MigrateThemeOnly = 1 << 1,
+}
+
 export function themeToVar(
   designSystem: DesignSystem,
   _userConfig: Config,
   rawCandidate: string,
 ): string {
-  function convertBasic(input: string, modernizeThemeOnly = false) {
-    return substituteFunctionsInValue(input, (path, fallback) => {
-      let parts = segment(path, '/').map((part) => part.trim())
+  for (let candidate of parseCandidate(rawCandidate, designSystem)) {
+    let clone = structuredClone(candidate)
+    let changed = false
 
-      if (
-        // The path contains a `/`, which means that there is a modifier such as
-        // `theme(colors.red.500/50%)`. We can't convert this to a CSS variable.
-        parts.length > 1 ||
-        // In some scenarios (variants), we can't migrate to `var(…)` if it ends
-        // up in the `@media (…)` part. To be safe, let's just modernize the
-        // `theme(…)`
-        modernizeThemeOnly
-      ) {
-        let path = parts.shift()!
-
-        // Convert to modern `theme(…)` syntax using CSS variable syntax if we
-        // can.
-        let variable = `--${keyPathToCssProperty(toKeyPath(path))}` as const
-
-        if (!designSystem.theme.get([variable])) {
-          return null
+    if (clone.kind === 'arbitrary') {
+      let [newValue, modifier] = convert(
+        clone.value,
+        clone.modifier === null ? Convert.MigrateModifier : Convert.None,
+      )
+      if (newValue !== clone.value) {
+        changed = true
+        clone.value = newValue
+
+        if (modifier !== null) {
+          clone.modifier = modifier
         }
+      }
+    } else if (clone.kind === 'functional' && clone.value?.kind === 'arbitrary') {
+      let [newValue, modifier] = convert(
+        clone.value.value,
+        clone.modifier === null ? Convert.MigrateModifier : Convert.None,
+      )
+      if (newValue !== clone.value.value) {
+        changed = true
+        clone.value.value = newValue
 
-        let modifier = parts.length > 0 ? `/${parts.join('/')}` : ''
+        if (modifier !== null) {
+          clone.modifier = modifier
+        }
+      }
+    }
 
-        return fallback
-          ? `theme(${variable}${modifier}, ${fallback})`
-          : `theme(${variable}${modifier})`
+    // Handle variants
+    for (let variant of variants(clone)) {
+      if (variant.kind === 'arbitrary') {
+        let [newValue] = convert(variant.selector, Convert.MigrateThemeOnly)
+        if (newValue !== variant.selector) {
+          changed = true
+          variant.selector = newValue
+        }
+      } else if (variant.kind === 'functional' && variant.value?.kind === 'arbitrary') {
+        let [newValue] = convert(variant.value.value, Convert.MigrateThemeOnly)
+        if (newValue !== variant.value.value) {
+          changed = true
+          variant.value.value = newValue
+        }
       }
+    }
 
-      // Convert to `var(…)`
-      let variable = `--${keyPathToCssProperty(toKeyPath(path))}` as const
-      if (!designSystem.theme.get([variable])) return null
+    return changed ? printCandidate(designSystem, clone) : rawCandidate
+  }
 
-      return fallback ? `var(${variable}, ${fallback})` : `var(${variable})`
+  function convert(input: string, options = Convert.None): [string, CandidateModifier | null] {
+    // In some scenarios (e.g.: variants), we can't migrate to `var(…)` if it
+    // ends up in the `@media (…)` part. In this case we only have to migrate to
+    // the new `theme(…)` notation.
+    if (options & Convert.MigrateThemeOnly) {
+      return [substituteFunctionsInValue(input, toTheme), null]
+    }
+
+    let ast = ValueParser.parse(input)
+
+    let themeUsageCount = 0
+    let themeModifierCount = 0
+
+    // Analyze AST
+    ValueParser.walk(ast, (node) => {
+      if (node.kind !== 'function') return
+      if (node.value !== 'theme') return
+
+      // We are only interested in the `theme` function
+      themeUsageCount += 1
+
+      // Figure out if a modifier is used
+      ValueParser.walk(node.nodes, (child) => {
+        // If we see a `,`, it means that we have a fallback value
+        if (child.kind === 'separator' && child.value.includes(',')) {
+          return ValueParser.ValueWalkAction.Stop
+        }
+
+        // If we see a `/`, we have a modifier
+        else if (child.kind === 'separator' && child.value === '/') {
+          themeModifierCount += 1
+          return ValueParser.ValueWalkAction.Stop
+        }
+
+        return ValueParser.ValueWalkAction.Skip
+      })
     })
-  }
 
-  function convert(input: string, containsModifier = false) {
-    if (containsModifier) return [convertBasic(input), null] as const
+    // No `theme(…)` calls, nothing to do
+    if (themeUsageCount === 0) {
+      return [input, null]
+    }
+
+    // No `theme(…)` with modifiers, we can migrate to `var(…)`
+    if (themeModifierCount === 0) {
+      return [substituteFunctionsInValue(input, toVar), null]
+    }
 
-    let modifiers: CandidateModifier[] = []
+    // Multiple modifiers which means that there are multiple `theme(…/…)`
+    // values. In this case, we can't convert the modifier to a candidate
+    // modifier.
+    //
+    // We also can't migrate to `var(…)` because that would lose the modifier.
+    //
+    // Try to convert each `theme(…)` call to the modern syntax.
+    if (themeModifierCount > 1) {
+      return [substituteFunctionsInValue(input, toTheme), null]
+    }
+
+    // Only a single `theme(…)` with a modifier left, that modifier will be
+    // migrated to a candidate modifier.
+    let modifier: CandidateModifier | null = null
     let result = substituteFunctionsInValue(input, (path, fallback) => {
       let parts = segment(path, '/').map((part) => part.trim())
 
+      // Multiple `/` separators, which makes this an invalid path
+      if (parts.length > 2) {
+        return null
+      }
+
       // The path contains a `/`, which means that there is a modifier such as
       // `theme(colors.red.500/50%)`.
       //
       // Currently, we are assuming that this is only being used for colors,
       // which means that we can typically convert them to a modifier on the
       // candidate itself.
-      if (parts.length === 2) {
+      if (parts.length === 2 && options & Convert.MigrateModifier) {
         let [pathPart, modifierPart] = parts
 
         // 50% -> /50
         if (/^\d+%$/.test(modifierPart)) {
-          modifiers.push({ kind: 'named', value: modifierPart.slice(0, -1) })
+          modifier = { kind: 'named', value: modifierPart.slice(0, -1) }
         }
 
         // .12 -> /12
         // .12345 -> /[12.345]
         else if (/^0?\.\d+$/.test(modifierPart)) {
           let value = Number(modifierPart) * 100
-          modifiers.push({
+          modifier = {
             kind: Number.isInteger(value) ? 'named' : 'arbitrary',
             value: value.toString(),
-          })
+          }
         }
 
         // Anything else becomes arbitrary
         else {
-          modifiers.push({ kind: 'arbitrary', value: modifierPart })
+          modifier = { kind: 'arbitrary', value: modifierPart }
         }
 
         // Update path to be the first part
         path = pathPart
       }
 
-      let variable = `--${keyPathToCssProperty(toKeyPath(path))}` as const
-      if (!designSystem.theme.get([variable])) return null
-
-      return fallback ? `var(${variable}, ${fallback})` : `var(${variable})`
+      return toVar(path, fallback) || toTheme(path, fallback)
     })
 
-    // Multiple modifiers which means that there are multiple `theme(…/…)`
-    // values. In this case, we can't convert the modifier to a candidate
-    // modifier. Try to convert each `theme(…)` call to the modern syntax.
-    if (modifiers.length > 1) return [convertBasic(input), null] as const
-
-    return [result, modifiers[0]] as const
+    return [result, modifier]
   }
 
-  for (let candidate of parseCandidate(rawCandidate, designSystem)) {
-    let clone = structuredClone(candidate)
-    let changed = false
+  function pathToVariableName(path: string) {
+    let variable = `--${keyPathToCssProperty(toKeyPath(path))}` as const
+    if (!designSystem.theme.get([variable])) return null
 
-    if (clone.kind === 'arbitrary') {
-      let [newValue, modifier] = convert(clone.value, clone.modifier !== null)
-      if (newValue !== clone.value) {
-        changed = true
-        clone.value = newValue
+    return variable
+  }
 
-        if (modifier !== null) {
-          clone.modifier = modifier
-        }
-      }
-    } else if (clone.kind === 'functional' && clone.value?.kind === 'arbitrary') {
-      let [newValue, modifier] = convert(clone.value.value, clone.modifier !== null)
-      if (newValue !== clone.value.value) {
-        changed = true
-        clone.value.value = newValue
+  function toVar(path: string, fallback?: string) {
+    let variable = pathToVariableName(path)
+    if (!variable) return null
 
-        if (modifier !== null) {
-          clone.modifier = modifier
-        }
-      }
-    }
+    return fallback ? `var(${variable}, ${fallback})` : `var(${variable})`
+  }
 
-    // Handle variants
-    for (let variant of variants(clone)) {
-      if (variant.kind === 'arbitrary') {
-        let newValue = convertBasic(variant.selector, true)
-        if (newValue !== variant.selector) {
-          changed = true
-          variant.selector = newValue
-        }
-      } else if (variant.kind === 'functional' && variant.value?.kind === 'arbitrary') {
-        let newValue = convertBasic(variant.value.value, true)
-        if (newValue !== variant.value.value) {
-          changed = true
-          variant.value.value = newValue
-        }
-      }
-    }
+  function toTheme(path: string, fallback?: string) {
+    let parts = segment(path, '/').map((part) => part.trim())
+    path = parts.shift()!
 
-    return changed ? printCandidate(designSystem, clone) : rawCandidate
+    let variable = pathToVariableName(path)
+    if (!variable) return null
+
+    let modifier = parts.length > 0 ? `/${parts.join('/')}` : ''
+    return fallback ? `theme(${variable}${modifier}, ${fallback})` : `theme(${variable}${modifier})`
   }
 
   return rawCandidate

packages/tailwindcss/src/value-parser.ts

@@ -38,7 +38,7 @@ function separator(value: string): ValueSeparatorNode {
   }
 }
 
-enum ValueWalkAction {
+export enum ValueWalkAction {
   /** Continue walking, which is the default */
   Continue,