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.

Sign up for GitHub

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

Jump to bottom

chore: accurate null checks #132

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

chore: accurate null checks #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 8 additions & 8 deletions lib/adapters/autocomplete-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,12 @@ class PossiblyResolvedCompletionItem {
/** Public: Adapts the language server protocol "textDocument/completion" to the Atom AutoComplete+ package. */
export default class AutocompleteAdapter {
public static canAdapt(serverCapabilities: ServerCapabilities): boolean {
return serverCapabilities.completionProvider != null
return serverCapabilities.completionProvider !== undefined
}

public static canResolve(serverCapabilities: ServerCapabilities): boolean {
return (
serverCapabilities.completionProvider != null && serverCapabilities.completionProvider.resolveProvider === true
serverCapabilities.completionProvider !== undefined && serverCapabilities.completionProvider.resolveProvider === true
)
}

Expand All @@ -89,7 +89,7 @@ export default class AutocompleteAdapter {
shouldReplace: ShouldReplace = false
): Promise<ac.AnySuggestion[]> {
const triggerChars =
server.capabilities.completionProvider != null
server.capabilities.completionProvider !== undefined
? server.capabilities.completionProvider.triggerCharacters || []
: []

Expand Down Expand Up @@ -233,11 +233,11 @@ export default class AutocompleteAdapter {
const cache = this._suggestionCache.get(server)
if (cache) {
const possiblyResolvedCompletionItem = cache.suggestionMap.get(suggestion)
if (possiblyResolvedCompletionItem != null && possiblyResolvedCompletionItem.isResolved === false) {
if (possiblyResolvedCompletionItem !== undefined && possiblyResolvedCompletionItem.isResolved === false) {
const resolvedCompletionItem = await server.connection.completionItemResolve(
possiblyResolvedCompletionItem.completionItem
)
if (resolvedCompletionItem != null) {
if (resolvedCompletionItem !== null) {
AutocompleteAdapter.resolveSuggestion(resolvedCompletionItem, suggestion, request, onDidConvertCompletionItem)
possiblyResolvedCompletionItem.isResolved = true
}
Expand All @@ -254,7 +254,7 @@ export default class AutocompleteAdapter {
): void {
// only the `documentation` and `detail` properties may change when resolving
AutocompleteAdapter.applyDetailsToSuggestion(resolvedCompletionItem, suggestion)
if (onDidConvertCompletionItem != null) {
if (onDidConvertCompletionItem !== undefined) {
onDidConvertCompletionItem(resolvedCompletionItem, suggestion as ac.AnySuggestion, request)
}
}
Expand Down Expand Up @@ -417,7 +417,7 @@ export default class AutocompleteAdapter {
shouldReplace
)
AutocompleteAdapter.applySnippetToSuggestion(item, suggestion as SnippetSuggestion)
if (onDidConvertCompletionItem != null) {
if (onDidConvertCompletionItem !== undefined) {
onDidConvertCompletionItem(item, suggestion as ac.AnySuggestion, request)
}

Expand Down Expand Up @@ -448,7 +448,7 @@ export default class AutocompleteAdapter {
suggestion.description = item.documentation
}

if (item.documentation != null && typeof item.documentation === "object") {
if (item.documentation !== undefined && typeof item.documentation === "object") {
// Newer format specifies the kind of documentation, assign appropriately
if (item.documentation.kind === "markdown") {
suggestion.descriptionMarkdown = item.documentation.value
Expand Down
6 changes: 3 additions & 3 deletions lib/adapters/code-action-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export default class CodeActionAdapter {
range: Range,
diagnostics: atomIde.Diagnostic[]
): Promise<atomIde.CodeAction[]> {
if (linterAdapter == null) {
if (linterAdapter === undefined) {
return []
}
assert(serverCapabilities.codeActionProvider, "Must have the textDocument/codeAction capability")
Expand Down Expand Up @@ -100,9 +100,9 @@ export default class CodeActionAdapter {
// Until the Linter API provides a place to store the code,
// there's no real way for the code actions API to give it back to us.
const converted = Convert.atomIdeDiagnosticToLSDiagnostic(diagnostic)
if (diagnostic.range != null && diagnostic.text != null) {
if (diagnostic.text !== undefined) {
const code = linterAdapter.getDiagnosticCode(editor, diagnostic.range, diagnostic.text)
if (code != null) {
if (code !== null) {
converted.code = code
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/adapters/command-execution-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export default class CommandExecutionAdapter {
private static commandsCustomCallbacks = new Map<string, CommandCustomCallbackFunction>()

public static canAdapt(serverCapabilities: ServerCapabilities): boolean {
return serverCapabilities.executeCommandProvider != null
return serverCapabilities.executeCommandProvider !== undefined
}

public static registerCustomCallbackForCommand(command: string, callback: CommandCustomCallbackFunction): void {
Expand Down
5 changes: 3 additions & 2 deletions lib/adapters/datatip-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@ export default class DatatipAdapter {
const documentPositionParams = Convert.editorToTextDocumentPositionParams(editor, point)

const hover = await connection.hover(documentPositionParams)
if (hover == null || DatatipAdapter.isEmptyHover(hover)) {
if (hover === null || DatatipAdapter.isEmptyHover(hover)) {
return null
}

const range = hover.range == null ? Utils.getWordAtPosition(editor, point) : Convert.lsRangeToAtomRange(hover.range)
const range = hover.range === undefined ? Utils.getWordAtPosition(editor, point) : Convert.lsRangeToAtomRange(hover.range)

const markedStrings = (Array.isArray(hover.contents) ? hover.contents : [hover.contents]).map((str) =>
DatatipAdapter.convertMarkedString(editor, str)
Expand All @@ -47,6 +47,7 @@ export default class DatatipAdapter {
}

private static isEmptyHover(hover: Hover): boolean {
// TODO hover.contents is never null!
Copy link
Contributor

Choose a reason for hiding this comment

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

The types can lie. Especially Microsoft's types. You sure it's never null?

Copy link
Member

Choose a reason for hiding this comment

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

And are we sure something won't change to make it be null in the future? I think we should keep these checks unless we have tests so future dependency updates won't break something these checks would catch.

Copy link
Member

Choose a reason for hiding this comment

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

I would rather disable an eslint rule than remove these checks and waste time figuring out where a null bug is coming from.

Copy link
Contributor

Choose a reason for hiding this comment

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

And are we sure something won't change to make it be null in the future?

Ideally, that should be caught by types on dependency update. But as I said, types can lie. The "proper" thing to do is to fix type definitions, and not pepper the code base with redundant checks. Regrettably, fixing type definitions is not always straightforward.

Copy link
Member

Choose a reason for hiding this comment

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

The "proper" thing to do is to fix type definitions, and not pepper the code base with redundant checks.

Yes Ideally this would be caught by typescript. Unfortunately we don't live in an ideal world and I would estimate that at any given time about 50% of the types in DefinitelyTyped are wrong because the dependency updated and no one bothered to update the types.

If we were only dealing with strictly typescript packages (where they have some process to catch when types are wrong) that would be one thing, but we are not. Which is why these checks are here.

Essentially, types do not catch runtime errors, but these checks do.

Copy link
Member Author

@aminya aminya Mar 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The types can lie. Especially Microsoft's types. You sure it's never null?

Research is needed. One of us (MS vs Atom) is wrong. No matter who is right, it will result in a better code/interface.

waste time figuring out where a null bug is coming from.

This is not wasting time. If there is a null bug, we should either commit to it by updating the original types or remove the excess check.

  • If the issue is that an undocumented null/undefined is being returned from the types coming from @atom/types or lsp types, then the original types should be updated, and then we update our interfaces to take that into account. TypeScript will do a new analysis based on the available information. This is not the same as throwing some random null checks here and there in the dark.

  • If the interface is correct, then we are doing an extra null-check for no reason. By removing it we simplify the code.

And are we sure something won't change to make it be null in the future?

Keeping the implementation loose, encourages using the interfaces loosely, which means people send null, and based on their tests it works although it wasn't documented. So, this is their mistake and not us. By tightening the interface and implementation, we prevent others from making mistakes.

Essentially, types do not catch runtime errors, but these checks do.

Random runtime checks have no value and encourage more errors. If you intend to keep the interface loose, then we should commit to it. We need to update the interface, so TypeScript analyzes the new interface, and we correctly handle it instead of randomly checking things.

Copy link
Member

@UziTech UziTech Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Random runtime checks have no value and encourage more errors.

I totally agree. I think the best thing to do when given the wrong data is throw an error. Unfortunately the way it is currently written is very loose which means us changing this could very easily break someone else's code that used to pass. I'm not advocating that we keep the checks, I'm advocating that if we are going to change our api like this (even if it is wrong) we need to bump the major version to let people know the api changed, or we need to keep the checks so the api doesn't change.

This is not wasting time. If there is a null bug, we should either commit to it by updating the original types or remove the excess check.

Where is your commitment to the null bugs in minimap. Your solution was to remove parts of the code that were correct instead of finding the actual bug. I agree, in an ideal world, unfortunately we don't live in an ideal world and sometimes (probably most times) it is good enough to write checks instead of doing it correctly. </rant>

Copy link
Member

@UziTech UziTech Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Typescript handles most compile time errors but do not catch run time errors. If we wanted to actually do it correctly we would keep these check and throw appropriate errors so the code is not so loose and bugs are easier to fix.

return (
hover.contents == null ||
(typeof hover.contents === "string" && hover.contents.length === 0) ||
Expand Down
4 changes: 2 additions & 2 deletions lib/adapters/definition-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,14 +43,14 @@ export default class DefinitionAdapter {
const definitionLocations = DefinitionAdapter.normalizeLocations(
await connection.gotoDefinition(documentPositionParams)
)
if (definitionLocations == null || definitionLocations.length === 0) {
if (definitionLocations === null || definitionLocations.length === 0) {
return null
}

let queryRange
if (serverCapabilities.documentHighlightProvider) {
const highlights = await connection.documentHighlight(documentPositionParams)
if (highlights != null && highlights.length > 0) {
if (highlights.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, are we sure connection always returns something? I know with tsserver that is not the case, the official types lie a lot about null.

queryRange = highlights.map((h) => Convert.lsRangeToAtomRange(h.range))
}
}
Expand Down
13 changes: 7 additions & 6 deletions lib/adapters/document-sync-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,12 +103,12 @@ export default class DocumentSyncAdapter {

private _handleGrammarChange(editor: TextEditor): void {
const sync = this._editors.get(editor)
if (sync != null && !this._editorSelector(editor)) {
if (sync !== undefined && !this._editorSelector(editor)) {
this._editors.delete(editor)
this._disposable.remove(sync)
sync.didClose()
sync.dispose()
} else if (sync == null && this._editorSelector(editor)) {
} else if (sync === undefined && this._editorSelector(editor)) {
this._handleNewEditor(editor)
}
}
Expand Down Expand Up @@ -160,10 +160,11 @@ export class TextEditorSyncAdapter {
private _versions: Map<string, number>,
private _reportBusyWhile: Utils.ReportBusyWhile
) {
// TODO atom.project.onDidChangeFiles is never null
this._fakeDidChangeWatchedFiles = atom.project.onDidChangeFiles == null

const changeTracking = this.setupChangeTracking(_documentSync)
if (changeTracking != null) {
if (changeTracking !== null) {
this._disposable.add(changeTracking)
}

Expand Down Expand Up @@ -279,7 +280,7 @@ export class TextEditorSyncAdapter {

private _bumpVersion(): void {
const filePath = this._editor.getPath()
if (filePath == null) {
if (filePath === undefined) {
return
}
this._versions.set(filePath, this._getVersion(filePath) + 1)
Expand All @@ -291,7 +292,7 @@ export class TextEditorSyncAdapter {
*/
private didOpen(): void {
const filePath = this._editor.getPath()
if (filePath == null) {
if (filePath === undefined) {
return
} // Not yet saved

Expand All @@ -315,7 +316,7 @@ export class TextEditorSyncAdapter {

/** Called when the {TextEditor} is closed and sends the 'didCloseTextDocument' notification to the connected language server. */
public didClose(): void {
if (this._editor.getPath() == null) {
if (this._editor.getPath() === undefined) {
return
} // Not yet saved

Expand Down
4 changes: 0 additions & 4 deletions lib/adapters/find-references-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,6 @@ export default class FindReferencesAdapter {
projectRoot: string | null
): Promise<atomIde.FindReferencesReturn | null> {
const locations = await connection.findReferences(FindReferencesAdapter.createReferenceParams(editor, point))
if (locations == null) {
return null
}

const references: atomIde.Reference[] = locations.map(FindReferencesAdapter.locationToReference)
return {
type: "data",
Expand Down
4 changes: 2 additions & 2 deletions lib/adapters/linter-push-v2-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ export default class LinterPushV2Adapter {
*/
public getDiagnosticCode(editor: atom.TextEditor, range: atom.Range, text: string): DiagnosticCode | null {
const path = editor.getPath()
if (path != null) {
if (path !== undefined) {
const diagnosticCodes = this._diagnosticCodes.get(path)
if (diagnosticCodes != null) {
if (diagnosticCodes !== undefined) {
return diagnosticCodes.get(getCodeKey(range, text)) || null
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/adapters/notifications-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export default class NotificationsAdapter {
text: a.title,
onDidClick: () => {
resolve(a)
if (notification != null) {
if (notification !== null) {
notification.dismiss()
}
},
Expand All @@ -51,7 +51,7 @@ export default class NotificationsAdapter {

const notification = addNotificationForMessage(params.type, params.message, options)

if (notification != null) {
if (notification !== null) {
notification.onDidDismiss(() => {
resolve(null)
})
Expand Down
24 changes: 12 additions & 12 deletions lib/adapters/outline-view-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export default class OutlineViewAdapter {
return symbols.map((symbol) => {
const tree = OutlineViewAdapter.hierarchicalSymbolToOutline(symbol)

if (symbol.children != null) {
if (symbol.children !== undefined) {
tree.children = OutlineViewAdapter.createHierarchicalOutlineTrees(symbol.children)
}

Expand Down Expand Up @@ -123,37 +123,37 @@ export default class OutlineViewAdapter {
// Create a map of containers by name with all items that have that name
const containers = allItems.reduce((map, item) => {
const name = item.outline.representativeName
if (name != null) {
if (name !== undefined) {
const container = map.get(name)
if (container == null) {
if (container === undefined) {
map.set(name, [item.outline])
} else {
container.push(item.outline)
}
}
return map
}, new Map())
}, new Map<string, atomIde.OutlineTree[]>())

const roots: atomIde.OutlineTree[] = []

// Put each item within its parent and extract out the roots
for (const item of allItems) {
const containerName = item.containerName
const child = item.outline
if (containerName == null || containerName === "") {
if (containerName === undefined || containerName === "") {
roots.push(item.outline)
} else {
const possibleParents = containers.get(containerName)
let closestParent = OutlineViewAdapter._getClosestParent(possibleParents, child)
if (closestParent == null) {
if (closestParent === null) {
closestParent = {
plainText: containerName,
representativeName: containerName,
startPosition: new Point(0, 0),
children: [child],
}
roots.push(closestParent)
if (possibleParents == null) {
if (possibleParents === undefined) {
containers.set(containerName, [closestParent])
} else {
possibleParents.push(closestParent)
Expand All @@ -168,10 +168,10 @@ export default class OutlineViewAdapter {
}

private static _getClosestParent(
candidates: atomIde.OutlineTree[] | null,
candidates: atomIde.OutlineTree[] | undefined,
child: atomIde.OutlineTree
): atomIde.OutlineTree | null {
if (candidates == null || candidates.length === 0) {
if (candidates === undefined || candidates.length === 0) {
return null
}

Expand All @@ -186,7 +186,7 @@ export default class OutlineViewAdapter {
if (
parent === undefined ||
parent.startPosition.isLessThanOrEqual(candidate.startPosition) ||
(parent.endPosition != null &&
(parent.endPosition !== undefined &&
candidate.endPosition &&
parent.endPosition.isGreaterThanOrEqual(candidate.endPosition))
) {
Expand Down Expand Up @@ -215,7 +215,7 @@ export default class OutlineViewAdapter {
value: symbol.name,
},
],
icon: icon != null ? icon : undefined,
icon: icon !== null ? icon : undefined,
representativeName: symbol.name,
startPosition: Convert.positionToPoint(symbol.selectionRange.start),
endPosition: Convert.positionToPoint(symbol.selectionRange.end),
Expand All @@ -238,7 +238,7 @@ export default class OutlineViewAdapter {
value: symbol.name,
},
],
icon: icon != null ? icon : undefined,
icon: icon !== null ? icon : undefined,
representativeName: symbol.name,
startPosition: Convert.positionToPoint(symbol.location.range.start),
endPosition: Convert.positionToPoint(symbol.location.range.end),
Expand Down
4 changes: 2 additions & 2 deletions lib/adapters/signature-help-adapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export default class SignatureHelpAdapter {

/** @returns A {Boolean} indicating this adapter can adapt the server based on the given serverCapabilities. */
public static canAdapt(serverCapabilities: ServerCapabilities): boolean {
return serverCapabilities.signatureHelpProvider != null
return serverCapabilities.signatureHelpProvider !== undefined
}

public dispose(): void {
Expand All @@ -28,7 +28,7 @@ export default class SignatureHelpAdapter {

public attach(register: atomIde.SignatureHelpRegistry): void {
const { signatureHelpProvider } = this._capabilities
assert(signatureHelpProvider != null)
assert(signatureHelpProvider !== undefined)

let triggerCharacters: Set<string> | undefined
if (signatureHelpProvider && Array.isArray(signatureHelpProvider.triggerCharacters)) {
Expand Down