From 540714393acf851d133f74d6c11a31a1163c6303 Mon Sep 17 00:00:00 2001 From: DonIsaac <22823424+DonIsaac@users.noreply.github.com> Date: Thu, 5 Sep 2024 01:33:08 +0000 Subject: [PATCH] feat(semantic): check for non-declared, non-abstract class accessors without bodies (#5460) This should be causing more negative tests to fail than it actually is. This is because our typescript coverage tests use the "declarations" option to change the source type to `.d.ts`, which is incorrect. This setting enables `.d.ts` emits, it does not mean that input files should be treated as `.d.ts` themselves. --- crates/oxc_ast/src/ast_impl/js.rs | 5 +++ crates/oxc_semantic/src/checker/typescript.rs | 33 ++++++++++++++++--- tasks/coverage/parser_babel.snap | 19 +++++++++-- tasks/coverage/parser_typescript.snap | 8 +++++ 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/crates/oxc_ast/src/ast_impl/js.rs b/crates/oxc_ast/src/ast_impl/js.rs index b740c64372a46..ce40ea6713ab9 100644 --- a/crates/oxc_ast/src/ast_impl/js.rs +++ b/crates/oxc_ast/src/ast_impl/js.rs @@ -1423,6 +1423,11 @@ impl MethodDefinitionKind { matches!(self, Self::Get) } + /// Returns `true` if this method is a getter or a setter. + pub fn is_accessor(&self) -> bool { + matches!(self, Self::Get | Self::Set) + } + pub fn scope_flags(self) -> ScopeFlags { match self { Self::Constructor => ScopeFlags::Constructor | ScopeFlags::Function, diff --git a/crates/oxc_semantic/src/checker/typescript.rs b/crates/oxc_semantic/src/checker/typescript.rs index 5cf3852d3184a..9422d00d7ac8e 100644 --- a/crates/oxc_semantic/src/checker/typescript.rs +++ b/crates/oxc_semantic/src/checker/typescript.rs @@ -287,8 +287,29 @@ fn parameter_property_only_in_constructor_impl(span: Span) -> OxcDiagnostic { .with_label(span) } +/// Getter or setter without a body. There is no corresponding TS error code, +/// since in TSC this is a parse error. +fn accessor_without_body(span: Span) -> OxcDiagnostic { + OxcDiagnostic::error("Getters and setters must have an implementation.").with_label(span) +} + pub fn check_method_definition<'a>(method: &MethodDefinition<'a>, ctx: &SemanticBuilder<'a>) { - if method.r#type.is_abstract() { + let is_abstract = method.r#type.is_abstract(); + let is_declare = ctx.class_table_builder.current_class_id.map_or( + ctx.source_type.is_typescript_definition(), + |id| { + let ast_node_id = ctx.class_table_builder.classes.declarations[id]; + let AstKind::Class(class) = ctx.nodes.get_node(ast_node_id).kind() else { + #[cfg(debug_assertions)] + panic!("current_class_id is set, but does not point to a Class node."); + #[cfg(not(debug_assertions))] + return ctx.source_type.is_typescript_definition(); + }; + class.declare || ctx.source_type.is_typescript_definition() + }, + ); + + if is_abstract { // constructors cannot be abstract, no matter what if method.kind.is_constructor() { ctx.error(illegal_abstract_modifier(method.key.span())); @@ -313,16 +334,20 @@ pub fn check_method_definition<'a>(method: &MethodDefinition<'a>, ctx: &Semantic } } + let is_empty_body = method.value.r#type == FunctionType::TSEmptyBodyFunctionExpression; // Illegal to have `constructor(public foo);` - if method.kind.is_constructor() - && method.value.r#type == FunctionType::TSEmptyBodyFunctionExpression - { + if method.kind.is_constructor() && is_empty_body { for param in &method.value.params.items { if param.accessibility.is_some() { ctx.error(parameter_property_only_in_constructor_impl(param.span)); } } } + + // Illegal to have `get foo();` or `set foo(a)` + if method.kind.is_accessor() && is_empty_body && !is_abstract && !is_declare { + ctx.error(accessor_without_body(method.key.span())); + } } pub fn check_property_definition<'a>(prop: &PropertyDefinition<'a>, ctx: &SemanticBuilder<'a>) { diff --git a/tasks/coverage/parser_babel.snap b/tasks/coverage/parser_babel.snap index 53b509750fadc..6c2205a0f28b9 100644 --- a/tasks/coverage/parser_babel.snap +++ b/tasks/coverage/parser_babel.snap @@ -3,7 +3,7 @@ commit: 3bcfee23 parser_babel Summary: AST Parsed : 2093/2101 (99.62%) Positive Passed: 2083/2101 (99.14%) -Negative Passed: 1381/1493 (92.50%) +Negative Passed: 1382/1493 (92.57%) Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/annex-b/enabled/3.1-sloppy-labeled-functions-if-body/input.js Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/core/categorized/invalid-fn-decl-labeled-inside-if/input.js Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/core/categorized/invalid-fn-decl-labeled-inside-loop/input.js @@ -39,7 +39,6 @@ Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/ty Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/typescript/class/constructor-with-invalid-order-modifiers-3/input.ts Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/typescript/class/constructor-with-type-parameters/input.ts Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/typescript/class/constructor-with-type-parameters-babel-7/input.ts -Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/typescript/class/declare-accessor/input.ts Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/typescript/class/declare-field-initializer/input.ts Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/typescript/class/declare-initializer/input.ts Expect Syntax Error: tasks/coverage/babel/packages/babel-parser/test/fixtures/typescript/class/declare-method/input.ts @@ -10039,6 +10038,22 @@ Expect to Parse: tasks/coverage/babel/packages/babel-parser/test/fixtures/typesc 8 │ abstract accessor f = 1; ╰──── + × Getters and setters must have an implementation. + ╭─[babel/packages/babel-parser/test/fixtures/typescript/class/declare-accessor/input.ts:2:15] + 1 │ class Foo { + 2 │ declare get foo() + · ─── + 3 │ declare set foo(v) + ╰──── + + × Getters and setters must have an implementation. + ╭─[babel/packages/babel-parser/test/fixtures/typescript/class/declare-accessor/input.ts:3:15] + 2 │ declare get foo() + 3 │ declare set foo(v) + · ─── + 4 │ } + ╰──── + × Expected a semicolon or an implicit semicolon after a statement, but found none ╭─[babel/packages/babel-parser/test/fixtures/typescript/class/declare-new-line-abstract/input.ts:1:8] 1 │ declare abstract diff --git a/tasks/coverage/parser_typescript.snap b/tasks/coverage/parser_typescript.snap index 5c1d09924673c..67750680b0822 100644 --- a/tasks/coverage/parser_typescript.snap +++ b/tasks/coverage/parser_typescript.snap @@ -5021,6 +5021,14 @@ Expect to Parse: tasks/coverage/typescript/tests/cases/conformance/salsa/typeFro ╰──── help: Try insert a semicolon here + × Getters and setters must have an implementation. + ╭─[typescript/tests/cases/compiler/abstractPropertyNegative.ts:16:9] + 15 │ abstract notAllowed: string; + 16 │ get concreteWithNoBody(): string; + · ────────────────── + 17 │ } + ╰──── + × TS(1253): Abstract properties can only appear within an abstract class. ╭─[typescript/tests/cases/compiler/abstractPropertyNegative.ts:15:14] 14 │ readonly ro = "readonly please";