Skip to content

Commit

Permalink
feat(semantic): check for non-declared, non-abstract class accessors …
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
DonIsaac committed Sep 5, 2024
1 parent 052e94c commit 5407143
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 6 deletions.
5 changes: 5 additions & 0 deletions crates/oxc_ast/src/ast_impl/js.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
33 changes: 29 additions & 4 deletions crates/oxc_semantic/src/checker/typescript.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
Expand All @@ -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>) {
Expand Down
19 changes: 17 additions & 2 deletions tasks/coverage/parser_babel.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions tasks/coverage/parser_typescript.snap
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down

0 comments on commit 5407143

Please sign in to comment.