feat: Support extends constraints on infer type #4018
Conversation
nissy-dev
requested review from
MichaReiser,
xunilrj,
ematipico and
a team
as code owners
✅ Deploy Preview for docs-rometools ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
nissy-dev
changed the title
There was a problem hiding this comment.
I found some TypeScript test cases that the current implementation doesn't handle correctly
You can run cargo coverage --language=ts to run the typescript test cases (I hope our TS submodule is recent enough). Or you can paste the content of this file in the playground and see if it gets parsed correctly.
I suspect that the issue is related to the fact that #48112 uses speculative parsing:
It only parses out the constraint if conditional types are disallowed or the current token after parsing the type isn't a ? token.
nissy-dev
force-pushed
the
support-extends-constraints-on-infer-type
branch
from
48f7993
to
4c655a0
Compare
|
@MichaReiser type EdgeCase1<T> = T extends { [P in infer U extends keyof T ? 1 : 0]: 1; } ? 1 : 0;
type EdgeCase2<T> = T extends ((...a: any[]) => infer R extends string) ? R : never;Log |
crates/rome_js_parser/src/state.rs
Outdated
| /// TODO | ||
| pub(crate) allow_conditional_type: bool, |
There was a problem hiding this comment.
Something to explore once we get the implementation working.
Would it be possible to move this flag out of the ParserState and instead introduce a TypeContext that's passed to all type parsing methods similar to the ExpressionContext used when parsing expressions?
The benefit of a scoped context vs adding global state to the parser is that:
- It doesn't increase the size of the parser snapshot (and, therefore, the size of creating a snapshot).
- Explicitly passing the context makes it more evident where the new flag is used
I looked into this more closely because it wasn't clear to me why the parsing is failing, and I created a draft PR that uses Case 1You can use the Case 2Not entirely sure but I think the issue here is that you missed to allow conditional types. |
|
!bench_parser |
|
Thanks for updating the PR. Do you plan to refactor the code to use a There are a few tests failing that should pass
|
Parser Benchmark Results |
|
@MichaReiser I would like to do the refactoring and fix the failing tests if possible, but it will be the day after tomorrow before I can start. This task might have been a bit difficult for me. Therefore, I am willing to have you continue the task on your end. |
That's alright. We're all taking a few days off. Enjoy the holidays |
I could fix these tests and pass all prettier tests!
I tried to refactor in a few days and made PR nissy-dev#6. |
| /// ```javascript | ||
| /// type Type<A> = A extends ((a: string) => infer B extends string) ? B : never; // true | ||
| /// ``` | ||
| pub(crate) fn is_includes_inferred_return_types_with_extends_constraints( |
There was a problem hiding this comment.
I referred to prettier/prettier#13275
This PR and your other PR looks good. The only thing that we need to be careful about is to make sure to pass |
If the scope of the refactoring is up to making sure the context is passed correctly, I think it is better to separate PR about the refactoring task. This is because there are too many functions which the context should be passed and this refactoring affects many logic. And then, it is really hard for me to pass context through correctly. I tried, but I couldn't do that. I would like you to do this refactoring on your end if possible. |
| match node.kind() { | ||
| JsSyntaxKind::TS_FUNCTION_TYPE => { | ||
| let function_type = TsFunctionType::unwrap_cast(node.clone()); | ||
| if let Ok(AnyTsReturnType::AnyTsType(AnyTsType::TsInferType(infer_type))) = | ||
| function_type.return_type() | ||
| { | ||
| if infer_type.constraint().is_some() { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| JsSyntaxKind::TS_CONSTRUCTOR_TYPE => { | ||
| let constructor_type = TsConstructorType::unwrap_cast(node.clone()); | ||
| if let Ok(AnyTsType::TsInferType(infer_type)) = constructor_type.return_type() { | ||
| if infer_type.constraint().is_some() { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| _ => (), | ||
| } |
There was a problem hiding this comment.
Nit
| match node.kind() { | |
| JsSyntaxKind::TS_FUNCTION_TYPE => { | |
| let function_type = TsFunctionType::unwrap_cast(node.clone()); | |
| if let Ok(AnyTsReturnType::AnyTsType(AnyTsType::TsInferType(infer_type))) = | |
| function_type.return_type() | |
| { | |
| if infer_type.constraint().is_some() { | |
| return true; | |
| } | |
| } | |
| } | |
| JsSyntaxKind::TS_CONSTRUCTOR_TYPE => { | |
| let constructor_type = TsConstructorType::unwrap_cast(node.clone()); | |
| if let Ok(AnyTsType::TsInferType(infer_type)) = constructor_type.return_type() { | |
| if infer_type.constraint().is_some() { | |
| return true; | |
| } | |
| } | |
| } | |
| _ => (), | |
| } | |
| let return_type = match node.kind() { | |
| JsSyntaxKind::TS_FUNCTION_TYPE => { | |
| let function_type = TsFunctionType::unwrap_cast(node.clone()); | |
| function_type.return_type() | |
| } | |
| JsSyntaxKind::TS_CONSTRUCTOR_TYPE => { | |
| let constructor_type = TsConstructorType::unwrap_cast(node.clone()); | |
| constructor_type.return_type() | |
| } | |
| _ => { | |
| return false; | |
| } | |
| }; | |
| match return_type { | |
| Ok(AnyTsType::TsInferType(infer_type)) => infer_type.constraint().is_some(), | |
| _ => false | |
| } |
|
That sounds good to me. Thank you @nissy-dev for another excellent parser contribution! |
MichaReiser
force-pushed
the
support-extends-constraints-on-infer-type
branch
from
51a7efe
to
f5c64b5
Compare
|
!bench_parser |
Parser Benchmark Results |
Summary
Part of #2400
This PR is initial implementation for "extends constraints on infer type" released in TS.4.7.
Test Plan
I add some parser tests and confirmed the prettier tests are almost passed.
The only following case is not passed, so I'm currently considering about how to pass it.