WIP: YASNCB (Yet Another Strict Null Checks Branch) #1703

corasaurus-hex · 2022-05-01T07:09:02Z

What has Changed?

This turns on strictNullChecks in TypeScript and fixes all the remaining issues in the codebase that were preventing us from turning this setting on.

My Calva PR Checklist

I have:

Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik

This is an odd one. The types were very different for useRowCol = true and useRowCol = false and so I needed to use generics to clean it up.

corasaurus-hex · 2022-05-01T07:54:40Z

src/cursor-doc/model.ts

@@ -185,7 +185,7 @@ export class LineInputModel implements EditableModel {
          }
          return [a, b];
        })
-        .filter((x) => x !== null)
+        .filter((x) => x !== null) as [number, number][]


corasaurus-hex · 2022-05-01T07:55:39Z

src/cursor-doc/model.ts

@@ -224,7 +224,7 @@ export class LineInputModel implements EditableModel {
    const seen = new Set<number>();
    this.dirtyLines.sort();
    while (this.dirtyLines.length) {
-      let nextIdx = this.dirtyLines.shift();
+      let nextIdx = this.dirtyLines.shift() as number;


Again, TypeScript isn't smart enough to figure out that the length check on the previous line means this will never be undefined and will always be a number.

corasaurus-hex · 2022-05-01T07:57:58Z

src/cursor-doc/token-cursor.ts

+  cursorStartField: StartFieldKey,
+  cursorEndField: EndFieldKey
+): [LispTokenCursor[StartFieldKey], LispTokenCursor[EndFieldKey]][] => {
+  const ranges: [LispTokenCursor[StartFieldKey], LispTokenCursor[EndFieldKey]][] = [];


The explanation for this function is that we needed a generic version of _rangesForSexpsInList that didn't need really long types and weird type contortions in order to use it.

It's probably an improvement. But with my limited command over the typing system, right now this turned from ”this is messy” to ”I don't understand what I'm reading at all”. Not sure what to do about that...

It might also be that I am having a hard time trying to puzzle together the code in my head from the diffs.

corasaurus-hex · 2022-05-01T07:59:25Z

src/cursor-doc/undo.ts

+      if (step) {
+        step.undo(c);
+        this.redos.push(step);
+      }


More TypeScript silliness. The length check means pop will always work but TypeScript can't figure that out.

corasaurus-hex · 2022-05-01T08:01:17Z

src/custom-snippets.ts


  const options = {};

  if (pick !== undefined) {
-    options['evaluationSendCodeToOutputWindow'] =
-      snippetsDict[pick].evaluationSendCodeToOutputWindow;
+    options['evaluationSendCodeToOutputWindow'] = replSnippet?.evaluationSendCodeToOutputWindow;


strange note: evaluationSendCodeToOutputWindow was missing from customREPLCommandSnippet and I had to add it.

corasaurus-hex · 2022-05-01T08:02:12Z

src/debugger/calva-debug.ts

@@ -271,6 +271,7 @@ class CalvaDebugSession extends LoggingDebugSession {
    // Pass scheme in path argument to Source contructor so that if it's a jar file it's handled correctly
    const source = new Source(basename(debugResponse.file), debugResponse.file);
    const name = tokenCursor.getFunctionName();
+    util.assertIsDefined(name, 'Expected to find a function name!');


StackFrame doesn't allow for an undefined name.

corasaurus-hex · 2022-05-01T08:03:46Z

src/doc-mirror/index.ts

@@ -169,18 +169,21 @@ export class MirroredDocument implements EditableDocument {
  }

  public delete(): Thenable<boolean> {
-    return vscode.commands.executeCommand('deleteRight');
+    return vscode.commands.executeCommand('deleteRight').then((v) => !!v);


The actual return type here was Thenable<true | undefined> and so this fixes the type so it's Thenable<boolean> again.

corasaurus-hex · 2022-05-01T08:12:32Z

src/nrepl/index.ts

@@ -381,6 +382,7 @@ export class NReplSession {
          if (evaluation.onMessage(msg, pprintOptions)) {
            return true;
          }
+          return false;


I'm unsure of this behavior but we must return a boolean from this function and it clearly shouldn't return true.

corasaurus-hex · 2022-05-01T19:00:18Z

src/type-checks.ts

+  }
+}
+
+export { isNonEmptyString, isNullOrUndefined, isDefined, assertIsDefined };


We needed to split this out into its own file so it could be imported in files where vscode isn't available. Importing utilities was breaking unit tests because of this.

failing

PEZ · 2022-05-02T05:42:25Z

I found this in the noise where the integration test broke:

UNRESPONSIVE extension host: 'betterthantomorrow.calva' took 100% of 5100.624ms, saved PROFILE here: 'file:///tmp/exthost-19cc55.cpuprofile' [ { id: 'betterthantomorrow.calva', total: 5100381, percentage: 100 } ]

https://app.circleci.com/pipelines/github/BetterThanTomorrow/calva/5020/workflows/5edc3168-da3a-4cf8-ab29-02c887c079c0/jobs/21872?invite=true#step-104-114

Before that I also find:

[137:0501/213503.162639:ERROR:bus.cc(397)] Failed to connect to the bus: Could not parse server address: Unknown address type (examples of valid types are "tcp" and on UNIX "unix")

Which I am not sure if we usually see or not, but if not maybe it could be the cause of the timeout.

Anyway, those UNRESPONSIVE extension host: 'betterthantomorrow.calva' took 100% of 5100.624ms messages I sometimes see in the dev console when I have introduced some change causing Calva to go into a spin while activating.

I would see if I could run these tests locally. If you haven't before, it's surprisingly easy:

Quit VS Code Insiders if you have it running
In an integrated terminal of regular VS Code: npm run integration-test

corasaurus-hex · 2022-05-02T23:22:10Z

I ran it locally and profiled the code to find where it's stuck. I just need to figure out why it's stuck. I have the last known working commit sha for integration tests but it's not clear why it's broken by the following commit

PEZ · 2022-05-03T05:46:01Z

Which commits does the bisect give you? Maybe we can see what could be the cause with more eyes on it.

PEZ · 2022-05-03T05:48:49Z

Is it here?

fcf14e0

If so, that is still a bit opaque to me. 😄 I don't know what the implications are for enabling strict null checks.

PEZ · 2022-05-03T06:00:29Z

src/connector.ts

      if (menuSelections && menuSelections.cljsDefaultBuild && useDefaultBuild) {
        build = menuSelections.cljsDefaultBuild;
        useDefaultBuild = false;
      } else {
        if (typeof initCode === 'object' || initCode.includes('%BUILD%')) {
+          const buildsForSelection = startedBuilds


I stopped and wondered if we somehow consider the current selection here. Maybe name it builds? Or buildCandidates?.

PEZ · 2022-05-03T06:13:01Z

src/cursor-doc/paredit.ts

+    const previous = cursor.clone().previous();
+    assertIsDefined(previous, 'Expected a token to be before the cursor!');
+    const offset = previous.offsetStart;


I'm not sure I am reading this correctly. But generally in Paredit and token cursor code we need to gracefully handle when the structure does not match some expectation. We most often leave things unchanged in those cases. Does this change mean that we croak? I realize that that would be happening before the change as well, I'm more wondering about what we should be doing here...

corasaurus-hex added 30 commits April 30, 2022 18:26

Fix nulls/undefineds and improve repl info parser for params

aa2250b

Stop using deprecated function and use our own instead

a697c11

Fix an undefined value in utilities

02d317d

Help typescript out with some type assertions in cursor-doc model

02960ca

Fix null checks for project-types

0f72124

Fix null checks for completion provider

9426650

Fix nulls in src/connector.ts

c73129e

Fix null in src/converters.ts

3dd6b85

Fix nulls in cursor-doc indent and improve performance

6e6640f

Fix nulls in clojure-lexer

5aad863

Fix nulls in lexer

0510888

Fix nulls in paredit.ts

347e7eb

Fix nulls for _rangesForSexpsInList

2527798

This is an odd one. The types were very different for useRowCol = true and useRowCol = false and so I needed to use generics to clean it up.

Fix remaining nulls for token-cursor.ts

be6765b

Fix more nulls in paredit

46fa6ad

Fix nulls in undo.ts

ff9f09d

Fix types for evaluating custom snippets

6e17118

Fix types in repl-session

39c3208

Fix null checks for clojuredocs

e2622a2

Fix debugger-related null checks

7f77027

Fix more null checks in the debugger

28cf0f3

Another null check issue fixed, this time in doc-mirror

25af9b9

Fix null check issues in src/evaluate.ts

4043c42

Fix some null check issues in extension tests

74ab8f9

Fix null check issues in src/extension.ts

6a04743

Fix a ton of null check issues in extension.ts for highlighting

b91edb1

Fix live-share.ts null check issues

a58fbf1

Fix a number of strict null check issues in lsp code

30cc621

Fix null check issues in providers

6e26d72

Fix strict null check issue in repl-history.ts

387326f

corasaurus-hex commented May 1, 2022

View reviewed changes

Ensure same behavior here

1e921d6

corasaurus-hex commented May 1, 2022

View reviewed changes

corasaurus-hex added 10 commits May 1, 2022 03:21

Split project root local fns into tryToGet + get

61437fe

Move undefined check closer to where it is needed

d0cd19c

Forgot a bang !

0778866

Swap out use of regex for much faster substring search

857054d

Use function instead of try + assert

5106ee8

Compare against the right types

18d7761

Get rid of unused import

3d1aec3

Move type checking functions out to an ns without vscode

b83c26d

Fix some null checks

13d7e82

Fix the token cursor genericized function

bc7fadd

corasaurus-hex commented May 1, 2022

View reviewed changes

corasaurus-hex added 4 commits May 1, 2022 14:21

Reformat the tsconfig

cd0d893

Use prettier to format JSON files in the project

6414e32

Debug lsp downloads

f5c1fbd

Add some debugging so we can figure out why integration tests are

df16f82

failing

PEZ reviewed May 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: YASNCB (Yet Another Strict Null Checks Branch) #1703

WIP: YASNCB (Yet Another Strict Null Checks Branch) #1703

corasaurus-hex commented May 1, 2022

corasaurus-hex May 1, 2022

corasaurus-hex May 1, 2022

corasaurus-hex May 1, 2022

PEZ May 3, 2022

PEZ May 3, 2022

corasaurus-hex May 1, 2022

corasaurus-hex May 1, 2022

corasaurus-hex May 1, 2022

corasaurus-hex May 1, 2022

corasaurus-hex May 1, 2022

corasaurus-hex May 1, 2022 •

edited

Loading

PEZ commented May 2, 2022

corasaurus-hex commented May 2, 2022

PEZ commented May 3, 2022

PEZ commented May 3, 2022

PEZ May 3, 2022

PEZ May 3, 2022

WIP: YASNCB (Yet Another Strict Null Checks Branch) #1703

Are you sure you want to change the base?

WIP: YASNCB (Yet Another Strict Null Checks Branch) #1703

Conversation

corasaurus-hex commented May 1, 2022

What has Changed?

My Calva PR Checklist

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

corasaurus-hex May 1, 2022 • edited Loading

Choose a reason for hiding this comment

PEZ commented May 2, 2022

corasaurus-hex commented May 2, 2022

PEZ commented May 3, 2022

PEZ commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corasaurus-hex May 1, 2022 •

edited

Loading