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.

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

Strict nulls for everything except logconsole #7657

Merged
merged 16 commits into from Dec 23, 2019

Conversation

vidartf
Copy link
Member

@vidartf vidartf commented Dec 19, 2019

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
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Dec 19, 2019

Thanks for making a pull request to JupyterLab!

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

@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 19, 2019

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).

Copy link
Member

@blink1073 blink1073 left a comment

Thanks for tackling this!

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
Copy link
Member Author

@vidartf vidartf commented Dec 19, 2019

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:

  1. Should we type those attributes with | null, and have all code handle those paths
  2. 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
  3. Should we actually leave the values of those attributes defined (disposed?) and risk some memory leak

@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 19, 2019

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
Copy link
Contributor

@jasongrout jasongrout commented Dec 19, 2019

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

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 19, 2019

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

@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 20, 2019

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:

.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
Copy link
Member Author

@vidartf vidartf commented Dec 20, 2019

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

@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 20, 2019

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
Copy link
Member Author

@vidartf vidartf commented Dec 20, 2019

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

@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 20, 2019

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 added 9 commits Dec 20, 2019
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 vidartf changed the title Strict nulls for everything except services Strict nulls for everything except services/logconsole Dec 20, 2019
@vidartf vidartf marked this pull request as ready for review Dec 20, 2019
@@ -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> {
Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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).

/**
* Create a new VDomRenderer
*/
constructor(model: T extends null ? void : T) {
Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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?

Copy link
Member

@blink1073 blink1073 Dec 20, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

No test failures 😉

Copy link
Member Author

@vidartf vidartf Dec 22, 2019

Choose a reason for hiding this comment

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

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

@@ -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) {
Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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).

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

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

I think this resolves a possible race condition?

user: key in user ? copy(user[key]) : undefined
composite:
composite[key] !== undefined ? copy(composite[key]!) : undefined,
user: user[key] !== undefined ? copy(user[key]!) : undefined
Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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

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

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

@jasongrout Will these be logically equivalent?

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

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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?

@@ -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> {
Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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...

Copy link
Member Author

@vidartf vidartf Dec 21, 2019

Choose a reason for hiding this comment

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

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

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

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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

outputArea.rendermime = change.newValue;
} else {
outputArea.dispose();
}
Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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

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

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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

packages/terminal-extension/src/index.ts Show resolved Hide resolved
tests/test-cells/src/widget.spec.ts Outdated Show resolved Hide resolved
implements IStateDB<T> {
export class StateDB<
T extends ReadonlyPartialJSONValue = ReadonlyPartialJSONValue
> implements IStateDB<T> {
Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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

@@ -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
Copy link
Member Author

@vidartf vidartf Dec 20, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

@blink1073 blink1073 Dec 20, 2019

Choose a reason for hiding this comment

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

@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 20, 2019

Heads up, #7252 caused a bunch of conflicts.

@mbektas
Copy link
Member

@mbektas mbektas commented Dec 20, 2019

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 vidartf changed the title Strict nulls for everything except services/logconsole Strict nulls for everything except logconsole Dec 22, 2019
@vidartf
Copy link
Member Author

@vidartf vidartf commented Dec 22, 2019

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

@vidartf vidartf mentioned this pull request Dec 22, 2019
@blink1073 blink1073 added this to the 2.0 milestone Dec 23, 2019
@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 23, 2019

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

@blink1073 blink1073 merged commit 72d9e17 into jupyterlab:master Dec 23, 2019
10 checks passed
@vidartf vidartf deleted the strict-nulls branch Dec 23, 2019
@lock lock bot added the status:resolved-locked label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants