fix #3072: workarounds for bugs in safari <= 16.2

evanw · May 14, 2023 · 2f2b90a · 2f2b90a
1 parent d47ab43
commit 2f2b90a
Show file tree

Hide file tree

Showing 5 changed files with 183 additions and 59 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,34 @@
 # Changelog
 
+## Unreleased
+
+* Add a workaround for bugs in Safari 16.2 and earlier ([#3072](https://github.com/evanw/esbuild/issues/3072))
+
+    Safari's JavaScript parser had a bug (which has now been fixed) where at least something about unary/binary operators nested inside default arguments nested inside either a function or class expression was incorrectly considered a syntax error if that expression was the target of a property assignment. Here are some examples that trigger this Safari bug:
+
+    ```
+    ❱ x(function (y = -1) {}.z = 2)
+    SyntaxError: Left hand side of operator '=' must be a reference.
+
+    ❱ x(class { f(y = -1) {} }.z = 2)
+    SyntaxError: Left hand side of operator '=' must be a reference.
+    ```
+
+    It's not clear what the exact conditions are that trigger this bug. However, a workaround for this bug appears to be to post-process your JavaScript to wrap any in function and class declarations that are the direct target of a property access expression in parentheses. That's the workaround that UglifyJS applies for this issue: [mishoo/UglifyJS#2056](https://github.com/mishoo/UglifyJS/pull/2056). So that's what esbuild now does starting with this release:
+
+    ```js
+    // Original code
+    x(function (y = -1) {}.z = 2, class { f(y = -1) {} }.z = 2)
+
+    // Old output (with --minify --target=safari16.2)
+    x(function(c=-1){}.z=2,class{f(c=-1){}}.z=2);
+
+    // New output (with --minify --target=safari16.2)
+    x((function(c=-1){}).z=2,(class{f(c=-1){}}).z=2);
+    ```
+
+    This fix is not enabled by default. It's only enabled when `--target=` contains Safari 16.2 or earlier, such as with `--target=safari16.2`. You can also explicitly enable or disable this specific transform (called `function-or-class-property-access`) with `--supported:function-or-class-property-access=false`.
+
 ## 0.17.19
 
 * Fix CSS transform bugs with nested selectors that start with a combinator ([#3096](https://github.com/evanw/esbuild/issues/3096))

diff --git a/internal/compat/js_table.go b/internal/compat/js_table.go
@@ -78,6 +78,7 @@ const (
 	ExportStarAs
 	ForAwait
 	ForOf
+	FunctionOrClassPropertyAccess
 	Generator
 	Hashbang
 	ImportAssertions
@@ -109,60 +110,61 @@ const (
 )
 
 var StringToJSFeature = map[string]JSFeature{
-	"arbitrary-module-namespace-names": ArbitraryModuleNamespaceNames,
-	"array-spread":                     ArraySpread,
-	"arrow":                            Arrow,
-	"async-await":                      AsyncAwait,
-	"async-generator":                  AsyncGenerator,
-	"bigint":                           Bigint,
-	"class":                            Class,
-	"class-field":                      ClassField,
-	"class-private-accessor":           ClassPrivateAccessor,
-	"class-private-brand-check":        ClassPrivateBrandCheck,
-	"class-private-field":              ClassPrivateField,
-	"class-private-method":             ClassPrivateMethod,
-	"class-private-static-accessor":    ClassPrivateStaticAccessor,
-	"class-private-static-field":       ClassPrivateStaticField,
-	"class-private-static-method":      ClassPrivateStaticMethod,
-	"class-static-blocks":              ClassStaticBlocks,
-	"class-static-field":               ClassStaticField,
-	"const-and-let":                    ConstAndLet,
-	"decorators":                       Decorators,
-	"default-argument":                 DefaultArgument,
-	"destructuring":                    Destructuring,
-	"dynamic-import":                   DynamicImport,
-	"exponent-operator":                ExponentOperator,
-	"export-star-as":                   ExportStarAs,
-	"for-await":                        ForAwait,
-	"for-of":                           ForOf,
-	"generator":                        Generator,
-	"hashbang":                         Hashbang,
-	"import-assertions":                ImportAssertions,
-	"import-meta":                      ImportMeta,
-	"inline-script":                    InlineScript,
-	"logical-assignment":               LogicalAssignment,
-	"nested-rest-binding":              NestedRestBinding,
-	"new-target":                       NewTarget,
-	"node-colon-prefix-import":         NodeColonPrefixImport,
-	"node-colon-prefix-require":        NodeColonPrefixRequire,
-	"nullish-coalescing":               NullishCoalescing,
-	"object-accessors":                 ObjectAccessors,
-	"object-extensions":                ObjectExtensions,
-	"object-rest-spread":               ObjectRestSpread,
-	"optional-catch-binding":           OptionalCatchBinding,
-	"optional-chain":                   OptionalChain,
-	"regexp-dot-all-flag":              RegexpDotAllFlag,
-	"regexp-lookbehind-assertions":     RegexpLookbehindAssertions,
-	"regexp-match-indices":             RegexpMatchIndices,
-	"regexp-named-capture-groups":      RegexpNamedCaptureGroups,
-	"regexp-set-notation":              RegexpSetNotation,
-	"regexp-sticky-and-unicode-flags":  RegexpStickyAndUnicodeFlags,
-	"regexp-unicode-property-escapes":  RegexpUnicodePropertyEscapes,
-	"rest-argument":                    RestArgument,
-	"template-literal":                 TemplateLiteral,
-	"top-level-await":                  TopLevelAwait,
-	"typeof-exotic-object-is-object":   TypeofExoticObjectIsObject,
-	"unicode-escapes":                  UnicodeEscapes,
+	"arbitrary-module-namespace-names":  ArbitraryModuleNamespaceNames,
+	"array-spread":                      ArraySpread,
+	"arrow":                             Arrow,
+	"async-await":                       AsyncAwait,
+	"async-generator":                   AsyncGenerator,
+	"bigint":                            Bigint,
+	"class":                             Class,
+	"class-field":                       ClassField,
+	"class-private-accessor":            ClassPrivateAccessor,
+	"class-private-brand-check":         ClassPrivateBrandCheck,
+	"class-private-field":               ClassPrivateField,
+	"class-private-method":              ClassPrivateMethod,
+	"class-private-static-accessor":     ClassPrivateStaticAccessor,
+	"class-private-static-field":        ClassPrivateStaticField,
+	"class-private-static-method":       ClassPrivateStaticMethod,
+	"class-static-blocks":               ClassStaticBlocks,
+	"class-static-field":                ClassStaticField,
+	"const-and-let":                     ConstAndLet,
+	"decorators":                        Decorators,
+	"default-argument":                  DefaultArgument,
+	"destructuring":                     Destructuring,
+	"dynamic-import":                    DynamicImport,
+	"exponent-operator":                 ExponentOperator,
+	"export-star-as":                    ExportStarAs,
+	"for-await":                         ForAwait,
+	"for-of":                            ForOf,
+	"function-or-class-property-access": FunctionOrClassPropertyAccess,
+	"generator":                         Generator,
+	"hashbang":                          Hashbang,
+	"import-assertions":                 ImportAssertions,
+	"import-meta":                       ImportMeta,
+	"inline-script":                     InlineScript,
+	"logical-assignment":                LogicalAssignment,
+	"nested-rest-binding":               NestedRestBinding,
+	"new-target":                        NewTarget,
+	"node-colon-prefix-import":          NodeColonPrefixImport,
+	"node-colon-prefix-require":         NodeColonPrefixRequire,
+	"nullish-coalescing":                NullishCoalescing,
+	"object-accessors":                  ObjectAccessors,
+	"object-extensions":                 ObjectExtensions,
+	"object-rest-spread":                ObjectRestSpread,
+	"optional-catch-binding":            OptionalCatchBinding,
+	"optional-chain":                    OptionalChain,
+	"regexp-dot-all-flag":               RegexpDotAllFlag,
+	"regexp-lookbehind-assertions":      RegexpLookbehindAssertions,
+	"regexp-match-indices":              RegexpMatchIndices,
+	"regexp-named-capture-groups":       RegexpNamedCaptureGroups,
+	"regexp-set-notation":               RegexpSetNotation,
+	"regexp-sticky-and-unicode-flags":   RegexpStickyAndUnicodeFlags,
+	"regexp-unicode-property-escapes":   RegexpUnicodePropertyEscapes,
+	"rest-argument":                     RestArgument,
+	"template-literal":                  TemplateLiteral,
+	"top-level-await":                   TopLevelAwait,
+	"typeof-exotic-object-is-object":    TypeofExoticObjectIsObject,
+	"unicode-escapes":                   UnicodeEscapes,
 }
 
 func (features JSFeature) Has(feature JSFeature) bool {
@@ -445,6 +447,20 @@ var jsTable = map[JSFeature]map[Engine][]versionRange{
 		Opera:   {{start: v{38, 0, 0}}},
 		Safari:  {{start: v{10, 0, 0}}},
 	},
+	FunctionOrClassPropertyAccess: {
+		Chrome:  {{start: v{0, 0, 0}}},
+		Deno:    {{start: v{0, 0, 0}}},
+		Edge:    {{start: v{0, 0, 0}}},
+		ES:      {{start: v{0, 0, 0}}},
+		Firefox: {{start: v{0, 0, 0}}},
+		Hermes:  {{start: v{0, 0, 0}}},
+		IE:      {{start: v{0, 0, 0}}},
+		IOS:     {{start: v{0, 0, 0}}},
+		Node:    {{start: v{0, 0, 0}}},
+		Opera:   {{start: v{0, 0, 0}}},
+		Rhino:   {{start: v{0, 0, 0}}},
+		Safari:  {{start: v{16, 3, 0}}},
+	},
 	Generator: {
 		Chrome:  {{start: v{50, 0, 0}}},
 		Deno:    {{start: v{1, 0, 0}}},
@@ -698,12 +714,15 @@ var jsTable = map[JSFeature]map[Engine][]versionRange{
 	},
 	TypeofExoticObjectIsObject: {
 		Chrome:  {{start: v{0, 0, 0}}},
+		Deno:    {{start: v{0, 0, 0}}},
 		Edge:    {{start: v{0, 0, 0}}},
 		ES:      {{start: v{2020, 0, 0}}},
 		Firefox: {{start: v{0, 0, 0}}},
+		Hermes:  {{start: v{0, 0, 0}}},
 		IOS:     {{start: v{0, 0, 0}}},
 		Node:    {{start: v{0, 0, 0}}},
 		Opera:   {{start: v{0, 0, 0}}},
+		Rhino:   {{start: v{0, 0, 0}}},
 		Safari:  {{start: v{0, 0, 0}}},
 	},
 	UnicodeEscapes: {

diff --git a/internal/js_printer/js_printer.go b/internal/js_printer/js_printer.go
@@ -1807,6 +1807,7 @@ const (
 	isInsideForAwait
 	isDeleteTarget
 	isCallTargetOrTemplateTag
+	isPropertyAccessTarget
 	parentWasUnaryOrBinary
 )
 
@@ -2349,7 +2350,7 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 			}
 			flags &= ^hasNonOptionalChainParent
 		}
-		p.printExpr(e.Target, js_ast.LPostfix, flags&(forbidCall|hasNonOptionalChainParent))
+		p.printExpr(e.Target, js_ast.LPostfix, (flags&(forbidCall|hasNonOptionalChainParent))|isPropertyAccessTarget)
 		if p.canPrintIdentifier(e.Name) {
 			if e.OptionalChain != js_ast.OptionalChainStart && p.needSpaceBeforeDot == len(p.js) {
 				// "1.toString" is a syntax error, so print "1 .toString" instead
@@ -2411,7 +2412,7 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 			}
 			flags &= ^hasNonOptionalChainParent
 		}
-		p.printExpr(e.Target, js_ast.LPostfix, flags&(forbidCall|hasNonOptionalChainParent))
+		p.printExpr(e.Target, js_ast.LPostfix, (flags&(forbidCall|hasNonOptionalChainParent))|isPropertyAccessTarget)
 		if e.OptionalChain == js_ast.OptionalChainStart {
 			p.print("?.")
 		}
@@ -2539,7 +2540,8 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 
 	case *js_ast.EFunction:
 		n := len(p.js)
-		wrap := p.stmtStart == n || p.exportDefaultStart == n
+		wrap := p.stmtStart == n || p.exportDefaultStart == n ||
+			((flags&isPropertyAccessTarget) != 0 && p.options.UnsupportedFeatures.Has(compat.FunctionOrClassPropertyAccess))
 		if wrap {
 			p.print("(")
 		}
@@ -2566,7 +2568,8 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 
 	case *js_ast.EClass:
 		n := len(p.js)
-		wrap := p.stmtStart == n || p.exportDefaultStart == n
+		wrap := p.stmtStart == n || p.exportDefaultStart == n ||
+			((flags&isPropertyAccessTarget) != 0 && p.options.UnsupportedFeatures.Has(compat.FunctionOrClassPropertyAccess))
 		if wrap {
 			p.print("(")
 		}

diff --git a/scripts/compat-table.js b/scripts/compat-table.js
@@ -256,18 +256,64 @@ mergeVersions('DynamicImport', {
   safari11_1: true,
 })
 
-// This is a problem specific to Internet explorer. See https://github.com/tc39/ecma262/issues/1440
+// This is a problem specific to Internet Explorer. See https://github.com/tc39/ecma262/issues/1440
 mergeVersions('TypeofExoticObjectIsObject', {
   chrome0: true,
+  deno0: true,
   edge0: true,
   es0: true,
   firefox0: true,
+  hermes0: true,
   ios0: true,
   node0: true,
   opera0: true,
+  rhino0: true,
   safari0: true,
 })
 
+// This is a problem specific to JavaScriptCore. Some examples of when the
+// problematic case happens (checked in Safari 12.1):
+//
+//   ❱ x(function(y=-1){}.z=2)
+//   SyntaxError: Left hand side of operator '=' must be a reference.
+//
+//   ❱ x(class{f(y=-1){}}.z=2)
+//   SyntaxError: Left hand side of operator '=' must be a reference.
+//
+// Some examples of cases that aren't problematic (checked in Safari 12.1):
+//
+//   // Adding parentheses makes it ok
+//   x((function(y=-1){}).z=2)
+//   x((class{f(y=-1){}}).z=2)
+//
+//   // Not using a unary operator in the default argument makes it ok
+//   x(function(y=1){}.z=2)
+//   x(class{f(y=1){}}.z=2)
+//
+//   // Methods in object literals are not affected
+//   x({f(y=-1){}}.z=2)
+//
+// We don't attempt to reverse-engineer the specific conditions that cause JSC
+// to exhibit the bug. Instead we just always wrap function and class literals
+// when they are nested inside of a property access. This workaround is overly
+// conservative but is the same thing that UglifyJS does to handle this case.
+//
+// See https://github.com/mishoo/UglifyJS/pull/2056 and https://github.com/evanw/esbuild/issues/3072
+mergeVersions('FunctionOrClassPropertyAccess', {
+  chrome0: true,
+  deno0: true,
+  edge0: true,
+  es0: true,
+  firefox0: true,
+  hermes0: true,
+  ie0: true,
+  ios0: true,
+  node0: true,
+  opera0: true,
+  rhino0: true,
+  safari16_3: true, // These bugs are known to be fixed in Safari 16.3+
+})
+
 // If you want to embed the output directly in HTML, then closing HTML tags must
 // be marked as unsupported. Doing this escapes "</script>" in JS and "</style>"
 // in CSS. Otherwise the containing HTML tag will be ended early.

diff --git a/scripts/js-api-tests.js b/scripts/js-api-tests.js
@@ -6451,6 +6451,33 @@ let transformTests = {
     assert.strictEqual((await esbuild.transform(`a ||= b`, { target: 'eSnExT' })).code, `a ||= b;\n`)
   },
 
+  async propertyAccessBugWorkaroundForWebKit({ esbuild }) {
+    const check = async (target, input, expected) =>
+      assert.strictEqual((await esbuild.transform(input, { target })).code, expected)
+    await Promise.all([
+      check('safari16.2', `x(class{}.y=z)`, `x((class {\n}).y = z);\n`),
+      check('safari16.3', `x(class{}.y=z)`, `x(class {\n}.y = z);\n`),
+
+      check('safari16.2', `x(function(){}.y=z)`, `x((function() {\n}).y = z);\n`),
+      check('safari16.3', `x(function(){}.y=z)`, `x(function() {\n}.y = z);\n`),
+
+      check('safari16.2', `x(function*(){}.y=z)`, `x((function* () {\n}).y = z);\n`),
+      check('safari16.3', `x(function*(){}.y=z)`, `x(function* () {\n}.y = z);\n`),
+
+      check('safari16.2', `x(async function(){}.y=z)`, `x((async function() {\n}).y = z);\n`),
+      check('safari16.3', `x(async function(){}.y=z)`, `x(async function() {\n}.y = z);\n`),
+
+      check('safari16.2', `x(async function*(){}.y=z)`, `x((async function* () {\n}).y = z);\n`),
+      check('safari16.3', `x(async function*(){}.y=z)`, `x(async function* () {\n}.y = z);\n`),
+
+      // This should not be enabled by default
+      check('esnext', `x(class{}.y=z)`, `x(class {\n}.y = z);\n`),
+
+      // This doesn't need to be applied for methods in object literals
+      check('safari16.2', `x({f(a=-1){}}.y=z)`, `x({ f(a = -1) {\n} }.y = z);\n`),
+    ])
+  },
+
   async multipleEngineTargets({ esbuild }) {
     const check = async (target, expected) =>
       assert.strictEqual((await esbuild.transform(`foo(a ?? b)`, { target })).code, expected)