Type guard by deep property #38839
Type guard by deep property #38839
Conversation
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
12ab497
to
d221e6b
Compare
|
thanks to @KoyamaSohei, I use his test code. |
|
Friendly Ping @andrewbranch ~ |
|
Thanks for putting this together @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! |
|
Oh, glad to know the plan, just at your own pace! @andrewbranch |
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
f3e4ba4
to
9ac7412
Compare
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 @ahejlsberg and/or @weswigham to review the implementation. Thanks!
tests/baselines/reference/typeGuardAccordingToProperty.errors.txt
Outdated
Show resolved
Hide resolved
|
@typescript-bot test this |
|
Heya @andrewbranch, I've started to run the parallelized community code test suite on this PR at 9ac7412. You can monitor the build here. |
|
Heya @andrewbranch, I've started to run the extended test suite on this PR at 9ac7412. You can monitor the build here. |
|
Heya @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! |
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@andrewbranch Here they are:Comparison Report - master..38839
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
bba4520
to
840cb01
Compare
|
Let's take this up at the next design meeting. |
src/compiler/checker.ts
Outdated
| 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 { |
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?
src/compiler/checker.ts
Outdated
| 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)); |
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.
|
@DanielRosenwasser Thanks for taking this up and a lot of suggestions which help code much more readable |
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
73be1db
to
4ae2a07
Compare
|
Now that I am a member of MS, follow my orders, robot! @typescript-bot test this |
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
e0c5a62
to
8be3a1d
Compare
This comment has been minimized.
This comment has been minimized.
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
0392be2
to
40521d7
Compare
|
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 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. |
|
@ShuiRuTian @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)? |
|
@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! |
…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
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
9370627
to
233d341
Compare
|
The conflict is resolved. Ready to receive feedback at any time :) |
| return candidate; | ||
| } | ||
| } | ||
| // if (clauseStart < clauseEnd && type.flags & TypeFlags.Union && getKeyPropertyName(type as UnionType) === getAccessedPropertyName(access)) { |
| } | ||
| } | ||
| } | ||
| // if ((operator === SyntaxKind.EqualsEqualsEqualsToken || operator === SyntaxKind.ExclamationEqualsEqualsToken) && type.flags & TypeFlags.Union) { |
| @@ -14996,7 +14996,7 @@ namespace ts { | |||
| links.resolvedType = getTypeFromTypeNode(node.type); | |||
| 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.
src/compiler/checker.ts
Outdated
| @@ -31443,7 +31734,7 @@ namespace ts { | |||
| const type = checkNewTargetMetaProperty(node); | |||
| 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.
| @@ -40150,7 +40442,7 @@ namespace ts { | |||
| // If we hit an export assignment in an illegal context, just bail out to avoid cascading errors. | |||
| 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.
src/compiler/sys.ts
Outdated
| @@ -1283,7 +1283,7 @@ namespace ts { | |||
|
|
|||
| 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.
src/server/project.ts
Outdated
| @@ -307,7 +307,7 @@ namespace ts.server { | |||
| this.compilerOptions.types = []; | |||
| break; | |||
| default: | |||
| Debug.assertNever(projectService.serverMode); | |||
| throw new Error("never"); | |||
There was a problem hiding this comment.
Again, should still be assertNever.
src/server/session.ts
Outdated
| @@ -798,7 +798,7 @@ namespace ts.server { | |||
| ); | |||
| break; | |||
| default: | |||
| Debug.assertNever(this.projectService.serverMode); | |||
| throw new Error("never"); | |||
There was a problem hiding this comment.
Again, should still be assertNever.
src/server/session.ts
Outdated
| @@ -1516,6 +1516,7 @@ namespace ts.server { | |||
| // 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.
src/services/findAllReferences.ts
Outdated
| @@ -1525,7 +1525,7 @@ namespace ts.FindAllReferences { | |||
| addClassStaticThisReferences(referenceLocation, search, state); | |||
| break; | |||
| default: | |||
| Debug.assertNever(state.specialSearchKind); | |||
| throw new Error("never"); | |||
There was a problem hiding this comment.
Again, should still be assertNever.
|
@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 Now, let's say we have code |
|
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. |
|
This would also maybe have addressed #42384? |
Fixes #32399
Fixes #18758
support
ifclause,switchclause and??to narrow type by nest property forTypeof,Truthiness,DiscriminantThere some limitations described here #38839 (comment)
And we need to do more things described here as following PR #38839 (comment)