Filtering mapped types declarations #51650
Conversation
tests/baselines/reference/mappedTypeWithAsClauseAndLateBoundProperty.errors.txt
Show resolved
Hide resolved
| !!! error TS4118: The type of this node cannot be serialized because its property '[iterator]' cannot be serialized. | ||
| ~~~~~ | ||
| !!! error TS4118: The type of this node cannot be serialized because its property '[unscopables]' cannot be serialized. |
There was a problem hiding this comment.
It's probably highly related to what I've mentioned in the other comment here. Those symbols~ should be serializable though (IMHO), so it looks like a good change
|
|
||
|
|
||
| //// [mappedTypeWithAsClauseAndLateBoundProperty2.d.ts] | ||
| export declare const thing: { |
There was a problem hiding this comment.
this on the other hand is a direct result of the fact that we now can serialize those symbols properly, so the type for this could be emitted here
| //// [DtsFileErrors] | ||
|
|
||
|
|
||
| tests/cases/compiler/mappedTypeWithAsClauseAndLateBoundProperty2.d.ts(24,118): error TS2526: A 'this' type is available only in a non-static member of a class or interface. |
There was a problem hiding this comment.
This is a little bit worrying though. It doesn't exactly look like a new problem though. It's just accidental that by making this thing serializable I now emit this error. I think that this should be handled separately. WDYT?
There was a problem hiding this comment.
Definitely it can be handled separately; I can't tell if it's a bug or not that this is emitted as a return type of an object literal, but it's worth investigating.
| ////const filtered: { [P in keyof typeof obj as P extends 'b' ? never : P]: 0; } = { a: 0 }; | ||
| ////filtered.[|/*ref*/a|]; | ||
|
|
||
| verify.goToDefinition("ref", "def"); |
There was a problem hiding this comment.
I try to add a test for symbol renaming too but I can't figure out how to write one properly. Any tips would be highly appreciated.
So far I have this:
/// <reference path='fourslash.ts' />
////const obj = { [|[|{| "contextRangeIndex": 0 |}a|]: 1|], b: 2 };
////const filtered: { [P in keyof typeof obj as P extends 'b' ? never : P]: 0; } = { [|a|]: 0 };
////filtered.[|a|]();
const [r0Def, r0, r1, r2] = test.ranges();
verify.renameLocations(r0, [r0, r1, r2]);
verify.renameLocations(r1, [r0, r1, r2]);
verify.renameLocations(r2, [r0, r1, r2]);There was a problem hiding this comment.
Note that it works OK in the IDE as I've confirmed that by using the built/local/tsserver.js.
There was a problem hiding this comment.
It seems that I'm using this whole contextRangeIndex incorrectly. I've tried without it too but no luck so far.
There was a problem hiding this comment.
Generally if find-all-refs works then goto-def and rename work too. Can you write a similar test that uses verify.baselineFindAllReferences from at least the last line, but preferably the first and second. I think usage of that function is simpler and you can just prefix the locations with /*1*/, /*2*/, etc.
There was a problem hiding this comment.
Thanks for the tip! I've pushed out such a test and it looks correct to me. Note that this was the first time I've used this API so I'm not exactly sure how to interpret all of the information in the generated baseline but the references property for all 3 markers looks alright to me.
| ////const filtered: { [P in keyof typeof obj as P extends 'b' ? never : P]: 0; } = { a: 0 }; | ||
| ////filtered.[|/*ref*/a|]; | ||
|
|
||
| verify.goToDefinition("ref", "def"); |
There was a problem hiding this comment.
Generally if find-all-refs works then goto-def and rename work too. Can you write a similar test that uses verify.baselineFindAllReferences from at least the last line, but preferably the first and second. I think usage of that function is simpler and you can just prefix the locations with /*1*/, /*2*/, etc.
| //// [DtsFileErrors] | ||
|
|
||
|
|
||
| tests/cases/compiler/mappedTypeWithAsClauseAndLateBoundProperty2.d.ts(24,118): error TS2526: A 'this' type is available only in a non-static member of a class or interface. |
There was a problem hiding this comment.
Definitely it can be handled separately; I can't tell if it's a bug or not that this is emitted as a return type of an object literal, but it's worth investigating.
src/compiler/checker.ts
Outdated
| @@ -12881,6 +12881,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const typeParameter = getTypeParameterFromMappedType(type); | |||
| const constraintType = getConstraintTypeFromMappedType(type); | |||
| const nameType = getNameTypeFromMappedType(type.target as MappedType || type); | |||
| const isFilteringMappedType = nameType && isTypeAssignableTo(nameType, typeParameter); | |||
There was a problem hiding this comment.
This seems like more work than the rest of the function does, so I'm worried about performance problems on mapped types with a nameType. I'll run the perf tests, but I don't know that any such mapped types are in there. @weswigham do you have an idea if this isTypeAssignableTo call is likely to be (relatively) slow here?
There was a problem hiding this comment.
Note that it's the same check that got used here:
https://github.com/microsoft/TypeScript/pull/48699/files#diff-d9ab6589e714c71e657f601cf30ff51dfc607fc98419bf72e04f6b0fa92cc4b8R15745
That said - that other PR calls this just from within getSimplifiedIndexedAccessType and this is in the resolveMappedTypeMembers so the impact on the perf could still be different as it depends on how frequent are calls to those caller functions
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 7741604. You can monitor the build here. Update: The results are in! |
|
@sandersn Here they are:
CompilerComparison Report - main..51650
System
Hosts
Scenarios
TSServerComparison Report - main..51650
System
Hosts
Scenarios
StartupComparison Report - main..51650
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@sandersn this would be quite a nice improvement for some libraries, is there any chance (without rushing it, of course) that this could get into the approaching 5.0? |
|
ping @weswigham |
|
@typescript-bot test this |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at bf58389. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at bf58389. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at bf58389. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at bf58389. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at bf58389. You can monitor the build here. Update: The results are in! |
|
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
|
@DanielRosenwasser Here are the results of running the user test suite comparing Something interesting changed - please have a look. Details
|
|
Heya @DanielRosenwasser, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
1 similar comment
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
…s-declarations # Conflicts: # tests/baselines/reference/mappedTypeWithAsClauseAndLateBoundProperty.errors.txt
|
@weswigham friendly 🏓 |
This should help with things like:
closes #47813