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

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

Merged
merged 10 commits into from Jan 23, 2019

Conversation

Projects
None yet
4 participants
@rudi-c
Copy link
Contributor

rudi-c commented Dec 14, 2018

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

This comment has been minimized.

Copy link

msftclas commented Dec 14, 2018

CLA assistant check
All CLA requirements met.

@rudi-c rudi-c force-pushed the rudi-c:snc-1 branch from 65f0af5 to 17c82ea Dec 14, 2018

@rudi-c rudi-c force-pushed the rudi-c:snc-1 branch from 17c82ea to e48c4a2 Dec 14, 2018

@rudi-c 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

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

This comment has been minimized.

@jrieken

jrieken Dec 17, 2018

Member

why is count gone?

This comment has been minimized.

@rudi-c

rudi-c Dec 18, 2018

Author Contributor

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

@@ -90,10 +90,18 @@ export abstract class BreadcrumbsConfig<T> {
readonly name = name;
readonly onDidChange = onDidChange.event;
getValue(overrides?: IConfigurationOverrides): T {
return service.getValue(name, overrides);
if (overrides) {

This comment has been minimized.

@jrieken

jrieken Dec 17, 2018

Member

@sandy081 is that really needed?

This comment has been minimized.

@sandy081

sandy081 Dec 17, 2018

Member

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

This comment has been minimized.

@rudi-c

rudi-c Dec 18, 2018

Author Contributor

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);
    					                              ~~~~~~~~~

This comment has been minimized.

@sandy081

sandy081 Dec 18, 2018

Member

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.

This comment has been minimized.

@jrieken

jrieken Dec 19, 2018

Member

please relax @sandy081

@@ -90,10 +90,18 @@ export abstract class BreadcrumbsConfig<T> {
readonly name = name;
readonly onDidChange = onDidChange.event;
getValue(overrides?: IConfigurationOverrides): T {
return service.getValue(name, overrides);
if (overrides) {

This comment has been minimized.

@sandy081

sandy081 Dec 17, 2018

Member

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


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

This comment has been minimized.

@jrieken

jrieken Dec 18, 2018

Member

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

This comment has been minimized.

@rudi-c

rudi-c Dec 18, 2018

Author Contributor

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.

This comment has been minimized.

@jrieken

jrieken Dec 19, 2018

Member

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

This comment has been minimized.

@rudi-c

rudi-c Dec 20, 2018

Author Contributor

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

Show resolved Hide resolved src/vs/editor/contrib/documentSymbols/outlineModel.ts Outdated

for (; start < markers.length && Range.areIntersecting(item.symbol.range, markers[start]); start++) {
// remove markers intersecting with this outline element
// and store them in a 'private' array.
let marker = markers[start];
myMarkers.push(marker);
markers[start] = undefined;
filteredMarkers[start] = undefined;

This comment has been minimized.

@jrieken

jrieken Dec 18, 2018

Member

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

This comment has been minimized.

@rudi-c

rudi-c Dec 18, 2018

Author Contributor

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.

This comment has been minimized.

@jrieken

jrieken Dec 19, 2018

Member

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

This comment has been minimized.

@rudi-c

rudi-c Dec 20, 2018

Author Contributor

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;

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

This comment has been minimized.

@jrieken

jrieken Dec 18, 2018

Member

Please leave the code as it was before

This comment has been minimized.

@rudi-c

rudi-c Dec 18, 2018

Author Contributor

👍 going to use ! instead

@@ -145,7 +146,12 @@ export class EditorBreadcrumbsModel {
this._updateOutlineElements([]);
}

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

This comment has been minimized.

@jrieken

jrieken Dec 18, 2018

Member

use ! instead

This comment has been minimized.

@rudi-c

rudi-c Dec 18, 2018

Author Contributor

👍

rudi-c added some commits Dec 18, 2018

@rudi-c

This comment has been minimized.

Copy link
Contributor Author

rudi-c commented Jan 8, 2019

@jrieken Any thoughts on the comments above?

@jrieken jrieken added the engineering label Jan 8, 2019

@jrieken

This comment has been minimized.

Copy link
Member

jrieken commented Jan 11, 2019

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

This comment has been minimized.

Copy link
Contributor Author

rudi-c commented Jan 23, 2019

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

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

@jrieken

This comment has been minimized.

Copy link
Member

jrieken commented Jan 23, 2019

Thanks. Merging!

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

2 checks passed

VS Code #20190123.13 succeeded
Details
license/cla All CLA requirements met.
Details

@rudi-c rudi-c deleted the rudi-c:snc-1 branch Jan 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment