chore: accurate null checks #132

aminya · 2021-03-15T21:58:19Z

No description provided.

lierdakil · 2021-03-16T07:59:33Z

lib/adapters/apply-edit-adapter.ts

@@ -90,7 +90,7 @@ export default class ApplyEditAdapter {
    const startRow = edit.oldRange.start.row
    const startCol = edit.oldRange.start.column
    const lineLength = buffer.lineLengthForRow(startRow)
-    if (lineLength == null || startCol > lineLength) {
+    if (startCol > lineLength) {


Uhh... this is technically an error. The function can return undefined, and the docs lie:

We should fix the original types then. Writing code based on the wrong types or docs is not a good idea!

Sure. My point is, we really should verify we don't accidentally break stuff by these linter-driven changes. Regrettably, TS isn't Haskell, so it can lie about types, especially when definitions are written by fallible humans.

lierdakil · 2021-03-16T08:01:58Z

lib/adapters/datatip-adapter.ts

@@ -53,6 +53,7 @@ export default class DatatipAdapter {
  }

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


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

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.

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

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.

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.

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.

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>

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.

lierdakil · 2021-03-16T08:03:10Z

lib/adapters/definition-adapter.ts

      return null
    }

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


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.

aminya added bug Something isn't working dependencies Pull requests that update a dependency file labels Mar 15, 2021

lierdakil reviewed Mar 16, 2021

View reviewed changes

aminya mentioned this pull request Apr 13, 2021

chore: update eslint-config-atomic #145

Merged

UziTech mentioned this pull request Apr 25, 2021

Added null check to some adapters and changed type definition #150

Merged

fix: use === in the comparisons + remove excess null checks (wip)

5aef4a6

aminya force-pushed the update-eslint-config-atomic branch from af5804e to 5aef4a6 Compare May 1, 2021 10:24

aminya changed the title ~~chore: update eslint-config-atomic~~ chore: accurate null checks May 1, 2021

aminya added low-priority and removed dependencies Pull requests that update a dependency file labels May 18, 2021

aminya closed this Jul 22, 2021

aminya reopened this Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: accurate null checks #132

chore: accurate null checks #132

aminya commented Mar 15, 2021

lierdakil Mar 16, 2021

aminya Mar 16, 2021

lierdakil Mar 16, 2021

lierdakil Mar 16, 2021

UziTech Mar 16, 2021

UziTech Mar 16, 2021

lierdakil Mar 16, 2021

UziTech Mar 16, 2021

aminya Mar 16, 2021 •

edited

Loading

UziTech Mar 17, 2021 •

edited

Loading

UziTech Mar 17, 2021 •

edited

Loading

lierdakil Mar 16, 2021

chore: accurate null checks #132

Are you sure you want to change the base?

chore: accurate null checks #132

Conversation

aminya commented Mar 15, 2021

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

aminya Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

UziTech Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

UziTech Mar 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aminya Mar 16, 2021 •

edited

Loading

UziTech Mar 17, 2021 •

edited

Loading

UziTech Mar 17, 2021 •

edited

Loading