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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c14ac10
strickNullChecks breadcrumbs.ts
rudi-c Dec 14, 2018
9bbe60f
strictNullCheck outlineModel.ts except class assignment error
rudi-c Dec 14, 2018
38d4a54
strictNullCheck outlineModel.ts inheritance errors
rudi-c Dec 14, 2018
e48c4a2
strictNullChecks breadcrumbsModel
rudi-c Dec 14, 2018
e69d6ce
Review: reintroduce count
rudi-c Dec 18, 2018
3b11ac4
Review round 2
rudi-c Dec 18, 2018
760081d
Put comment back in place
rudi-c Dec 20, 2018
beb8c6f
Review: cast instead of creating new variable
rudi-c Jan 23, 2019
77710c1
Merge remote-tracking branch 'refs/remotes/origin/master' into snc-1
rudi-c Jan 23, 2019
06e4e22
strictNullCheck merge conflicts
rudi-c Jan 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/tsconfig.strictNullChecks.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@
"./vs/editor/contrib/comment/test/blockCommentCommand.test.ts",
"./vs/editor/contrib/contextmenu/contextmenu.ts",
"./vs/editor/contrib/cursorUndo/cursorUndo.ts",
"./vs/editor/contrib/documentSymbols/outlineModel.ts",
"./vs/editor/contrib/dnd/dnd.ts",
"./vs/editor/contrib/dnd/dragAndDropCommand.ts",
"./vs/editor/contrib/find/findController.ts",
Expand Down Expand Up @@ -526,6 +527,8 @@
"./vs/workbench/browser/composite.ts",
"./vs/workbench/browser/panel.ts",
"./vs/workbench/browser/part.ts",
"./vs/workbench/browser/parts/editor/breadcrumbs.ts",
"./vs/workbench/browser/parts/editor/breadcrumbsModel.ts",
"./vs/workbench/browser/parts/editor/editorWidgets.ts",
"./vs/workbench/browser/parts/editor/rangeDecorations.ts",
"./vs/workbench/browser/parts/notifications/notificationsAlerts.ts",
Expand Down
77 changes: 38 additions & 39 deletions src/vs/editor/contrib/documentSymbols/outlineModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,14 @@ export abstract class TreeElement {

abstract id: string;
abstract children: { [id: string]: TreeElement };
abstract parent: TreeElement;
abstract parent: TreeElement | undefined;

abstract adopt(newParent: TreeElement): TreeElement;

remove(): void {
delete this.parent.children[this.id];
if (this.parent) {
delete this.parent.children[this.id];
}
}

static findId(candidate: DocumentSymbol | string, container: TreeElement): string {
Expand All @@ -49,7 +51,7 @@ export abstract class TreeElement {
return id;
}

static getElementById(id: string, element: TreeElement): TreeElement {
static getElementById(id: string, element: TreeElement): TreeElement | undefined {
if (!id) {
return undefined;
}
Expand Down Expand Up @@ -88,18 +90,18 @@ export abstract class TreeElement {
export class OutlineElement extends TreeElement {

children: { [id: string]: OutlineElement; } = Object.create(null);
score: FuzzyScore = FuzzyScore.Default;
marker: { count: number, topSev: MarkerSeverity };
score: FuzzyScore | undefined = FuzzyScore.Default;
marker: { count: number, topSev: MarkerSeverity } | undefined;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

readonly symbol: DocumentSymbol
) {
super();
}

adopt(parent: OutlineModel | OutlineGroup | OutlineElement): OutlineElement {
adopt(parent: TreeElement): OutlineElement {
let res = new OutlineElement(this.id, parent, this.symbol);
forEach(this.children, entry => res.children[entry.key] = entry.value.adopt(res));
return res;
Expand All @@ -112,33 +114,33 @@ export class OutlineGroup extends TreeElement {

constructor(
readonly id: string,
public parent: OutlineModel,
public parent: TreeElement | undefined,
readonly provider: DocumentSymbolProvider,
readonly providerIndex: number,
) {
super();
}

adopt(parent: OutlineModel): OutlineGroup {
adopt(parent: TreeElement): OutlineGroup {
let res = new OutlineGroup(this.id, parent, this.provider, this.providerIndex);
forEach(this.children, entry => res.children[entry.key] = entry.value.adopt(res));
return res;
}

updateMatches(pattern: string, topMatch: OutlineElement): OutlineElement {
updateMatches(pattern: string, topMatch: OutlineElement | undefined): OutlineElement | undefined {
for (const key in this.children) {
topMatch = this._updateMatches(pattern, this.children[key], topMatch);
}
return topMatch;
}

private _updateMatches(pattern: string, item: OutlineElement, topMatch: OutlineElement): OutlineElement {
private _updateMatches(pattern: string, item: OutlineElement, topMatch: OutlineElement | undefined): OutlineElement | undefined {

item.score = pattern
? fuzzyScore(pattern, pattern.toLowerCase(), 0, item.symbol.name, item.symbol.name.toLowerCase(), 0, true)
: FuzzyScore.Default;

if (item.score && (!topMatch || item.score[0] > topMatch.score[0])) {
if (item.score && (!topMatch || !topMatch.score || item.score[0] > topMatch.score[0])) {
topMatch = item;
}
for (const key in item.children) {
Expand All @@ -152,11 +154,11 @@ export class OutlineGroup extends TreeElement {
return topMatch;
}

getItemEnclosingPosition(position: IPosition): OutlineElement {
getItemEnclosingPosition(position: IPosition): OutlineElement | undefined {
return position ? this._getItemEnclosingPosition(position, this.children) : undefined;
}

private _getItemEnclosingPosition(position: IPosition, children: { [id: string]: OutlineElement }): OutlineElement {
private _getItemEnclosingPosition(position: IPosition, children: { [id: string]: OutlineElement }): OutlineElement | undefined {
for (let key in children) {
let item = children[key];
if (!item.symbol.range || !Range.containsPosition(item.symbol.range, position)) {
Expand All @@ -174,6 +176,7 @@ export class OutlineGroup extends TreeElement {
}

private _updateMarker(markers: IMarker[], item: OutlineElement): void {
let filteredMarkers: Array<IMarker | undefined> = markers;

item.marker = undefined;

Expand All @@ -190,14 +193,14 @@ export class OutlineGroup extends TreeElement {
}

let myMarkers: IMarker[] = [];
let myTopSev: MarkerSeverity;
let myTopSev: MarkerSeverity | undefined = undefined;
jrieken marked this conversation as resolved.
Show resolved Hide resolved

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;

if (!myTopSev || marker.severity > myTopSev) {
myTopSev = marker.severity;
}
Expand All @@ -218,13 +221,13 @@ export class OutlineGroup extends TreeElement {
};
}

coalesceInPlace(markers);
coalesceInPlace(filteredMarkers);
}
}

export class OutlineModel extends TreeElement {

private static readonly _requests = new LRUCache<string, { promiseCnt: number, source: CancellationTokenSource, promise: Promise<any>, model: OutlineModel }>(9, .75);
private static readonly _requests = new LRUCache<string, { promiseCnt: number, source: CancellationTokenSource, promise: Promise<any>, model: OutlineModel | undefined }>(9, .75);
private static readonly _keys = new class {

private _counter = 1;
Expand Down Expand Up @@ -265,25 +268,25 @@ export class OutlineModel extends TreeElement {
OutlineModel._requests.set(key, data);
}

if (data.model) {
if (data!.model) {
// resolved -> return data
return Promise.resolve(data.model);
}

// increase usage counter
data.promiseCnt += 1;
data!.promiseCnt += 1;

token.onCancellationRequested(() => {
// last -> cancel provider request, remove cached promise
if (--data.promiseCnt === 0) {
data.source.cancel();
if (--data!.promiseCnt === 0) {
data!.source.cancel();
OutlineModel._requests.delete(key);
}
});

return new Promise((resolve, reject) => {
data.promise.then(model => {
data.model = model;
data!.promise.then(model => {
data!.model = model;
resolve(model);
}, err => {
OutlineModel._requests.delete(key);
Expand Down Expand Up @@ -331,7 +334,7 @@ export class OutlineModel extends TreeElement {
container.children[res.id] = res;
}

static get(element: TreeElement): OutlineModel {
static get(element: TreeElement | undefined): OutlineModel | undefined {
while (element) {
if (element instanceof OutlineModel) {
return element;
Expand All @@ -358,26 +361,22 @@ export class OutlineModel extends TreeElement {
}

private _compact(): this {
let count = 0;
for (const key in this._groups) {
let group = this._groups[key];
if (first(group.children) === undefined) { // empty
delete this._groups[key];
} else {
count += 1;
}
}
if (count !== 1) {
//
this.children = this._groups;
} else {
let group = first(this._groups);
Copy link
Member

Choose a reason for hiding this comment

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

why is count gone?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (group) {
// adopt all elements of the first group
let group = first(this._groups);
for (let key in group.children) {
let child = group.children[key];
child.parent = this;
this.children[child.id] = child;
}
} else {
this.children = this._groups;
}
return this;
}
Expand All @@ -394,23 +393,23 @@ export class OutlineModel extends TreeElement {
return true;
}

private _matches: [string, OutlineElement];
private _matches: [string, OutlineElement | undefined];

updateMatches(pattern: string): OutlineElement {
updateMatches(pattern: string): OutlineElement | undefined {
if (this._matches && this._matches[0] === pattern) {
return this._matches[1];
}
let topMatch: OutlineElement;
let topMatch: OutlineElement | undefined;
for (const key in this._groups) {
topMatch = this._groups[key].updateMatches(pattern, topMatch);
}
this._matches = [pattern, topMatch];
return topMatch;
}

getItemEnclosingPosition(position: IPosition, context?: OutlineElement): OutlineElement {
getItemEnclosingPosition(position: IPosition, context?: OutlineElement): OutlineElement | undefined {

let preferredGroup: OutlineGroup;
let preferredGroup: OutlineGroup | undefined;
if (context) {
let candidate = context.parent;
while (candidate && !preferredGroup) {
Expand All @@ -432,7 +431,7 @@ export class OutlineModel extends TreeElement {
return result;
}

getItemById(id: string): TreeElement {
getItemById(id: string): TreeElement | undefined {
return TreeElement.getElementById(id, this);
}

Expand Down
12 changes: 10 additions & 2 deletions src/vs/workbench/browser/parts/editor/breadcrumbs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

@sandy081 is that really needed?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@sandy081 sandy081 Dec 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

please relax @sandy081

return service.getValue(name, overrides);
} else {
return service.getValue(name);
}
}
updateValue(newValue: T, overrides?: IConfigurationOverrides): Promise<void> {
return service.updateValue(name, newValue, overrides);
if (overrides) {
return service.updateValue(name, newValue, overrides);
} else {
return service.updateValue(name, newValue);
}
}
dispose(): void {
listener.dispose();
Expand Down
36 changes: 21 additions & 15 deletions src/vs/workbench/browser/parts/editor/breadcrumbsModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export class FileElement {

export type BreadcrumbElement = FileElement | OutlineModel | OutlineGroup | OutlineElement;

type FileInfo = { path: FileElement[], folder: IWorkspaceFolder };
type FileInfo = { path: FileElement[], folder?: IWorkspaceFolder };

export class EditorBreadcrumbsModel {

Expand Down Expand Up @@ -105,16 +105,17 @@ export class EditorBreadcrumbsModel {
}

let info: FileInfo = {
folder: workspaceService.getWorkspaceFolder(uri),
folder: workspaceService.getWorkspaceFolder(uri) || undefined,
path: []
};

while (uri.path !== '/') {
if (info.folder && isEqual(info.folder.uri, uri)) {
let uriPrefix: URI | null = uri;
while (uriPrefix && uriPrefix.path !== '/') {
if (info.folder && isEqual(info.folder.uri, uriPrefix)) {
break;
}
info.path.unshift(new FileElement(uri, info.path.length === 0 ? FileKind.FILE : FileKind.FOLDER));
uri = dirname(uri);
info.path.unshift(new FileElement(uriPrefix, info.path.length === 0 ? FileKind.FILE : FileKind.FOLDER));
uriPrefix = dirname(uriPrefix);
}

if (info.folder && workspaceService.getWorkbenchState() === WorkbenchState.WORKSPACE) {
Expand Down Expand Up @@ -145,7 +146,12 @@ export class EditorBreadcrumbsModel {
this._updateOutlineElements([]);
}

const buffer = this._editor.getModel();
const editor = this._editor;
Copy link
Member

Choose a reason for hiding this comment

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

use ! instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

if (!editor) {
return;
}

const buffer = editor.getModel();
if (!buffer || !DocumentSymbolProviderRegistry.has(buffer) || !isEqual(buffer.uri, this._uri)) {
return;
}
Expand All @@ -171,11 +177,11 @@ export class EditorBreadcrumbsModel {
// copy the model
model = model.adopt();

this._updateOutlineElements(this._getOutlineElements(model, this._editor.getPosition()));
this._outlineDisposables.push(this._editor.onDidChangeCursorPosition(_ => {
this._updateOutlineElements(this._getOutlineElements(model, editor.getPosition()));
this._outlineDisposables.push(editor.onDidChangeCursorPosition(_ => {
timeout.cancelAndSet(() => {
if (!buffer.isDisposed() && versionIdThen === buffer.getVersionId() && this._editor.getModel()) {
this._updateOutlineElements(this._getOutlineElements(model, this._editor.getPosition()));
if (!buffer.isDisposed() && versionIdThen === buffer.getVersionId() && editor.getModel()) {
this._updateOutlineElements(this._getOutlineElements(model, editor.getPosition()));
}
}, 150);
}));
Expand All @@ -186,11 +192,11 @@ export class EditorBreadcrumbsModel {
});
}

private _getOutlineElements(model: OutlineModel, position: IPosition): Array<OutlineModel | OutlineGroup | OutlineElement> {
if (!model) {
private _getOutlineElements(model: OutlineModel, position: IPosition | null): Array<OutlineModel | OutlineGroup | OutlineElement> {
if (!model || !position) {
return [];
}
let item: OutlineGroup | OutlineElement = model.getItemEnclosingPosition(position);
let item: OutlineGroup | OutlineElement | undefined = model.getItemEnclosingPosition(position);
if (!item) {
return [model];
}
Expand All @@ -201,7 +207,7 @@ export class EditorBreadcrumbsModel {
if (parent instanceof OutlineModel) {
break;
}
if (parent instanceof OutlineGroup && size(parent.parent.children) === 1) {
if (parent instanceof OutlineGroup && parent.parent && size(parent.parent.children) === 1) {
break;
}
item = parent;
Expand Down