Type guard by deep property - #38839
Conversation
Sorry, something went wrong.
|
thanks to Sohei Koyama (@KoyamaSohei), I use his test code. |
Sorry, something went wrong.
|
Friendly Ping Andrew Branch (@andrewbranch) ~ |
Sorry, something went wrong.
|
Thanks for putting this together Song Gao (@ShuiRuTian)! I looked through a bunch of the changed test baselines last week, and my suspicion is that this change is going to be too big for 4.0 now that we’ve released the beta (we try not to add any big features or breaking changes during this period). I’m on DefinitelyTyped duty this week, but will try to give this a more careful review next week and discuss with the team. Just wanted to set expectations that even if all the changes look perfect, we may decide it needs to wait until 4.1. Thanks again! |
Sorry, something went wrong.
|
Oh, glad to know the plan, just at your own pace! Andrew Branch (@andrewbranch) |
Sorry, something went wrong.
Andrew Branch (andrewbranch)
left a comment
There was a problem hiding this comment.
The baseline changes definitely look like desirable changes to me. I’m not very familiar with control flow in the checker so I’d want Anders Hejlsberg (@ahejlsberg) and/or Wesley Wigham (@weswigham) to review the implementation. Thanks!
Sorry, something went wrong.
|
TypeScript Bot (@typescript-bot) test this |
Sorry, something went wrong.
|
Heya Andrew Branch (@andrewbranch), I've started to run the parallelized community code test suite on this PR at 9ac7412. You can monitor the build here. |
Sorry, something went wrong.
|
Heya Andrew Branch (@andrewbranch), I've started to run the extended test suite on this PR at 9ac7412. You can monitor the build here. |
Sorry, something went wrong.
|
Heya Andrew Branch (@andrewbranch), I've started to run the perf test suite on this PR at 9ac7412. You can monitor the build here. Update: The results are in! |
Sorry, something went wrong.
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
Sorry, something went wrong.
|
Andrew Branch (@andrewbranch) Comparison Report - master..38839
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sorry, something went wrong.
|
Let's take this up at the next design meeting. |
Sorry, something went wrong.
| return isTypeArrayDiscriminant(propertyTypeArray, isRootHasUndefinedOrNull); | ||
| } | ||
|
|
||
| function narrowTypeByDiscriminantNew(type: Type, access: AccessExpression, narrowTypeCb: (t: Type) => Type): Type { |
There was a problem hiding this comment.
narrowTypeCb used to just be called narrowType, and I think that's what it's still called elsewhere.
| function narrowTypeByDiscriminantNew(type: Type, access: AccessExpression, narrowTypeCb: (t: Type) => Type): Type { | |
| function narrowTypeByDiscriminantNew(type: Type, access: AccessExpression, narrowType: (t: Type) => Type): Type { |
Sorry, something went wrong.
There was a problem hiding this comment.
I add postfix because there is another function really named narrowType, I confuse them for some times.
I have no idea, is it a bad choice in fact?
Sorry, something went wrong.
| if (propType.flags & TypeFlags.Union) { | ||
| (propType as UnionType).types.forEach(t => subtypes.push(t)); | ||
| } | ||
| else subtypes.push(propType); |
There was a problem hiding this comment.
| if (propType.flags & TypeFlags.Union) { | |
| (propType as UnionType).types.forEach(t => subtypes.push(t)); | |
| } | |
| else subtypes.push(propType); | |
| forEachType(propType, t => subtypes.push(t)); |
Sorry, something went wrong.
There was a problem hiding this comment.
Suprising, I find that this is not equal. forEach would not always run callback on each item.
forEachType(propType, t => {subtypes.push(t)});
Only when the callback not return value, they are totally same. Maybe this is not a bug, just a little annoying.
Sorry, something went wrong.
|
Daniel Rosenwasser (@DanielRosenwasser) Thanks for taking this up and a lot of suggestions which help code much more readable |
Sorry, something went wrong.
|
The TypeScript team hasn't accepted the linked issue #32399. If you can get it accepted, this PR will have a better chance of being reviewed. |
Sorry, something went wrong.
|
🤨 TypeScript Bot (@typescript-bot) go home, you’re drunk |
Sorry, something went wrong.
|
Now that I am a member of MS, follow my orders, robot! TypeScript Bot (@typescript-bot) test this |
Sorry, something went wrong.
This comment has been minimized.
|
Auh, we need some more PR to do things correctly. Here are the list and reasons:
And there are some bugs I have no idea how to fix, so I use @ts-ignore to hide them. This is pretty bad, but it is an existing behavior Also, this PR #42556 brings a good optimize, but it only works for direct constituent. I will consider how to bring it back in the following PR. |
Sorry, something went wrong.
|
Song Gao (@ShuiRuTian) Andrew Branch (@andrewbranch) what's the current status of this PR - is there anything I could do to help here (including implementation or investigating some uncovered/undecided stuff)? |
Sorry, something went wrong.
|
Mateusz Burzyński (@Andarist) Andrew tried bringing the PR to some releases, but obviously, the effort failed. Personally, I think it might be even a good thing. It proves the team does not rush to a new feature, but make decisions carefully. PS: Chinese New Year is coming, Happy Tiger Year! |
Sorry, something went wrong.
…truthiness add tests update tests fix remove useless code fix test reference clean code clean code one level alias could be narrowed by deep property fix fix bootstrap
|
The conflict is resolved. Ready to receive feedback at any time :) |
Sorry, something went wrong.
Wesley Wigham (weswigham)
left a comment
There was a problem hiding this comment.
Looks like a merge went a bit wrong? Buncha things seem off that prior reviews would have flagged.
Sorry, something went wrong.
| return candidate; | ||
| } | ||
| } | ||
| // if (clauseStart < clauseEnd && type.flags & TypeFlags.Union && getKeyPropertyName(type as UnionType) === getAccessedPropertyName(access)) { |
There was a problem hiding this comment.
Commented out code?
Sorry, something went wrong.
| } | ||
| } | ||
| } | ||
| // if ((operator === SyntaxKind.EqualsEqualsEqualsToken || operator === SyntaxKind.ExclamationEqualsEqualsToken) && type.flags & TypeFlags.Union) { |
There was a problem hiding this comment.
Also commented out code?
Sorry, something went wrong.
| break; | ||
| default: | ||
| throw Debug.assertNever(node.operator); | ||
| throw new Error("never"); |
There was a problem hiding this comment.
You shouldn't need to make this change. assertNever is here to ensure all possible cases for node.operator are handled.
Sorry, something went wrong.
| return isErrorType(type) ? errorType : createNewTargetExpressionType(type); | ||
| default: | ||
| Debug.assertNever(node.keywordToken); | ||
| throw new Error("never"); |
There was a problem hiding this comment.
Likewise, Debug.assertNever is ansuring node.keywordToken is exhaustively handled - it shouldn't be removed.
Sorry, something went wrong.
| return; | ||
| } | ||
|
|
||
| // @ts-ignore |
There was a problem hiding this comment.
Definitely can't have a //@ts-ignore for any reason in our codebase. There's always some other way to handle whatever's going on.
Sorry, something went wrong.
| const platform: string = _os.platform(); | ||
| const useCaseSensitiveFileNames = isFileSystemCaseSensitive(); | ||
| const realpathSync = _fs.realpathSync.native ?? _fs.realpathSync; | ||
| const realpathSync = _fs.realpathSync.native; |
There was a problem hiding this comment.
Bad merge? Pretty sure the fallback should still be here.
Sorry, something went wrong.
| break; | ||
| default: | ||
| Debug.assertNever(projectService.serverMode); | ||
| throw new Error("never"); |
There was a problem hiding this comment.
Again, should still be assertNever.
Sorry, something went wrong.
| break; | ||
| default: | ||
| Debug.assertNever(this.projectService.serverMode); | ||
| throw new Error("never"); |
There was a problem hiding this comment.
Again, should still be assertNever.
Sorry, something went wrong.
| // filter handles case when 'projects' is undefined | ||
| projects = filter(projects, p => p.languageServiceEnabled && !p.isOrphan()); | ||
| if (!ignoreNoProjectError && (!projects || !projects.length) && !symLinkedProjects) { | ||
| // @ts-ignore |
There was a problem hiding this comment.
Again, can't have a //@ts-ignore.
Sorry, something went wrong.
| break; | ||
| default: | ||
| Debug.assertNever(state.specialSearchKind); | ||
| throw new Error("never"); |
There was a problem hiding this comment.
Again, should still be assertNever.
Sorry, something went wrong.
|
Wesley Wigham (@weswigham) enum FooKind {
one,
two,
three,
}
interface Foo {
kind: FooKind
}
function FooFunc(tmp: Foo) {
switch (tmp.kind) {
case FooKind.one:
break;
case FooKind.two:
break;
case FooKind.three:
break;
default:
tmp; // what should here be? should it be `Foo` or `never`?
}
} In the new PR, I narrow tmp to never, otherwise, what could it be? Now, let's say we have code assertNever(tmp.foo) |
Sorry, something went wrong.
|
From the looks of it, this feature is unfortunately much more complex to implement than we had anticipated, and we don't think that the cost/benefit ratio is good in this case. As much as we want to support this pattern, the implications of these changes feel too far-reaching (even if they're necessary to actually support the scenario). We'll leave the original issue open in case later on down the line a simpler method of approach becomes available, but we aren't comfortable with introducing this much complexity in a critical codepath right now. All that said, putting this much time into the investigation is something we appreciate a ton. Thank you for all the effort you've put in into authoring this. |
Sorry, something went wrong.
|
This would also maybe have addressed #42384? |
Sorry, something went wrong.
Fixes #32399
Fixes #18758
support if clause, switch clause and ?? to narrow type by nest property for
Typeof, Truthiness, Discriminant
There some limitations described here #38839 (comment)
And we need to do more things described here as following PR #38839 (comment)