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

Revisit and remove unused code #38414

Closed
jrieken opened this Issue Nov 15, 2017 · 19 comments

Comments

Projects
None yet
8 participants
@jrieken
Member

jrieken commented Nov 15, 2017

We should revisit our source code and remove things we don't use. I have used the https://github.com/jrieken/ts-unused to find symbols we don't use. Take its result with a grain of salt, the results are based on calling reference search and that means some structural typing tricks aren't seen as usage/reference.

out2.txt, out3.txt, out4.txt, out5.txt, out6.txt,
out7.txt

cc @Microsoft/vscode

Found 128 unused symbols with potential for saving 1185 lines of unused code
src/typings/windows-mutex.ts|isActive:9 -> potentially save ~1 lines
src/vs/base/common/errors.ts|getUnexpectedErrorHandler:114 -> potentially save ~3 lines
src/vs/base/common/event.ts|toPromise:254 -> potentially save ~8 lines
src/vs/base/browser/dom.ts|getContentWidth:593 -> potentially save ~5 lines
src/vs/base/browser/builder.ts|domSelect:442 -> potentially save ~11 lines
src/vs/base/browser/builder.ts|name:666 -> potentially save ~5 lines
src/vs/base/browser/builder.ts|parent:1290 -> potentially save ~5 lines
src/vs/base/common/htmlContent.ts|appendMarkdown:30 -> potentially save ~4 lines
src/vs/base/common/color.ts|getContrastRatio:322 -> potentially save ~5 lines
src/vs/base/common/color.ts|isDarker:332 -> potentially save ~4 lines
src/vs/base/common/color.ts|blend:383 -> potentially save ~18 lines
src/vs/base/common/color.ts|green:432 -> potentially save ~1 lines
src/vs/base/common/color.ts|formatRGB:442 -> potentially save ~7 lines
src/vs/base/common/color.ts|formatHSL:454 -> potentially save ~7 lines
src/vs/base/browser/ui/selectBox/selectBox.ts|style:122 -> potentially save ~7 lines
src/vs/base/browser/ui/actionbar/actionbar.ts|blur:171 -> potentially save ~5 lines
src/vs/base/browser/ui/actionbar/actionbar.ts|blur:791 -> potentially save ~5 lines
src/vs/base/browser/ui/button/button.ts|style:103 -> potentially save ~8 lines
src/vs/base/browser/ui/countBadge/countBadge.ts|style:72 -> potentially save ~7 lines
src/vs/base/browser/ui/list/list.ts|IListElementEvent:18 -> potentially save ~5 lines
src/vs/base/browser/ui/list/listWidget.ts|isDOMFocused:901 -> potentially save ~3 lines
src/vs/base/browser/ui/list/listPaging.ts|selectNext:121 -> potentially save ~3 lines
src/vs/base/browser/ui/list/listPaging.ts|selectPrevious:125 -> potentially save ~3 lines
src/vs/base/common/events.ts|PropertyChangeEvent:19 -> potentially save ~13 lines
src/vs/base/common/events.ts|ISelectionEvent:43 -> potentially save ~5 lines
src/vs/base/common/events.ts|IFocusEvent:49 -> potentially save ~5 lines
src/vs/base/common/events.ts|IHighlightEvent:55 -> potentially save ~5 lines
src/vs/base/common/graph.ts|traverse:42 -> potentially save ~7 lines
src/vs/base/common/jsonEdit.ts|removeProperty:10 -> potentially save ~3 lines
src/vs/base/common/parsers.ts|fatal:79 -> potentially save ~3 lines
src/vs/base/common/winjs.polyfill.promise.ts|all:15 -> potentially save ~11 lines
src/vs/base/common/winjs.polyfill.promise.ts|race:27 -> potentially save ~5 lines
src/vs/base/common/winjs.polyfill.promise.ts|reject:37 -> potentially save ~3 lines
src/vs/base/common/diff/diff2.ts|LcsDiff2:52 -> potentially save ~274 lines
src/vs/base/common/diff/diff2.ts|ComputeDiff:133 -> potentially save ~36 lines
src/vs/base/node/config.ts|getValue:205 -> potentially save ~11 lines
src/vs/base/node/id.ts|value:54 -> potentially save ~25 lines
src/vs/base/parts/tree/browser/treeModel.ts|IItemRefreshEvent:225 -> potentially save ~1 lines
src/vs/base/parts/tree/browser/treeModel.ts|render:719 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeModel.ts|onDidDisposeItem:918 -> potentially save ~1 lines
src/vs/base/parts/tree/browser/treeModel.ts|isHighlighted:1137 -> potentially save ~9 lines
src/vs/base/parts/tree/browser/tree.ts|IElementCallback:628 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/tree.ts|ICallback:632 -> potentially save ~1 lines
src/vs/base/parts/tree/browser/treeDefaults.ts|DefaultSorter:405 -> potentially save ~6 lines
src/vs/base/parts/tree/browser/treeViewModel.ts|itemsCount:177 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeViewModel.ts|withItemsInRange:185 -> potentially save ~7 lines
src/vs/base/parts/tree/browser/treeImpl.ts|onDidDispose:83 -> potentially save ~1 lines
src/vs/base/parts/tree/browser/treeImpl.ts|toggleExpansionAll:185 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|isHighlighted:226 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|select:234 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|selectRange:238 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|selectAll:246 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|deselectAll:254 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|isSelected:266 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|selectNext:278 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|selectPrevious:282 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|selectParent:286 -> potentially save ~3 lines
src/vs/base/parts/tree/browser/treeImpl.ts|toggleTrait:350 -> potentially save ~4 lines
src/vs/base/parts/tree/browser/treeImpl.ts|hasTrait:355 -> potentially save ~3 lines
src/vs/base/parts/quickopen/browser/quickOpenWidget.ts|style:349 -> potentially save ~5 lines
src/vs/platform/contextkey/common/contextkey.ts|getValue:431 -> potentially save ~3 lines
src/vs/platform/update/common/update.ts|ExplicitState:20 -> potentially save ~4 lines
src/vs/platform/request/electron-main/requestService.ts|getRawRequest:13 -> potentially save ~3 lines
src/vs/code/node/cliProcessMain.ts|main:176 -> potentially save ~48 lines
src/vs/editor/browser/viewParts/overviewRuler/overviewRulerImpl.ts|getLanesCount:75 -> potentially save ~3 lines
src/vs/editor/browser/viewParts/overviewRuler/overviewRulerImpl.ts|setLanesCount:79 -> potentially save ~7 lines
src/vs/editor/browser/viewParts/overviewRuler/overviewRulerImpl.ts|setThemeType:87 -> potentially save ~7 lines
src/vs/editor/contrib/find/findWidget.ts|focus:950 -> potentially save ~3 lines
src/vs/editor/contrib/find/findController.ts|getHistory:178 -> potentially save ~3 lines
src/vs/editor/contrib/folding/foldingRanges.ts|hidesLine:170 -> potentially save ~3 lines
src/vs/editor/contrib/referenceSearch/referencesModel.ts|nextReference:256 -> potentially save ~15 lines
src/vs/editor/common/model/editableTextModel.ts|setEditableRange:708 -> potentially save ~21 lines
src/vs/platform/opener/common/opener.ts|open:29 -> potentially save ~1 lines
src/vs/editor/contrib/colorPicker/colorPickerWidget.ts|getId:350 -> potentially save ~3 lines
src/vs/editor/browser/services/bulkEdit.ts|progress:304 -> potentially save ~3 lines
src/vs/editor/browser/services/bulkEdit.ts|add:308 -> potentially save ~3 lines
src/vs/editor/browser/services/bulkEdit.ts|finish:328 -> potentially save ~35 lines
src/vs/editor/browser/services/bulkEdit.ts|ariaMessage:364 -> potentially save ~11 lines
src/vs/platform/contextview/browser/contextMenuService.ts|setContainer:25 -> potentially save ~3 lines
src/vs/platform/contextview/browser/contextViewService.ts|setContainer:31 -> potentially save ~3 lines
src/vs/editor/common/modes/supports/tokenization.ts|getThemeTrieElement:205 -> potentially save ~3 lines
src/vs/editor/standalone/browser/quickOpen/quickOpenEditorWidget.ts|isVisible:77 -> potentially save ~3 lines
src/vs/editor/standalone/browser/quickOpen/quickOpenEditorWidget.ts|hide:93 -> potentially save ~5 lines
src/vs/workbench/common/editor.ts|hasOptionsDefined:645 -> potentially save ~3 lines
src/vs/workbench/services/textfile/common/textFileEditorModel.ts|getLastSaveAttemptTime:864 -> potentially save ~3 lines
src/vs/workbench/services/textfile/common/textFileEditorModel.ts|getStat:978 -> potentially save ~3 lines
src/vs/workbench/parts/files/common/editors/fileEditorInput.ts|getPreferredEncoding:97 -> potentially save ~3 lines
src/vs/workbench/browser/editor.ts|getName:78 -> potentially save ~3 lines
src/vs/workbench/browser/editor.ts|getEditors:176 -> potentially save ~3 lines
src/vs/workbench/browser/editor.ts|setEditors:180 -> potentially save ~3 lines
src/vs/workbench/browser/editor.ts|getEditorInputs:184 -> potentially save ~10 lines
src/vs/workbench/parts/files/electron-browser/views/explorerDecorationsProvider.ts|label:17 -> potentially save ~1 lines
src/vs/workbench/common/editor/editorStacksModel.ts|unpin:481 -> potentially save ~20 lines
src/vs/workbench/common/editor/editorStacksModel.ts|getGroup:831 -> potentially save ~5 lines
src/vs/workbench/common/editor/editorStacksModel.ts|closeGroups:921 -> potentially save ~11 lines
src/vs/workbench/browser/parts/editor/editorActions.ts|LABEL:1243 -> potentially save ~1 lines
src/vs/workbench/parts/preferences/common/preferencesModels.ts|findValueMatches:116 -> potentially save ~1 lines
src/vs/workbench/parts/preferences/common/preferencesModels.ts|save:166 -> potentially save ~3 lines
src/vs/workbench/parts/preferences/common/preferencesModels.ts|WorkspaceConfigModel:370 -> potentially save ~137 lines
src/vs/workbench/parts/preferences/browser/preferencesRenderers.ts|collapseAll:366 -> potentially save ~3 lines
src/vs/workbench/parts/preferences/electron-browser/preferencesSearch.ts|filterPreferences:93 -> potentially save ~22 lines
src/vs/workbench/parts/search/common/searchModel.ts|replace:566 -> potentially save ~3 lines
src/vs/workbench/parts/search/common/searchModel.ts|folderCount:610 -> potentially save ~3 lines
src/vs/workbench/parts/search/browser/searchActions.ts|LABEL:191 -> potentially save ~1 lines
src/vs/workbench/parts/search/browser/searchActions.ts|LABEL:206 -> potentially save ~1 lines
src/vs/workbench/services/scm/common/scm.ts|IBaselineResourceProvider:17 -> potentially save ~3 lines
src/vs/workbench/parts/scm/electron-browser/scm.contribution.ts|ID:25 -> potentially save ~1 lines
src/vs/workbench/parts/scm/electron-browser/scm.contribution.ts|LABEL:26 -> potentially save ~1 lines
src/vs/workbench/parts/debug/browser/debugActionItems.ts|blur:141 -> potentially save ~3 lines
src/vs/workbench/parts/markers/common/markersModel.ts|getStatisticsLabel:313 -> potentially save ~9 lines
src/vs/workbench/parts/markers/browser/markersFileDecorations.ts|label:24 -> potentially save ~1 lines
src/vs/workbench/parts/html/browser/webview.ts|activateSelection:32 -> potentially save ~1 lines
src/vs/workbench/parts/html/browser/webviewEditor.ts|HtmlPreviewEditorViewState:21 -> potentially save ~3 lines
src/vs/workbench/parts/extensions/electron-browser/extensionsActions.ts|isEnabled:51 -> potentially save ~3 lines
src/vs/workbench/parts/extensions/browser/extensionEditor.ts|hideFind:382 -> potentially save ~5 lines
src/vs/workbench/parts/output/common/outputLinkComputer.ts|create:180 -> potentially save ~3 lines
src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts|registerLinkMatcher:383 -> potentially save ~3 lines
src/vs/workbench/parts/terminal/electron-browser/terminalInstance.ts|onData:826 -> potentially save ~15 lines
src/vs/workbench/browser/parts/editor/editorCommands.ts|handler:173 -> potentially save ~3 lines
src/vs/workbench/browser/parts/editor/editorCommands.ts|handler:183 -> potentially save ~3 lines
src/vs/workbench/browser/parts/editor/editorCommands.ts|handler:203 -> potentially save ~14 lines
src/vs/workbench/services/textfile/electron-browser/textFileService.ts|resolveTextContent:55 -> potentially save ~16 lines
src/vs/workbench/parts/update/electron-browser/releaseNotesEditor.ts|getViewState:141 -> potentially save ~5 lines
src/vs/workbench/services/thread/common/threadService.ts|_suppressCompilerUnusedWarning:26 -> potentially save ~1 lines
src/vs/workbench/electron-browser/commands.ts|handler:376 -> potentially save ~4 lines
src/vs/workbench/electron-browser/commands.ts|handler:387 -> potentially save ~4 lines
src/vs/workbench/electron-browser/main.ts|startup:48 -> potentially save ~21 lines
src/vs/workbench/services/search/node/fileSearch.ts|readStdout:365 -> potentially save ~14 lines

@jrieken jrieken self-assigned this Nov 15, 2017

jrieken added a commit that referenced this issue Nov 15, 2017

jrieken added a commit that referenced this issue Nov 15, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 15, 2017

Contributor

Thanks for this. Done.

Contributor

isidorn commented Nov 15, 2017

Thanks for this. Done.

isidorn added a commit that referenced this issue Nov 15, 2017

@bpasero bpasero self-assigned this Nov 15, 2017

@bpasero bpasero added this to the November 2017 milestone Nov 15, 2017

isidorn added a commit that referenced this issue Nov 15, 2017

isidorn added a commit that referenced this issue Nov 15, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 15, 2017

Contributor

Also it would be great if you could re-generate this list in like a week, so I can do another pass to check if I missed something.

Contributor

isidorn commented Nov 15, 2017

Also it would be great if you could re-generate this list in like a week, so I can do another pass to check if I missed something.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 15, 2017

Member

Sure. Tho it takes roughly 90mins... I'll update later today or tomorrow

Member

jrieken commented Nov 15, 2017

Sure. Tho it takes roughly 90mins... I'll update later today or tomorrow

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 15, 2017

Member

Re-running it right now... This time a reference to a test doesn't count so expect more symbols to be flagged as 🗑

Member

jrieken commented Nov 15, 2017

Re-running it right now... This time a reference to a test doesn't count so expect more symbols to be flagged as 🗑

@alexandrudima alexandrudima self-assigned this Nov 15, 2017

alexandrudima added a commit that referenced this issue Nov 15, 2017

bpasero added a commit that referenced this issue Nov 15, 2017

roblourens added a commit that referenced this issue Nov 15, 2017

@chrmarti chrmarti self-assigned this Nov 15, 2017

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 16, 2017

Member

FYI, I have updated the list with symbols that are only referenced from tests and it's now sorted by save-potential

Member

jrieken commented Nov 16, 2017

FYI, I have updated the list with symbols that are only referenced from tests and it's now sorted by save-potential

alexandrudima added a commit that referenced this issue Nov 16, 2017

jrieken added a commit that referenced this issue Nov 16, 2017

jrieken added a commit that referenced this issue Nov 16, 2017

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Nov 16, 2017

Member

@jrieken thanks for doing this. I cleaned up my code. Can we for the next run exclude taskConfiguration.ts. It has a lot of false positives since I use structural typing with namespace but a namespace can't implement an interface. These methods are used, although not always referenced directly:

	isEmpty(value: T): boolean;
	assignProperties(target: T, source: T): T;
	fillProperties(target: T, source: T): T;
	fillDefaults(value: T, context: ParseContext): T;
	freeze(value: T): Readonly<T>;

Alternatively I could rewrite everything to use classes and then implement the ParserType which defines all these methods.

Member

dbaeumer commented Nov 16, 2017

@jrieken thanks for doing this. I cleaned up my code. Can we for the next run exclude taskConfiguration.ts. It has a lot of false positives since I use structural typing with namespace but a namespace can't implement an interface. These methods are used, although not always referenced directly:

	isEmpty(value: T): boolean;
	assignProperties(target: T, source: T): T;
	fillProperties(target: T, source: T): T;
	fillDefaults(value: T, context: ParseContext): T;
	freeze(value: T): Readonly<T>;

Alternatively I could rewrite everything to use classes and then implement the ParserType which defines all these methods.

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 16, 2017

Member

@dbaeumer Sure, exclusion aren't easy to add.

Member

jrieken commented Nov 16, 2017

@dbaeumer Sure, exclusion aren't easy to add.

isidorn added a commit that referenced this issue Nov 16, 2017

bpasero added a commit that referenced this issue Nov 16, 2017

bpasero added a commit that referenced this issue Nov 16, 2017

@joaomoreno

This comment has been minimized.

Show comment
Hide comment
@joaomoreno

joaomoreno Nov 17, 2017

Member

Gotta exclude this:

src/vs/code/node/cliProcessMain.ts -> main:177,1, potentially save ~48 lines
Member

joaomoreno commented Nov 17, 2017

Gotta exclude this:

src/vs/code/node/cliProcessMain.ts -> main:177,1, potentially save ~48 lines

sandy081 added a commit that referenced this issue Nov 19, 2017

jrieken added a commit that referenced this issue Nov 20, 2017

jrieken added a commit that referenced this issue Nov 20, 2017

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 20, 2017

Member

@bpasero Is it possible that IWorkbenchContribution#getId is just hot air? I don't see a single caller of that method..

Member

jrieken commented Nov 20, 2017

@bpasero Is it possible that IWorkbenchContribution#getId is just hot air? I don't see a single caller of that method..

ramya-rao-a added a commit to ramya-rao-a/vscode that referenced this issue Nov 20, 2017

bpasero added a commit that referenced this issue Nov 21, 2017

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 21, 2017

Member

@jrieken yeah, removed via 66f4c0e

Member

bpasero commented Nov 21, 2017

@jrieken yeah, removed via 66f4c0e

@alexandrudima alexandrudima removed their assignment Nov 21, 2017

alexandrudima added a commit that referenced this issue Nov 21, 2017

ramya-rao-a added a commit that referenced this issue Nov 21, 2017

@bpasero bpasero removed their assignment Nov 22, 2017

bpasero added a commit that referenced this issue Nov 22, 2017

jrieken added a commit that referenced this issue Nov 23, 2017

sandy081 added a commit that referenced this issue Nov 24, 2017

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 27, 2017

Member

@bpasero Can you take another look at the builder... There is still lot of unused stuff in there, also those that the tool doesn't find because overloads are used, e.g.

public margin(margin: string): Builder;
public margin(top: number, right?: number, bottom?: number, left?: number): Builder;
public margin(top: string, right?: string, bottom?: string, left?: string): Builder;
public margin(top: any, right?: any, bottom?: any, left?: any): Builder {
	if (types.isString(top) && top.indexOf(' ') >= 0) {
		return this.margin.apply(this, top.split(' '));
Member

jrieken commented Nov 27, 2017

@bpasero Can you take another look at the builder... There is still lot of unused stuff in there, also those that the tool doesn't find because overloads are used, e.g.

public margin(margin: string): Builder;
public margin(top: number, right?: number, bottom?: number, left?: number): Builder;
public margin(top: string, right?: string, bottom?: string, left?: string): Builder;
public margin(top: any, right?: any, bottom?: any, left?: any): Builder {
	if (types.isString(top) && top.indexOf(' ') >= 0) {
		return this.margin.apply(this, top.split(' '));

alexandrudima added a commit that referenced this issue Nov 27, 2017

bpasero added a commit that referenced this issue Nov 27, 2017

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 27, 2017

Member

@jrieken yeah, though there can be implicit usages because in the doSetAttr method we just dynamically call into the methods depending on the properties passed over. If someone calls attr({ margin: ...}) we end up calling Builder#margin(). I think Builder might need some special treatment to see if it can be replaced with something more straight foward and lightweight...

Member

bpasero commented Nov 27, 2017

@jrieken yeah, though there can be implicit usages because in the doSetAttr method we just dynamically call into the methods depending on the properties passed over. If someone calls attr({ margin: ...}) we end up calling Builder#margin(). I think Builder might need some special treatment to see if it can be replaced with something more straight foward and lightweight...

@jrieken

This comment has been minimized.

Show comment
Hide comment
@jrieken

jrieken Nov 27, 2017

Member

into the methods depending on the properties passed over. If someone calls attr({ margin: ...})

😱 no type-safety?

Member

jrieken commented Nov 27, 2017

into the methods depending on the properties passed over. If someone calls attr({ margin: ...})

😱 no type-safety?

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 27, 2017

Member

Dude, builder is from a time where there was no type safety...

Member

bpasero commented Nov 27, 2017

Dude, builder is from a time where there was no type safety...

@jrieken jrieken removed their assignment Nov 30, 2017

@chrmarti chrmarti removed their assignment Nov 30, 2017

@roblourens roblourens removed their assignment Dec 1, 2017

@bpasero bpasero removed their assignment Dec 3, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Dec 4, 2017

Contributor

@jrieken assigning to you to decide if this should be closed since it shows up in my inbox tracker query

Contributor

isidorn commented Dec 4, 2017

@jrieken assigning to you to decide if this should be closed since it shows up in my inbox tracker query

@jrieken jrieken closed this Dec 4, 2017

Tyriar added a commit that referenced this issue Dec 11, 2017

@vscodebot vscodebot bot locked and limited conversation to collaborators Jan 18, 2018

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