Skip to content

Commit

Permalink
use "check" for decorator validation, not "guess"
Browse files Browse the repository at this point in the history
  • Loading branch information
@evanw
evanw committed Nov 23, 2023
1 parent d6a1255 commit a8313d2
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 116 deletions.
5 changes: 4 additions & 1 deletion internal/js_ast/js_ast.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,7 @@ var EUndefinedShared = &EUndefined{}
var SDebuggerShared = &SDebugger{}
var SEmptyShared = &SEmpty{}
var STypeScriptShared = &STypeScript{}
var STypeScriptSharedWasDeclareClass = &STypeScript{WasDeclareClass: true}

type ENew struct {
Target Expr
Expand Down Expand Up @@ -918,7 +919,9 @@ type SBlock struct {
type SEmpty struct{}

// This is a stand-in for a TypeScript type declaration
type STypeScript struct{}
type STypeScript struct {
WasDeclareClass bool
}

type SComment struct {
Text string
Expand Down
117 changes: 39 additions & 78 deletions internal/js_parser/js_parser.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3588,16 +3588,8 @@ func (p *parser) parsePrefix(level js_ast.L, errors *deferredErrors, flags exprF
return p.parseClassExpr(nil)

case js_lexer.TAt:
// Parse decorators before class statements, which are potentially exported
scopeIndex := len(p.scopesInOrder)
// Parse decorators before class expressions
decorators := p.parseDecorators(p.currentScope, logger.Range{}, decoratorBeforeClassExpr)

// "@decorator class {}"
// "@decorator class Foo {}"
if p.lexer.Token != js_lexer.TClass {
p.logDecoratorWithoutFollowingClassError(loc, scopeIndex)
}

return p.parseClassExpr(decorators)

case js_lexer.TNew:
Expand Down Expand Up @@ -6139,7 +6131,8 @@ func (p *parser) parseClassStmt(loc logger.Loc, opts parseStmtOpts) js_ast.Stmt
p.hasNonLocalExportDeclareInsideNamespace = true
}

return js_ast.Stmt{Loc: loc, Data: js_ast.STypeScriptShared}
// Remember that this was a "declare class" so we can allow decorators on it
return js_ast.Stmt{Loc: loc, Data: js_ast.STypeScriptSharedWasDeclareClass}
}

p.popScope()
Expand All @@ -6149,7 +6142,7 @@ func (p *parser) parseClassStmt(loc logger.Loc, opts parseStmtOpts) js_ast.Stmt
func (p *parser) parseClassExpr(decorators []js_ast.Decorator) js_ast.Expr {
classKeyword := p.lexer.Range()
p.markSyntaxFeature(compat.Class, classKeyword)
p.lexer.Next()
p.lexer.Expect(js_lexer.TClass)
var name *ast.LocRef

opts := parseClassOpts{
Expand Down Expand Up @@ -6545,11 +6538,6 @@ func (p *parser) parseFnStmt(loc logger.Loc, opts parseStmtOpts, isAsync bool, a

type deferredDecorators struct {
decorators []js_ast.Decorator

// If this turns out to be a "declare class" statement, we need to undo the
// scopes that were potentially pushed while parsing the decorator arguments.
scopeIndex int

firstAtLoc logger.Loc
}

Expand Down Expand Up @@ -6697,20 +6685,6 @@ func (p *parser) parseDecorator() js_ast.Expr {
return memberExpr
}

func (p *parser) logDecoratorWithoutFollowingClassError(firstAtLoc logger.Loc, scopeIndex int) {
found := fmt.Sprintf("%q", p.lexer.Raw())
if p.lexer.Token == js_lexer.TEndOfFile {
found = "end of file"
}

// Try to be helpful by pointing out the decorator
p.lexer.AddRangeErrorWithNotes(p.lexer.Range(), fmt.Sprintf("Expected \"class\" after decorator but found %s", found), []logger.MsgData{
p.tracker.MsgData(logger.Range{Loc: firstAtLoc, Len: 1}, "The preceding decorator is here:"),
{Text: "Decorators can only be used with class declarations."},
})
p.discardScopesUpTo(scopeIndex)
}

type lexicalDecl uint8

const (
Expand Down Expand Up @@ -6759,18 +6733,6 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
}
p.lexer.Next()

// TypeScript decorators only work on class declarations
// "@decorator export class Foo {}"
// "@decorator export abstract class Foo {}"
// "@decorator export default class Foo {}"
// "@decorator export default abstract class Foo {}"
// "@decorator export declare class Foo {}"
// "@decorator export declare abstract class Foo {}"
if opts.deferredDecorators != nil && p.lexer.Token != js_lexer.TClass && p.lexer.Token != js_lexer.TDefault &&
!p.lexer.IsContextualKeyword("abstract") && !p.lexer.IsContextualKeyword("declare") {
p.logDecoratorWithoutFollowingClassError(opts.deferredDecorators.firstAtLoc, opts.deferredDecorators.scopeIndex)
}

switch p.lexer.Token {
case js_lexer.TClass, js_lexer.TConst, js_lexer.TFunction, js_lexer.TVar:
opts.isExport = true
Expand Down Expand Up @@ -6877,13 +6839,6 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
return defaultName
}

// TypeScript decorators only work on class declarations
// "@decorator export default class Foo {}"
// "@decorator export default abstract class Foo {}"
if opts.deferredDecorators != nil && p.lexer.Token != js_lexer.TClass && !p.lexer.IsContextualKeyword("abstract") {
p.logDecoratorWithoutFollowingClassError(opts.deferredDecorators.firstAtLoc, opts.deferredDecorators.scopeIndex)
}

if p.lexer.IsContextualKeyword("async") {
asyncRange := p.lexer.Range()
p.lexer.Next()
Expand Down Expand Up @@ -7095,25 +7050,37 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
opts.deferredDecorators = &deferredDecorators{
firstAtLoc: loc,
decorators: decorators,
scopeIndex: scopeIndex,
}

// "@decorator class Foo {}"
// "@decorator abstract class Foo {}"
// "@decorator declare class Foo {}"
// "@decorator declare abstract class Foo {}"
// "@decorator export class Foo {}"
// "@decorator export abstract class Foo {}"
// "@decorator export declare class Foo {}"
// "@decorator export declare abstract class Foo {}"
// "@decorator export default class Foo {}"
// "@decorator export default abstract class Foo {}"
if p.lexer.Token != js_lexer.TClass && p.lexer.Token != js_lexer.TExport &&
(!p.options.ts.Parse || (!p.lexer.IsContextualKeyword("abstract") && !p.lexer.IsContextualKeyword("declare"))) {
p.logDecoratorWithoutFollowingClassError(opts.deferredDecorators.firstAtLoc, opts.deferredDecorators.scopeIndex)
stmt := p.parseStmt(opts)

// Check for valid decorator targets
switch s := stmt.Data.(type) {
case *js_ast.SClass:
return stmt

case *js_ast.SExportDefault:
switch s.Value.Data.(type) {
case *js_ast.SClass:
return stmt
}

case *js_ast.STypeScript:
if s.WasDeclareClass {
// If this is a type declaration, discard any scopes that were pushed
// while parsing decorators. Unlike with the class statements above,
// these scopes won't end up being visited during the upcoming visit
// pass because type declarations aren't visited at all.
p.discardScopesUpTo(scopeIndex)
return stmt
}
}

return p.parseStmt(opts)
// Forbid decorators on anything other than a class statement
p.log.AddError(&p.tracker, logger.Range{Loc: loc, Len: 1}, "Decorators are not valid here")
stmt.Data = js_ast.STypeScriptShared
p.discardScopesUpTo(scopeIndex)
return stmt

case js_lexer.TClass:
if opts.lexicalDecl != lexicalDeclAllowAll {
Expand Down Expand Up @@ -7857,12 +7824,6 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
opts.lexicalDecl = lexicalDeclAllowAll
opts.isTypeScriptDeclare = true

// "@decorator declare class Foo {}"
// "@decorator declare abstract class Foo {}"
if opts.deferredDecorators != nil && p.lexer.Token != js_lexer.TClass && !p.lexer.IsContextualKeyword("abstract") {
p.logDecoratorWithoutFollowingClassError(opts.deferredDecorators.firstAtLoc, opts.deferredDecorators.scopeIndex)
}

// "declare global { ... }"
if p.lexer.IsContextualKeyword("global") {
p.lexer.Next()
Expand All @@ -7876,12 +7837,16 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
scopeIndex := len(p.scopesInOrder)
oldLexer := p.lexer
stmt := p.parseStmt(opts)
switch stmt.Data.(type) {
typeDeclarationData := js_ast.STypeScriptShared
switch s := stmt.Data.(type) {
case *js_ast.SEmpty:
return js_ast.Stmt{Loc: loc, Data: &js_ast.SExpr{Value: expr}}

case *js_ast.STypeScript:
// Type declarations are expected
// Type declarations are expected. Propagate the "declare class"
// status in case our caller is a decorator that needs to know
// this was a "declare class" statement.
typeDeclarationData = s

case *js_ast.SLocal:
// This is also a type declaration (but doesn't use "STypeScript"
Expand All @@ -7900,11 +7865,7 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
p.lexer = oldLexer
p.lexer.Unexpected()
}
if opts.deferredDecorators != nil {
p.discardScopesUpTo(opts.deferredDecorators.scopeIndex)
} else {
p.discardScopesUpTo(scopeIndex)
}
p.discardScopesUpTo(scopeIndex)

// Unlike almost all uses of "declare", statements that use
// "export declare" with "var/let/const" inside a namespace affect
Expand Down Expand Up @@ -7942,7 +7903,7 @@ func (p *parser) parseStmt(opts parseStmtOpts) js_ast.Stmt {
}
}

return js_ast.Stmt{Loc: loc, Data: js_ast.STypeScriptShared}
return js_ast.Stmt{Loc: loc, Data: typeDeclarationData}
}
}
}
Expand Down
18 changes: 5 additions & 13 deletions internal/js_parser/js_parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2027,19 +2027,16 @@ func TestDecorators(t *testing.T) {
expectPrinted(t, "@(() => {}) class Foo {}", "@(() => {\n})\nclass Foo {\n}\n")
expectPrinted(t, "class Foo { #x = @y.#x.y.#x class {} }", "class Foo {\n #x = @y.#x.y.#x class {\n };\n}\n")
expectParseError(t, "@123 class Foo {}", "<stdin>: ERROR: Expected identifier but found \"123\"\n")
expectParseError(t, "@x[y] class Foo {}",
"<stdin>: ERROR: Expected \"class\" after decorator but found \"[\"\n<stdin>: NOTE: The preceding decorator is here:\n"+
"NOTE: Decorators can only be used with class declarations.\n<stdin>: ERROR: Expected \";\" but found \"class\"\n")
expectParseError(t, "@x[y] class Foo {}", "<stdin>: ERROR: Expected \";\" but found \"class\"\n")
expectParseError(t, "@x?.() class Foo {}", "<stdin>: ERROR: Expected \".\" but found \"?.\"\n")
expectParseError(t, "@x?.y() class Foo {}", "<stdin>: ERROR: Expected \".\" but found \"?.\"\n")
expectParseError(t, "@x?.[y]() class Foo {}", "<stdin>: ERROR: Expected \".\" but found \"?.\"\n")
expectParseError(t, "@new Function() class Foo {}", "<stdin>: ERROR: Expected identifier but found \"new\"\n")
expectParseError(t, "@() => {} class Foo {}", "<stdin>: ERROR: Unexpected \")\"\n")
expectParseError(t, "x = @y function() {}", "<stdin>: ERROR: Expected \"class\" but found \"function\"\n")

// See: https://github.com/microsoft/TypeScript/issues/55336
expectParseError(t, "@x().y() class Foo {}",
"<stdin>: ERROR: Expected \"class\" after decorator but found \".\"\n"+
"<stdin>: NOTE: The preceding decorator is here:\nNOTE: Decorators can only be used with class declarations.\n")
expectParseError(t, "@x().y() class Foo {}", "<stdin>: ERROR: Unexpected \".\"\n")

errorText := "<stdin>: ERROR: Transforming JavaScript decorators to the configured target environment is not supported yet\n"
expectParseErrorWithUnsupportedFeatures(t, compat.Decorators, "@dec class Foo {}", errorText)
Expand All @@ -2051,13 +2048,8 @@ func TestDecorators(t *testing.T) {
expectParseErrorWithUnsupportedFeatures(t, compat.Decorators, "class Foo { @dec static accessor x }", errorText)

// Check ASI for "abstract"
expectParseError(t, "@x abstract class Foo {}",
"<stdin>: ERROR: Expected \"class\" after decorator but found \"abstract\"\n"+
"<stdin>: NOTE: The preceding decorator is here:\nNOTE: Decorators can only be used with class declarations.\n"+
"<stdin>: ERROR: Expected \";\" but found \"class\"\n")
expectParseError(t, "@x abstract\nclass Foo {}",
"<stdin>: ERROR: Expected \"class\" after decorator but found \"abstract\"\n"+
"<stdin>: NOTE: The preceding decorator is here:\nNOTE: Decorators can only be used with class declarations.\n")
expectParseError(t, "@x abstract class Foo {}", "<stdin>: ERROR: Expected \";\" but found \"class\"\n")
expectParseError(t, "@x abstract\nclass Foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
}

func TestGenerator(t *testing.T) {
Expand Down
46 changes: 22 additions & 24 deletions internal/js_parser/ts_parser_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1859,24 +1859,22 @@ func TestTSExperimentalDecorator(t *testing.T) {
expectPrintedExperimentalDecoratorTS(t, "declare class Foo { foo(@dec(() => 0) x) } {let foo}", "{\n let foo;\n}\n")

// Decorators must only work on class statements
notes := "<stdin>: NOTE: The preceding decorator is here:\n" +
"NOTE: Decorators can only be used with class declarations.\n"
expectParseErrorExperimentalDecoratorTS(t, "@dec enum foo {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"enum\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec namespace foo {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"namespace\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec function foo() {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"function\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec enum foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec namespace foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec function foo() {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec abstract", "<stdin>: ERROR: Expected \"class\" but found end of file\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec declare: x", "<stdin>: ERROR: Expected \"class\" after decorator but found \":\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec declare enum foo {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"enum\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec declare namespace foo {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"namespace\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec declare function foo()", "<stdin>: ERROR: Expected \"class\" after decorator but found \"function\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec export {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"{\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec export enum foo {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"enum\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec export namespace foo {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"namespace\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec export function foo() {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"function\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec declare: x", "<stdin>: ERROR: Unexpected \":\"\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec declare enum foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec declare namespace foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec declare function foo()", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec export {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec export enum foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec export namespace foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec export function foo() {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec export default abstract", "<stdin>: ERROR: Expected \"class\" but found end of file\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec export declare enum foo {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"enum\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec export declare namespace foo {}", "<stdin>: ERROR: Expected \"class\" after decorator but found \"namespace\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec export declare function foo()", "<stdin>: ERROR: Expected \"class\" after decorator but found \"function\"\n"+notes)
expectParseErrorExperimentalDecoratorTS(t, "@dec export declare enum foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec export declare namespace foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
expectParseErrorExperimentalDecoratorTS(t, "@dec export declare function foo()", "<stdin>: ERROR: Decorators are not valid here\n")

// Decorators must be forbidden outside class statements
note := "<stdin>: NOTE: This is a class expression, not a class declaration:\n"
Expand Down Expand Up @@ -1994,14 +1992,15 @@ func TestTSExperimentalDecorator(t *testing.T) {
expectPrintedExperimentalDecoratorTS(t, "@x?.y() class Foo {}", "let Foo = class {\n};\nFoo = __decorateClass([\n x?.y()\n], Foo);\n")
expectPrintedExperimentalDecoratorTS(t, "@x?.[y]() class Foo {}", "let Foo = class {\n};\nFoo = __decorateClass([\n x?.[y]()\n], Foo);\n")
expectPrintedExperimentalDecoratorTS(t, "@new Function() class Foo {}", "let Foo = class {\n};\nFoo = __decorateClass([\n new Function()\n], Foo);\n")
expectParseErrorExperimentalDecoratorTS(t, "@x[y] class Foo {}",
"<stdin>: ERROR: Expected \"class\" after decorator but found \"[\"\n<stdin>: NOTE: The preceding decorator is here:\n"+
"NOTE: Decorators can only be used with class declarations.\n<stdin>: ERROR: Expected \";\" but found \"class\"\n")
expectParseErrorExperimentalDecoratorTS(t, "@x[y] class Foo {}", "<stdin>: ERROR: Expected \";\" but found \"class\"\n")
expectParseErrorExperimentalDecoratorTS(t, "@() => {} class Foo {}", "<stdin>: ERROR: Unexpected \")\"\n")
expectParseErrorExperimentalDecoratorTS(t, "x = @y function() {}",
"<stdin>: ERROR: Experimental decorators cannot be used in expression position in TypeScript\n"+
"<stdin>: ERROR: Expected \"class\" but found \"function\"\n")

// Check ASI for "abstract"
expectPrintedExperimentalDecoratorTS(t, "@x abstract class Foo {}", "let Foo = class {\n};\nFoo = __decorateClass([\n x\n], Foo);\n")
expectParseErrorExperimentalDecoratorTS(t, "@x abstract\nclass Foo {}", "") // TODO
expectParseErrorExperimentalDecoratorTS(t, "@x abstract\nclass Foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
}

func TestTSDecorators(t *testing.T) {
Expand Down Expand Up @@ -2033,14 +2032,13 @@ func TestTSDecorators(t *testing.T) {
expectPrintedTS(t, "@(() => {}) class Foo {}", "@(() => {\n})\nclass Foo {\n}\n")
expectPrintedTS(t, "class Foo { #x = @y.#x.y.#x class {} }", "class Foo {\n #x = @y.#x.y.#x class {\n };\n}\n")
expectParseErrorTS(t, "@123 class Foo {}", "<stdin>: ERROR: Expected identifier but found \"123\"\n")
expectParseErrorTS(t, "@x[y] class Foo {}",
"<stdin>: ERROR: Expected \"class\" after decorator but found \"[\"\n<stdin>: NOTE: The preceding decorator is here:\n"+
"NOTE: Decorators can only be used with class declarations.\n<stdin>: ERROR: Expected \";\" but found \"class\"\n")
expectParseErrorTS(t, "@x[y] class Foo {}", "<stdin>: ERROR: Expected \";\" but found \"class\"\n")
expectParseErrorTS(t, "@x?.() class Foo {}", "<stdin>: ERROR: Expected \".\" but found \"?.\"\n")
expectParseErrorTS(t, "@x?.y() class Foo {}", "<stdin>: ERROR: Expected \".\" but found \"?.\"\n")
expectParseErrorTS(t, "@x?.[y]() class Foo {}", "<stdin>: ERROR: Expected \".\" but found \"?.\"\n")
expectParseErrorTS(t, "@new Function() class Foo {}", "<stdin>: ERROR: Expected identifier but found \"new\"\n")
expectParseErrorTS(t, "@() => {} class Foo {}", "<stdin>: ERROR: Unexpected \")\"\n")
expectParseErrorTS(t, "x = @y function() {}", "<stdin>: ERROR: Expected \"class\" but found \"function\"\n")

expectPrintedTS(t, "class Foo { @x<{}> y: any }", "class Foo {\n @x\n y;\n}\n")
expectPrintedTS(t, "class Foo { @x<{}>() y: any }", "class Foo {\n @x()\n y;\n}\n")
Expand All @@ -2063,7 +2061,7 @@ func TestTSDecorators(t *testing.T) {

// Check ASI for "abstract"
expectPrintedTS(t, "@x abstract class Foo {}", "@x\nclass Foo {\n}\n")
expectParseErrorTS(t, "@x abstract\nclass Foo {}", "") // TODO
expectParseErrorTS(t, "@x abstract\nclass Foo {}", "<stdin>: ERROR: Decorators are not valid here\n")
}

func TestTSTry(t *testing.T) {
Expand Down

0 comments on commit a8313d2

Please sign in to comment.