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

feat(es/parser): support type-only import/export specifiers #2309

Merged
merged 18 commits into from
Sep 28, 2021
Merged

feat(es/parser): support type-only import/export specifiers #2309

merged 18 commits into from
Sep 28, 2021

Conversation

g-plane
Copy link
Contributor

@g-plane g-plane commented Sep 27, 2021

Ref: microsoft/TypeScript#45998

This PR allows:

import { type foo } from 'mod'
import { type foo as bar } from 'mod'

but rejects:

import type { type foo } from 'mod'

if orig_name.sym == js_word!("type")
&& is!(self, IdentName)
&& self.syntax().typescript()
// invalid: `import type { type something } from 'mod'`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should still parse this scenario even though it is invalid. It would be useful in dprint-plugin-typescript for example to parse this then convert import type { type something} from 'mod' to import type { something } from "mod";. Thoughts? (by the way, nice PR)

Copy link
Contributor Author

@g-plane g-plane Sep 28, 2021

Choose a reason for hiding this comment

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

I think dprint is a formatter, and it shouldn't accept code with invalid syntax.

Copy link
Contributor

@dsherret dsherret Sep 28, 2021

Choose a reason for hiding this comment

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

In the TS parser implementation they always parse this: https://github.com/microsoft/TypeScript/pull/45998/files#diff-12a6724be007eb1a19d80018c7a63bbc73525ca793a9b3e5da49a4e86bbf457c

That allows the language service and other code analysis to understand this code even though it might not be correct.

Then in the checker.ts code that's where they check if this is allowed: https://github.com/microsoft/TypeScript/pull/45998/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R43439-R43450

Is there any harm in always parsing this even when it is a type-only import/export? I also think parsing this would align more with swc's type checker.

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 prefer that this would be syntax errors, but I'm OK to treat it as valid syntax, because this isn't a huge change.

The biggest annoying problem is that once I change the code, rustc will cost very very long time.

Copy link
Member

Choose a reason for hiding this comment

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

It can be syntax error and can be valid ast in same time, and I think it's the right direction to go.
emit_err can be used for such purpose.

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

Thanks!
Parser changes LGTM, but I found some small issues about the role of crates.

ecmascript/ast/src/module_decl.rs Show resolved Hide resolved
ecmascript/ast/src/module_decl.rs Show resolved Hide resolved
ecmascript/codegen/src/lib.rs Outdated Show resolved Hide resolved
ecmascript/codegen/src/lib.rs Outdated Show resolved Hide resolved
if orig_name.sym == js_word!("type")
&& is!(self, IdentName)
&& self.syntax().typescript()
// invalid: `import type { type something } from 'mod'`
Copy link
Member

Choose a reason for hiding this comment

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

It can be syntax error and can be valid ast in same time, and I think it's the right direction to go.
emit_err can be used for such purpose.

ecmascript/parser/src/parser/stmt/module_item.rs Outdated Show resolved Hide resolved
ecmascript/parser/src/parser/stmt/module_item.rs Outdated Show resolved Hide resolved
ecmascript/parser/src/parser/stmt/module_item.rs Outdated Show resolved Hide resolved
@g-plane g-plane requested a review from kdy1 September 28, 2021 12:11
@g-plane
Copy link
Contributor Author

g-plane commented Sep 28, 2021

I've used emit_err in ecmascript/parser/src/parser/stmt/module_item.rs, but I can't hide the review conversation above.

Copy link
Member

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot!

@kdy1 kdy1 added this to the v1.2.92 milestone Sep 28, 2021
@kdy1 kdy1 merged commit 2b292e6 into swc-project:master Sep 28, 2021
@g-plane g-plane deleted the type-only-specifiers branch September 28, 2021 13:36
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants