Enable strictNullChecks in tsconfig.json #179

daviwil · 2018-02-15T01:21:10Z

Please let me know if there are any places where these changes are incorrect!

daviwil · 2018-02-15T01:24:02Z

lib/adapters/apply-edit-adapter.ts

+  private static validateEdit(
+    buffer: TextBuffer,
+    edit: atomIde.TextEdit,
+    prevEdit: atomIde.TextEdit | undefined | null,


Could revert the addition of undefined here.

daviwil · 2018-02-15T01:25:17Z

lib/adapters/autocomplete-adapter.ts

@@ -267,11 +267,11 @@ export default class AutocompleteAdapter {
  // * `editor` An Atom {TextEditor} used to obtain the necessary text replacement.
  // * `suggestion` An {atom$AutocompleteSuggestion} to set the replacementPrefix and text properties of.
  public static applyTextEditToSuggestion(
-    textEdit: TextEdit | null,
+    textEdit: TextEdit | undefined,


This TextEdit comes from CompletionItem.textEdit which is typed as TextEdit | undefined, makes more sense to use undefined here

daviwil · 2018-02-15T01:26:26Z

lib/adapters/autocomplete-adapter.ts

@@ -296,7 +296,7 @@ export default class AutocompleteAdapter {
  //
  // Returns a {String} containing the AutoComplete+ suggestion type equivalent
  // to the given completion kind.
-  public static completionKindToSuggestionType(kind: number | null): string {
+  public static completionKindToSuggestionType(kind: number | undefined): string {


CompletionItem.kind uses number | undefined (or more accurately, a bunch of specific numbers unioned with undefined

daviwil · 2018-02-15T01:26:57Z

lib/adapters/datatip-adapter.ts

@@ -88,5 +88,8 @@ export default class DatatipAdapter {
        value: markedString.value,
      };
    }
+
+    // Catch-all case
+    return { type: 'markdown', value: markedString.toString() };


TypeScript compiler wouldn't let this method live without a catch-all case. Is there a better result value?

daviwil · 2018-02-15T01:29:56Z

lib/adapters/code-action-adapter.ts

@@ -27,7 +27,7 @@ export default class CodeActionAdapter {
  public static async getCodeActions(
    connection: LanguageClientConnection,
    serverCapabilities: ServerCapabilities,
-    linterAdapter: LinterPushV2Adapter | null,
+    linterAdapter: LinterPushV2Adapter | undefined ,


ActiveServer.linterPushV2 is typed as LinterPushV2Adapter | undefined, that gets passed into this method for this parameter

daviwil · 2018-02-15T01:30:59Z

lib/adapters/document-sync-adapter.ts

@@ -64,7 +64,7 @@ export default class DocumentSyncAdapter {
  //                    indicating whether this adapter should care about the contents of the editor.
  constructor(
    connection: LanguageClientConnection,
-    documentSyncKind: TextDocumentSyncOptions | number | null,
+    documentSyncKind: TextDocumentSyncOptions | number | undefined,


ServerCapabilities.textDocumentSync uses undefined instead of null

daviwil · 2018-02-15T01:31:24Z

lib/adapters/linter-push-v2-adapter.ts

      }
    }
    return null;
  }
 }

 function getCodeKey(range: atom.Range, text: string): string {
-  return [].concat(...range.serialize(), text).join(',');
+  return ([] as any[]).concat(...range.serialize(), text).join(',');


A little hack to make the type system happy

I wonder if we should just ${range.serialize()},${text} here?

I'll give it a shot!

range.serialize() won't work because it returns an array of arrays, that's why the [].concat() with the spread operator is used. It builds a flat array of items that can be joined. I'll leave it as it is for now

daviwil · 2018-02-15T01:32:08Z

lib/adapters/linter-push-v2-adapter.ts

@@ -113,13 +113,13 @@ export default class LinterPushV2Adapter {
    if (path != null) {
      const diagnosticCodes = this._diagnosticCodes.get(path);
      if (diagnosticCodes != null) {
-        return diagnosticCodes.get(getCodeKey(range, text));
+        return diagnosticCodes.get(getCodeKey(range, text)) || null;


Maps return undefined when a key has no value, so I'm coercing it to null to conform to the existing signature

daviwil · 2018-02-15T01:32:42Z

lib/adapters/outline-view-adapter.ts

    for (const candidate of candidates) {
      if (
        candidate !== child &&
        candidate.startPosition.isLessThanOrEqual(child.startPosition) &&
-        (candidate.endPosition == null || candidate.endPosition.isGreaterThanOrEqual(child.endPosition))
+        (candidate.endPosition === undefined ||


endPosition is an optional property which is undefined instead of null

daviwil · 2018-02-15T01:33:15Z

lib/adapters/signature-help-adapter.ts

@@ -31,8 +31,8 @@ export default class SignatureHelpAdapter {
    const {signatureHelpProvider} = this._capabilities;
    assert(signatureHelpProvider != null);

-    let triggerCharacters = null;
-    if (Array.isArray(signatureHelpProvider.triggerCharacters)) {
+    let triggerCharacters: Set<string> | undefined;


Not yet assigned, must use undefined in type

daviwil · 2018-02-15T01:33:57Z

lib/auto-languageclient.ts

@@ -39,21 +39,6 @@ import SignatureHelpAdapter from './adapters/signature-help-adapter';

 export type ConnectionType = 'stdio' | 'socket' | 'ipc';

-// Public: Defines the minimum surface area for an object that resembles a


Decided to move this to ServerManager's file because ServerManager was still using ChildProcess to refer to the language server process.

daviwil · 2018-02-15T01:34:44Z

lib/convert.ts

@@ -159,7 +159,7 @@ export default class Convert {
      case 'deleted':
        return [{uri: Convert.pathToUri(fileEvent.path), type: ls.FileChangeType.Deleted}];
      case 'renamed': {
-        const results = [];
+        const results: Array<{ uri: string, type: ls.FileChangeType }> = [];


TypeScript uses a type of never[] when you define a plain array with no typing. You have to make the type specific so that the type system doesn't give a bunch of weird errors.

daviwil · 2018-02-15T01:34:58Z

lib/download-file.ts

@@ -31,7 +31,7 @@ export default (async function downloadFile(
    throw Error('No response body');
  }

-  const finalLength = length || parseInt(response.headers.get('Content-Length' || '0'), 10);
+  const finalLength = length || parseInt(response.headers.get('Content-Length') || '0', 10);


Bug in the previous code?

Ha, looks like it. Nice catch.

daviwil · 2018-02-15T01:36:12Z

lib/download-file.ts

@@ -72,7 +72,7 @@ async function streamWithProgress(
      writer.write(Buffer.from(chunk));
      if (progressCallback != null) {
        bytesDone += chunk.byteLength;
-        const percent: number = length === 0 ? null : Math.floor(bytesDone / length * 100);
+        const percent: number | undefined = length === 0 ? undefined : Math.floor(bytesDone / length * 100);


percent gets passed into ByteProgressCallback's optional percent parameter, undefined matches it

daviwil · 2018-02-15T01:36:30Z

test/adapters/autocomplete-adapter.test.ts

@@ -20,7 +20,7 @@ describe('AutoCompleteAdapter', () => {
      capabilities: {completionProvider: { }},
      connection: new ls.LanguageClientConnection(createSpyConnection()),
      disposable: new CompositeDisposable(),
-      process: undefined,
+      process: undefined as any,


Type system hack to not have to mock a whole process

daviwil · 2018-02-15T01:36:52Z

test/adapters/code-highlight-adapter.test.ts

@@ -50,8 +50,10 @@ describe('CodeHighlightAdapter', () => {
      expect(highlightStub.called).to.be.true;

      invariant(result != null);
-      expect(result.length).to.equal(1);
-      expect(result[0].isEqual(new Range([0, 1], [0, 2]))).to.be.true;
+      if (result) {


Tests required a bunch of null/undefined checks so that the compiler would be happy

daviwil · 2018-02-15T01:38:23Z

typings/atom-ide/index.d.ts

@@ -178,7 +178,7 @@ export interface CodeActionProvider {
    editor: TextEditor,
    range: Range,
    diagnostics: Diagnostic[],
-  ): Promise<CodeAction[]>;
+  ): Promise<CodeAction[] | null>;


The implementation of getCodeActions in AutoLanguageClient can return null, so had to add it here

laughedelic · 2018-02-16T01:42:30Z

@daviwil thanks for keeping me in the loop. I should clarify that I'm neither Typescript, nor Flow expert 😅I'm writing a Scala.js type-facade for this library (here). So I just want to understand what's changing here, why and how it influences my work.

I'm going to go through all your comments, but before that have a general question: wouldn't it make more sense to use consistently only one kind of "default"/absent value (null or undefined) everywhere in the code? (at least where you return them explicitly, I understand that external dependencies are out of control)

daviwil · 2018-02-16T18:52:16Z

@laughedelic It seems that there's a lot of debate over null vs undefined and whether null should ever be used. In my changes here, I decided to keep the use of null where it was intended to mean "no result" so that we don't break existing packages and then change any other usages when a value or property hadn't been defined. In the cases where we definitely return null, I didn't add | undefined to the type just so that the return type is clear.

Let me know if you have any other questions!

laughedelic · 2018-02-18T00:39:19Z

@daviwil thanks for the answer and the link! That thread is huge, but has a lot of interesting opinions. I got your point, it will be useful for me when working with the library. The inconvenience of using null in Scala.js is that you can't enforce strictNullChecks to make null a special value which doesn't belong to any other type, in Scala it's a value of any type, so you cannot really say T | null explicitly, because it's the same as just T, while undefined is a special value (the only value of it's type) and it can be explicitly separated in the type annotation T | undefined (UndefOr[T] in Scala.js and is similar to the common Option[T]).

daviwil requested a review from damieng February 15, 2018 01:21

daviwil commented Feb 15, 2018

View reviewed changes

daviwil force-pushed the dw-strict-null-checks branch from efd506a to a449d20 Compare February 15, 2018 19:07

daviwil added 2 commits February 15, 2018 11:08

Enable strictNullChecks in tsconfig.json

a148122

Move LanguageServerProcess to lib/server-manager.js

f42e380

daviwil force-pushed the dw-strict-null-checks branch from a449d20 to f42e380 Compare February 15, 2018 19:08

damieng approved these changes Feb 16, 2018

View reviewed changes

daviwil merged commit f35633c into master Feb 16, 2018

daviwil deleted the dw-strict-null-checks branch February 16, 2018 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable strictNullChecks in tsconfig.json #179

Enable strictNullChecks in tsconfig.json #179

daviwil commented Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

damieng Feb 16, 2018 •

edited

Loading

daviwil Feb 16, 2018

daviwil Feb 16, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

damieng Feb 16, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

daviwil Feb 15, 2018

laughedelic commented Feb 16, 2018

daviwil commented Feb 16, 2018

laughedelic commented Feb 18, 2018

		@@ -39,21 +39,6 @@ import SignatureHelpAdapter from './adapters/signature-help-adapter';

		export type ConnectionType = 'stdio' \| 'socket' \| 'ipc';

		// Public: Defines the minimum surface area for an object that resembles a

Enable strictNullChecks in tsconfig.json #179

Enable strictNullChecks in tsconfig.json #179

Conversation

daviwil commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damieng Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laughedelic commented Feb 16, 2018

daviwil commented Feb 16, 2018

laughedelic commented Feb 18, 2018

damieng Feb 16, 2018 •

edited

Loading