Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type guard by deep property #38839

Closed

Conversation

ShuiRuTian
Copy link
Contributor

@ShuiRuTian ShuiRuTian commented May 29, 2020

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)

@ShuiRuTian
Copy link
Contributor Author

thanks to @KoyamaSohei, I use his test code.

@ShuiRuTian ShuiRuTian marked this pull request as ready for review June 18, 2020 03:39
@andrewbranch andrewbranch added the For Backlog Bug PRs that fix a backlog bug label Jun 22, 2020
@ShuiRuTian
Copy link
Contributor Author

Friendly Ping @andrewbranch ~

@andrewbranch
Copy link
Member

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!

@ShuiRuTian
Copy link
Contributor Author

Oh, glad to know the plan, just at your own pace! @andrewbranch

@ShuiRuTian ShuiRuTian force-pushed the typeof-coud-narrow-union-type branch from f3e4ba4 to 9ac7412 Compare July 2, 2020 17:22
Copy link
Member

@andrewbranch andrewbranch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@andrewbranch
Copy link
Member

@typescript-bot test this
@typescript-bot user test this
@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 2, 2020

Heya @andrewbranch, I've started to run the parallelized community code test suite on this PR at 9ac7412. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 2, 2020

Heya @andrewbranch, I've started to run the extended test suite on this PR at 9ac7412. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 2, 2020

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!

@typescript-bot
Copy link
Collaborator

The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master.

@typescript-bot
Copy link
Collaborator

@andrewbranch
The results of the perf run you requested are in!

Here they are:

Comparison Report - master..38839

Metric master 38839 Delta Best Worst
Angular - node (v10.16.3, x64)
Memory used 343,873k (± 0.02%) 343,455k (± 0.02%) -419k (- 0.12%) 343,343k 343,562k
Parse Time 2.00s (± 0.30%) 2.01s (± 0.88%) +0.00s (+ 0.25%) 1.98s 2.05s
Bind Time 0.81s (± 0.73%) 0.81s (± 0.68%) -0.00s (- 0.25%) 0.80s 0.82s
Check Time 4.73s (± 0.35%) 4.76s (± 0.42%) +0.03s (+ 0.68%) 4.71s 4.81s
Emit Time 5.18s (± 0.81%) 5.17s (± 0.50%) -0.01s (- 0.21%) 5.12s 5.24s
Total Time 12.73s (± 0.38%) 12.75s (± 0.41%) +0.02s (+ 0.18%) 12.67s 12.87s
Monaco - node (v10.16.3, x64)
Memory used 339,136k (± 0.03%) 339,135k (± 0.02%) -2k (- 0.00%) 338,961k 339,327k
Parse Time 1.59s (± 0.33%) 1.58s (± 0.70%) -0.01s (- 0.38%) 1.56s 1.61s
Bind Time 0.71s (± 0.70%) 0.71s (± 0.48%) +0.00s (+ 0.14%) 0.70s 0.71s
Check Time 4.87s (± 0.65%) 4.94s (± 0.76%) +0.06s (+ 1.29%) 4.87s 5.04s
Emit Time 2.75s (± 0.46%) 2.75s (± 0.76%) +0.00s (+ 0.11%) 2.71s 2.80s
Total Time 9.92s (± 0.40%) 9.98s (± 0.51%) +0.06s (+ 0.58%) 9.88s 10.09s
TFS - node (v10.16.3, x64)
Memory used 302,006k (± 0.01%) 302,059k (± 0.02%) +53k (+ 0.02%) 301,883k 302,135k
Parse Time 1.21s (± 0.48%) 1.21s (± 0.49%) -0.01s (- 0.58%) 1.20s 1.22s
Bind Time 0.67s (± 0.51%) 0.66s (± 1.47%) -0.01s (- 1.20%) 0.64s 0.67s
Check Time 4.38s (± 0.53%) 4.43s (± 0.58%) +0.04s (+ 1.03%) 4.37s 4.50s
Emit Time 2.92s (± 0.66%) 2.87s (± 0.64%) -0.05s (- 1.71%) 2.83s 2.90s
Total Time 9.18s (± 0.39%) 9.16s (± 0.38%) -0.02s (- 0.23%) 9.08s 9.23s
material-ui - node (v10.16.3, x64)
Memory used 459,428k (± 0.02%) 459,175k (± 0.02%) -252k (- 0.05%) 459,000k 459,445k
Parse Time 2.05s (± 0.63%) 2.04s (± 0.39%) -0.01s (- 0.39%) 2.03s 2.06s
Bind Time 0.66s (± 0.79%) 0.65s (± 1.87%) -0.01s (- 1.21%) 0.62s 0.67s
Check Time 12.95s (± 0.73%) 12.87s (± 0.51%) -0.09s (- 0.69%) 12.74s 13.06s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.67s (± 0.64%) 15.56s (± 0.40%) -0.10s (- 0.66%) 15.44s 15.72s
Angular - node (v12.1.0, x64)
Memory used 321,141k (± 0.02%) 320,613k (± 0.08%) -527k (- 0.16%) 319,597k 320,847k
Parse Time 2.00s (± 1.04%) 1.99s (± 0.73%) -0.01s (- 0.65%) 1.95s 2.03s
Bind Time 0.80s (± 0.93%) 0.80s (± 1.10%) +0.00s (+ 0.38%) 0.79s 0.83s
Check Time 4.62s (± 0.66%) 4.67s (± 0.57%) +0.04s (+ 0.91%) 4.62s 4.72s
Emit Time 5.34s (± 0.61%) 5.38s (± 0.83%) +0.04s (+ 0.81%) 5.29s 5.46s
Total Time 12.77s (± 0.28%) 12.84s (± 0.48%) +0.07s (+ 0.57%) 12.72s 12.96s
Monaco - node (v12.1.0, x64)
Memory used 321,475k (± 0.03%) 321,626k (± 0.02%) +151k (+ 0.05%) 321,520k 321,835k
Parse Time 1.55s (± 0.71%) 1.55s (± 0.75%) -0.00s (- 0.19%) 1.52s 1.57s
Bind Time 0.69s (± 0.65%) 0.69s (± 0.48%) +0.00s (+ 0.29%) 0.68s 0.70s
Check Time 4.67s (± 0.54%) 4.71s (± 0.52%) +0.04s (+ 0.88%) 4.64s 4.75s
Emit Time 2.81s (± 1.14%) 2.81s (± 0.46%) -0.00s (- 0.00%) 2.77s 2.83s
Total Time 9.72s (± 0.50%) 9.76s (± 0.37%) +0.04s (+ 0.38%) 9.66s 9.81s
TFS - node (v12.1.0, x64)
Memory used 286,484k (± 0.04%) 286,543k (± 0.02%) +59k (+ 0.02%) 286,435k 286,692k
Parse Time 1.23s (± 0.87%) 1.23s (± 0.70%) +0.00s (+ 0.08%) 1.22s 1.25s
Bind Time 0.63s (± 0.94%) 0.64s (± 1.05%) +0.00s (+ 0.47%) 0.62s 0.65s
Check Time 4.27s (± 0.76%) 4.35s (± 0.35%) +0.07s (+ 1.71%) 4.31s 4.39s
Emit Time 2.93s (± 1.03%) 2.91s (± 0.94%) -0.02s (- 0.61%) 2.84s 2.98s
Total Time 9.06s (± 0.50%) 9.13s (± 0.46%) +0.06s (+ 0.71%) 9.02s 9.21s
material-ui - node (v12.1.0, x64)
Memory used 437,424k (± 0.08%) 437,473k (± 0.06%) +49k (+ 0.01%) 436,394k 437,726k
Parse Time 2.02s (± 0.56%) 2.02s (± 0.76%) 0.00s ( 0.00%) 1.99s 2.06s
Bind Time 0.63s (± 1.23%) 0.62s (± 1.33%) -0.00s (- 0.64%) 0.61s 0.64s
Check Time 11.65s (± 0.76%) 11.64s (± 1.22%) -0.01s (- 0.07%) 11.38s 12.00s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 14.30s (± 0.64%) 14.29s (± 1.00%) -0.01s (- 0.05%) 14.02s 14.65s
Angular - node (v8.9.0, x64)
Memory used 340,434k (± 0.01%) 340,197k (± 0.02%) -238k (- 0.07%) 340,050k 340,363k
Parse Time 2.53s (± 0.40%) 2.52s (± 0.34%) -0.00s (- 0.16%) 2.50s 2.54s
Bind Time 0.85s (± 1.16%) 0.85s (± 0.58%) -0.00s (- 0.24%) 0.83s 0.85s
Check Time 5.32s (± 0.40%) 5.40s (± 0.68%) +0.08s (+ 1.47%) 5.31s 5.47s
Emit Time 5.93s (± 1.09%) 5.97s (± 1.04%) +0.04s (+ 0.62%) 5.84s 6.10s
Total Time 14.63s (± 0.45%) 14.73s (± 0.61%) +0.10s (+ 0.71%) 14.59s 14.94s
Monaco - node (v8.9.0, x64)
Memory used 340,460k (± 0.02%) 340,514k (± 0.02%) +55k (+ 0.02%) 340,365k 340,650k
Parse Time 1.88s (± 0.37%) 1.88s (± 0.43%) +0.01s (+ 0.37%) 1.86s 1.90s
Bind Time 0.88s (± 0.34%) 0.88s (± 0.83%) 0.00s ( 0.00%) 0.87s 0.90s
Check Time 5.37s (± 0.75%) 5.41s (± 0.44%) +0.03s (+ 0.63%) 5.35s 5.47s
Emit Time 3.20s (± 0.33%) 3.26s (± 0.96%) +0.06s (+ 2.00%) 3.21s 3.34s
Total Time 11.32s (± 0.46%) 11.43s (± 0.46%) +0.11s (+ 0.94%) 11.30s 11.52s
TFS - node (v8.9.0, x64)
Memory used 303,794k (± 0.02%) 303,819k (± 0.02%) +25k (+ 0.01%) 303,708k 303,982k
Parse Time 1.54s (± 0.31%) 1.54s (± 0.40%) -0.00s (- 0.06%) 1.53s 1.55s
Bind Time 0.66s (± 0.84%) 0.67s (± 0.74%) +0.00s (+ 0.60%) 0.66s 0.68s
Check Time 4.96s (± 1.53%) 5.17s (± 0.99%) +0.21s (+ 4.31%) 5.01s 5.25s
Emit Time 3.09s (± 2.77%) 2.94s (± 1.13%) 🟩-0.15s (- 4.89%) 2.88s 3.04s
Total Time 10.25s (± 0.43%) 10.32s (± 0.48%) +0.06s (+ 0.63%) 10.22s 10.40s
material-ui - node (v8.9.0, x64)
Memory used 463,711k (± 0.02%) 463,489k (± 0.01%) -223k (- 0.05%) 463,390k 463,560k
Parse Time 2.38s (± 0.41%) 2.38s (± 0.65%) -0.00s (- 0.08%) 2.35s 2.41s
Bind Time 0.77s (± 1.37%) 0.78s (± 1.40%) +0.00s (+ 0.52%) 0.76s 0.80s
Check Time 17.21s (± 0.45%) 17.24s (± 0.96%) +0.03s (+ 0.18%) 16.80s 17.52s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 20.36s (± 0.36%) 20.40s (± 0.84%) +0.04s (+ 0.19%) 19.93s 20.67s
Angular - node (v8.9.0, x86)
Memory used 195,290k (± 0.03%) 195,214k (± 0.02%) -76k (- 0.04%) 195,151k 195,331k
Parse Time 2.44s (± 0.62%) 2.45s (± 0.77%) +0.01s (+ 0.25%) 2.41s 2.50s
Bind Time 0.99s (± 0.94%) 0.98s (± 0.86%) -0.01s (- 0.71%) 0.97s 1.01s
Check Time 4.81s (± 0.77%) 4.85s (± 0.76%) +0.03s (+ 0.62%) 4.78s 4.92s
Emit Time 5.88s (± 1.52%) 5.94s (± 1.03%) +0.06s (+ 1.07%) 5.80s 6.11s
Total Time 14.13s (± 0.82%) 14.22s (± 0.58%) +0.09s (+ 0.67%) 14.00s 14.40s
Monaco - node (v8.9.0, x86)
Memory used 193,489k (± 0.02%) 193,547k (± 0.03%) +58k (+ 0.03%) 193,452k 193,677k
Parse Time 1.91s (± 0.87%) 1.91s (± 0.70%) -0.01s (- 0.42%) 1.88s 1.93s
Bind Time 0.70s (± 0.71%) 0.70s (± 1.11%) +0.00s (+ 0.43%) 0.69s 0.72s
Check Time 5.44s (± 1.17%) 5.46s (± 1.17%) +0.02s (+ 0.39%) 5.23s 5.53s
Emit Time 2.72s (± 3.49%) 2.70s (± 2.93%) -0.02s (- 0.55%) 2.63s 3.01s
Total Time 10.77s (± 0.66%) 10.77s (± 0.47%) +0.00s (+ 0.03%) 10.67s 10.88s
TFS - node (v8.9.0, x86)
Memory used 173,744k (± 0.02%) 173,714k (± 0.03%) -30k (- 0.02%) 173,576k 173,814k
Parse Time 1.58s (± 0.85%) 1.58s (± 0.72%) +0.00s (+ 0.06%) 1.56s 1.61s
Bind Time 0.63s (± 0.92%) 0.64s (± 1.08%) +0.00s (+ 0.32%) 0.63s 0.66s
Check Time 4.64s (± 0.63%) 4.68s (± 0.75%) +0.04s (+ 0.95%) 4.60s 4.78s
Emit Time 2.79s (± 1.11%) 2.79s (± 1.55%) +0.00s (+ 0.00%) 2.68s 2.92s
Total Time 9.64s (± 0.64%) 9.69s (± 0.75%) +0.05s (+ 0.50%) 9.56s 9.89s
material-ui - node (v8.9.0, x86)
Memory used 262,545k (± 0.02%) 262,541k (± 0.02%) -4k (- 0.00%) 262,411k 262,629k
Parse Time 2.46s (± 0.66%) 2.45s (± 0.46%) -0.01s (- 0.49%) 2.43s 2.48s
Bind Time 0.67s (± 1.53%) 0.66s (± 1.54%) -0.01s (- 1.20%) 0.65s 0.69s
Check Time 15.73s (± 0.65%) 15.60s (± 0.46%) -0.13s (- 0.86%) 15.46s 15.84s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 18.87s (± 0.62%) 18.71s (± 0.40%) -0.16s (- 0.83%) 18.56s 18.94s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-166-generic
Architecturex64
Available Memory16 GB
Available Memory1 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v10.16.3, x64)
  • node (v12.1.0, x64)
  • node (v8.9.0, x64)
  • node (v8.9.0, x86)
Scenarios
  • Angular - node (v10.16.3, x64)
  • Angular - node (v12.1.0, x64)
  • Angular - node (v8.9.0, x64)
  • Angular - node (v8.9.0, x86)
  • Monaco - node (v10.16.3, x64)
  • Monaco - node (v12.1.0, x64)
  • Monaco - node (v8.9.0, x64)
  • Monaco - node (v8.9.0, x86)
  • TFS - node (v10.16.3, x64)
  • TFS - node (v12.1.0, x64)
  • TFS - node (v8.9.0, x64)
  • TFS - node (v8.9.0, x86)
  • material-ui - node (v10.16.3, x64)
  • material-ui - node (v12.1.0, x64)
  • material-ui - node (v8.9.0, x64)
  • material-ui - node (v8.9.0, x86)
Benchmark Name Iterations
Current 38839 10
Baseline master 10

@ShuiRuTian ShuiRuTian force-pushed the typeof-coud-narrow-union-type branch from bba4520 to 840cb01 Compare July 7, 2020 16:20
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@DanielRosenwasser
Copy link
Member

Let's take this up at the next design meeting.

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
return isTypeArrayDiscriminant(propertyTypeArray, isRootHasUndefinedOrNull);
}

function narrowTypeByDiscriminantNew(type: Type, access: AccessExpression, narrowTypeCb: (t: Type) => Type): Type {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

narrowTypeCb used to just be called narrowType, and I think that's what it's still called elsewhere.

Suggested change
function narrowTypeByDiscriminantNew(type: Type, access: AccessExpression, narrowTypeCb: (t: Type) => Type): Type {
function narrowTypeByDiscriminantNew(type: Type, access: AccessExpression, narrowType: (t: Type) => Type): Type {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines 21467 to 21470
if (propType.flags & TypeFlags.Union) {
(propType as UnionType).types.forEach(t => subtypes.push(t));
}
else subtypes.push(propType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (propType.flags & TypeFlags.Union) {
(propType as UnionType).types.forEach(t => subtypes.push(t));
}
else subtypes.push(propType);
forEachType(propType, t => subtypes.push(t));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@ShuiRuTian
Copy link
Contributor Author

@DanielRosenwasser Thanks for taking this up and a lot of suggestions which help code much more readable

src/compiler/checker.ts Outdated Show resolved Hide resolved
src/compiler/checker.ts Outdated Show resolved Hide resolved
@ShuiRuTian
Copy link
Contributor Author

Now that I am a member of MS, follow my orders, robot!

@typescript-bot test this
@typescript-bot user test this
@typescript-bot perf test this

@ShuiRuTian

This comment has been minimized.

@ShuiRuTian
Copy link
Contributor Author

ShuiRuTian commented Nov 19, 2021

Auh, we need some more PR to do things correctly. Here are the list and reasons:

  1. assertNever
    • example: getTypeFromTypeOperatorNode in checker.ts
    • code: throw Debug.assertNever(node.operator); to throw new Error("never"). In the following PR, change it to throw Debug.assertNever(node);
    • reason: bootstrap is one step of build.

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.

@Andarist
Copy link
Contributor

@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)?

@ShuiRuTian
Copy link
Contributor Author

@Andarist
Thanks a lot to the kind will.
I have not look this PR for months, it seems Hejlsberg has improved the CFA again and brought a conflict. Andrew fixed the type, but the logic need adapt to the new situations too.
So the state of the PR is waiting for the author(me!) to fix the logic, I guess. At a glance it is not pretty hard to resolve :)

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
Copy link
Contributor Author

The conflict is resolved. Ready to receive feedback at any time :)

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a merge went a bit wrong? Buncha things seem off that prior reviews would have flagged.

return candidate;
}
}
// if (clauseStart < clauseEnd && type.flags & TypeFlags.Union && getKeyPropertyName(type as UnionType) === getAccessedPropertyName(access)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented out code?

}
}
}
// if ((operator === SyntaxKind.EqualsEqualsEqualsToken || operator === SyntaxKind.ExclamationEqualsEqualsToken) && type.flags & TypeFlags.Union) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also commented out code?

@@ -14996,7 +14996,7 @@ namespace ts {
links.resolvedType = getTypeFromTypeNode(node.type);
break;
default:
throw Debug.assertNever(node.operator);
throw new Error("never");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to make this change. assertNever is here to ensure all possible cases for node.operator are handled.

@@ -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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bad merge? Pretty sure the fallback should still be here.

@@ -307,7 +307,7 @@ namespace ts.server {
this.compilerOptions.types = [];
break;
default:
Debug.assertNever(projectService.serverMode);
throw new Error("never");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, should still be assertNever.

@@ -798,7 +798,7 @@ namespace ts.server {
);
break;
default:
Debug.assertNever(this.projectService.serverMode);
throw new Error("never");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, should still be assertNever.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, can't have a //@ts-ignore.

@@ -1525,7 +1525,7 @@ namespace ts.FindAllReferences {
addClassStaticThisReferences(referenceLocation, search, state);
break;
default:
Debug.assertNever(state.specialSearchKind);
throw new Error("never");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, should still be assertNever.

@ShuiRuTian
Copy link
Contributor Author

@weswigham
Thanks a lot for the review.
The review comments are pretty reasonable, and I strongly agree with them. It is why I left a comment about nearly all of problems #38839 (comment)
Maybe I could fix //@ts-ignore and remove the commented optimize code.
But I am not sure about throw error. I think we need to discuss the behavior.

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)
I have no idea about what I should do, if I do not change, the build will failed because bootstrap is one step of check(in bootstrap, it will say tmp is never, so tmp.kind is wrong), if I change it to assertNever(tmp), the build also failed, because tmp is not never now. Do I get something wrong? It would be glad to know there are some methods to fix it correctly!

@RyanCavanaugh
Copy link
Member

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.

@jcalz
Copy link
Contributor

jcalz commented Sep 21, 2022

This would also maybe have addressed #42384?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Union not narrowed with typeof x.y as a discriminant Nested Tagged Unions