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

Enable '--strictNullChecks' #22088

Merged
105 commits merged into from
May 22, 2018
Merged

Enable '--strictNullChecks' #22088

105 commits merged into from
May 22, 2018

Conversation

ghost
Copy link

@ghost ghost commented Feb 21, 2018

Fixes #9432

The changes generally amount to:

  • Adding | undefined as appropriate.
  • Adding ! in many places
    • Particularly on optional members of interfaces like Symbol and Type.
    • Also, flags enums are often used as E | undefined, so we need to write e! & E.SomeFlag.
    • The no-unnecessary-type-assertion lint rule ensures that these are only added where necessary.
    • Suspicious uses of ! are marked with Tracking invalid type assertions #18217.
  • Adding !! to make expressions be boolean instead of boolean | undefined.
    • Also sometimes need to add = false to initialize the boolean.
    • Also change condition && optional to condition ? optional : undefined to avoid getting a Optional | undefined | false.
  • Change Debug.fail() to return Debug.fail() to tell the compiler that we definitely return from a function

@ghost ghost force-pushed the undefinedzilla branch 4 times, most recently from d386f8f to 34662b7 Compare February 21, 2018 19:22
@ghost ghost force-pushed the undefinedzilla branch from 34662b7 to dc2f7dc Compare February 21, 2018 19:35
@ghost ghost force-pushed the undefinedzilla branch from dc2f7dc to 8deee11 Compare February 21, 2018 19:37
@sandersn
Copy link
Member

  • Looks like the public API needs to be updated, which is why Travis fails.
  • How does this change compile time of the compiler? I think this is higher value than the all-unions change, but I would still hope it's not too much slower than what we have now.

@weswigham
Copy link
Member

weswigham commented Feb 22, 2018

@sandersn On my workstation jake clean tests goes from a wallclock time of ~1m7s to ~1m10s, so pretty much no difference. Seems good to me.

@weswigham
Copy link
Member

I'm on board with all the changes except the non-optional non-undefined parent-pointers. While internally we use them with reckless abandon (the checker only operates on fully bound trees, after all), people using our transforms APIs won't have parent pointers bound (and shouldn't use them) - and making our public API correct is important. The, like, 1000 extra assertions we'd need internally are probably worth it.

Also flags! | <some flags> is a bit odd (I mean, the assertion is never going to go away)? Maybe we should probably be allowing undefined in binary arithmetic without an error?

@ghost
Copy link
Author

ghost commented Feb 28, 2018

API tests pass when I run them locally on either windows or linux -- don't know why travis is failing.

@ghost ghost force-pushed the undefinedzilla branch from a0c795b to bce4875 Compare March 1, 2018 21:48
@ghost ghost force-pushed the undefinedzilla branch from bce4875 to e31b4e9 Compare March 1, 2018 21:50
@weswigham
Copy link
Member

The travis failure repros locally for me (looks like tests\baselines\reference\api\typescript.d.ts has more undefined's than it needs). Do you, perhaps, have an different, cached copy of the input or baselined typescript.d.ts knocking around locally that just won't flush?

@ghost ghost force-pushed the undefinedzilla branch from fcdd2a7 to aadf42a Compare March 1, 2018 22:49
@ghost
Copy link
Author

ghost commented Mar 1, 2018

Just needed to get Jake to run with --strictNullChecks too, thanks for the help.

@weswigham
Copy link
Member

TIL jenkins will refuse to build when there are merge conflicts.

@ghost ghost force-pushed the undefinedzilla branch from aadf42a to eb54c27 Compare March 1, 2018 23:08
@ghost ghost force-pushed the undefinedzilla branch from eb54c27 to 28991e1 Compare March 2, 2018 18:30
@ghost ghost force-pushed the undefinedzilla branch from ee9c28c to afea2dc Compare May 18, 2018 19:56
@ghost ghost merged commit e53e56c into master May 22, 2018
@ghost ghost deleted the undefinedzilla branch May 22, 2018 21:47
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants