Strict nulls for everything except logconsole #7657

vidartf · 2019-12-19T02:21:10Z

References

Continues #7607.

Code changes

Turns on strict null checks for all packages except services (to avoid double work related to #7252).

User-facing changes

None, maybe some bugfixes.

Backwards-incompatible changes

Typings on some public API gets "fixed", but these are sweeping enough that it should be considered backwards incompatible.

TODO:

Ensure all packages compile
Ensure all tests still work and pass
Review non-trivial changes

jupyterlab-dev-mode · 2019-12-19T02:21:12Z

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

vidartf · 2019-12-19T02:25:31Z

Status: Most "core dependencies" now compile, so most errors are moving towards being in "leaf" packages.

Note: To better handle typings vs. dispose, I some places change from setting the cleared variables to null, to using the delete operator. We could also consider doing a this._var = null! pattern, but I assume this will attract opinions either way. I did it this way for now as it is a the path of least resistance, and will allow that discussion to be sorted in a follow-up PR (there is enough things to review here as it is).

blink1073

Thanks for tackling this!

packages/application/src/router.ts

packages/rendermime/src/attachmentmodel.ts

packages/application/src/mimerenderers.ts

vidartf · 2019-12-19T21:08:48Z

One issue that keeps popping up for strict nulls is this: MainAreaWidget will set its contents and toolbar attributes to null (after disposing them) if there is an exception during async initialization. A lot of code simply uses those properties as if they will always be non-null (which will be the case except in error states). The question then becomes:

Should we type those attributes with | null, and have all code handle those paths
Should we say that the behavior is undefined when it fails (a lot of other errors are likely to follow due to a bad init), and not include | undefined in the typing
Should we actually leave the values of those attributes defined (disposed?) and risk some memory leak

vidartf · 2019-12-19T21:11:05Z

For clarity, (2) will be closest to today's situation, so I will likely default to this option if no-one else chimes in. It is also not clear to me how such errored widgets will be tracked by various widget trackers.

jasongrout · 2019-12-19T21:11:17Z

I think type them as not null, and when disposed, set them to null!. Our API contract is no longer valid after disposal.

jasongrout · 2019-12-19T21:12:06Z

(I think that is essentially your option 2, but using null instead of undefined.)

vidartf · 2019-12-20T00:28:23Z

Our API contract is no longer valid after disposal.

Note, that the case I'm talking about is not disposal, but rather showing an error message instead:

jupyterlab/packages/apputils/src/mainareawidget.ts

Lines 75 to 92 in 0c2ef44

    
           .catch(e => { 
        
             // Show a revealed promise error. 
        
             const error = new Widget(); 
        
             // Show the error to the user. 
        
             const pre = document.createElement('pre'); 
        
             pre.textContent = String(e); 
        
             error.node.appendChild(pre); 
        
             BoxLayout.setStretch(error, 1); 
        
             this.node.removeChild(spinner.node); 
        
             spinner.dispose(); 
        
             content.dispose(); 
        
             this._content = null; 
        
             toolbar.dispose(); 
        
             this._toolbar = null; 
        
             layout.addWidget(error); 
        
             this._isRevealed = true; 
        
             throw error; 
        
           });

vidartf · 2019-12-20T00:30:47Z

Status update: All packages now compile, so this can be considered a rough draft. Next is getting tests to compile.

vidartf · 2019-12-20T02:58:28Z

Tests passing locally. Once it turns green for CI, I'll rebase and restructure the commits to make reviewing easier. I'll also address the TODO comments above.

vidartf · 2019-12-20T14:31:12Z

All green, and TODOs fixed. Will do a rebase / history-rewrite to clean up commit messages and hopefully ease review.

vidartf · 2019-12-20T19:04:56Z

I see that the LogConsoleOutputArea inherits from OutputArea, but also allows rendermime to be writable and have null as a value. This causes a rather hard to solve error as I don't think we should "upstream" this typing to the OutputArea class. My current plan is to disable null checks for the logconsole[-extension] packages, unless someone else can think of something smarter?

CC @jasongrout / @mbektasbbg

Should not require detailed review.

These should not change the logic when operating normally, but might end up silently supressing failures instead of throwing errors in certain failure cases. On the other hand, the suppression might be what we want here.

Some of these are likely not correct, some of them are. Hard to tell, but they are unlikely to affect the public API, so punting.

These made sense to me, but might be the wrong fixes!

vidartf · 2019-12-20T19:34:04Z

packages/coreutils/src/interfaces.ts

@@ -15,7 +15,7 @@ import { ISignal } from '@lumino/signaling';
 /**
 * A generic interface for change emitter payloads.
 */
-export interface IChangedArgs<T, U extends string = string> {
+export interface IChangedArgs<T, OldT = T, U extends string = string> {


This commit adds an optional generic arg to allow change notifications to specify different typings for the old and new values (nice e.g. when the initial value is null, but it can never be set back to null after that). Note that this changes the order of the generic args (the name typing gets pushed later).

buildutils/src/ensure-repo.ts

vidartf · 2019-12-20T19:38:44Z

packages/apputils/src/vdom.ts

+  /**
+   * Create a new VDomRenderer
+   */
+  constructor(model: T extends null ? void : T) {


This commit changes the VDomRenderer to take the model as a constructor arg, in order to make it non-nullable. This was an easy fit here, but might give less flexibility for 3rd party renderers?

The reason not to have it in the constructor was because setting the model calls override-able methods, which would not call overrridden versions of those methods in the subclass.

No test failures 😉

Do you want to revert this behavior then? Or perhaps have the constructor call a protected setter method (or something similar)?

vidartf · 2019-12-20T19:42:34Z

packages/csvviewer/src/model.ts

@@ -83,9 +83,9 @@ export class DSVModel extends DataModel implements IDisposable {
    this._parseAsync();

    // Cache the header row.
-    if (header === true && this._columnCount > 0) {
+    if (header === true && this._columnCount! > 0) {


It was really hard to track down when the various variables in the CSV viewer code was null/undefined, so for now I simply added non-null asserts everywhere. This commit has a lot of other such asserts as well, and basically maintains the status-quo, even if they might have revealed some incorrect nullable handling. The main point to review this for is if any fix of an incorrect call here would cause an API breakage (we could probably put those as "fixing" typings as a bug, so might be ok).

vidartf · 2019-12-20T19:44:12Z

packages/apputils/src/printing.ts

@@ -90,10 +90,11 @@ export namespace Printing {
      await resolveWhenLoaded(iframe);
    } else {
      iframe.src = 'about:blank';
+      await resolveWhenLoaded(iframe);


I think this resolves a possible race condition?

vidartf · 2019-12-20T19:45:12Z

packages/coreutils/src/settingregistry.ts

-        user: key in user ? copy(user[key]) : undefined
+        composite:
+          composite[key] !== undefined ? copy(composite[key]!) : undefined,
+        user: user[key] !== undefined ? copy(user[key]!) : undefined


I think this should behave correctly, but it is a logic change.

vidartf · 2019-12-20T19:45:36Z

packages/csvviewer/src/parse.ts

+        if (ncols === undefined) {
+          if (nrows !== 1) {
+            throw new Error('Error parsing default number of columns');
+          }


@jasongrout Will these be logically equivalent?

vidartf · 2019-12-20T19:46:23Z

packages/docmanager-extension/src/index.ts

+          body: 'No context found for current widget!',
+          buttons: [Dialog.okButton()]
+        });
+      }


I added a bunch of failure dialogs like this here, based on the pattern for one of the commands. Not sure if silent failure is better?

vidartf · 2019-12-20T19:47:26Z

packages/docmanager/src/widgetmanager.ts

@@ -252,7 +252,7 @@ export class DocumentWidgetManager implements IDisposable {
   *
   * @param widget - The target widget.
   */
-  protected setCaption(widget: Widget): Promise<void> {
+  protected async setCaption(widget: Widget): Promise<void> {


This should fix the return type from the naked return statements below from untitled to Promise<void>. I assume this is a fix, and not an API break...

i.e. I assume the return type should stay the currently typed Promise<void>, not the currently implemented Promise<void> | undefined.

vidartf · 2019-12-20T19:47:47Z

packages/logconsole-extension/src/index.tsx

@@ -270,7 +270,7 @@ function activateLogConsole(
    } else {
      source = null;
    }
-    if (logConsoleWidget) {
+    if (logConsolePanel) {


I assume this is a fix, and not a break...

vidartf · 2019-12-20T19:48:36Z

packages/logconsole/src/widget.ts

+            outputArea.rendermime = change.newValue;
+          } else {
+            outputArea.dispose();
+          }


I'm unsure why I made this change, but it think it made sense at the time. Please review carefully...

vidartf · 2019-12-20T19:49:12Z

packages/terminal-extension/src/index.ts

@@ -336,6 +336,7 @@ export function addCommands(
        Terminal = (await Private.ensureWidget()).Terminal;
      } catch (err) {
        Private.showErrorMessage(err);
+        return;


I assume this is a fix, and not a break...

packages/terminal-extension/src/index.ts

tests/test-cells/src/widget.spec.ts

vidartf · 2019-12-20T19:52:43Z

packages/coreutils/src/statedb.ts

-  implements IStateDB<T> {
+export class StateDB<
+  T extends ReadonlyPartialJSONValue = ReadonlyPartialJSONValue
+> implements IStateDB<T> {


Here I switch the StateDB to use partial types as well, as that seems to be needed / more accurate. On the other hand, it is harder to tighten up this types later if we incorrectly relax them now. Thoughts? CC @afshin

vidartf · 2019-12-20T19:53:07Z

packages/codemirror-extension/src/index.ts

@@ -348,7 +348,7 @@ function activateEditorCommands(
      }
      modeMenu.addItem({
        command: CommandIDs.changeMode,
-        args: { ...spec }
+        args: { ...spec } as any // TODO: Casting to `any` until lumino typings are fixed


Xref this comment: jupyterlab/lumino#32 (comment)

PR: jupyterlab/lumino#33

blink1073 · 2019-12-20T19:59:52Z

Heads up, #7252 caused a bunch of conflicts.

mbektas · 2019-12-20T20:35:49Z

I see that the LogConsoleOutputArea inherits from OutputArea, but also allows rendermime to be writable and have null as a value. This causes a rather hard to solve error as I don't think we should "upstream" this typing to the OutputArea class. My current plan is to disable null checks for the logconsole[-extension] packages, unless someone else can think of something smarter?

CC @jasongrout / @mbektasbbg

@vidartf I looked into this but I don't see an easy solution. Your plan to disable null checks for log console sounds good to me, at least until we have a better solution.

vidartf · 2019-12-22T13:02:16Z

I've merged in master, and it is all green again. (one outstanding comment from @blink1073 above to resolve)

blink1073 · 2019-12-23T16:01:05Z

Merging and iterating on this, since it affects so many packages. Thanks!

jtpio mentioned this pull request Dec 19, 2019

Update the notebook tools on tracker.currentChanged #7659

Merged

blink1073 reviewed Dec 19, 2019

View reviewed changes

packages/application/src/router.ts Outdated Show resolved Hide resolved

packages/rendermime/src/attachmentmodel.ts Outdated Show resolved Hide resolved

packages/application/src/mimerenderers.ts Outdated Show resolved Hide resolved

vidartf added 3 commits December 20, 2019 15:16

Fix lint commands for windows

0bdce26

Update to lumino/commands 1.9

479815c

Turn on strict null checks (except for services)

df0b0f7

vidartf added 9 commits December 20, 2019 19:30

Add generic arg to IChangeArgs helper

98e2a16

Simple typings changes for strict nulls

db21fd2

Should not require detailed review.

Might supress wanted errors

a9efefe

These should not change the logic when operating normally, but might end up silently supressing failures instead of throwing errors in certain failure cases. On the other hand, the suppression might be what we want here.

non-null asserts

f5019d8

Some of these are likely not correct, some of them are. Hard to tell, but they are unlikely to affect the public API, so punting.

Vdom always has model

51f9943

dispose

fed1b82

Logic changes

4569342

These made sense to me, but might be the wrong fixes!

More partial types

56bbec9

misc uncertainties

cf623f1

vidartf changed the title ~~Strict nulls for everything except services~~ Strict nulls for everything except services/logconsole Dec 20, 2019

vidartf force-pushed the strict-nulls branch from 879c914 to cf623f1 Compare December 20, 2019 19:30

vidartf marked this pull request as ready for review December 20, 2019 19:31

vidartf commented Dec 20, 2019

View reviewed changes

buildutils/src/ensure-repo.ts Show resolved Hide resolved

vidartf commented Dec 20, 2019

View reviewed changes

vidartf requested review from blink1073, jasongrout, telamonian and afshin December 20, 2019 19:53

vidartf added 4 commits December 20, 2019 23:27

Strict nulls for services

e84817e

integrity

d5995b2

Fixes for default value of session clientId

3238bf4

Clean up some tests

1b3930d

vidartf changed the title ~~Strict nulls for everything except services/logconsole~~ Strict nulls for everything except logconsole Dec 22, 2019

vidartf mentioned this pull request Dec 22, 2019

Coreutils split #7681

Merged

blink1073 added this to the 2.0 milestone Dec 23, 2019

blink1073 merged commit 72d9e17 into jupyterlab:master Dec 23, 2019

vidartf deleted the strict-nulls branch December 23, 2019 19:21

telamonian mentioned this pull request Dec 30, 2019

Fixes lint-on-commit hook #7708

Merged

lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jan 24, 2020

lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strict nulls for everything except logconsole #7657

Strict nulls for everything except logconsole #7657

vidartf commented Dec 19, 2019 •

edited

jupyterlab-dev-mode bot commented Dec 19, 2019

vidartf commented Dec 19, 2019

blink1073 left a comment

vidartf commented Dec 19, 2019 •

edited

vidartf commented Dec 19, 2019

jasongrout commented Dec 19, 2019

jasongrout commented Dec 19, 2019 •

edited

vidartf commented Dec 20, 2019

vidartf commented Dec 20, 2019

vidartf commented Dec 20, 2019

vidartf commented Dec 20, 2019

vidartf commented Dec 20, 2019 •

edited

vidartf Dec 20, 2019

vidartf Dec 20, 2019

blink1073 Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 22, 2019

vidartf Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 21, 2019 •

edited

vidartf Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 20, 2019

vidartf Dec 20, 2019

blink1073 Dec 20, 2019

blink1073 commented Dec 20, 2019

mbektas commented Dec 20, 2019

vidartf commented Dec 22, 2019

blink1073 commented Dec 23, 2019

Strict nulls for everything except logconsole #7657

Strict nulls for everything except logconsole #7657

Conversation

vidartf commented Dec 19, 2019 • edited

References

Code changes

User-facing changes

Backwards-incompatible changes

TODO:

jupyterlab-dev-mode bot commented Dec 19, 2019

vidartf commented Dec 19, 2019

blink1073 left a comment

Choose a reason for hiding this comment

vidartf commented Dec 19, 2019 • edited

vidartf commented Dec 19, 2019

jasongrout commented Dec 19, 2019

jasongrout commented Dec 19, 2019 • edited

vidartf commented Dec 20, 2019

vidartf commented Dec 20, 2019

vidartf commented Dec 20, 2019

vidartf commented Dec 20, 2019

vidartf commented Dec 20, 2019 • edited

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

vidartf Dec 21, 2019 • edited

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

blink1073 commented Dec 20, 2019

mbektas commented Dec 20, 2019

vidartf commented Dec 22, 2019

blink1073 commented Dec 23, 2019

vidartf commented Dec 19, 2019 •

edited

vidartf commented Dec 19, 2019 •

edited

jasongrout commented Dec 19, 2019 •

edited

vidartf commented Dec 20, 2019 •

edited

vidartf Dec 21, 2019 •

edited