From 47e54feebaef998b705589f02e29d49ec1a47dcc Mon Sep 17 00:00:00 2001 From: Evan Wallace Date: Mon, 20 Feb 2023 00:25:24 -0500 Subject: [PATCH] fix #2936: compare `url()` tokens by import record --- CHANGELOG.md | 21 ++++ internal/bundler_tests/bundler_css_test.go | 6 + .../bundler_tests/snapshots/snapshots_css.txt | 17 +++ internal/css_ast/css_ast.go | 111 +++++++++++------- internal/css_parser/css_parser.go | 51 ++++++-- internal/linker/linker.go | 2 +- 6 files changed, 156 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 82f2b7b6957..9e906666460 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ # Changelog +## Unreleased + +* Fix cross-file CSS rule deduplication involving `url()` tokens ([#2936](https://github.com/evanw/esbuild/issues/2936)) + + Previously cross-file CSS rule deduplication didn't handle `url()` tokens correctly. These tokens contain references to import paths which may be internal (i.e. in the bundle) or external (i.e. not in the bundle). When comparing two `url()` tokens for equality, the underlying import paths should be compared instead of their references. This release of esbuild fixes `url()` token comparisons. One side effect is that `@font-face` rules should now be deduplicated correctly across files: + + ```css + /* Original code */ + @import "data:text/css, \ + @import 'http://example.com/style.css'; \ + @font-face { src: url(http://example.com/font.ttf) }"; + @import "data:text/css, \ + @font-face { src: url(http://example.com/font.ttf) }"; + + /* Old output (with --bundle --minify) */ + @import"http://example.com/style.css";@font-face{src:url(http://example.com/font.ttf)}@font-face{src:url(http://example.com/font.ttf)} + + /* New output (with --bundle --minify) */ + @import"http://example.com/style.css";@font-face{src:url(http://example.com/font.ttf)} + ``` + ## 0.17.9 * Parse rest bindings in TypeScript types ([#2937](https://github.com/evanw/esbuild/issues/2937)) diff --git a/internal/bundler_tests/bundler_css_test.go b/internal/bundler_tests/bundler_css_test.go index 2080ebb0331..47269a0acc7 100644 --- a/internal/bundler_tests/bundler_css_test.go +++ b/internal/bundler_tests/bundler_css_test.go @@ -775,6 +775,11 @@ func TestDeduplicateRules(t *testing.T) { "/across-files-0.css": "a { color: red; color: red }", "/across-files-1.css": "a { color: green }", "/across-files-2.css": "a { color: red }", + + "/across-files-url.css": "@import 'across-files-url-0.css'; @import 'across-files-url-1.css'; @import 'across-files-url-2.css';", + "/across-files-url-0.css": "@import 'http://example.com/some.css'; @font-face { src: url(http://example.com/some.font); }", + "/across-files-url-1.css": "@font-face { src: url(http://example.com/some.other.font); }", + "/across-files-url-2.css": "@font-face { src: url(http://example.com/some.font); }", }, entryPaths: []string{ "/yes0.css", @@ -790,6 +795,7 @@ func TestDeduplicateRules(t *testing.T) { "/no6.css", "/across-files.css", + "/across-files-url.css", }, options: config.Options{ Mode: config.ModeBundle, diff --git a/internal/bundler_tests/snapshots/snapshots_css.txt b/internal/bundler_tests/snapshots/snapshots_css.txt index c5d5ec2246f..ff71d3e5c86 100644 --- a/internal/bundler_tests/snapshots/snapshots_css.txt +++ b/internal/bundler_tests/snapshots/snapshots_css.txt @@ -327,6 +327,23 @@ a { /* across-files.css */ +---------- /out/across-files-url.css ---------- +@import "http://example.com/some.css"; + +/* across-files-url-0.css */ + +/* across-files-url-1.css */ +@font-face { + src: url(http://example.com/some.other.font); +} + +/* across-files-url-2.css */ +@font-face { + src: url(http://example.com/some.font); +} + +/* across-files-url.css */ + ================================================================================ TestExternalImportURLInCSS ---------- /out/entry.css ---------- diff --git a/internal/css_ast/css_ast.go b/internal/css_ast/css_ast.go index fa9df1ecf11..138bd62b857 100644 --- a/internal/css_ast/css_ast.go +++ b/internal/css_ast/css_ast.go @@ -79,13 +79,42 @@ const ( WhitespaceAfter ) -func (a Token) Equal(b Token) bool { - if a.Kind == b.Kind && a.Text == b.Text && a.ImportRecordIndex == b.ImportRecordIndex && a.Whitespace == b.Whitespace { +// This is necessary when comparing tokens between two different files +type CrossFileEqualityCheck struct { + ImportRecordsA []ast.ImportRecord + ImportRecordsB []ast.ImportRecord +} + +func (a Token) Equal(b Token, check *CrossFileEqualityCheck) bool { + if a.Kind == b.Kind && a.Text == b.Text && a.Whitespace == b.Whitespace { + // URLs should be compared based on the text of the associated import record + // (which is what will actually be printed) instead of the original text + if a.Kind == css_lexer.TURL { + if check == nil { + // If both tokens are in the same file, just compare the index + if a.ImportRecordIndex != b.ImportRecordIndex { + return false + } + } else { + // If the tokens come from separate files, compare the import records + // themselves instead of comparing the indices. This can happen when + // the linker runs a "DuplicateRuleRemover" during bundling. This + // doesn't compare the source indices because at this point during + // linking, paths inside the bundle (e.g. due to the "copy" loader) + // should have already been converted into text (e.g. the "unique key" + // string). + if check.ImportRecordsA[a.ImportRecordIndex].Path.Text != + check.ImportRecordsB[b.ImportRecordIndex].Path.Text { + return false + } + } + } + if a.Children == nil && b.Children == nil { return true } - if a.Children != nil && b.Children != nil && TokensEqual(*a.Children, *b.Children) { + if a.Children != nil && b.Children != nil && TokensEqual(*a.Children, *b.Children, check) { return true } } @@ -93,12 +122,12 @@ func (a Token) Equal(b Token) bool { return false } -func TokensEqual(a []Token, b []Token) bool { +func TokensEqual(a []Token, b []Token, check *CrossFileEqualityCheck) bool { if len(a) != len(b) { return false } - for i, c := range a { - if !c.Equal(b[i]) { + for i, ai := range a { + if !ai.Equal(b[i], check) { return false } } @@ -110,7 +139,9 @@ func HashTokens(hash uint32, tokens []Token) uint32 { for _, t := range tokens { hash = helpers.HashCombine(hash, uint32(t.Kind)) - hash = helpers.HashCombineString(hash, t.Text) + if t.Kind != css_lexer.TURL { + hash = helpers.HashCombineString(hash, t.Text) + } if t.Children != nil { hash = HashTokens(hash, *t.Children) } @@ -262,16 +293,16 @@ type Rule struct { } type R interface { - Equal(rule R) bool + Equal(rule R, check *CrossFileEqualityCheck) bool Hash() (uint32, bool) } -func RulesEqual(a []Rule, b []Rule) bool { +func RulesEqual(a []Rule, b []Rule, check *CrossFileEqualityCheck) bool { if len(a) != len(b) { return false } - for i, c := range a { - if !c.Data.Equal(b[i].Data) { + for i, ai := range a { + if !ai.Data.Equal(b[i].Data, check) { return false } } @@ -294,7 +325,7 @@ type RAtCharset struct { Encoding string } -func (a *RAtCharset) Equal(rule R) bool { +func (a *RAtCharset) Equal(rule R, check *CrossFileEqualityCheck) bool { b, ok := rule.(*RAtCharset) return ok && a.Encoding == b.Encoding } @@ -310,7 +341,7 @@ type RAtImport struct { ImportRecordIndex uint32 } -func (*RAtImport) Equal(rule R) bool { +func (*RAtImport) Equal(rule R, check *CrossFileEqualityCheck) bool { return false } @@ -329,7 +360,7 @@ type KeyframeBlock struct { Rules []Rule } -func (a *RAtKeyframes) Equal(rule R) bool { +func (a *RAtKeyframes) Equal(rule R, check *CrossFileEqualityCheck) bool { if b, ok := rule.(*RAtKeyframes); ok && a.AtToken == b.AtToken && a.Name == b.Name && len(a.Blocks) == len(b.Blocks) { for i, ai := range a.Blocks { bi := b.Blocks[i] @@ -341,7 +372,7 @@ func (a *RAtKeyframes) Equal(rule R) bool { return false } } - if !RulesEqual(ai.Rules, bi.Rules) { + if !RulesEqual(ai.Rules, bi.Rules, check) { return false } } @@ -371,9 +402,9 @@ type RKnownAt struct { Rules []Rule } -func (a *RKnownAt) Equal(rule R) bool { +func (a *RKnownAt) Equal(rule R, check *CrossFileEqualityCheck) bool { b, ok := rule.(*RKnownAt) - return ok && a.AtToken == b.AtToken && TokensEqual(a.Prelude, b.Prelude) && RulesEqual(a.Rules, b.Rules) + return ok && a.AtToken == b.AtToken && TokensEqual(a.Prelude, b.Prelude, check) && RulesEqual(a.Rules, b.Rules, check) } func (r *RKnownAt) Hash() (uint32, bool) { @@ -390,9 +421,9 @@ type RUnknownAt struct { Block []Token } -func (a *RUnknownAt) Equal(rule R) bool { +func (a *RUnknownAt) Equal(rule R, check *CrossFileEqualityCheck) bool { b, ok := rule.(*RUnknownAt) - return ok && a.AtToken == b.AtToken && TokensEqual(a.Prelude, b.Prelude) && TokensEqual(a.Block, b.Block) + return ok && a.AtToken == b.AtToken && TokensEqual(a.Prelude, b.Prelude, check) && TokensEqual(a.Block, b.Block, check) } func (r *RUnknownAt) Hash() (uint32, bool) { @@ -409,15 +440,15 @@ type RSelector struct { HasAtNest bool } -func (a *RSelector) Equal(rule R) bool { +func (a *RSelector) Equal(rule R, check *CrossFileEqualityCheck) bool { b, ok := rule.(*RSelector) if ok && len(a.Selectors) == len(b.Selectors) && a.HasAtNest == b.HasAtNest { - for i, sel := range a.Selectors { - if !sel.Equal(b.Selectors[i]) { + for i, ai := range a.Selectors { + if !ai.Equal(b.Selectors[i], check) { return false } } - return RulesEqual(a.Rules, b.Rules) + return RulesEqual(a.Rules, b.Rules, check) } return false @@ -450,9 +481,9 @@ type RQualified struct { Rules []Rule } -func (a *RQualified) Equal(rule R) bool { +func (a *RQualified) Equal(rule R, check *CrossFileEqualityCheck) bool { b, ok := rule.(*RQualified) - return ok && TokensEqual(a.Prelude, b.Prelude) && RulesEqual(a.Rules, b.Rules) + return ok && TokensEqual(a.Prelude, b.Prelude, check) && RulesEqual(a.Rules, b.Rules, check) } func (r *RQualified) Hash() (uint32, bool) { @@ -470,9 +501,9 @@ type RDeclaration struct { Important bool } -func (a *RDeclaration) Equal(rule R) bool { +func (a *RDeclaration) Equal(rule R, check *CrossFileEqualityCheck) bool { b, ok := rule.(*RDeclaration) - return ok && a.KeyText == b.KeyText && TokensEqual(a.Value, b.Value) && a.Important == b.Important + return ok && a.KeyText == b.KeyText && TokensEqual(a.Value, b.Value, check) && a.Important == b.Important } func (r *RDeclaration) Hash() (uint32, bool) { @@ -500,9 +531,9 @@ type RBadDeclaration struct { Tokens []Token } -func (a *RBadDeclaration) Equal(rule R) bool { +func (a *RBadDeclaration) Equal(rule R, check *CrossFileEqualityCheck) bool { b, ok := rule.(*RBadDeclaration) - return ok && TokensEqual(a.Tokens, b.Tokens) + return ok && TokensEqual(a.Tokens, b.Tokens, check) } func (r *RBadDeclaration) Hash() (uint32, bool) { @@ -515,7 +546,7 @@ type RComment struct { Text string } -func (a *RComment) Equal(rule R) bool { +func (a *RComment) Equal(rule R, check *CrossFileEqualityCheck) bool { b, ok := rule.(*RComment) return ok && a.Text == b.Text } @@ -531,7 +562,7 @@ type RAtLayer struct { Rules []Rule } -func (a *RAtLayer) Equal(rule R) bool { +func (a *RAtLayer) Equal(rule R, check *CrossFileEqualityCheck) bool { if b, ok := rule.(*RAtLayer); ok && len(a.Names) == len(b.Names) && len(a.Rules) == len(b.Rules) { for i, ai := range a.Names { bi := b.Names[i] @@ -544,7 +575,7 @@ func (a *RAtLayer) Equal(rule R) bool { } } } - if !RulesEqual(a.Rules, b.Rules) { + if !RulesEqual(a.Rules, b.Rules, check) { return false } } @@ -568,7 +599,7 @@ type ComplexSelector struct { Selectors []CompoundSelector } -func (a ComplexSelector) Equal(b ComplexSelector) bool { +func (a ComplexSelector) Equal(b ComplexSelector, check *CrossFileEqualityCheck) bool { if len(a.Selectors) != len(b.Selectors) { return false } @@ -589,7 +620,7 @@ func (a ComplexSelector) Equal(b ComplexSelector) bool { return false } for j, aj := range ai.SubclassSelectors { - if !aj.Equal(bi.SubclassSelectors[j]) { + if !aj.Equal(bi.SubclassSelectors[j], check) { return false } } @@ -632,7 +663,7 @@ func (a NamespacedName) Equal(b NamespacedName) bool { } type SS interface { - Equal(ss SS) bool + Equal(ss SS, check *CrossFileEqualityCheck) bool Hash() uint32 } @@ -640,7 +671,7 @@ type SSHash struct { Name string } -func (a *SSHash) Equal(ss SS) bool { +func (a *SSHash) Equal(ss SS, check *CrossFileEqualityCheck) bool { b, ok := ss.(*SSHash) return ok && a.Name == b.Name } @@ -655,7 +686,7 @@ type SSClass struct { Name string } -func (a *SSClass) Equal(ss SS) bool { +func (a *SSClass) Equal(ss SS, check *CrossFileEqualityCheck) bool { b, ok := ss.(*SSClass) return ok && a.Name == b.Name } @@ -673,7 +704,7 @@ type SSAttribute struct { MatcherModifier byte // Either 0 or one of: 'i' 'I' 's' 'S' } -func (a *SSAttribute) Equal(ss SS) bool { +func (a *SSAttribute) Equal(ss SS, check *CrossFileEqualityCheck) bool { b, ok := ss.(*SSAttribute) return ok && a.NamespacedName.Equal(b.NamespacedName) && a.MatcherOp == b.MatcherOp && a.MatcherValue == b.MatcherValue && a.MatcherModifier == b.MatcherModifier @@ -693,9 +724,9 @@ type SSPseudoClass struct { IsElement bool // If true, this is prefixed by "::" instead of ":" } -func (a *SSPseudoClass) Equal(ss SS) bool { +func (a *SSPseudoClass) Equal(ss SS, check *CrossFileEqualityCheck) bool { b, ok := ss.(*SSPseudoClass) - return ok && a.Name == b.Name && TokensEqual(a.Args, b.Args) && a.IsElement == b.IsElement + return ok && a.Name == b.Name && TokensEqual(a.Args, b.Args, check) && a.IsElement == b.IsElement } func (ss *SSPseudoClass) Hash() uint32 { diff --git a/internal/css_parser/css_parser.go b/internal/css_parser/css_parser.go index 19d27281b70..70eacbd0fcd 100644 --- a/internal/css_parser/css_parser.go +++ b/internal/css_parser/css_parser.go @@ -290,7 +290,7 @@ loop: } if p.options.MinifySyntax { - rules = mangleRules(rules, context.isTopLevel) + rules = p.mangleRules(rules, context.isTopLevel) } return rules } @@ -305,7 +305,7 @@ func (p *parser) parseListOfDeclarations() (list []css_ast.Rule) { case css_lexer.TEndOfFile, css_lexer.TCloseBrace: list = p.processDeclarations(list) if p.options.MinifySyntax { - list = mangleRules(list, false /* isTopLevel */) + list = p.mangleRules(list, false /* isTopLevel */) } return @@ -325,7 +325,7 @@ func (p *parser) parseListOfDeclarations() (list []css_ast.Rule) { } } -func mangleRules(rules []css_ast.Rule, isTopLevel bool) []css_ast.Rule { +func (p *parser) mangleRules(rules []css_ast.Rule, isTopLevel bool) []css_ast.Rule { type hashEntry struct { indices []uint32 } @@ -378,12 +378,12 @@ func mangleRules(rules []css_ast.Rule, isTopLevel bool) []css_ast.Rule { // "a { color: red; } b { color: red; }" => "a, b { color: red; }" if prevNonComment != nil { if r, ok := rule.Data.(*css_ast.RSelector); ok { - if prev, ok := prevNonComment.(*css_ast.RSelector); ok && css_ast.RulesEqual(r.Rules, prev.Rules) && + if prev, ok := prevNonComment.(*css_ast.RSelector); ok && css_ast.RulesEqual(r.Rules, prev.Rules, nil) && isSafeSelectors(r.Selectors) && isSafeSelectors(prev.Selectors) { nextSelector: for _, sel := range r.Selectors { for _, prevSel := range prev.Selectors { - if sel.Equal(prevSel) { + if sel.Equal(prevSel, nil) { // Don't add duplicate selectors more than once continue nextSelector } @@ -411,25 +411,39 @@ func mangleRules(rules []css_ast.Rule, isTopLevel bool) []css_ast.Rule { // Mangle non-top-level rules using a back-to-front pass. Top-level rules // will be mangled by the linker instead for cross-file rule mangling. if !isTopLevel { - rules = MakeDuplicateRuleMangler().RemoveDuplicateRulesInPlace(rules) + remover := MakeDuplicateRuleMangler() + rules = remover.RemoveDuplicateRulesInPlace(rules, p.importRecords) } return rules } +type ruleEntry struct { + data css_ast.R + callCounter uint32 +} + type hashEntry struct { - rules []css_ast.R + rules []ruleEntry } type DuplicateRuleRemover struct { entries map[uint32]hashEntry + calls [][]ast.ImportRecord + check css_ast.CrossFileEqualityCheck } func MakeDuplicateRuleMangler() DuplicateRuleRemover { return DuplicateRuleRemover{entries: make(map[uint32]hashEntry)} } -func (remover DuplicateRuleRemover) RemoveDuplicateRulesInPlace(rules []css_ast.Rule) []css_ast.Rule { +func (remover *DuplicateRuleRemover) RemoveDuplicateRulesInPlace(rules []css_ast.Rule, importRecords []ast.ImportRecord) []css_ast.Rule { + // The caller may call this function multiple times, each with a different + // set of import records. Remember each set of import records for equality + // checks later. + callCounter := uint32(len(remover.calls)) + remover.calls = append(remover.calls, importRecords) + // Remove duplicate rules, scanning from the back so we keep the last // duplicate. Note that the linker calls this, so we do not want to do // anything that modifies the rules themselves. One reason is that ASTs @@ -445,12 +459,27 @@ skipRule: // For duplicate rules, omit all but the last copy if hash, ok := rule.Data.Hash(); ok { entry := remover.entries[hash] - for _, data := range entry.rules { - if rule.Data.Equal(data) { + for _, current := range entry.rules { + var check *css_ast.CrossFileEqualityCheck + + // If this rule was from another file, then pass along both arrays + // of import records so that the equality check for "url()" tokens + // can use them to check for equality. + if current.callCounter != callCounter { + // Reuse the same memory allocation + check = &remover.check + check.ImportRecordsA = importRecords + check.ImportRecordsB = remover.calls[current.callCounter] + } + + if rule.Data.Equal(current.data, check) { continue skipRule } } - entry.rules = append(entry.rules, rule.Data) + entry.rules = append(entry.rules, ruleEntry{ + data: rule.Data, + callCounter: callCounter, + }) remover.entries[hash] = entry } diff --git a/internal/linker/linker.go b/internal/linker/linker.go index 67a29e974cc..86d85ad6d97 100644 --- a/internal/linker/linker.go +++ b/internal/linker/linker.go @@ -5225,7 +5225,7 @@ func (c *linkerContext) generateChunkCSS(chunkIndex int, chunkWaitGroup *sync.Wa // Remove top-level duplicate rules across files if c.options.MinifySyntax { - rules = remover.RemoveDuplicateRulesInPlace(rules) + rules = remover.RemoveDuplicateRulesInPlace(rules, ast.ImportRecords) } ast.Rules = rules