Turn on --strictNullChecks in codebase

[DanielRosenwasser]

This is important for consumers of our API and for testing the feature in general

[ajpalkovic]

Greetings. OOC, has any thought been given on how best to convert a large codebase to use strictNullChecks? It seems like an awesome feature and I'd love to turn it on. But my team has ~400k LOC in typescript. Turning this on all at once thus results in about 33 thousand lines of error output from 5,170 distinct errors :( That is certainly not your fault but it does make it really hard to use the new feature on a large existing codebase. Is there a magic way to turn this on for only some files at once so a migration can be done more incrementally? es5 seems to have foreseen this problem so that added 'use strict' so that you can opt-in a file to the new safety checks but you don't have to convert your entire code base to the new safety checks all at once and legacy code will continue to work as is. But yea, any thoughts on that? cc @bobthecow

[mhegazy]

We have the same issues with the TypeScript code base moving it strict null checks. The approach we have taken is to start with components/modules that are lowest in the dependency graph, turn on --strictNullChecks and look at the errors for this specific component/module, fix them, but do not turn on the flag for the whole build. iterate until the whole code base is in a good shape, then switch it on for all components/modules.

[realyze]

Hmmm, would there be any big implementation problems with per-file flag that would turn strict checking on just for that file? IMHO it would definitely help with adoption of this great feature.

[RyanCavanaugh]

It's extremely unclear what per-file typechecking flags would even mean. Types can span multiple files; is it that the relation changes within a specific file, or that the types themselves change depending on where they are declared? Both have serious problems.

[ajpalkovic]

My 2c: The goal is to be able to incrementally convert a large code base to a much stricter compilation option. The per-file flag is just one way of doing that, we just need some tool to help us make that happen. My hope would be that there is a reasonable way to express "only produce strictNullChecks compiler errors for files with this magic flag at the top of the file"?

[realyze]

@ajpalkovic Right, that's actually what I meant (strict error reporting for a subset of files only), I just didn't explain myself well. Thanks!

@RyanCavanaugh Gut feeling is that's a pretty low-hanging fruit, amiright? And I believe it would help with adoption a lot. Right now - regardless of the absolute awesomeness of this feature - it's extremely hard to turn it on with existing codebases.

[RyanCavanaugh]

Gut feeling is that's a pretty low-hanging fruit, amiright?

It's the hardest-to-reach fruit. It's not clear where in the tree this fruit even is, other than that it's really high up. Consider something like this:

// a.ts
declare function fn(x: string): boolean;
declare function fn(x: number | undefined): string;

// b.ts
let x = fn(undefined);

// c.ts
// strictNullChecks: off
let z: string = x;

To really emulate strictNullChecks being off in c.ts, you'd have to completely redo the compilation of the entire program. It's not even clear what it would mean to have strictNullChecks off in b.ts? Would a different overload in a.ts be selected?

Honestly the only remotely correct way to do this would be to:

1. Compile everything with strictNullChecks on
2. Compile everything with strictNullChecks off
3. Filter the resulting errors based on whether or not they occurred in both passes

But even at this point it's not guaranteed that compiling each file individually with strictNullChecks on and getting 0 errors means that the entire program is error-free with strictNullChecks on globally.

[realyze]

@RyanCavanaugh Is it possible that you're perhaps overthinking this somewhat? Or maybe I'm underthinking it, I'm not sure. :-) But this would be only about hiding strict-null errors from the output for the other files than the ones with a flag (say /** ts: strict-null **/ or something similar).

See I agree only checking a single file would be hard (or even impossible to do in a meaningful way) - but we're talking about managing the output here, not about changing the semantics.

Does that makes sense?

[RyanCavanaugh]

There's no such thing as a "strict null error". There's no place where we say if (badNullAssignment) { error (...). The strict null check flag simply changes which values are in the domain of certain types, which causes errors to occur much later in the process. It's like changing + to mean *, doing a bunch of math under this change, and then trying to work out what the answer would have been had you not made that change without knowing what the process that got you to that result was.

[RyanCavanaugh]

Relevant XKCD

[realyze]

Did I just get xkcded? Well played @RyanCavanaugh ! :-D

Anyhow, I hear what you're saying, I failed to realise it's just another error and not a special kind.

Could you perhaps think of any other way to help migrate existing codebases? I'd really love to enable strict checking but I need some way to handle the flurry of 100s/1000s of errors.

[rkirov]

Maybe we can offer some advice from a successful migration of a large Google internal TypeScript codebase to use strictNullChecks.

First, if you codebase is large enough, consider having separate units of TypeScript compilation (invocations of tsc). Each compilation produces .d.ts files and other compilations consume only the .d.ts of their dependencies.

Within this setup you can consider turning on SNC for some compilations first. IMHO, this is the right granularity, and not per-file as originally proposed. That said, this approach is not without issues, see #8995, but might give you some incrementality in adopting SNC.

The approach that worked for us is big-bang SNC flip. We manually fixed SNC errors per project and checked in the fixes before the final flip. As long as you use TS 2.0+ adding | null and | undefined is harmless (with SNC off), so you can work towards getting the codebase to the point where the flag can be flipped. It is a moving target, but in our case the rate of errors fixed was a lot higher than new errors introduced.

The final observation that is key in making this migration is that all errors introduced with SNC on, are build errors. Build errors are a lot faster to observe and fix, than runtime errors (in tests or actual product). Roughly, there are three main ways to resolve an error - 1) change a type annotation to be nullable, 2) add a runtime null check, 3) use the non-null type assertion (!) operator on an expression.

Option 1) can propagate to an error in another place of the code and introduce more errors. Option 2) can make tests fail or worse introduce new bugs. Option 3) despite being unsound, has the nice property of being completely local and has no runtime implications. Consider using Option 3 heavily on a first pass, and have more incremental passes to remove (!) and switch to Option 1 or 2 once SNC is on.

[kitsonk]

Also, a bit of advice we (@dojo) found, is that we were able to convert most of our packages straight to --strictNullChecks (as well as --noImplicitThis) as we migrated to TypeScript 2.0. Because our code is in packages, it made it fairly straight forward to migrate, though one of our biggest and most complex packages (dojo/widgets) we are still working on because it was playing very loose and fast with values. We also transitioned from a single global declaration file per package to individual UMD declaration files.

As @rkirov says, having some way of breaking up the code base into separate compilation certainly helps, and having a UMD declaration files also localises breaking changes. Invariably you will find some sort of code that you thought was perfectly safe was actually a bit dangerous that will have further downstream impacts. The good thing is if everything is compiling on TS2+ then having something upstream being "strict safe" usually works downstream just fine.

We were really strict about our functional code, but our unit tests, we allowed coercion to any and the ! escape hatch to avoid having to do lots of work on the tests to get them to be null safe.

We also discovered some ignorant patterns we weren't aware of. For example we had interfaces that took callback with optional properties, assuming that callbacks needed to be optional to allow a callback to ignore parameters. We have learned that that is wholly unnecessary, but what it causes is that every argument passed to the callback might be undefined. For example:

interface Foo {
    forEach(callback: (item?: any, index?: number, source?: Foo): void;
}

But overall, it is worth the effort, though it can take some hard work. Clearly new work will simply be written with strictNullChecks enabled from the get go, but in 99% of the cases it was really minor tweaks.

[HolgerJeromin]

We are in the process of transforming the code base for SNC, too.
The transition phase would be easier when the tooltip in VS2015 would not omit the new types with SNC off.

The *.d.ts is correct:
static getWebsocketReadyState(): number | null

[RyanCavanaugh]

🍰 🎂 🎈 🎉 🥇 💯