-
Notifications
You must be signed in to change notification settings - Fork 659
feat(rome_js_parser): Support optional variance annotation #4250
Conversation
…ETHATGUY/tools into feat/optional-variance-annotation
…annotation' into optional-variance-annotation Co-authored-by: IWANABETHATGUY <iwanabethatguy@qq.com>
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
I'm still working in progress. Mainly, I'm thinking about how to build AST and parse syntax for type parameter modifiers. Considering in/out modifier, it is not tough task and there is a reference PR like #2556. But const modifier also will land in TS 5.0. Considering about in/out/const modifier, the parse logic become more complex. Therefore, it have taken much time than I expected. If anyone have mush time for this task, it is ok if this task is assigned to others. |
I will work for reporting error about duplicated modifiers and modifiers order. I update the PR in a few days. |
This PR is ready for review. |
pub(crate) fn parse_ts_type_parameters_with_modifiers( | ||
p: &mut JsParser, | ||
context: TypeContext, | ||
allow_in_out_modifier: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any cases where allow_in_out_modifier
is false
. Is it a bug or did I miss some code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This argument was currently not used. But, when supporting const modifier (see: #4227), it will be used. parse_ts_type_parameters_with_modifiers
function need to supports these cases.
function, method: parse only const modifier
interface, type alias: parse only in/out modifier
class: parse const and in/out modifier
I think it is ok to remove this argument.
kind: TypeParameterModifierKind::Out, | ||
range: p.cur_range(), | ||
}, | ||
_ => unreachable!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually, it's helpful to add a message about why this is unreachable. I suppose here it's unreachable because keywords that are not "in" and "out" are checked earlier
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added comments 👍
m.complete(p, TS_TYPE_PARAMETER) | ||
}) | ||
// test_err ts type_parameter_modifier1 | ||
// export default function foo<in T>() {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we test cases where we use incorrect words?
type Foo<innn T> = {}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the test 👍
fn parse_ts_type_parameter( | ||
p: &mut JsParser, | ||
context: TypeContext, | ||
allow_in_out_modifier: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind turning this into an enum
?
InOutModifier {
Allowed,
Disallowed
}
The reason why I am asking is that we can document when the modifier is allowed or when it's not. Or, we in alternative, we should document the function and explains when allow_in_out_modifier
is true
and when it's false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agreed 👍 I refactor bool flag to bitflags like DISALLOW_CONDITIONAL_TYPES
in TypeContext.
@@ -61,15 +61,15 @@ impl JsSyntaxKind { | |||
/// Returns `true` for any contextual (await) or non-contextual keyword | |||
#[inline] | |||
pub const fn is_keyword(self) -> bool { | |||
(self as u16) <= (JsSyntaxKind::OF_KW as u16) | |||
(self as u16) <= (JsSyntaxKind::OUT_KW as u16) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This function judge whether the kind is keyword or not by enum order in JsSyntaxKind
see: https://github.com/nissy-dev/tools/blob/979d302e9d0360c1108f56c5ff6ccd2d3b9ef3d6/crates/rome_js_syntax/src/generated/kind.rs#L8
// declare class Foo<in T> {} | ||
// declare class Foo<out T> {} | ||
// declare interface Foo<in T> {} | ||
// declare interface Foo<out T> {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's HUGE! 🔥🔥
/// Whether 'in' and 'out' modifiers are allowed in the current context. | ||
/// | ||
/// By default, 'in' and 'out' modifiers are not allowed. | ||
const ALLOW_IN_OUT_MODIFIER = 1 << 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great use of context!
* feat: 🎸 init * fix: 🐛 parse error * test: 💍 update more test case * test: 💍 update test case * test: 💍 update error test case * test: 💍 update test case * style: 💄 clippy and fmt * test: 💍 add extra test case * chore: 🤖 conflict resolve * test: 💍 update snapshot * chore: 🤖 update test result * test: 💍 update snapshot * chore: 🤖 update test parser * style: 💄 fmt * test: 💍 update format test snapshot * feat: update parse logic * feat: separate parse_ts_type_parameters and parse_ts_type_parameters_with_modifiers * fix: update test case * feat: update parser * fix: update tests * feat: refactor allow_in_out_modifier arguments * feat: revert unnecessary diff --------- Co-authored-by: IWANABETHATGUY <iwanabethatguy@qq.com>
Summary
Fix #2400
This PR is based on #2556
Test Plan
Documentation