Various Isolated declarations fixes #3976
Conversation
Your org has enabled the Graphite merge queue for merging into mainAdd the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
github-actions
bot
added
A-codegen
| is_function_overloads = false; | ||
| continue; | ||
| } | ||
| // if method.value.body.is_none() { |
There was a problem hiding this comment.
TODO either fix, or uncomment
| @@ -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; | |||
| @@ -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") | |||
There was a problem hiding this comment.
I wanted to give feedback, that a certain transformation is not supported yet, instead of silently generating wrong .d.ts
There was a problem hiding this comment.
I agree, can you provide the examples that need to report this error?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
can you provide the examples that need to report this error?
- I added it here because
return None;causesexport const {a} = b()to be transformed intoexport declare const;instead ofexport declare const a: unknown;. - I also considered adding it here because with current
// Skip implementation of function overloadsit 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 {}
}
There was a problem hiding this comment.
can you provide the examples that need to report this error?
- I added it here because
return None;causesexport const {a} = b()to be transformed intoexport declare const;instead ofexport 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
- I also considered adding it here because with current
// Skip implementation of function overloadsit 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
There was a problem hiding this comment.
the first case is fixed in #4025, right?
|
@Boshen hi! |
|
Thank you in advance. I have copied over the codegen changes in https://app.graphite.dev/github/pr/oxc-project/oxc/3980 so I can make a quicker release. |
|
Thank you for working on this! If you need to add test, please add in fixtures. The |
|
Thank you for your feedback! All known bugs are about to be fixed. Please Feel free to send an RP fix if you find other bugs. |
|
@Dunqing I think these commits are still actual I'll create another PR for them with proper tests |
Yes, go ahead! |
I'm playing with "isolated-declarations" in a project with 30k files.
These are fixes I did while attempting to get a successful typecheck.
For now, there is at least one test for each case, and I plan to write more later, but I think I'll need help to make them better.