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

refactor(js_parser): parser TsImportType align with TypeScript Parser #4454

Merged
merged 6 commits into from
Nov 8, 2024

Conversation

fireairforce
Copy link
Contributor

@fireairforce fireairforce commented Nov 3, 2024

Summary

closes: #4334

Refer to the issue: #4421 by @simon-paris , I update the TS_IMPORT_TYPE parser Structure like:

// a: typeof import("mod", { with: { "resolution-mode": "import" } })
//                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TsImportTypeAssertion = 
  assertion_kind: ('assert' | 'with')
  ':'
  '{'
  assertions: JsImportAssertionEntryList
  '}'

// a: typeof import("mod", { with: { "resolution-mode": "import" } })
//                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TsImportTypeAssertionBlock = 
	'{'
	type_assertion: TsImportTypeAssertion
	'}'

// a: typeof import("mod", { with: { "resolution-mode": "import" } })
//                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TsImportTypeArguments = 
  '('  
     argument: AnyTsType
	  (
		','
		TsImportTypeAssertionBlock
	  )?
  ')'

// a: import("./test").T<typeof X>
//    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// a: import("./test")<string>
//    ^^^^^^^^^^^^^^^^^^^^^^^^^
// a: typeof import("./test", { with: { "resolution-mode": "import" } })
//    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TsImportType =
	'typeof'?
	'import'
	arguments: TsImportTypeArguments
	qualifier_clause: TsImportTypeQualifier?
	type_arguments: TsTypeArguments?

The parser structure which refers to TypeScript Parser: https://github.com/microsoft/TypeScript/blob/main/src/compiler/parser.ts#L4546

You can see the detail at js.ungram at crates/biome_js_parser and i also update the biome_js_formatter to formatter the statement.

Test Plan

Add both test code for biome_js_parser and biome_js_formatter, the formatter test case from the issue #4334 .

@github-actions github-actions bot added A-Parser Area: parser A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages labels Nov 3, 2024
// a: import("./test").T<typeof X>
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// a: import("./test")<string>
// ^^^^^^^^^^^^^^^^^^^^^^^^^
TsImportType =
'typeof'?
'import'
arguments: JsCallArguments
arguments: TsImportArguments
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey, based on the typescript source, it looks like it should be like this:

// (not tested)
TsImportType =
	'typeof'?
	'import'
	argument: AnyTsType
	(
		','
		'{'
		('assert' | 'with')
		':'
		'{'
		assertions: TsImportTypeAssertionList
		'}'
		'}'
	)?
	qualifier_clause: TsImportTypeQualifier?
	type_arguments: TsTypeArguments?

TsImportTypeAssertionList =
	(TsImportTypeAssertion (',' TsImportTypeAssertion)* ','?)

TsImportTypeAssertion =
	key: ('identifier' | 'js_string_literal')
	':'
	value: AnyJsExpression // parse_assignment_expression_or_higher

https://github.com/microsoft/TypeScript/blob/main/src/compiler/parser.ts#L4546

  • The first argument is any TS type
    • (not only a string - the "String literal expected" error comes from the typechecker)
  • The ,{assert:{ and }} are hardcoded tokens, it's not a comma separated list
  • The part inside the inner braces is a comma separated list of key:expression where key can only be an identifier or string literal, and expression is a JS expression (parseAssignmentExpressionOrHigher).
    • Trailing commas are allowed

Examples:

type A = typeof import(1); // ok (typechecker error)
type B = typeof import("a.json",{}); // syntax error - missing assert keyword
type C = typeof import("a.json",{assert:{}}); // ok
type D = typeof import("a.json",{assert:{a:"1"}}); // ok
type E = typeof import("a.json",{assert:{[a]:"1"}}); // syntax error - keys need to be identifiers or string literals
type F = typeof import("a.json",{assert:{"a":"1"}}); // ok
type G = typeof import("a.json",{assert:{a:"1", b:"2",}}); // ok
type H = typeof import("a.json",{assert:{}}, 1); // syntax error - 3rd arg not valid
type I = typeof import(import(Record<string,string>),{assert:{a:await p}}); // ok (typechecker error)

https://www.typescriptlang.org/play/?#code/CYUwxgNghgTiAEBbA9sArhBAiKA6AVgM7IB2W8A3gL4BQNALgJ4AOCAgvALzxOvIBm8AJaJmyGPQAUARgCUAbngB6JfGQBreJN7gAFuHUgY8IzHGyGLBACEuPKwOGjxUnAWJkANNQXLVhRhJ6KAAPExgzYwBaJCFCQiESAHN4KHijenhDRgB3cWBLVngAYTsdRxExCUk3IlIsbzTCDIAuaipfFTV1QoQAETKHQUqXGrw6rwom1qmWrGksKg7FLo1e+ABRQb5h52rajwap9Ik2gG0oAF05haXO-0DgsNNxeBjswngSEBBgHmR4AAjBBCUBBIT8IRGT6vQj0GCJFIQIT0IxQCCEdYAMW2IAqe1c40OjRO9DaOCwN0Wyz83XWAHFcfiqoT3PUSc1TrN5g0gXMAEwNO4rVRrHTwAASTN2LLGbMm0y5S088DkIvgASCoXCkTe8AAzDA-rAUiRkJkAG7o0HrACS0qcspG1QASuB8gAeOEI5Keb2IgB8sg5MygLSgOSgKPgzGFtI0Wh0YH0YEMxheMFkQA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😯, thank you very much to provide the typescript context, i will base this on to update my code here, I haven't seen the relevant parser specification reference for this before.

Comment on lines 1677 to 1680
// test ts ts_import_type_arguments
// type A = import("foo");
// type B = import("foo", "bar");
// type C = typeof import("foo");
Copy link
Member

Choose a reason for hiding this comment

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

We moved away from this testing. Now just create a file inside the specs and run cargo t to generate the snapshot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, I asked why I panic here when I execute cargo codegen test recently. i will update later~ now the parser js test case is align with css parser.

Copy link
Member

Choose a reason for hiding this comment

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

Exactly. The comments in the source code are still there because the PR was too big. We will have to delete them eventually

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 will deal this later

@fireairforce fireairforce force-pushed the fix-4334 branch 4 times, most recently from 76b0731 to 6aa9987 Compare November 6, 2024 06:14
Copy link

codspeed-hq bot commented Nov 6, 2024

CodSpeed Performance Report

Merging #4454 will not alter performance

Comparing fireairforce:fix-4334 (45ab7fd) with main (3fe9193)

Summary

✅ 99 untouched benchmarks

@fireairforce fireairforce marked this pull request as ready for review November 6, 2024 06:18
@fireairforce fireairforce force-pushed the fix-4334 branch 3 times, most recently from 1814d50 to dc77291 Compare November 6, 2024 11:48
xtask/codegen/js.ungram Outdated Show resolved Hide resolved
crates/biome_js_parser/src/syntax/typescript/types.rs Outdated Show resolved Hide resolved

i 'with' expected.

> 1 │ type A = typeof import("a.json",{});
Copy link
Member

Choose a reason for hiding this comment

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

nit: The range here is incorrect, because with must be after {, which means that with should be where }. Hence we should check for the next token, IMHO

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, Typescript compiler do the same thing, i will update later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done~

Comment on lines 17 to 32
if comma_token.is_some() && ts_import_type_assertion_block.is_some() {
write!(
f,
[
l_paren_token.format(),
argument.format(),
comma_token.format(),
space(),
ts_import_type_assertion_block.format(),
space(),
r_paren_token.format()
]
)
} else {
Err(SyntaxError)
}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the code that caused the regression. When you have optional tokens, you can't emit a syntax error if they aren't present. In your case, you should do something like this:

fn main() {
            write!(
                f,
                [
                    l_paren_token.format(),
                    argument.format()
                ]
            )?;
	
	if let Some(comma_token) = comma_token {
		write!(f, [comma_token.format(), space()])?;
	}

	
	if let Some(ts_import_type_assertion_block) = ts_import_type_assertion_block {
		write!(f, [ts_import_type_assertion_block.format(), space()])?;
	}	

	write!(f, [r_paren_token.format()])
}

Also, I strongly suggest you to check how the JS import assertions are formatted. https://github.com/biomejs/biome/blob/main/crates/biome_js_formatter/src/js/module/import_assertion.rs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wow, thank you here, i will take a look on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I followed this method, but I found that the import statement would bring a newline and I added a few spaces here. Now I realize that maybe the newline here is the correct way to handle it. I update the code here of my import_type_assertion.rs to align with import_assertion.rs

@github-actions github-actions bot added the A-Changelog Area: changelog label Nov 7, 2024
@fireairforce fireairforce changed the title fix(js_formatter): don't insert trailing comma on type import statement refactor(js_parser): TsImportType align with TypeScript Parser Nov 7, 2024
@fireairforce fireairforce changed the title refactor(js_parser): TsImportType align with TypeScript Parser refactor(js_parser): parser TsImportType align with TypeScript Parser Nov 7, 2024
Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

Thank you for addressing my comments!

@ematipico ematipico merged commit d0faaf4 into biomejs:main Nov 8, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Formatter Area: formatter A-Parser Area: parser A-Tooling Area: internal tools L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

📝 Redundant trailing comma on import statement
3 participants