diff --git a/internal/js_parser/js_parser.go b/internal/js_parser/js_parser.go index 4317addad14..59db5ae7448 100644 --- a/internal/js_parser/js_parser.go +++ b/internal/js_parser/js_parser.go @@ -155,13 +155,14 @@ type parser struct { // The visit pass binds identifiers to declared symbols, does constant // folding, substitutes compile-time variable definitions, and lowers certain // syntactic constructs as appropriate. - stmtExprValue js_ast.E - callTarget js_ast.E - dotOrIndexTarget js_ast.E - templateTag js_ast.E - deleteTarget js_ast.E - loopBody js_ast.S - moduleScope *js_ast.Scope + stmtExprValue js_ast.E + callTarget js_ast.E + dotOrIndexTarget js_ast.E + templateTag js_ast.E + deleteTarget js_ast.E + loopBody js_ast.S + suspiciousLogicalOperatorInsideArrow js_ast.E + moduleScope *js_ast.Scope // This is internal-only data used for the implementation of Yarn PnP manifestForYarnPnP js_ast.Expr @@ -14804,6 +14805,17 @@ func (p *parser) visitExprInOut(expr js_ast.Expr, in exprIn) (js_ast.Expr, exprO nameToKeep = p.nameToKeep } + // Prepare for suspicious logical operator checking + if e.PreferExpr && len(e.Args) == 1 && e.Args[0].DefaultOrNil.Data == nil && len(e.Body.Block.Stmts) == 1 { + if _, ok := e.Args[0].Binding.Data.(*js_ast.BIdentifier); ok { + if stmt, ok := e.Body.Block.Stmts[0].Data.(*js_ast.SReturn); ok { + if binary, ok := stmt.ValueOrNil.Data.(*js_ast.EBinary); ok && (binary.Op == js_ast.BinOpLogicalAnd || binary.Op == js_ast.BinOpLogicalOr) { + p.suspiciousLogicalOperatorInsideArrow = binary + } + } + } + } + asyncArrowNeedsToBeLowered := e.IsAsync && p.options.unsupportedJSFeatures.Has(compat.AsyncAwait) oldFnOrArrowData := p.fnOrArrowDataVisit p.fnOrArrowDataVisit = fnOrArrowDataVisit{ @@ -15242,6 +15254,25 @@ func (v *binaryExprVisitor) visitRightAndFinish(p *parser) js_ast.Expr { case js_ast.BinOpLogicalOr: if boolean, sideEffects, ok := js_ast.ToBooleanWithSideEffects(e.Left.Data); ok { + // Warn about potential bugs + if e == p.suspiciousLogicalOperatorInsideArrow { + // "return foo => 1 || foo <= 0" + var which string + if boolean { + which = "left" + } else { + which = "right" + } + if arrowLoc := p.source.RangeOfOperatorBefore(v.loc, "=>"); arrowLoc.Loc.Start+2 == p.source.LocBeforeWhitespace(v.loc).Start { + note := p.tracker.MsgData(arrowLoc, + "The \"=>\" symbol creates an arrow function expression in JavaScript. Did you mean to use the greater-than-or-equal-to operator \">=\" here instead?") + note.Location.Suggestion = ">=" + rOp := p.source.RangeOfOperatorBefore(e.Right.Loc, "||") + p.log.AddIDWithNotes(logger.MsgID_JS_SuspiciousLogicalOperator, logger.Warning, &p.tracker, rOp, + fmt.Sprintf("The \"||\" operator here will always return the %s operand", which), []logger.MsgData{note}) + } + } + if boolean { return e.Left } else if sideEffects == js_ast.NoSideEffects { @@ -15266,6 +15297,25 @@ func (v *binaryExprVisitor) visitRightAndFinish(p *parser) js_ast.Expr { case js_ast.BinOpLogicalAnd: if boolean, sideEffects, ok := js_ast.ToBooleanWithSideEffects(e.Left.Data); ok { + // Warn about potential bugs + if e == p.suspiciousLogicalOperatorInsideArrow { + // "return foo => 0 && foo <= 1" + var which string + if !boolean { + which = "left" + } else { + which = "right" + } + if arrowLoc := p.source.RangeOfOperatorBefore(v.loc, "=>"); arrowLoc.Loc.Start+2 == p.source.LocBeforeWhitespace(v.loc).Start { + note := p.tracker.MsgData(arrowLoc, + "The \"=>\" symbol creates an arrow function expression in JavaScript. Did you mean to use the greater-than-or-equal-to operator \">=\" here instead?") + note.Location.Suggestion = ">=" + rOp := p.source.RangeOfOperatorBefore(e.Right.Loc, "&&") + p.log.AddIDWithNotes(logger.MsgID_JS_SuspiciousLogicalOperator, logger.Warning, &p.tracker, rOp, + fmt.Sprintf("The \"&&\" operator here will always return the %s operand", which), []logger.MsgData{note}) + } + } + if !boolean { return e.Left } else if sideEffects == js_ast.NoSideEffects { diff --git a/internal/js_parser/js_parser_test.go b/internal/js_parser/js_parser_test.go index 1d937d6ad17..72b7b61cc67 100644 --- a/internal/js_parser/js_parser_test.go +++ b/internal/js_parser/js_parser_test.go @@ -3260,6 +3260,24 @@ func TestWarningNullishCoalescing(t *testing.T) { expectParseError(t, "x = void a ?? y", alwaysRight) } +func TestWarningLogicalOperator(t *testing.T) { + expectParseError(t, "x(a => b && a <= c)", "") + expectParseError(t, "x(a => b || a <= c)", "") + expectParseError(t, "x(a => (0 && a <= 1))", "") + expectParseError(t, "x(a => (-1 && a <= 0))", "") + expectParseError(t, "x(a => (0 || a <= -1))", "") + expectParseError(t, "x(a => (1 || a <= 0))", "") + + expectParseError(t, "x(a => 0 && a <= 1)", ": WARNING: The \"&&\" operator here will always return the left operand\n"+ + ": NOTE: The \"=>\" symbol creates an arrow function expression in JavaScript. Did you mean to use the greater-than-or-equal-to operator \">=\" here instead?\n") + expectParseError(t, "x(a => -1 && a <= 0)", ": WARNING: The \"&&\" operator here will always return the right operand\n"+ + ": NOTE: The \"=>\" symbol creates an arrow function expression in JavaScript. Did you mean to use the greater-than-or-equal-to operator \">=\" here instead?\n") + expectParseError(t, "x(a => 0 || a <= -1)", ": WARNING: The \"||\" operator here will always return the right operand\n"+ + ": NOTE: The \"=>\" symbol creates an arrow function expression in JavaScript. Did you mean to use the greater-than-or-equal-to operator \">=\" here instead?\n") + expectParseError(t, "x(a => 1 || a <= 0)", ": WARNING: The \"||\" operator here will always return the left operand\n"+ + ": NOTE: The \"=>\" symbol creates an arrow function expression in JavaScript. Did you mean to use the greater-than-or-equal-to operator \">=\" here instead?\n") +} + func TestMangleFor(t *testing.T) { expectPrintedMangle(t, "var a; while (1) ;", "for (var a; ; )\n ;\n") expectPrintedMangle(t, "let a; while (1) ;", "let a;\nfor (; ; )\n ;\n") diff --git a/internal/logger/msg_ids.go b/internal/logger/msg_ids.go index f1de9ca9f6d..3f2773da08b 100644 --- a/internal/logger/msg_ids.go +++ b/internal/logger/msg_ids.go @@ -35,6 +35,7 @@ const ( MsgID_JS_SemicolonAfterReturn MsgID_JS_SuspiciousBooleanNot MsgID_JS_SuspiciousDefine + MsgID_JS_SuspiciousLogicalOperator MsgID_JS_SuspiciousNullishCoalescing MsgID_JS_ThisIsUndefinedInESM MsgID_JS_UnsupportedDynamicImport @@ -141,6 +142,8 @@ func StringToMsgIDs(str string, logLevel LogLevel, overrides map[MsgID]LogLevel) overrides[MsgID_JS_SuspiciousBooleanNot] = logLevel case "suspicious-define": overrides[MsgID_JS_SuspiciousDefine] = logLevel + case "suspicious-logical-operator": + overrides[MsgID_JS_SuspiciousLogicalOperator] = logLevel case "suspicious-nullish-coalescing": overrides[MsgID_JS_SuspiciousNullishCoalescing] = logLevel case "this-is-undefined-in-esm": @@ -269,6 +272,8 @@ func MsgIDToString(id MsgID) string { return "suspicious-boolean-not" case MsgID_JS_SuspiciousDefine: return "suspicious-define" + case MsgID_JS_SuspiciousLogicalOperator: + return "suspicious-logical-operator" case MsgID_JS_SuspiciousNullishCoalescing: return "suspicious-nullish-coalescing" case MsgID_JS_ThisIsUndefinedInESM: