Enables strictNullChecks to breadcrumbs.ts, outlineModel.ts, breadcrumbsModel.ts#65062

rudi-c · 2018-12-14T08:33:18Z

This PR enables strictNullChecks to breadcrumbs.ts, outlineModel.ts and breadcrumbsModel.ts as per #60565

Most of it consisted of straightforward | undefined or if (...) additions. The one tricky bit is that subclasses of TreeElement were being clever in restricting their allowed parent classes, but that could technically be incorrect (e.g. if we later added code to reparent TreeElement subtrees).

msftclas · 2018-12-14T08:33:29Z

All CLA requirements met.

jrieken · 2018-12-17T08:58:22Z

src/vs/editor/contrib/documentSymbols/outlineModel.ts

-			//
-			this.children = this._groups;
-		} else {
+		let group = first(this._groups);


why is count gone?

Good catch -- I think I misread the intent of the code and though I could get away with checking if first(this._groups) is null, but that changes functionality. I'm appending a commit to this PR to reintroduce count

jrieken · 2018-12-17T08:58:40Z

src/vs/workbench/browser/parts/editor/breadcrumbs.ts

 					readonly onDidChange = onDidChange.event;
 					getValue(overrides?: IConfigurationOverrides): T {
-						return service.getValue(name, overrides);
+						if (overrides) {


@sandy081 is that really needed?

service.getValue(name, overrides) this should be good enough. Does the TS complains when using this?

Yeah, I get

src/vs/workbench/browser/parts/editor/breadcrumbs.ts:93:37 - error TS2345: Argument of type 'IConfigurationOverrides | undefined' is not assignable to parameter of type 'IConfigurationOverrides'. Type 'undefined' is not assignable to type 'IConfigurationOverrides'. 93 return service.getValue(name, overrides); ~~~~~~~~~

I see.. looks like the API is strict here to not to accept undefined value for overrides. I can relax it if needed or you can go with above if/else branch.

@jrieken up to you.

please relax @sandy081

sandy081 · 2018-12-17T19:04:36Z

src/vs/workbench/browser/parts/editor/breadcrumbs.ts

 					readonly onDidChange = onDidChange.event;
 					getValue(overrides?: IConfigurationOverrides): T {
-						return service.getValue(name, overrides);
+						if (overrides) {


service.getValue(name, overrides) this should be good enough. Does the TS complains when using this?

jrieken · 2018-12-18T09:10:58Z

src/vs/editor/contrib/documentSymbols/outlineModel.ts

 	constructor(
 		readonly id: string,
-		public parent: OutlineModel | OutlineGroup | OutlineElement,
+		public parent: TreeElement | undefined,


please refrain from making changes aren't needed for strict null checks

I was surprised and spent quite a while trying to understand why, but this use of inheritance did cause errors under strict null checks. You can see them by checking out commit 9bbe60f (I separated the 'normal' null error fixes from the inheritance-related ones in separate commits).

src/vs/editor/contrib/documentSymbols/outlineModel.ts:90:2 - error TS2416: Property 'children' in type 'OutlineElement' is not assignable to the same property in base type 'TreeElement'. Type '{ [id: string]: OutlineElement; }' is not assignable to type '{ [id: string]: TreeElement; }'. Index signatures are incompatible. Type 'OutlineElement' is not assignable to type 'TreeElement'. Types of property 'parent' are incompatible. Type 'OutlineElement | OutlineModel | OutlineGroup' is not assignable to type 'TreeElement'. Type 'OutlineModel' is not assignable to type 'TreeElement'. Types of property 'children' are incompatible. Type '{ [id: string]: OutlineElement | OutlineGroup; }' is not assignable to type '{ [id: string]: TreeElement; }'. Index signatures are incompatible. Type 'OutlineElement | OutlineGroup' is not assignable to type 'TreeElement'. Type 'OutlineGroup' is not assignable to type 'TreeElement'. Types of property 'adopt' are incompatible. Type '(parent: OutlineModel) => OutlineGroup' is not assignable to type '(newParent: TreeElement) => TreeElement'. Types of parameters 'parent' and 'newParent' are incompatible. Type 'TreeElement' is missing the following properties from type 'OutlineModel': _groups, textModel, _compact, merge, and 5 more. 90 children: { [id: string]: OutlineElement; } = Object.create(null);

I'm not 100% confident in my understand of this error but I think the error prevents a hypothetical error that could be make like this:

class SomeNewTypeOfTreeElement { ... } const tree: TreeElement = new OutlineElement(...) tree.parent = new SomeNewTypeOfTreeElement(...) // this works because TreeElement doesn't know about OutlineElement tree.doSomethingWithParentThatOutlineElementDoesNotExpect()

There's no such code right now, of course, but the type system doesn't care. Happy to learn if there's a better way to silence these errors though.

Hm, intersting. Again, in doubt try to use the ! operator. My challenge is that it's hard to reason about these things but that I know that the code in its current shape runs. So, I'd prefer to not really change it

I understand, but this just changes the type annotation from OutlineModel | OutlineGroup | OutlineElement to their parent TreeElement, so I think it's safe? See 38d4a54

src/vs/editor/contrib/documentSymbols/outlineModel.ts

jrieken · 2018-12-18T09:12:51Z

src/vs/editor/contrib/documentSymbols/outlineModel.ts

 			let marker = markers[start];
 			myMarkers.push(marker);
-			markers[start] = undefined;
+			filteredMarkers[start] = undefined;


it's important to modify markers, don't create another another "name" for it

The issue is that markers is of type Array<IMarker> which can't accept undefined values. By the end of the function, those undefined values are removed through coalesceInPlace, but in the meantime we need to let the type system know about the temporary undefineds.

I tried changing the type of markers to Array<IMarker | undefined> instead, but then more errors get throw for uses of binarySearch, Range.areIntersecting and others in that function so I went for a new alias as the solution that requires minimal code changes, though I agree it feels like a pretty hacky solution. Alternatives could include rewriting some of the logic to be a little it more functional (e.g. use filter) so that the array doesn't contain temporary undefined values, but that requires even more logic changes.

when in doubt always use the ! operator. that makes it a lot easier for me/us to reason about these changes.

Right that makes perfect sense, I'm just not sure how to apply it in this case because the ! can't operate in a templated type (turn Array<T | undefined> into Array<T>). In this case, markers is actually both Array<T | undefined> and Array<T> at different points in the function due to mutation.

What do you think of this other idea? Use type casting: (markers as Array<IMarker | undefined>)[start] = undefined;

jrieken · 2018-12-18T09:13:57Z

src/vs/editor/contrib/documentSymbols/outlineModel.ts

-			this.children = this._groups;
-		} else {
+		let group = first(this._groups);
+		if (group && count === 1) {


Please leave the code as it was before

👍 going to use ! instead

jrieken · 2018-12-18T09:16:09Z

src/vs/workbench/browser/parts/editor/breadcrumbsModel.ts

 		}

-		const buffer = this._editor.getModel();
+		const editor = this._editor;


use ! instead

rudi-c · 2019-01-08T00:56:32Z

@jrieken Any thoughts on the comments above?

jrieken · 2019-01-11T08:55:48Z

Sounds reasonable. I am in favour of adding casts et al because it make it simpler to reason about the changes for now. Please rebase and then I can merge

rudi-c · 2019-01-23T17:12:17Z

@jrieken It's done -- apologies for the delay!

jrieken · 2019-01-23T19:55:06Z

Thanks. Merging!

rudi-c force-pushed the snc-1 branch from 65f0af5 to 17c82ea Compare December 14, 2018 08:34

rudi-c added 4 commits December 14, 2018 10:44

strickNullChecks breadcrumbs.ts

c14ac10

strictNullCheck outlineModel.ts except class assignment error

9bbe60f

strictNullCheck outlineModel.ts inheritance errors

38d4a54

strictNullChecks breadcrumbsModel

e48c4a2

rudi-c force-pushed the snc-1 branch from 17c82ea to e48c4a2 Compare December 14, 2018 18:48

mjbvz assigned jrieken Dec 14, 2018

rudi-c changed the title ~~Enables strictNullChecks to breadcrumbs.ts, outlineModel.ts, breadcrumbbsModel.ts~~ Enables strictNullChecks to breadcrumbs.ts, outlineModel.ts, breadcrumbsModel.ts Dec 14, 2018

jrieken requested changes Dec 17, 2018

View reviewed changes

sandy081 approved these changes Dec 17, 2018

View reviewed changes

Review: reintroduce count

e69d6ce

jrieken requested changes Dec 18, 2018

View reviewed changes

rudi-c added 2 commits December 18, 2018 12:57

Review round 2

3b11ac4

Put comment back in place

760081d

jrieken added the engineering VS Code - Build / issue tracking / etc. label Jan 8, 2019

rudi-c added 3 commits January 23, 2019 00:02

Review: cast instead of creating new variable

beb8c6f

Merge remote-tracking branch 'refs/remotes/origin/master' into snc-1

77710c1

strictNullCheck merge conflicts

06e4e22

jrieken added this to the December/January 2019 milestone Jan 23, 2019

jrieken merged commit 9526cba into microsoft:master Jan 23, 2019

rudi-c deleted the snc-1 branch January 26, 2019 07:39

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Conversation

rudi-c commented Dec 14, 2018

Uh oh!

msftclas commented Dec 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandy081 Dec 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rudi-c commented Jan 8, 2019

Uh oh!

jrieken commented Jan 11, 2019

Uh oh!

rudi-c commented Jan 23, 2019

Uh oh!

jrieken commented Jan 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

msftclas commented Dec 14, 2018 •

edited

Loading

sandy081 Dec 18, 2018 •

edited

Loading