Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various Isolated declarations fixes #3976

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions crates/oxc_codegen/src/gen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2095,6 +2095,9 @@ impl<'a, const MINIFY: bool> Gen<MINIFY> for Class<'a> {
if let Some(id) = &self.id {
p.print_hard_space();
id.gen(p, ctx);
if let Some(type_parameters) = self.type_parameters.as_ref() {
type_parameters.gen(p, ctx);
}
}
if let Some(super_class) = self.super_class.as_ref() {
p.print_str(b" extends ");
Expand Down Expand Up @@ -2444,6 +2447,9 @@ impl<'a, const MINIFY: bool> Gen<MINIFY> for PropertyDefinition<'a> {
if self.r#static {
p.print_str(b"static ");
}
if self.readonly {
p.print_str(b"readonly ");
}
if self.computed {
p.print(b'[');
}
Expand Down Expand Up @@ -2819,6 +2825,10 @@ impl<'a, const MINIFY: bool> Gen<MINIFY> for TSMappedType<'a> {
p.print_str(b" = ");
default.gen(p, ctx);
}
if let Some(name_type) = &self.name_type {
p.print_str(b" as ");
name_type.gen(p, ctx);
}
p.print_str(b"]");
match self.optional {
TSMappedTypeModifierOperator::True => {
Expand Down Expand Up @@ -3256,6 +3266,9 @@ impl<'a, const MINIFY: bool> Gen<MINIFY> for TSTupleElement<'a> {
impl<'a, const MINIFY: bool> Gen<MINIFY> for TSNamedTupleMember<'a> {
fn gen(&self, p: &mut Codegen<{ MINIFY }>, ctx: Context) {
self.label.gen(p, ctx);
if self.optional {
p.print_str(b"?");
}
p.print_str(b":");
p.print_soft_space();
self.element_type.gen(p, ctx);
Expand Down
15 changes: 15 additions & 0 deletions crates/oxc_codegen/tests/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -352,3 +352,18 @@ fn unicode_escape() {
test("console.log('안녕하세요');", "console.log('안녕하세요');\n");
test("console.log('🧑‍🤝‍🧑');", "console.log('🧑‍🤝‍🧑');\n");
}

#[test]
fn ts_misc() {
test_ts(
"type A<T> = { [K in keyof T as K extends string ? B<K> : K ]: T[K] }",
"type A<T> = { [K in keyof T as K extends (string) ? B<K> : K] : T[K]};\n",
false,
);

test_ts(
"class A {readonly type = 'frame'}",
"class A {\n\treadonly type = 'frame';\n}\n",
false,
);
}
25 changes: 16 additions & 9 deletions crates/oxc_isolated_declarations/src/class.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,11 @@ impl<'a> IsolatedDeclarations<'a> {
.value
.as_ref()
.and_then(|expr| {
let ts_type = self.infer_type_from_expression(expr);
let ts_type = if property.readonly {
self.transform_expression_to_ts_type(expr)
} else {
self.infer_type_from_expression(expr)
};
if ts_type.is_none() {
self.error(property_must_have_explicit_type(property.key.span()));
}
Expand All @@ -76,6 +80,9 @@ impl<'a> IsolatedDeclarations<'a> {
})
};

// TODO if inferred type_annotations is TSLiteral, it should stand as value,
// so `field = 'string'` remain `field = 'string'` instead of `field: 'string'`

self.ast.class_property(
property.r#type,
property.span,
Expand Down Expand Up @@ -324,18 +331,18 @@ impl<'a> IsolatedDeclarations<'a> {

let mut has_private_key = false;
let mut elements = self.ast.new_vec();
let mut is_function_overloads = false;
// let mut is_function_overloads = false;
for element in &decl.body.body {
match element {
ClassElement::StaticBlock(_) => {}
ClassElement::MethodDefinition(ref method) => {
if method.value.body.is_none() {
is_function_overloads = true;
} else if is_function_overloads {
// Skip implementation of function overloads
is_function_overloads = false;
continue;
}
// if method.value.body.is_none() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO either fix, or uncomment

// is_function_overloads = true;
// } else if is_function_overloads {
// // Skip implementation of function overloads
// is_function_overloads = false;
// continue;
// }
if method.key.is_private_identifier() {
has_private_key = true;
continue;
Expand Down
3 changes: 2 additions & 1 deletion crates/oxc_isolated_declarations/src/declaration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use oxc_syntax::scope::ScopeFlags;
use crate::{
diagnostics::{
binding_element_export, inferred_type_of_expression, signature_computed_property_name,
variable_must_have_explicit_type,
variable_must_have_explicit_type, destructuring_export,
},
IsolatedDeclarations,
};
Expand Down Expand Up @@ -56,6 +56,7 @@ impl<'a> IsolatedDeclarations<'a> {
}
});
}
self.error(destructuring_export(decl.id.span()));
return None;
}

Expand Down
5 changes: 5 additions & 0 deletions crates/oxc_isolated_declarations/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,8 @@ pub fn type_containing_private_name(name: &str, span: Span) -> OxcDiagnostic {
))
.with_label(span)
}

pub fn destructuring_export(span: Span) -> OxcDiagnostic {
OxcDiagnostic::error("TS9999: Not supported yet")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to give feedback, that a certain transformation is not supported yet, instead of silently generating wrong .d.ts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, can you provide the examples that need to report this error?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TSXXXX error code is identical to the error reported by typescript. If you want to report an error that typescript doesn't have, you don't need to add TSXXXX.

Copy link
Contributor Author

@escaton escaton Jun 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you provide the examples that need to report this error?

  1. I added it here because return None; causes export const {a} = b() to be transformed into export declare const; instead of export declare const a: unknown;.
  2. I also considered adding it here because with current // Skip implementation of function overloads it generates incorrect code, and I don't have ideas how to fix it right away.

Methods in abstract classes could be empty, and it is recognized as overloading:

export abstract class A {
  b(): void;
  c(): void {};
  d(): void {}
}

->

export abstract class A {
  b(): void;
  d(): void {}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you provide the examples that need to report this error?

  1. I added it here because return None; causes export const {a} = b() to be transformed into export declare const; instead of export declare const a: unknown;.

This is a bug. Our goal is to keep the output the same as TypeScript, including diagnostics. if it's not the same, it's considered a bug. So we should report binding_element_export here

  1. I also considered adding it here because with current // Skip implementation of function overloads it generates incorrect code, and I don't have ideas how to fix it right away.

Methods in abstract classes could be empty, and it is recognized as overloading:

export abstract class A {
  b(): void;
  c(): void {};
  d(): void {}
}

->

export abstract class A {
  b(): void;
  d(): void {}
}

This code seems to be incorrect. TypeScript Playground. We assume that the transforms code is correct

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first case is fixed in #3976, The second case has a similar issue, and fixed in #4024

Copy link
Contributor Author

@escaton escaton Jul 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the first case is fixed in #4025, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!!

.with_label(span)
}
4 changes: 4 additions & 0 deletions crates/oxc_isolated_declarations/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ impl<'a> IsolatedDeclarations<'a> {
Declaration::TSModuleDeclaration(decl) => {
if decl.kind.is_global() {
transformed_indexes.push(new_stmts.len());

self.scope.visit_declaration(
stmt.to_declaration()
);
}
}
_ => {}
Expand Down
67 changes: 64 additions & 3 deletions crates/oxc_isolated_declarations/tests/deno/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@ mod tests {

let ret = IsolatedDeclarations::new(&allocator).build(&program);
let actual = CodeGenerator::new().build(&ret.program).source_text;
let expected_program = Parser::new(&allocator, expected, source_type).parse().program;
let expected = CodeGenerator::new().build(&expected_program).source_text;
// let expected_program = Parser::new(&allocator, expected, source_type).parse().program;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO revert

// let expected = CodeGenerator::new().build(&expected_program).source_text;

println!("program {:#?}", program);
println!("ret errors {:#?}", ret.errors);
println!("ret program {:#?}", ret.program);
println!("actual {:#?}", actual);
assert_eq!(actual.trim(), expected.trim());
}

Expand Down Expand Up @@ -157,7 +161,7 @@ export function foo(a: any): number {
transform_dts_test(
"export class Foo { declare a: string }",
"export declare class Foo {
a: string;
\ta: string;
}",
);
}
Expand Down Expand Up @@ -382,4 +386,61 @@ export function foo(a: any): number {
fn dts_default_export_all() {
transform_dts_test("export * as foo from \"foo\";", "export * as foo from \"foo\";");
}

#[test]
fn dts_class_type_param() {
transform_dts_test(
"export class A<T> extends B<T> implements C<T>{};",
"export declare class A<T> extends B<T> implements C<T> {}",
);
}

#[test]
fn dts_fn_type_param() {
transform_dts_test(
"export default function<T>(arg1: T) {}",
"export default function<T>(arg1: T);",
);
}

#[test]
fn dts_destructed_export() {
transform_dts_test("export const {a} = b()", "export declare const a: unknown;");
}

#[test]
fn dts_tuple_optional_item() {
transform_dts_test(
"export const a: [x?: number | null] = [null]",
"export declare const a: [x?: ((number) | (null))];\n",
);
}

#[test]
fn dts_abstract_class_methods() {
transform_dts_test(
"export abstract class A { b(): boolean; c(): void {};}",
"export declare abstract class A {\n\tb(): boolean;\n\tc(): void;\n}\n",
);
}

#[test]
fn dts_class_field_literal() {
transform_dts_test(
"export class A {readonly type = 'frame'}",
"export declare class A {\n\treadonly type: 'frame';\n}\n",
);
transform_dts_test(
"export class A {type = 'frame'}",
"export declare class A {\n\ttype: string;\n}\n",
);
}

#[test]
fn dts_global_declare_refs() {
transform_dts_test(
"import A from ''; declare global {a: A}",
"import A from '';\ndeclare global {\n\ta: A;\n}\n",
);
}
}