Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
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
|
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
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
|
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 |
|
@andrewbranch @DanielRosenwasser Please don't forget this PR. Please. |
|
@ShuiRuTian I tried to fix the merge conflicts, but there’s a significant conflict with #44730— |
|
@andrewbranch Thanks a lot! Edit: Edit 2: What blocks me now: interface A {
q: string;
inner: {
type: 'a'
}
}
interface B {
w: string;
inner: {
type: 'b'
}
}
function f(foo: A | B) {
const i = foo.inner;
const t = i.type;
if(t === 'a'){
i; // {type:'a'}, narrowed, exciting!
foo; // A | B, well ,this is not narrowed, but it would be great if narrowed.
}
}I am not sure whether this PR should handle this, but I want to have it! (at least for now, if it is not too hard...) Edit 3: type Shape =
| { kind: "circle", radius: number }
| { kind: "square", sideLength: number };
function area(shape: Shape): number {
const tmp = shape.kind;
const isCircle = shape.kind === "circle";
if (isCircle) {
// We know we have a circle here!
return Math.PI * shape.radius ** 2;
// However, we do not know tmp is "circle";
tmp;
}
else {
// We know we're left with a square here!
return shape.sideLength ** 2;
}
} |
|
Hi, great progress here. How is it going with merge? I want to encourage you to finish it. It would be unfortunate to stop here. A lot of devs having problems with this. Thanks. |
|
Oh wow, I had been waiting the whole last month on @ShuiRuTian to respond after the merge conflicts, and I missed it because the responses were all in edits, which doesn’t send me notifications! Sorry! The behavior under “Edit 3” makes sense to me. Is the behavior under “Edit 2” still the case? I’d have to compare with current behavior to see if that’s what I would expect. I agree it would be nice to have, but doesn’t seem necessary. |
|
Oops, sorry, my bad! However, I have overworked for several weeks(including weekends), and have almost no time left for this PR :( But goods new is that a 7-day holiday is coming, start at 10/1, hope I could finish the reaming work during these days! |
|
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. |
|
|
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
73be1db
to
4ae2a07
|
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
|
Anders has another PR which would influence this one. |
…truthiness add tests update tests fix remove useless code fix test reference
ShuiRuTian
force-pushed
the
typeof-coud-narrow-union-type
branch
from
0392be2
to
40521d7
|
Ready to be reviewed again. |
|
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. |
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)
The text was updated successfully, but these errors were encountered: