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

Strict null checking VS Code #60565

Open
mjbvz opened this Issue Oct 10, 2018 · 14 comments

Comments

Projects
None yet
5 participants
@mjbvz
Contributor

mjbvz commented Oct 10, 2018

Tracks engineering work to enable strictNullChecks in the VS Code code base.

Problem

A steady number of issues are reported each month that are caused by null reference exceptions. Most of these are just simple programming errors, such as forgetting to initialize a value or not realizing that a function may return undefined.

Proposal

To address this class of errors, I propose that we move to enable strictNullChecks in the main VS Code codebase. We have already successfully enabled strictNullChecks for our build scripts and for our built-in extensions, and I believe that these changes have been highly beneficial in catching common errors and enforcing better programming practices.

Today, building VS Code with strictNullChecks produces thousands of errors. Addressing these in a single pass is both impractical and would risk major regressions. I therefore propose we turn on strictNullChecks in our code base incrementally, opting in individual files until eventually strictNullChecks is enabled for the entire codebase.

Implementation

To accomplish this, a new tsconfig called tsconfig.strictNullChecks.json will be introduced. This tsconfig will only be used for error checking. It will use include and files to compile only files that should be strict null checked.

We will build tsconfig.strictNullChecks.json as part of our normal build process. A strictNullChecks failure in tsconfig.strictNullChecks.json should cause the entire VS Code build to fail. This should prevent us from regressing strictNullChecks changes that have already been added

How to help

This will be an long term engineer effort. The good news is that I believe we can do this incrementally and that each incremental change will improve the codebase in its own right.

Here's what's needed to make this work:

For regular development
Make sure your changes do not break the tsconfig.strictNullChecks.json build. You can check this manually by running: yarn strict-null-check

This script s part of our build pipelines. It is not run as part of yarn compile or yarn watch for performance reasons (doing so would essentially have us building the VS Code codebase twice)

Add a new file to be strict null checked

  1. Add the file in the files section of src/tsconfig.strictNullChecks.json

    "files": [
    	"./vs/base/common/iterator.ts"
    ]

    Keep in mind that this will also strict null check all files that the added file imports. Therefore it's best to start with files that either have very few imports or that only import files that are strict null checked.

  2. Run yarn strict-null-check or yarn strict-null-check -- --watch

  3. Fix all null check related errors. In general, this should involve:

    • Annotate nullable types and fix type errors
    • Use the ! not null operator to tell TypeScript that a value cannot be null. Only use this if you are certain that a value absolutely cannot be null or undefined (i.e. you already checked this in code)
    • Add conditionals to handle null. Only change the actual logic if you believe that strict null checks has revealed a real programming error that must be handled.
  4. Make sure you have not broken the normal VS Code build.

  5. Check in your changes

@mjbvz mjbvz added the engineering label Oct 10, 2018

@mjbvz mjbvz self-assigned this Oct 10, 2018

@roblourens

This comment has been minimized.

Member

roblourens commented Oct 10, 2018

Will I see errors inline from this? Basically can VS Code check two overlapping tsconfig projects at the same time?

@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Oct 10, 2018

No, unfortunately that's not possible using multiple, overlapping tsconfigs . I'll bring this up to the TS team to see if there is a better approach

@Tyriar

This comment has been minimized.

Member

Tyriar commented Oct 10, 2018

@mjbvz I add xterm.js' strict null check project as a pretest script, I assume most people run this before committing to that should help keep the build green.

mjbvz added a commit that referenced this issue Oct 10, 2018

Add tsconfig.strictNullChecks.json
Part of #60565

Adds a new `tsconfig.strictNullChecks.json` project that does not emit anything and is only used for enabling strict null checks on a subset of the vscode codebase.

Opt `iterator.ts` into strict null checking.

Fix our build scripts to properly handle `extends`
@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Oct 10, 2018

@Tyriar Yes that's a good idea. At some point running the check may become quite slow but we can worry about that when we hit it

@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Oct 10, 2018

Status:

  • Checked in src/tsconfig.strictNullChecks.json and opted in a few files
  • Added a yarn strict-null-check script to build this project.
  • Run this script in the precommit checks
  • Run this script in our Azure pipelines builds
@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Oct 11, 2018

Note: I'll try to update this list as the set of files changes


Here's a list of files that only import files that are already strict null checked. These are a good starting point if you are looking to help out with conversion:

updated: December 7, 2018

  • "./vs/base/browser/ui/menu/menu.ts" — Depended on by 5 files
  • "./vs/editor/contrib/documentSymbols/outlineModel.ts" — Depended on by 5 files
  • "./vs/editor/contrib/suggest/completionModel.ts" — Depended on by 5 files
  • "./vs/platform/extensionManagement/node/extensionGalleryService.ts" — Depended on by 4 files
  • "./vs/workbench/services/search/node/fileSearch.ts" — Depended on by 3 files
  • "./vs/workbench/parts/terminal/electron-browser/terminalConfigHelper.ts" — Depended on by 3 files
  • "./vs/workbench/parts/stats/node/workspaceStats.ts" — Depended on by 3 files
  • "./vs/platform/telemetry/node/appInsightsAppender.ts" — Depended on by 3 files
  • "./vs/editor/contrib/find/findWidget.ts" — Depended on by 2 files
  • "./vs/workbench/services/files/electron-browser/streams.ts" — Depended on by 2 files
  • "./vs/platform/extensionManagement/node/extensionManagementService.ts" — Depended on by 2 files
  • "./vs/workbench/parts/tasks/node/taskConfiguration.ts" — Depended on by 2 files
  • "./vs/workbench/services/configurationResolver/node/variableResolver.ts" — Depended on by 2 files
  • "./vs/workbench/parts/extensions/browser/extensionsWidgets.ts" — Depended on by 2 files
  • "./vs/platform/storage/node/storageIpc.ts" — Depended on by 2 files
  • "./vs/workbench/services/themes/electron-browser/fileIconThemeData.ts" — Depended on by 2 files
  • "./vs/workbench/electron-browser/resources.ts" — Depended on by 2 files
  • "./vs/workbench/parts/tasks/node/processTaskSystem.ts" — Depended on by 1 files
  • "./vs/platform/windows/electron-main/windowsService.ts" — Depended on by 1 files
  • "./vs/workbench/browser/parts/quickinput/quickInputBox.ts" — Depended on by 1 files
  • "./vs/platform/update/electron-main/updateService.win32.ts" — Depended on by 1 files
  • "./vs/platform/storage/common/storageLegacyMigration.ts" — Depended on by 1 files
  • "./vs/platform/localizations/node/localizations.ts" — Depended on by 1 files
  • "./vs/editor/contrib/codelens/codelensWidget.ts" — Depended on by 1 files
  • "./vs/workbench/services/issue/electron-browser/workbenchIssueService.ts" — Depended on by 1 files
  • "./vs/workbench/parts/tasks/electron-browser/terminalTaskSystem.ts" — Depended on by 1 files
  • "./vs/workbench/services/files/node/watcher/nsfw/nsfwWatcherService.ts" — Depended on by 1 files
  • "./vs/platform/extensionManagement/node/multiExtensionManagement.ts" — Depended on by 1 files
  • "./vs/platform/driver/electron-browser/driver.ts" — Depended on by 1 files
  • "./vs/editor/contrib/snippet/snippetSession.ts" — Depended on by 1 files
  • "./vs/workbench/services/extensions/electron-browser/extensionHostProfiler.ts" — Depended on by 1 files
  • "./vs/editor/contrib/hover/modesContentHover.ts" — Depended on by 1 files
  • "./vs/workbench/api/node/extHostSearch.fileIndex.ts" — Depended on by 1 files
  • "./vs/code/electron-browser/issue/issueReporterMain.ts" — Depended on by 0 files
  • "./vs/workbench/parts/tasks/electron-browser/jsonSchema_v2.ts" — Depended on by 0 files
  • "./vs/workbench/parts/tasks/electron-browser/jsonSchema_v1.ts" — Depended on by 0 files

File list is generated using this script

@roblourens

This comment has been minimized.

Member

roblourens commented Oct 11, 2018

If it ends up taking more than a few seconds to run, I would move it out of the precommit hook to scripts/test or something

@roblourens

This comment has been minimized.

Member

roblourens commented Oct 11, 2018

What if we have a simple extension that runs the script and shows errors inline?

Or like a clone of the TS extension that basically runs a second tsserver instance for the tsconfig

@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Oct 11, 2018

Yes I just used a script to opt a bunch of files in to strict null checks ( a2738f7 ) so I guess it's already time to worry :) I'll move the check to run in the test scripts instead

Extension is an interesting idea. I think we could also use a task and a problem matcher

@joaomoreno

This comment has been minimized.

Member

joaomoreno commented Oct 11, 2018

Love this. Yeah, I was coming in saying I am worried about the pre-commit check. Tests is fine, we are always watching the build.

alexandrudima added a commit that referenced this issue Oct 11, 2018

alexandrudima added a commit that referenced this issue Oct 11, 2018

alexandrudima added a commit that referenced this issue Oct 11, 2018

alexandrudima added a commit that referenced this issue Oct 11, 2018

Tyriar added a commit that referenced this issue Oct 11, 2018

alexandrudima added a commit that referenced this issue Oct 11, 2018

@mjbvz mjbvz removed the help wanted label Oct 11, 2018

alexandrudima added a commit that referenced this issue Oct 11, 2018

@alexandrudima

This comment has been minimized.

Member

alexandrudima commented Oct 11, 2018

👎 on running the compiler every time I invoke scripts/test.sh. I run the tests very often, it is part of the way I develop, and the extra slowness brought in is noticeable.

mjbvz added a commit that referenced this issue Oct 11, 2018

mjbvz added a commit that referenced this issue Oct 12, 2018

alexandrudima added a commit that referenced this issue Oct 12, 2018

alexandrudima added a commit that referenced this issue Oct 12, 2018

isidorn added a commit that referenced this issue Oct 12, 2018

@mjbvz

This comment has been minimized.

Contributor

mjbvz commented Oct 16, 2018

One common cause of strict typing errors in our code is nulling out properties in dispose implementations. To fix this, we can:

  • Mark these properties as nullable. This usually introduces a strict check error where ever else we try to use the property. These errors are correct but fixing them is not straightforward.

  • Decide that trying to use an object after it is disposed is undefined behavior and should possible throw reference errors. If we go this route, I propose we explicitly annotate why nulling out is ok:

    dispose(): void {
    	super.dispose();
    	this.el = null!; // StrictNullOverride: nulling out ok in dispose
    }

    This would let us revisit these decisions later if we want too. As part of this work, we could also add assert(!this.isDisposed) at the top of every public method that could trigger these type of reference errors

Thoughts?

@joaomoreno

This comment has been minimized.

Member

joaomoreno commented Oct 16, 2018

Another alternative is not to null non-nullable properties. Garbage collection should collect everything correctly if the last top level reference is indeed released.

mjbvz added a commit that referenced this issue Oct 18, 2018

Explicitly type simple array
Add typings for the pattern: `let x = []` which defaults to `let x: any[] = []`

#60565

mjbvz added a commit that referenced this issue Oct 18, 2018

mjbvz added a commit that referenced this issue Oct 22, 2018

mjbvz added a commit that referenced this issue Oct 22, 2018

mjbvz added a commit that referenced this issue Oct 22, 2018

mjbvz added a commit that referenced this issue Oct 22, 2018

mjbvz added a commit that referenced this issue Oct 22, 2018

mjbvz added a commit that referenced this issue Oct 22, 2018

mjbvz added a commit that referenced this issue Oct 23, 2018

RMacfarlane added a commit that referenced this issue Nov 6, 2018

RMacfarlane added a commit that referenced this issue Nov 6, 2018

rebornix added a commit that referenced this issue Nov 9, 2018

@kieferrm kieferrm referenced this issue Nov 10, 2018

Open

Iteration Plan for November 2018 #62876

28 of 46 tasks complete

alexandrudima added a commit that referenced this issue Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment