Skip to content

Commit d2bc26c

Browse files
evanwevanw
committed
css: attempt to warn about undefined composes 
1 parent a0910fd commit d2bc26c

File tree

8 files changed

+286
-12
lines changed

8 files changed

+286
-12
lines changed

‎internal/bundler_tests/bundler_css_test.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -855,6 +855,101 @@ func TestImportCSSFromJSComposesFromCircular(t *testing.T) {
855855
})
856856
}
857857

858+
func TestImportCSSFromJSComposesFromUndefined(t *testing.T) {
859+
note := "NOTE: The specification of \"composes\" does not define an order when class declarations from separate files are composed together. " +
860+
"The value of the \"zoom\" property for \"foo\" may change unpredictably as the code is edited. " +
861+
"Make sure that all definitions of \"zoom\" for \"foo\" are in a single file."
862+
css_suite.expectBundled(t, bundled{
863+
files: map[string]string{
864+
"/entry.js": `
865+
import styles from "./styles.css"
866+
console.log(styles)
867+
`,
868+
"/styles.css": `
869+
@import "well-defined.css";
870+
@import "undefined/case1.css";
871+
@import "undefined/case2.css";
872+
@import "undefined/case3.css";
873+
@import "undefined/case4.css";
874+
@import "undefined/case5.css";
875+
`,
876+
"/well-defined.css": `
877+
.z1 { composes: z2; zoom: 1; }
878+
.z2 { zoom: 2; }
879+
880+
.z4 { zoom: 4; }
881+
.z3 { composes: z4; zoom: 3; }
882+
883+
.z5 { composes: foo bar from "file-1.css"; }
884+
`,
885+
"/undefined/case1.css": `
886+
.foo {
887+
composes: foo from "../file-1.css";
888+
zoom: 2;
889+
}
890+
`,
891+
"/undefined/case2.css": `
892+
.foo {
893+
composes: foo from "../file-1.css";
894+
composes: foo from "../file-2.css";
895+
}
896+
`,
897+
"/undefined/case3.css": `
898+
.foo { composes: nested1 nested2; }
899+
.nested1 { zoom: 3; }
900+
.nested2 { composes: foo from "../file-2.css"; }
901+
`,
902+
"/undefined/case4.css": `
903+
.foo { composes: nested1 nested2; }
904+
.nested1 { composes: foo from "../file-1.css"; }
905+
.nested2 { zoom: 3; }
906+
`,
907+
"/undefined/case5.css": `
908+
.foo { composes: nested1 nested2; }
909+
.nested1 { composes: foo from "../file-1.css"; }
910+
.nested2 { composes: foo from "../file-2.css"; }
911+
`,
912+
"/file-1.css": `
913+
.foo { zoom: 1; }
914+
.bar { zoom: 2; }
915+
`,
916+
"/file-2.css": `
917+
.foo { zoom: 2; }
918+
`,
919+
},
920+
entryPaths: []string{"/entry.js"},
921+
options: config.Options{
922+
Mode: config.ModeBundle,
923+
AbsOutputDir: "/out",
924+
ExtensionToLoader: map[string]config.Loader{
925+
".js": config.LoaderJS,
926+
".css": config.LoaderLocalCSS,
927+
},
928+
},
929+
expectedCompileLog: `undefined/case1.css: WARNING: The value of "zoom" in the "foo" class is undefined
930+
file-1.css: NOTE: The first definition of "zoom" is here:
931+
undefined/case1.css: NOTE: The second definition of "zoom" is here:
932+
` + note + `
933+
undefined/case2.css: WARNING: The value of "zoom" in the "foo" class is undefined
934+
file-1.css: NOTE: The first definition of "zoom" is here:
935+
file-2.css: NOTE: The second definition of "zoom" is here:
936+
` + note + `
937+
undefined/case3.css: WARNING: The value of "zoom" in the "foo" class is undefined
938+
undefined/case3.css: NOTE: The first definition of "zoom" is here:
939+
file-2.css: NOTE: The second definition of "zoom" is here:
940+
` + note + `
941+
undefined/case4.css: WARNING: The value of "zoom" in the "foo" class is undefined
942+
file-1.css: NOTE: The first definition of "zoom" is here:
943+
undefined/case4.css: NOTE: The second definition of "zoom" is here:
944+
` + note + `
945+
undefined/case5.css: WARNING: The value of "zoom" in the "foo" class is undefined
946+
file-1.css: NOTE: The first definition of "zoom" is here:
947+
file-2.css: NOTE: The second definition of "zoom" is here:
948+
` + note + `
949+
`,
950+
})
951+
}
952+
858953
func TestImportCSSFromJSWriteToStdout(t *testing.T) {
859954
css_suite.expectBundled(t, bundled{
860955
files: map[string]string{

‎internal/bundler_tests/snapshots/snapshots_css.txt‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -821,6 +821,82 @@ console.log(styles_default);
821821
.styles_bar {
822822
}
823823

824+
================================================================================
825+
TestImportCSSFromJSComposesFromUndefined
826+
---------- /out/entry.js ----------
827+
// styles.css
828+
var styles_default = {};
829+
830+
// entry.js
831+
console.log(styles_default);
832+
833+
---------- /out/entry.css ----------
834+
/* well-defined.css */
835+
.well_defined_z1 {
836+
zoom: 1;
837+
}
838+
.well_defined_z2 {
839+
zoom: 2;
840+
}
841+
.well_defined_z4 {
842+
zoom: 4;
843+
}
844+
.well_defined_z3 {
845+
zoom: 3;
846+
}
847+
.well_defined_z5 {
848+
}
849+
850+
/* undefined/case1.css */
851+
.case1_foo {
852+
zoom: 2;
853+
}
854+
855+
/* undefined/case2.css */
856+
.case2_foo {
857+
}
858+
859+
/* undefined/case3.css */
860+
.case3_foo {
861+
}
862+
.case3_nested1 {
863+
zoom: 3;
864+
}
865+
.case3_nested2 {
866+
}
867+
868+
/* undefined/case4.css */
869+
.case4_foo {
870+
}
871+
.case4_nested1 {
872+
}
873+
.case4_nested2 {
874+
zoom: 3;
875+
}
876+
877+
/* file-1.css */
878+
.file_1_foo {
879+
zoom: 1;
880+
}
881+
.file_1_bar {
882+
zoom: 2;
883+
}
884+
885+
/* file-2.css */
886+
.file_2_foo {
887+
zoom: 2;
888+
}
889+
890+
/* undefined/case5.css */
891+
.case5_foo {
892+
}
893+
.case5_nested1 {
894+
}
895+
.case5_nested2 {
896+
}
897+
898+
/* styles.css */
899+
824900
================================================================================
825901
TestImportCSSFromJSLocalAtContainer
826902
---------- /out/entry.js ----------

‎internal/css_ast/css_ast.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,11 @@ type Composes struct {
5757
// .foo { composes: bar from url(bar.css) }
5858
//
5959
ImportedNames []ImportedComposesName
60+
61+
// This tracks what CSS properties each class uses so that we can warn when
62+
// "composes" is used incorrectly to compose two classes from separate files
63+
// that declare the same CSS properties.
64+
Properties map[string]logger.Loc
6065
}
6166

6267
type ImportedComposesName struct {

‎internal/css_parser/css_decls.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,28 @@ func (p *parser) processDeclarations(rules []css_ast.Rule, composesContext *comp
9494
inset.keyText = ""
9595
}
9696

97+
// If this is a local class selector, track which CSS properties it declares.
98+
// This is used to warn when CSS "composes" is used incorrectly.
99+
if composesContext != nil {
100+
for _, ref := range composesContext.parentRefs {
101+
composes, ok := p.composes[ref]
102+
if !ok {
103+
composes = &css_ast.Composes{}
104+
p.composes[ref] = composes
105+
}
106+
properties := composes.Properties
107+
if properties == nil {
108+
properties = make(map[string]logger.Loc)
109+
composes.Properties = properties
110+
}
111+
for _, rule := range rules {
112+
if decl, ok := rule.Data.(*css_ast.RDeclaration); ok && decl.Key != css_ast.DComposes {
113+
properties[decl.KeyText] = decl.KeyRange.Loc
114+
}
115+
}
116+
}
117+
}
118+
97119
for _, rule := range rules {
98120
rewrittenRules = append(rewrittenRules, rule)
99121
decl, ok := rule.Data.(*css_ast.RDeclaration)

‎internal/css_parser/css_decls_composes.go‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,6 @@ func (p *parser) handleComposesPragma(context composesContext, tokens []css_ast.
2323
var names []nameWithLoc
2424
fromGlobal := false
2525

26-
if p.composes == nil {
27-
p.composes = make(map[ast.Ref]*css_ast.Composes)
28-
}
29-
3026
for i, t := range tokens {
3127
if t.Kind == css_lexer.TIdent {
3228
// Check for a "from" clause at the end
@@ -49,10 +45,6 @@ func (p *parser) handleComposesPragma(context composesContext, tokens []css_ast.
4945
}
5046
for _, parentRef := range context.parentRefs {
5147
composes := p.composes[parentRef]
52-
if composes == nil {
53-
composes = &css_ast.Composes{}
54-
p.composes[parentRef] = composes
55-
}
5648
for _, name := range names {
5749
composes.ImportedNames = append(composes.ImportedNames, css_ast.ImportedComposesName{
5850
ImportRecordIndex: importRecordIndex,
@@ -102,10 +94,6 @@ func (p *parser) handleComposesPragma(context composesContext, tokens []css_ast.
10294
}
10395
for _, parentRef := range context.parentRefs {
10496
composes := p.composes[parentRef]
105-
if composes == nil {
106-
composes = &css_ast.Composes{}
107-
p.composes[parentRef] = composes
108-
}
10997
for _, name := range names {
11098
composes.Names = append(composes.Names, p.symbolForName(name.loc, name.text))
11199
}

‎internal/css_parser/css_parser.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ func Parse(log logger.Log, source logger.Source, options Options) css_ast.AST {
127127
allComments: result.AllComments,
128128
legalComments: result.LegalComments,
129129
prevError: logger.Loc{Start: -1},
130+
composes: make(map[ast.Ref]*css_ast.Composes),
130131
localScope: make(map[string]ast.LocRef),
131132
globalScope: make(map[string]ast.LocRef),
132133
makeLocalSymbols: options.symbolMode == symbolModeLocal,

‎internal/linker/linker.go‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1359,6 +1359,8 @@ func (c *linkerContext) scanImportsAndExports() {
13591359
}
13601360
}
13611361

1362+
c.validateComposesFromProperties(file, repr)
1363+
13621364
case *graph.JSRepr:
13631365
for importRecordIndex := range repr.AST.ImportRecords {
13641366
record := &repr.AST.ImportRecords[importRecordIndex]
@@ -2003,6 +2005,86 @@ func (c *linkerContext) scanImportsAndExports() {
20032005
c.timer.End("Step 6")
20042006
}
20052007

2008+
func (c *linkerContext) validateComposesFromProperties(rootFile *graph.LinkerFile, rootRepr *graph.CSSRepr) {
2009+
for _, local := range rootRepr.AST.LocalSymbols {
2010+
type propertyInFile struct {
2011+
file *graph.LinkerFile
2012+
loc logger.Loc
2013+
}
2014+
2015+
visited := make(map[ast.Ref]bool)
2016+
properties := make(map[string]propertyInFile)
2017+
var visit func(*graph.LinkerFile, *graph.CSSRepr, ast.Ref)
2018+
2019+
visit = func(file *graph.LinkerFile, repr *graph.CSSRepr, ref ast.Ref) {
2020+
if visited[ref] {
2021+
return
2022+
}
2023+
visited[ref] = true
2024+
2025+
composes, ok := repr.AST.Composes[ref]
2026+
if !ok {
2027+
return
2028+
}
2029+
2030+
for _, name := range composes.ImportedNames {
2031+
if record := repr.AST.ImportRecords[name.ImportRecordIndex]; record.SourceIndex.IsValid() {
2032+
otherFile := &c.graph.Files[record.SourceIndex.GetIndex()]
2033+
if otherRepr, ok := otherFile.InputFile.Repr.(*graph.CSSRepr); ok {
2034+
if otherName, ok := otherRepr.AST.LocalScope[name.Alias]; ok {
2035+
visit(otherFile, otherRepr, otherName.Ref)
2036+
}
2037+
}
2038+
}
2039+
}
2040+
2041+
for _, name := range composes.Names {
2042+
visit(file, repr, name.Ref)
2043+
}
2044+
2045+
// Warn about cross-file composition with the same CSS properties
2046+
for keyText, keyLoc := range composes.Properties {
2047+
property, ok := properties[keyText]
2048+
if !ok {
2049+
properties[keyText] = propertyInFile{file, keyLoc}
2050+
continue
2051+
}
2052+
if property.file == file || property.file == nil {
2053+
continue
2054+
}
2055+
2056+
localOriginalName := c.graph.Symbols.Get(local.Ref).OriginalName
2057+
c.log.AddMsgID(logger.MsgID_CSS_UndefinedComposesFrom, logger.Msg{
2058+
Kind: logger.Warning,
2059+
Data: rootFile.LineColumnTracker().MsgData(
2060+
css_lexer.RangeOfIdentifier(rootFile.InputFile.Source, local.Loc),
2061+
fmt.Sprintf("The value of %q in the %q class is undefined", keyText, localOriginalName),
2062+
),
2063+
Notes: []logger.MsgData{
2064+
property.file.LineColumnTracker().MsgData(
2065+
css_lexer.RangeOfIdentifier(property.file.InputFile.Source, property.loc),
2066+
fmt.Sprintf("The first definition of %q is here:", keyText),
2067+
),
2068+
file.LineColumnTracker().MsgData(
2069+
css_lexer.RangeOfIdentifier(file.InputFile.Source, keyLoc),
2070+
fmt.Sprintf("The second definition of %q is here:", keyText),
2071+
),
2072+
{Text: fmt.Sprintf("The specification of \"composes\" does not define an order when class declarations from separate files are composed together. "+
2073+
"The value of the %q property for %q may change unpredictably as the code is edited. "+
2074+
"Make sure that all definitions of %q for %q are in a single file.", keyText, localOriginalName, keyText, localOriginalName)},
2075+
},
2076+
})
2077+
2078+
// Don't warn more than once
2079+
property.file = nil
2080+
properties[keyText] = property
2081+
}
2082+
}
2083+
2084+
visit(rootFile, rootRepr, local.Ref)
2085+
}
2086+
}
2087+
20062088
func (c *linkerContext) generateCodeForLazyExport(sourceIndex uint32) {
20072089
file := &c.graph.Files[sourceIndex]
20082090
repr := file.InputFile.Repr.(*graph.JSRepr)

‎internal/logger/msg_ids.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ const (
4848
MsgID_CSS_InvalidAtLayer
4949
MsgID_CSS_InvalidCalc
5050
MsgID_CSS_JSCommentInCSS
51+
MsgID_CSS_UndefinedComposesFrom
5152
MsgID_CSS_UnsupportedAtCharset
5253
MsgID_CSS_UnsupportedAtNamespace
5354
MsgID_CSS_UnsupportedCSSProperty
@@ -161,6 +162,8 @@ func StringToMsgIDs(str string, logLevel LogLevel, overrides map[MsgID]LogLevel)
161162
overrides[MsgID_CSS_InvalidCalc] = logLevel
162163
case "js-comment-in-css":
163164
overrides[MsgID_CSS_JSCommentInCSS] = logLevel
165+
case "undefined-composes-from":
166+
overrides[MsgID_CSS_UndefinedComposesFrom] = logLevel
164167
case "unsupported-@charset":
165168
overrides[MsgID_CSS_UnsupportedAtCharset] = logLevel
166169
case "unsupported-@namespace":
@@ -283,6 +286,8 @@ func MsgIDToString(id MsgID) string {
283286
return "invalid-calc"
284287
case MsgID_CSS_JSCommentInCSS:
285288
return "js-comment-in-css"
289+
case MsgID_CSS_UndefinedComposesFrom:
290+
return "undefined-composes-from"
286291
case MsgID_CSS_UnsupportedAtCharset:
287292
return "unsupported-@charset"
288293
case MsgID_CSS_UnsupportedAtNamespace:

0 commit comments

Comments
 (0)