Skip to content

Commit

Permalink
try adding a warning about suspicious uses of =>
Browse files Browse the repository at this point in the history
  • Loading branch information
evanw committed Nov 19, 2023
1 parent e1dfdfc commit 50cead7
Show file tree
Hide file tree
Showing 3 changed files with 80 additions and 7 deletions.
64 changes: 57 additions & 7 deletions internal/js_parser/js_parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
18 changes: 18 additions & 0 deletions internal/js_parser/js_parser_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)", "<stdin>: WARNING: The \"&&\" operator here will always return the left operand\n"+
"<stdin>: 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)", "<stdin>: WARNING: The \"&&\" operator here will always return the right operand\n"+
"<stdin>: 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)", "<stdin>: WARNING: The \"||\" operator here will always return the right operand\n"+
"<stdin>: 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)", "<stdin>: WARNING: The \"||\" operator here will always return the left operand\n"+
"<stdin>: 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")
Expand Down
5 changes: 5 additions & 0 deletions internal/logger/msg_ids.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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":
Expand Down Expand Up @@ -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:
Expand Down

0 comments on commit 50cead7

Please sign in to comment.