Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Enable strictNullChecks in tsconfig.json #179

Merged
merged 2 commits into from
Feb 16, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions lib/adapters/apply-edit-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export default class ApplyEditAdapter {
}),
);

const checkpoints = [];
const checkpoints: Array<{ buffer: TextBuffer, checkpoint: number}> = [];
try {
for (let i = 0; i < editors.length; i++) {
const editor = editors[i] as TextEditor;
Expand All @@ -52,7 +52,7 @@ export default class ApplyEditAdapter {
const buffer = editor.getBuffer();
const checkpoint = buffer.createCheckpoint();
checkpoints.push({buffer, checkpoint});
let prevEdit = null;
let prevEdit: atomIde.TextEdit | null = null;
for (const edit of edits) {
ApplyEditAdapter.validateEdit(buffer, edit, prevEdit);
buffer.setTextInRange(edit.oldRange, edit.newText);
Expand All @@ -74,9 +74,13 @@ export default class ApplyEditAdapter {
}

// Private: Do some basic sanity checking on the edit ranges.
private static validateEdit(buffer: TextBuffer, edit: atomIde.TextEdit, prevEdit: atomIde.TextEdit | null): void {
private static validateEdit(
buffer: TextBuffer,
edit: atomIde.TextEdit,
prevEdit: atomIde.TextEdit | null,
): void {
const path = buffer.getPath() || '';
if (prevEdit != null && edit.oldRange.end.compare(prevEdit.oldRange.start) > 0) {
if (prevEdit && edit.oldRange.end.compare(prevEdit.oldRange.start) > 0) {
throw Error(`Found overlapping edit ranges in ${path}`);
}
const startRow = edit.oldRange.start.row;
Expand Down
6 changes: 3 additions & 3 deletions lib/adapters/autocomplete-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

editor: TextEditor,
suggestion: AutocompleteSuggestion,
): void {
if (textEdit != null) {
if (textEdit) {
suggestion.replacementPrefix = editor.getTextInBufferRange(Convert.lsRangeToAtomRange(textEdit.range));
suggestion.text = textEdit.newText;
}
Expand All @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

switch (kind) {
case CompletionItemKind.Constant:
return 'constant';
Expand Down
2 changes: 1 addition & 1 deletion lib/adapters/code-action-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export default class CodeActionAdapter {
public static async getCodeActions(
connection: LanguageClientConnection,
serverCapabilities: ServerCapabilities,
linterAdapter: LinterPushV2Adapter | null,
linterAdapter: LinterPushV2Adapter | undefined ,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

editor: TextEditor,
range: Range,
diagnostics: atomIde.Diagnostic[],
Expand Down
3 changes: 3 additions & 0 deletions lib/adapters/datatip-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,5 +88,8 @@ export default class DatatipAdapter {
value: markedString.value,
};
}

// Catch-all case
return { type: 'markdown', value: markedString.toString() };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
4 changes: 2 additions & 2 deletions lib/adapters/document-sync-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ServerCapabilities.textDocumentSync uses undefined instead of null

editorSelector: (editor: TextEditor) => boolean,
) {
this._connection = connection;
Expand Down Expand Up @@ -129,7 +129,7 @@ export default class DocumentSyncAdapter {
);
}

public getEditorSyncAdapter(editor: TextEditor): TextEditorSyncAdapter | null {
public getEditorSyncAdapter(editor: TextEditor): TextEditorSyncAdapter | undefined {
return this._editors.get(editor);
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/adapters/linter-push-v2-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
}
return null;
}
}

function getCodeKey(range: atom.Range, text: string): string {
return [].concat(...range.serialize(), text).join(',');
return ([] as any[]).concat(...range.serialize(), text).join(',');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little hack to make the type system happy

Copy link
Contributor

@damieng damieng Feb 16, 2018

Choose a reason for hiding this comment

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

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

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'll give it a shot!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}
13 changes: 8 additions & 5 deletions lib/adapters/outline-view-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -115,24 +115,27 @@ export default class OutlineViewAdapter {
return null;
}

let parent = null;
let parent: atomIde.OutlineTree | undefined;
for (const candidate of candidates) {
if (
candidate !== child &&
candidate.startPosition.isLessThanOrEqual(child.startPosition) &&
(candidate.endPosition == null || candidate.endPosition.isGreaterThanOrEqual(child.endPosition))
(candidate.endPosition === undefined ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

endPosition is an optional property which is undefined instead of null

(child.endPosition && candidate.endPosition.isGreaterThanOrEqual(child.endPosition)))
) {
if (
parent == null ||
parent === undefined ||
(parent.startPosition.isLessThanOrEqual(candidate.startPosition) ||
(parent.endPosition != null && parent.endPosition.isGreaterThanOrEqual(candidate.endPosition)))
(parent.endPosition != null &&
candidate.endPosition &&
parent.endPosition.isGreaterThanOrEqual(candidate.endPosition)))
) {
parent = candidate;
}
}
}

return parent;
return parent || null;
}

// Public: Convert an individual {SymbolInformation} from the language server
Expand Down
4 changes: 2 additions & 2 deletions lib/adapters/signature-help-adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not yet assigned, must use undefined in type

if (signatureHelpProvider && Array.isArray(signatureHelpProvider.triggerCharacters)) {
triggerCharacters = new Set(signatureHelpProvider.triggerCharacters);
}

Expand Down
23 changes: 4 additions & 19 deletions lib/auto-languageclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,13 @@ import * as rpc from 'vscode-jsonrpc';
import * as path from 'path';
import * as atomIde from 'atom-ide';
import * as linter from 'atom-linter';
import * as stream from 'stream';
import { Socket } from 'net';
import { EventEmitter } from 'events';
import { ConsoleLogger, NullLogger, Logger } from './logger';
import { ServerManager, ActiveServer } from './server-manager.js';
import { LanguageServerProcess, ServerManager, ActiveServer } from './server-manager.js';
import Convert from './convert.js';

export { LanguageServerProcess };

import {
AutocompleteDidInsert,
AutocompleteProvider,
Expand Down Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

// ChildProcess. This is used so that language packages with alternative
// language server process hosting strategies can return something compatible
// with AutoLanguageClient.startServerProcess.
export interface LanguageServerProcess extends EventEmitter {
stdin: stream.Writable;
stdout: stream.Readable;
stderr: stream.Readable;
pid: number;

kill(signal?: string): void;
on(event: 'error', listener: (err: Error) => void): this;
on(event: 'exit', listener: (code: number, signal: string) => void): this;
}

// Public: AutoLanguageClient provides a simple way to have all the supported
// Atom-IDE services wired up entirely for you by just subclassing it and
// implementing startServerProcess/getGrammarScopes/getLanguageName and
Expand All @@ -62,7 +47,7 @@ export default class AutoLanguageClient {
private _disposable = new CompositeDisposable();
private _serverManager: ServerManager;
private _linterDelegate: linter.V2IndieDelegate;
private _signatureHelpRegistry: atomIde.SignatureHelpRegistry;
private _signatureHelpRegistry: atomIde.SignatureHelpRegistry | null;
private _lastAutocompleteRequest: AutocompleteRequest;
private _isDeactivating: boolean;

Expand Down
2 changes: 1 addition & 1 deletion lib/convert.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 }> = [];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

if (fileEvent.oldPath) {
results.push({uri: Convert.pathToUri(fileEvent.oldPath || ''), type: ls.FileChangeType.Deleted});
}
Expand Down
4 changes: 2 additions & 2 deletions lib/download-file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bug in the previous code?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha, looks like it. Nice catch.

const reader = body.getReader();
const writer = fs.createWriteStream(targetFile);

Expand Down Expand Up @@ -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);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

progressCallback(bytesDone, percent);
}
}
Expand Down
23 changes: 20 additions & 3 deletions lib/server-manager.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,37 @@
import { Logger } from './logger';
import { EventEmitter } from 'events';
import LinterPushV2Adapter from './adapters/linter-push-v2-adapter';
import DocumentSyncAdapter from './adapters/document-sync-adapter';
import SignatureHelpAdapter from './adapters/signature-help-adapter';

import * as path from 'path';
import * as stream from 'stream';
import * as cp from 'child_process';
import * as ls from './languageclient';
import * as atomIde from 'atom-ide';
import Convert from './convert';
import { CompositeDisposable, ProjectFileEvent, TextEditor } from 'atom';

// Public: Defines the minimum surface area for an object that resembles a
// ChildProcess. This is used so that language packages with alternative
// language server process hosting strategies can return something compatible
// with AutoLanguageClient.startServerProcess.
export interface LanguageServerProcess extends EventEmitter {
stdin: stream.Writable;
stdout: stream.Readable;
stderr: stream.Readable;
pid: number;

kill(signal?: string): void;
on(event: 'error', listener: (err: Error) => void): this;
on(event: 'exit', listener: (code: number, signal: string) => void): this;
}

// The necessary elements for a server that has started or is starting.
export interface ActiveServer {
disposable: CompositeDisposable;
projectPath: string;
process: cp.ChildProcess;
process: LanguageServerProcess;
connection: ls.LanguageClientConnection;
capabilities: ls.ServerCapabilities;
linterPushV2?: LinterPushV2Adapter;
Expand Down Expand Up @@ -269,7 +286,7 @@ export class ServerManager {
if (filePath == null) {
return null;
}
return this._normalizedProjectPaths.find((d) => filePath.startsWith(d));
return this._normalizedProjectPaths.find((d) => filePath.startsWith(d)) || null;
}

public updateNormalizedProjectPaths(): void {
Expand All @@ -293,7 +310,7 @@ export class ServerManager {
}

for (const activeServer of this._activeServers) {
const changes = [];
const changes: ls.FileEvent[] = [];
for (const fileEvent of fileEvents) {
if (fileEvent.path.startsWith(activeServer.projectPath) && this._changeWatchedFileFilter(fileEvent.path)) {
changes.push(Convert.atomFileEventToLSFileEvents(fileEvent)[0]);
Expand Down
6 changes: 3 additions & 3 deletions test/adapters/autocomplete-adapter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ describe('AutoCompleteAdapter', () => {
capabilities: {completionProvider: { }},
connection: new ls.LanguageClientConnection(createSpyConnection()),
disposable: new CompositeDisposable(),
process: undefined,
process: undefined as any,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Type system hack to not have to mock a whole process

projectPath: '/',
};
}
Expand Down Expand Up @@ -279,7 +279,7 @@ describe('AutoCompleteAdapter', () => {

it('does not do anything if there is no textEdit', () => {
const completionItem: AutocompleteSuggestion = {};
AutoCompleteAdapter.applyTextEditToSuggestion(null, new TextEditor(), completionItem);
AutoCompleteAdapter.applyTextEditToSuggestion(undefined, new TextEditor(), completionItem);
expect(completionItem).deep.equals({});
});

Expand Down Expand Up @@ -315,7 +315,7 @@ describe('AutoCompleteAdapter', () => {
});

it('defaults to "value"', () => {
const result = AutoCompleteAdapter.completionKindToSuggestionType(null);
const result = AutoCompleteAdapter.completionKindToSuggestionType(undefined);
expect(result).equals('value');
});
});
Expand Down
6 changes: 4 additions & 2 deletions test/adapters/code-highlight-adapter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

expect(result.length).to.equal(1);
expect(result[0].isEqual(new Range([0, 1], [0, 2]))).to.be.true;
}
});

it('throws if document highlights are not supported', async () => {
Expand Down
26 changes: 14 additions & 12 deletions test/adapters/datatip-adapter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,21 +48,23 @@ describe('DatatipAdapter', () => {
expect(datatip).to.be.ok;
invariant(datatip != null);

expect(datatip.range.start.row).equal(0);
expect(datatip.range.start.column).equal(1);
expect(datatip.range.end.row).equal(0);
expect(datatip.range.end.column).equal(2);
if (datatip) {
expect(datatip.range.start.row).equal(0);
expect(datatip.range.start.column).equal(1);
expect(datatip.range.end.row).equal(0);
expect(datatip.range.end.column).equal(2);

expect(datatip.markedStrings).to.have.lengthOf(2);
expect(datatip.markedStrings[0]).eql({type: 'markdown', value: 'test'});
expect(datatip.markedStrings).to.have.lengthOf(2);
expect(datatip.markedStrings[0]).eql({type: 'markdown', value: 'test'});

const snippet = datatip.markedStrings[1];
expect(snippet.type).equal('snippet');
invariant(snippet.type === 'snippet');
expect((snippet as any).grammar.scopeName).equal('text.plain.null-grammar');
expect(snippet.value).equal('test snippet');
const snippet = datatip.markedStrings[1];
expect(snippet.type).equal('snippet');
invariant(snippet.type === 'snippet');
expect((snippet as any).grammar.scopeName).equal('text.plain.null-grammar');
expect(snippet.value).equal('test snippet');

expect(grammarSpy.calledWith('source.testlang')).to.be.true;
expect(grammarSpy.calledWith('source.testlang')).to.be.true;
}
});
});
});
2 changes: 1 addition & 1 deletion test/adapters/document-sync-adapter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('DocumentSyncAdapter', () => {

describe('constructor', () => {
function create(textDocumentSync) {
return new DocumentSyncAdapter(null, textDocumentSync, () => false);
return new DocumentSyncAdapter(null as any, textDocumentSync, () => false);
}

it('sets _documentSyncKind correctly Incremental for v2 capabilities', () => {
Expand Down
Loading