Enables strictNullChecks to breadcrumbs.ts, outlineModel.ts, breadcrumbsModel.ts by rudi-c · Pull Request #65062 · microsoft/vscode

Rudi Chen (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).

Microsoft Contribution License Agreements (msftclas) · 2018-12-14T08:33:29Z

All CLA requirements met.

Johannes Rieken (jrieken) · 2018-12-17T08:58:22Z

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

Johannes Rieken (jrieken) · 2018-12-17T08:58:40Z

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


Sandeep Somavarapu (@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.

Johannes Rieken (@jrieken) up to you.

please relax Sandeep Somavarapu (@sandy081)

Sandeep Somavarapu (sandy081) · 2018-12-17T19:04:36Z

 					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?

Johannes Rieken (jrieken) · 2018-12-18T09:10:58Z

 	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

Johannes Rieken (jrieken) · 2018-12-18T09:12:51Z

 			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;

Johannes Rieken (jrieken) · 2018-12-18T09:13:57Z

-			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

Johannes Rieken (jrieken) · 2018-12-18T09:16:09Z

 		}

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


use ! instead

Rudi Chen (rudi-c) · 2019-01-08T00:56:32Z

Johannes Rieken (@jrieken) Any thoughts on the comments above?

Johannes Rieken (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 Chen (rudi-c) · 2019-01-23T17:12:17Z

Johannes Rieken (@jrieken) It's done -- apologies for the delay!

Johannes Rieken (jrieken) · 2019-01-23T19:55:06Z

Thanks. Merging!

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

Rudi Chen (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 Chen (rudi-c) force-pushed the snc-1 branch from 17c82ea to e48c4a2 Compare December 14, 2018 18:48

Matt Bierner (mjbvz) assigned Johannes Rieken (jrieken) Dec 14, 2018

Rudi Chen (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

Johannes Rieken (jrieken) suggested changes Dec 17, 2018

View reviewed changes

Sandeep Somavarapu (sandy081) approved these changes Dec 17, 2018

View reviewed changes

Review: reintroduce count

e69d6ce

Johannes Rieken (jrieken) suggested changes Dec 18, 2018

View reviewed changes

Rudi Chen (rudi-c) added 2 commits December 18, 2018 12:57

Review round 2

3b11ac4

Put comment back in place

760081d

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

Rudi Chen (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

Johannes Rieken (jrieken) added this to the December/January 2019 milestone Jan 23, 2019

Johannes Rieken (jrieken) merged commit 9526cba into microsoft:master Jan 23, 2019

Rudi Chen (rudi-c) deleted the snc-1 branch January 26, 2019 07:39

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

Uh oh!

Conversation

Rudi Chen (rudi-c) commented Dec 14, 2018

Uh oh!

Microsoft Contribution License Agreements (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!

Sandeep Somavarapu (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 Chen (rudi-c) commented Jan 8, 2019

Uh oh!

Johannes Rieken (jrieken) commented Jan 11, 2019

Uh oh!

Rudi Chen (rudi-c) commented Jan 23, 2019

Uh oh!

Johannes Rieken (jrieken) commented Jan 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Microsoft Contribution License Agreements (msftclas) commented Dec 14, 2018 •

edited

Loading

Sandeep Somavarapu (sandy081) Dec 18, 2018 •

edited

Loading