diff --git a/internal/bundler_tests/bundler_css_test.go b/internal/bundler_tests/bundler_css_test.go index 83328f4ea0f..42ede9a2fa4 100644 --- a/internal/bundler_tests/bundler_css_test.go +++ b/internal/bundler_tests/bundler_css_test.go @@ -855,6 +855,101 @@ func TestImportCSSFromJSComposesFromCircular(t *testing.T) { }) } +func TestImportCSSFromJSComposesFromUndefined(t *testing.T) { + note := "NOTE: The specification of \"composes\" does not define an order when class declarations from separate files are composed together. " + + "The value of the \"zoom\" property for \"foo\" may change unpredictably as the code is edited. " + + "Make sure that all definitions of \"zoom\" for \"foo\" are in a single file." + css_suite.expectBundled(t, bundled{ + files: map[string]string{ + "/entry.js": ` + import styles from "./styles.css" + console.log(styles) + `, + "/styles.css": ` + @import "well-defined.css"; + @import "undefined/case1.css"; + @import "undefined/case2.css"; + @import "undefined/case3.css"; + @import "undefined/case4.css"; + @import "undefined/case5.css"; + `, + "/well-defined.css": ` + .z1 { composes: z2; zoom: 1; } + .z2 { zoom: 2; } + + .z4 { zoom: 4; } + .z3 { composes: z4; zoom: 3; } + + .z5 { composes: foo bar from "file-1.css"; } + `, + "/undefined/case1.css": ` + .foo { + composes: foo from "../file-1.css"; + zoom: 2; + } + `, + "/undefined/case2.css": ` + .foo { + composes: foo from "../file-1.css"; + composes: foo from "../file-2.css"; + } + `, + "/undefined/case3.css": ` + .foo { composes: nested1 nested2; } + .nested1 { zoom: 3; } + .nested2 { composes: foo from "../file-2.css"; } + `, + "/undefined/case4.css": ` + .foo { composes: nested1 nested2; } + .nested1 { composes: foo from "../file-1.css"; } + .nested2 { zoom: 3; } + `, + "/undefined/case5.css": ` + .foo { composes: nested1 nested2; } + .nested1 { composes: foo from "../file-1.css"; } + .nested2 { composes: foo from "../file-2.css"; } + `, + "/file-1.css": ` + .foo { zoom: 1; } + .bar { zoom: 2; } + `, + "/file-2.css": ` + .foo { zoom: 2; } + `, + }, + entryPaths: []string{"/entry.js"}, + options: config.Options{ + Mode: config.ModeBundle, + AbsOutputDir: "/out", + ExtensionToLoader: map[string]config.Loader{ + ".js": config.LoaderJS, + ".css": config.LoaderLocalCSS, + }, + }, + expectedCompileLog: `undefined/case1.css: WARNING: The value of "zoom" in the "foo" class is undefined +file-1.css: NOTE: The first definition of "zoom" is here: +undefined/case1.css: NOTE: The second definition of "zoom" is here: +` + note + ` +undefined/case2.css: WARNING: The value of "zoom" in the "foo" class is undefined +file-1.css: NOTE: The first definition of "zoom" is here: +file-2.css: NOTE: The second definition of "zoom" is here: +` + note + ` +undefined/case3.css: WARNING: The value of "zoom" in the "foo" class is undefined +undefined/case3.css: NOTE: The first definition of "zoom" is here: +file-2.css: NOTE: The second definition of "zoom" is here: +` + note + ` +undefined/case4.css: WARNING: The value of "zoom" in the "foo" class is undefined +file-1.css: NOTE: The first definition of "zoom" is here: +undefined/case4.css: NOTE: The second definition of "zoom" is here: +` + note + ` +undefined/case5.css: WARNING: The value of "zoom" in the "foo" class is undefined +file-1.css: NOTE: The first definition of "zoom" is here: +file-2.css: NOTE: The second definition of "zoom" is here: +` + note + ` +`, + }) +} + func TestImportCSSFromJSWriteToStdout(t *testing.T) { css_suite.expectBundled(t, bundled{ files: map[string]string{ diff --git a/internal/bundler_tests/snapshots/snapshots_css.txt b/internal/bundler_tests/snapshots/snapshots_css.txt index 860df104954..3e90693070c 100644 --- a/internal/bundler_tests/snapshots/snapshots_css.txt +++ b/internal/bundler_tests/snapshots/snapshots_css.txt @@ -821,6 +821,82 @@ console.log(styles_default); .styles_bar { } +================================================================================ +TestImportCSSFromJSComposesFromUndefined +---------- /out/entry.js ---------- +// styles.css +var styles_default = {}; + +// entry.js +console.log(styles_default); + +---------- /out/entry.css ---------- +/* well-defined.css */ +.well_defined_z1 { + zoom: 1; +} +.well_defined_z2 { + zoom: 2; +} +.well_defined_z4 { + zoom: 4; +} +.well_defined_z3 { + zoom: 3; +} +.well_defined_z5 { +} + +/* undefined/case1.css */ +.case1_foo { + zoom: 2; +} + +/* undefined/case2.css */ +.case2_foo { +} + +/* undefined/case3.css */ +.case3_foo { +} +.case3_nested1 { + zoom: 3; +} +.case3_nested2 { +} + +/* undefined/case4.css */ +.case4_foo { +} +.case4_nested1 { +} +.case4_nested2 { + zoom: 3; +} + +/* file-1.css */ +.file_1_foo { + zoom: 1; +} +.file_1_bar { + zoom: 2; +} + +/* file-2.css */ +.file_2_foo { + zoom: 2; +} + +/* undefined/case5.css */ +.case5_foo { +} +.case5_nested1 { +} +.case5_nested2 { +} + +/* styles.css */ + ================================================================================ TestImportCSSFromJSLocalAtContainer ---------- /out/entry.js ---------- diff --git a/internal/css_ast/css_ast.go b/internal/css_ast/css_ast.go index bda26e93e05..3ff285fd7f9 100644 --- a/internal/css_ast/css_ast.go +++ b/internal/css_ast/css_ast.go @@ -57,6 +57,11 @@ type Composes struct { // .foo { composes: bar from url(bar.css) } // ImportedNames []ImportedComposesName + + // This tracks what CSS properties each class uses so that we can warn when + // "composes" is used incorrectly to compose two classes from separate files + // that declare the same CSS properties. + Properties map[string]logger.Loc } type ImportedComposesName struct { diff --git a/internal/css_parser/css_decls.go b/internal/css_parser/css_decls.go index ef291153634..a18f86653db 100644 --- a/internal/css_parser/css_decls.go +++ b/internal/css_parser/css_decls.go @@ -94,6 +94,28 @@ func (p *parser) processDeclarations(rules []css_ast.Rule, composesContext *comp inset.keyText = "" } + // If this is a local class selector, track which CSS properties it declares. + // This is used to warn when CSS "composes" is used incorrectly. + if composesContext != nil { + for _, ref := range composesContext.parentRefs { + composes, ok := p.composes[ref] + if !ok { + composes = &css_ast.Composes{} + p.composes[ref] = composes + } + properties := composes.Properties + if properties == nil { + properties = make(map[string]logger.Loc) + composes.Properties = properties + } + for _, rule := range rules { + if decl, ok := rule.Data.(*css_ast.RDeclaration); ok && decl.Key != css_ast.DComposes { + properties[decl.KeyText] = decl.KeyRange.Loc + } + } + } + } + for _, rule := range rules { rewrittenRules = append(rewrittenRules, rule) decl, ok := rule.Data.(*css_ast.RDeclaration) diff --git a/internal/css_parser/css_decls_composes.go b/internal/css_parser/css_decls_composes.go index d19bcb13de7..5890d191942 100644 --- a/internal/css_parser/css_decls_composes.go +++ b/internal/css_parser/css_decls_composes.go @@ -23,10 +23,6 @@ func (p *parser) handleComposesPragma(context composesContext, tokens []css_ast. var names []nameWithLoc fromGlobal := false - if p.composes == nil { - p.composes = make(map[ast.Ref]*css_ast.Composes) - } - for i, t := range tokens { if t.Kind == css_lexer.TIdent { // Check for a "from" clause at the end @@ -49,10 +45,6 @@ func (p *parser) handleComposesPragma(context composesContext, tokens []css_ast. } for _, parentRef := range context.parentRefs { composes := p.composes[parentRef] - if composes == nil { - composes = &css_ast.Composes{} - p.composes[parentRef] = composes - } for _, name := range names { composes.ImportedNames = append(composes.ImportedNames, css_ast.ImportedComposesName{ ImportRecordIndex: importRecordIndex, @@ -102,10 +94,6 @@ func (p *parser) handleComposesPragma(context composesContext, tokens []css_ast. } for _, parentRef := range context.parentRefs { composes := p.composes[parentRef] - if composes == nil { - composes = &css_ast.Composes{} - p.composes[parentRef] = composes - } for _, name := range names { composes.Names = append(composes.Names, p.symbolForName(name.loc, name.text)) } diff --git a/internal/css_parser/css_parser.go b/internal/css_parser/css_parser.go index b83deae31a4..40cdf14cb38 100644 --- a/internal/css_parser/css_parser.go +++ b/internal/css_parser/css_parser.go @@ -127,6 +127,7 @@ func Parse(log logger.Log, source logger.Source, options Options) css_ast.AST { allComments: result.AllComments, legalComments: result.LegalComments, prevError: logger.Loc{Start: -1}, + composes: make(map[ast.Ref]*css_ast.Composes), localScope: make(map[string]ast.LocRef), globalScope: make(map[string]ast.LocRef), makeLocalSymbols: options.symbolMode == symbolModeLocal, diff --git a/internal/linker/linker.go b/internal/linker/linker.go index 21d606be217..51f9e49c37a 100644 --- a/internal/linker/linker.go +++ b/internal/linker/linker.go @@ -1359,6 +1359,8 @@ func (c *linkerContext) scanImportsAndExports() { } } + c.validateComposesFromProperties(file, repr) + case *graph.JSRepr: for importRecordIndex := range repr.AST.ImportRecords { record := &repr.AST.ImportRecords[importRecordIndex] @@ -2003,6 +2005,86 @@ func (c *linkerContext) scanImportsAndExports() { c.timer.End("Step 6") } +func (c *linkerContext) validateComposesFromProperties(rootFile *graph.LinkerFile, rootRepr *graph.CSSRepr) { + for _, local := range rootRepr.AST.LocalSymbols { + type propertyInFile struct { + file *graph.LinkerFile + loc logger.Loc + } + + visited := make(map[ast.Ref]bool) + properties := make(map[string]propertyInFile) + var visit func(*graph.LinkerFile, *graph.CSSRepr, ast.Ref) + + visit = func(file *graph.LinkerFile, repr *graph.CSSRepr, ref ast.Ref) { + if visited[ref] { + return + } + visited[ref] = true + + composes, ok := repr.AST.Composes[ref] + if !ok { + return + } + + for _, name := range composes.ImportedNames { + if record := repr.AST.ImportRecords[name.ImportRecordIndex]; record.SourceIndex.IsValid() { + otherFile := &c.graph.Files[record.SourceIndex.GetIndex()] + if otherRepr, ok := otherFile.InputFile.Repr.(*graph.CSSRepr); ok { + if otherName, ok := otherRepr.AST.LocalScope[name.Alias]; ok { + visit(otherFile, otherRepr, otherName.Ref) + } + } + } + } + + for _, name := range composes.Names { + visit(file, repr, name.Ref) + } + + // Warn about cross-file composition with the same CSS properties + for keyText, keyLoc := range composes.Properties { + property, ok := properties[keyText] + if !ok { + properties[keyText] = propertyInFile{file, keyLoc} + continue + } + if property.file == file || property.file == nil { + continue + } + + localOriginalName := c.graph.Symbols.Get(local.Ref).OriginalName + c.log.AddMsgID(logger.MsgID_CSS_UndefinedComposesFrom, logger.Msg{ + Kind: logger.Warning, + Data: rootFile.LineColumnTracker().MsgData( + css_lexer.RangeOfIdentifier(rootFile.InputFile.Source, local.Loc), + fmt.Sprintf("The value of %q in the %q class is undefined", keyText, localOriginalName), + ), + Notes: []logger.MsgData{ + property.file.LineColumnTracker().MsgData( + css_lexer.RangeOfIdentifier(property.file.InputFile.Source, property.loc), + fmt.Sprintf("The first definition of %q is here:", keyText), + ), + file.LineColumnTracker().MsgData( + css_lexer.RangeOfIdentifier(file.InputFile.Source, keyLoc), + fmt.Sprintf("The second definition of %q is here:", keyText), + ), + {Text: fmt.Sprintf("The specification of \"composes\" does not define an order when class declarations from separate files are composed together. "+ + "The value of the %q property for %q may change unpredictably as the code is edited. "+ + "Make sure that all definitions of %q for %q are in a single file.", keyText, localOriginalName, keyText, localOriginalName)}, + }, + }) + + // Don't warn more than once + property.file = nil + properties[keyText] = property + } + } + + visit(rootFile, rootRepr, local.Ref) + } +} + func (c *linkerContext) generateCodeForLazyExport(sourceIndex uint32) { file := &c.graph.Files[sourceIndex] repr := file.InputFile.Repr.(*graph.JSRepr) diff --git a/internal/logger/msg_ids.go b/internal/logger/msg_ids.go index 1e3700237a7..06a463360d3 100644 --- a/internal/logger/msg_ids.go +++ b/internal/logger/msg_ids.go @@ -48,6 +48,7 @@ const ( MsgID_CSS_InvalidAtLayer MsgID_CSS_InvalidCalc MsgID_CSS_JSCommentInCSS + MsgID_CSS_UndefinedComposesFrom MsgID_CSS_UnsupportedAtCharset MsgID_CSS_UnsupportedAtNamespace MsgID_CSS_UnsupportedCSSProperty @@ -161,6 +162,8 @@ func StringToMsgIDs(str string, logLevel LogLevel, overrides map[MsgID]LogLevel) overrides[MsgID_CSS_InvalidCalc] = logLevel case "js-comment-in-css": overrides[MsgID_CSS_JSCommentInCSS] = logLevel + case "undefined-composes-from": + overrides[MsgID_CSS_UndefinedComposesFrom] = logLevel case "unsupported-@charset": overrides[MsgID_CSS_UnsupportedAtCharset] = logLevel case "unsupported-@namespace": @@ -283,6 +286,8 @@ func MsgIDToString(id MsgID) string { return "invalid-calc" case MsgID_CSS_JSCommentInCSS: return "js-comment-in-css" + case MsgID_CSS_UndefinedComposesFrom: + return "undefined-composes-from" case MsgID_CSS_UnsupportedAtCharset: return "unsupported-@charset" case MsgID_CSS_UnsupportedAtNamespace: