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.
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.
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.
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.
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.
Do we test cases where we use incorrect words?
type Foo<innn T> = {}There was a problem hiding this comment.
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.
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.
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.
Are these changes correct?
There was a problem hiding this comment.
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> {} |
| /// 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; |
* 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