From 02cfc45057101e22faf86c010795745fe262bed5 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Tue, 7 Jan 2025 16:19:37 +0100 Subject: [PATCH] Ensure `@utility` is processed before using them (#15542) This PR fixes an issue where using an `@utility` before it is defined, and _if_ that `@utility` contains `@apply`, that it won't result in the expected output. But results in an `@apply` rule that is not substituted. Additionally, if you have multiple levels of `@apply`, we have to make sure that everything is applied (no pun intended) in the right order. Right now, the following steps are taken: 1. Collect all the `@utility` at-rules (and register them in the system as utilities). 2. Substitute `@apply` on the AST (including `@utility`'s ASTs) with the content of the utility. 3. Delete the `@utility` at-rules such that they are removed from the CSS output itself. The reason we do it in this order is because handling `@apply` during `@utility` handling means that we could rely on another `@utility` that is defined later and therefore the order of the utilities starts to matter. This is not a bad thing, but the moment you import multiple CSS files or plugins, this could become hard to manage. Another important step is that when using `@utility foo`, the implementation creates a `structuredClone` from its AST when first using the utility. The reason we do that is because `foo` and `foo!` generate different output and we don't want to accidentally mutate the same AST. This structured clone is the start of the problem in the linked issue (#15501). If we don't do the structured clone, then substituting the `@apply` rules would work, but then `foo` and `foo!` will generate the same output, which is bad. The linked issue has this structure: ```css .foo { @apply bar; } @utility bar { @apply flex; } ``` If we follow the steps above, this would substitute `@apply bar` first, which results in: ```css .foo { @apply flex; } ``` But the `bar` utility, was already cloned (and cached) so now we end up with an `@apply` rule that is not substituted. To properly solve this problem, we have to make sure that we collect all the `@apply` at-rules, and apply them in the correct order. To do this, we run a topological sort on them which ensures that all the dependencies are applied before substituting the current `@apply`. This means that in the above example, in order to process `@apply bar`, we have to process the `bar` utility first. If we run into a circular dependency, then we will throw an error like before. You'll notice that the error message in this PR is updated to a different spot. This one is a bit easier to grasp because it shows the error where the circular dependency _starts_ not where it _ends_ (and completes the circle). The previous message was not wrong (since it's a circle), but now it's a bit easier to reason about. Fixes: #15501 --- CHANGELOG.md | 1 + packages/tailwindcss/src/apply.ts | 189 +++++++++++++++++---- packages/tailwindcss/src/index.test.ts | 47 +++++ packages/tailwindcss/src/index.ts | 4 +- packages/tailwindcss/src/utilities.test.ts | 33 +++- 5 files changed, 237 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 787f17fe2..63ae55988 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix `inset-shadow-*` suggestions in IntelliSense ([#15471](https://github.com/tailwindlabs/tailwindcss/pull/15471)) - Only compile arbitrary values ending in `]` ([#15503](https://github.com/tailwindlabs/tailwindcss/pull/15503)) - Improve performance and memory usage ([#15529](https://github.com/tailwindlabs/tailwindcss/pull/15529)) +- Ensure `@apply` rules are processed in the correct order ([#15542](https://github.com/tailwindlabs/tailwindcss/pull/15542)) - _Upgrade (experimental)_: Do not extract class names from functions (e.g. `shadow` in `filter: 'drop-shadow(…)'`) ([#15566](https://github.com/tailwindlabs/tailwindcss/pull/15566)) ### Changed diff --git a/packages/tailwindcss/src/apply.ts b/packages/tailwindcss/src/apply.ts index 7af9cf10d..6a1fcdefe 100644 --- a/packages/tailwindcss/src/apply.ts +++ b/packages/tailwindcss/src/apply.ts @@ -1,12 +1,29 @@ import { Features } from '.' -import { walk, WalkAction, type AstNode } from './ast' +import { rule, toCss, walk, WalkAction, type AstNode } from './ast' import { compileCandidates } from './compile' import type { DesignSystem } from './design-system' -import { escape } from './utils/escape' +import { DefaultMap } from './utils/default-map' export function substituteAtApply(ast: AstNode[], designSystem: DesignSystem) { let features = Features.None - walk(ast, (node, { replaceWith }) => { + + // Wrap the whole AST in a root rule to make sure there is always a parent + // available for `@apply` at-rules. In some cases, the incoming `ast` just + // contains `@apply` at-rules which means that there is no proper parent to + // rely on. + let root = rule('&', ast) + + // Track all nodes containing `@apply` + let parents = new Set() + + // Track all the dependencies of an `AstNode` + let dependencies = new DefaultMap>(() => new Set()) + + // Track all `@utility` definitions by its root (name) + let definitions = new DefaultMap(() => new Set()) + + // Collect all new `@utility` definitions and all `@apply` rules first + walk([root], (node, { parent }) => { if (node.kind !== 'at-rule') return // Do not allow `@apply` rules inside `@keyframes` rules. @@ -19,9 +36,119 @@ export function substituteAtApply(ast: AstNode[], designSystem: DesignSystem) { return WalkAction.Skip } - if (node.name !== '@apply') return - features |= Features.AtApply + // `@utility` defines a utility, which is important information in order to + // do a correct topological sort later on. + if (node.name === '@utility') { + let name = node.params.replace(/-\*$/, '') + definitions.get(name).add(node) + // In case `@apply` rules are used inside `@utility` rules. + walk(node.nodes, (child) => { + if (child.kind !== 'at-rule' || child.name !== '@apply') return + + parents.add(node) + + for (let dependency of resolveApplyDependencies(child, designSystem)) { + dependencies.get(node).add(dependency) + } + }) + return + } + + // Any other `@apply` node. + if (node.name === '@apply') { + // `@apply` cannot be top-level, so we need to have a parent such that we + // can replace the `@apply` node with the actual utility classes later. + if (parent === null) return + + features |= Features.AtApply + + parents.add(parent) + + for (let dependency of resolveApplyDependencies(node, designSystem)) { + dependencies.get(parent).add(dependency) + } + } + }) + + // Topological sort before substituting `@apply` + let seen = new Set() + let sorted: AstNode[] = [] + let wip = new Set() + + function visit(node: AstNode, path: AstNode[] = []) { + if (seen.has(node)) { + return + } + + // Circular dependency detected + if (wip.has(node)) { + // Next node in the path is the one that caused the circular dependency + let next = path[(path.indexOf(node) + 1) % path.length] + + if ( + node.kind === 'at-rule' && + node.name === '@utility' && + next.kind === 'at-rule' && + next.name === '@utility' + ) { + walk(node.nodes, (child) => { + if (child.kind !== 'at-rule' || child.name !== '@apply') return + + let candidates = child.params.split(/\s+/g) + for (let candidate of candidates) { + for (let candidateAstNode of designSystem.parseCandidate(candidate)) { + switch (candidateAstNode.kind) { + case 'arbitrary': + break + + case 'static': + case 'functional': + if (next.params.replace(/-\*$/, '') === candidateAstNode.root) { + throw new Error( + `You cannot \`@apply\` the \`${candidate}\` utility here because it creates a circular dependency.`, + ) + } + break + + default: + candidateAstNode satisfies never + } + } + } + }) + } + + // Generic fallback error in case we cannot properly detect the origin of + // the circular dependency. + throw new Error( + `Circular dependency detected:\n\n${toCss([node])}\nRelies on:\n\n${toCss([next])}`, + ) + } + + wip.add(node) + + for (let dependencyId of dependencies.get(node)) { + for (let dependency of definitions.get(dependencyId)) { + path.push(node) + visit(dependency, path) + path.pop() + } + } + + seen.add(node) + wip.delete(node) + + sorted.push(node) + } + + for (let node of parents) { + visit(node) + } + + // Substitute the `@apply` at-rules in order + walk(sorted, (node, { replaceWith }) => { + if (node.kind !== 'at-rule' || node.name !== '@apply') return let candidates = node.params.split(/\s+/g) // Replace the `@apply` rule with the actual utility classes @@ -48,35 +175,33 @@ export function substituteAtApply(ast: AstNode[], designSystem: DesignSystem) { } } - // Verify that we don't have any circular dependencies by verifying that - // the current node does not appear in the new nodes. - walk(newNodes, (child) => { - if (child !== node) return - - // At this point we already know that we have a circular dependency. - // - // Figure out which candidate caused the circular dependency. This will - // help to create a useful error message for the end user. - for (let candidate of candidates) { - let selector = `.${escape(candidate)}` - - for (let rule of candidateAst) { - if (rule.kind !== 'rule') continue - if (rule.selector !== selector) continue - - walk(rule.nodes, (child) => { - if (child !== node) return - - throw new Error( - `You cannot \`@apply\` the \`${candidate}\` utility here because it creates a circular dependency.`, - ) - }) - } - } - }) - replaceWith(newNodes) } }) + return features } + +function* resolveApplyDependencies( + node: Extract, + designSystem: DesignSystem, +) { + for (let candidate of node.params.split(/\s+/g)) { + for (let node of designSystem.parseCandidate(candidate)) { + switch (node.kind) { + case 'arbitrary': + // Doesn't matter, because there is no lookup needed + break + + case 'static': + case 'functional': + // Lookup by "root" + yield node.root + break + + default: + node satisfies never + } + } + } +} diff --git a/packages/tailwindcss/src/index.test.ts b/packages/tailwindcss/src/index.test.ts index de59e08be..679d3f170 100644 --- a/packages/tailwindcss/src/index.test.ts +++ b/packages/tailwindcss/src/index.test.ts @@ -461,6 +461,53 @@ describe('@apply', () => { }" `) }) + + it('should recursively apply with custom `@utility`, which is used before it is defined', async () => { + expect( + await compileCss( + css` + @tailwind utilities; + + @layer base { + body { + @apply a; + } + } + + @utility a { + @apply b; + } + + @utility b { + @apply focus:c; + } + + @utility c { + @apply my-flex!; + } + + @utility my-flex { + @apply flex; + } + `, + ['a', 'b', 'c', 'flex', 'my-flex'], + ), + ).toMatchInlineSnapshot(` + ".a:focus, .b:focus, .c { + display: flex !important; + } + + .flex, .my-flex { + display: flex; + } + + @layer base { + body:focus { + display: flex !important; + } + }" + `) + }) }) describe('arbitrary variants', () => { diff --git a/packages/tailwindcss/src/index.ts b/packages/tailwindcss/src/index.ts index cbfad6b3f..161b1367d 100644 --- a/packages/tailwindcss/src/index.ts +++ b/packages/tailwindcss/src/index.ts @@ -538,10 +538,8 @@ async function parseCss( node.context = {} } - // Replace `@apply` rules with the actual utility classes. - features |= substituteAtApply(ast, designSystem) - features |= substituteFunctions(ast, designSystem.resolveThemeValue) + features |= substituteAtApply(ast, designSystem) // Remove `@utility`, we couldn't replace it before yet because we had to // handle the nested `@apply` at-rules first. diff --git a/packages/tailwindcss/src/utilities.test.ts b/packages/tailwindcss/src/utilities.test.ts index bd6d29a3b..9cbd0fc04 100644 --- a/packages/tailwindcss/src/utilities.test.ts +++ b/packages/tailwindcss/src/utilities.test.ts @@ -17375,7 +17375,7 @@ describe('custom utilities', () => { ['foo', 'bar'], ), ).rejects.toThrowErrorMatchingInlineSnapshot( - `[Error: You cannot \`@apply\` the \`dark:foo\` utility here because it creates a circular dependency.]`, + `[Error: You cannot \`@apply\` the \`hover:bar\` utility here because it creates a circular dependency.]`, ) }) @@ -17406,7 +17406,36 @@ describe('custom utilities', () => { ['foo', 'bar'], ), ).rejects.toThrowErrorMatchingInlineSnapshot( - `[Error: You cannot \`@apply\` the \`dark:foo\` utility here because it creates a circular dependency.]`, + `[Error: You cannot \`@apply\` the \`hover:bar\` utility here because it creates a circular dependency.]`, + ) + }) + + test('custom utilities with `@apply` causing circular dependencies should error (multiple levels)', async () => { + await expect(() => + compileCss( + css` + body { + @apply foo; + } + + @utility foo { + @apply flex-wrap hover:bar; + } + + @utility bar { + @apply flex dark:baz; + } + + @utility baz { + @apply flex-wrap hover:foo; + } + + @tailwind utilities; + `, + ['foo', 'bar'], + ), + ).rejects.toThrowErrorMatchingInlineSnapshot( + `[Error: You cannot \`@apply\` the \`hover:bar\` utility here because it creates a circular dependency.]`, ) }) })