Enable noImplicitOverride in our codebase

[mjbvz]

--noImplicitOverride

TS 4.3 adds a new strictness option: --noImplicitOverride. This requires that you use the override keyword when overriding and method:

class Foo {
   foo() {...}
}

class SubFoo extends Foo {
    foo() { ... } // ERROR: missing override specifier
}

To fix this, simply add the override keyword:

class SubFoo extends Foo {
    override foo() { ... } 
}

The override keyword does not change runtime behavior but can help programmers in a few ways:

• When reading code, it alerts you that a method is overriding one from the base class

• It is an error to try to override a method that does not exist on the base class. This can help catch errors caused by renaming a method in a base class but forgetting to update the method name in the subclasses.

Enabling

I propose enabling this new flag for both VS Code code and extensions.

Enabling it currently causes ~2200 compile errors, however unlike the previous strictness checks we enabled, I believe should simply use a script to automatically fix the vast majority of them (by inserting override in the necessary location)

[mjbvz]

Here's the simple python script I used to automate most of the rewrites:

from os import path
import re


def fixThisMember(file, line, col):
    with open(file, 'r') as f:
        lines = f.readlines()

    lineContent = lines[line - 1]

    # public? async? method(
    lineContent = re.sub(
        r'^(\s+)((?:public|private|protected) )?(async )?([\w_$]+[\(<])',
        r'\1\2\3override \4',
        lineContent)

    # public? get|set prop(
    lineContent = re.sub(
        r'^(\s+)((?:public|private|protected) )?(get|set) ([\w_$]+)\(',
        r'\1\2override \3 \4(',
        lineContent)

    # public? readonly? prop =
    lineContent = re.sub(
        r'^(\s+)((?:public|private|protected) )?(readonly )?([\w_$]+) =',
        r'\1\2\3override \4 =',
        lineContent)

    # public? readonly? prop:
    lineContent = re.sub(
        r'^(\s+)((?:public|private|protected) )?(readonly )?([\w_$]+):',
        r'\1\2\3override \4:',
        lineContent)

    if lineContent != lines[line - 1]:
        print(file)
        lines[line - 1] = lineContent
        with open(file, 'w') as f:
            f.writelines(lines)


# file containing `yarn watch` error text
with open('override-errors.text') as f:
    for line in f:
        pMatch = re.match(r'.+Error: (.+)\((\d+),(\d+)\): (This member must have an)',
                          line, flags=re.I)
        if pMatch:
            file = pMatch.group(1)
            line = int(pMatch.group(2))
            col = int(pMatch.group(3))
            fixThisMember(file, line, col)

This is a pretty simple script but takes us from 2200 errors to 200 or so.

The remaining errors fall into a few buckets:

• Cases where the property is declared in a constructor.

  Many of these seem to be errors (such as a subclass re-declaring a protected property)

  The rest need to be re-written manually

• declare readonly _serviceBrand: undefined

  Property using declare prompts to add 'override' even though that does not seem valid TypeScript#43566

  We should likely just change these to override readonly _serviceBrand: undefined

• Static properties overriding non-static property

  noImplicitOverride complains about static property not overriding non-static property TypeScript#43563

  Seems to be a bug on the TS side

[Kingwl]

Cool! But... Are there any tools could batch apply quickfix?

[mjbvz]

@Kingwl Not that I know off. For source actions, you can use https://marketplace.visualstudio.com/items?itemName=bierner.folder-source-actions

Hooking up individual quick fixes would be a little more difficult but should be doable (writing a simple python find/replace script was a lot quicker in this case)

[mjbvz]

Current status

I've automatically added override to simple cases with methods/properties with #120755. This does not change runtime behavior and just annotates a pattern that already exists in the code. You can review the changes in your areas of the codebase by searching for the override keyword. This automation took us from 2200 errors to around 200.

I also made two passes at the remaining errors. There were two types of issues I addressed here:

• Simple cases the automation missed

• Cases where a constructor was overriding a property with the same type. This was happening fairly often for services. Here's an example

Next steps

The remaining 100 errors need to be reviewed manually so I'm going to assign area owners. Please re-assign if needed

To test building with --noImplicitOverride so that you can fix these errors, simply run yarn no-implicit-override-watch. After making the change, be sure to check the code compiles with our normal yarn watch too

update: About 15 errors remaining:

• src/vs/base/browser/ui/tree/asyncDataTree.ts @joaomoreno
• src/vs/base/browser/ui/tree/dataTree.ts @joaomoreno
• src/vs/base/browser/ui/tree/indexTree.ts @joaomoreno
• src/vs/base/browser/ui/tree/objectTree.ts @joaomoreno
• src/vs/editor/browser/services/codeEditorServiceImpl.ts @alexdima
• src/vs/editor/common/services/editorSimpleWorker.ts @jrieken (this one seemed to break the Monaco build when override was added)
• src/vs/platform/list/browser/listService.ts @joaomoreno
• src/vs/workbench/browser/parts/views/viewPane.ts @sbatten
• src/vs/workbench/browser/parts/views/viewPaneContainer.ts @sbatten
• src/vs/workbench/browser/parts/views/viewsViewlet.ts @sbatten
• src/vs/workbench/contrib/debug/browser/debugService.ts @isidorn
• src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalService.ts (no longer needed?) @Tyriar

[bpasero]

Down to 80

[isidorn]

@mjbvz Thanks for doing this, I tackled mine via 845efe5
Down to 75 errors...

[jrieken]

Static properties overriding non-static property, microsoft/TypeScript#43563

We aren't tackling those given TS has acknowledged the issue and has a fix ready, right?

[Kingwl]

Static properties overriding non-static property, microsoft/TypeScript#43563

We aren't tackling those given TS has acknowledged the issue and has a fix ready, right?

Agreed.

[Kingwl]

Hi folks!

Seems the fix has already merged.

[mjbvz]

@alexdima @sbatten Just 6 errors remaining! Please take a look when you have a chance

@jrieken @alexdima Do you have any idea about why MirrorModel.version is causing issues with the esm build? After adding override to version, yarn gulp editor-esm-bundle results in:

Launching the TS compiler at /Users/matb/projects/vscode/out-editor-esm...
vs/editor/common/services/editorSimpleWorker.ts(94,22): error TS4113: This member cannot have an 'override' modifier because it is not declared in the base class 'MirrorTextModel'.

In our source, MirrorTextModel has a property:

	get version(): number {
		return this._versionId;
	}

However in the esm build, this property is missing from MirrorTextModel . It may be getting removed by our tree shaking

[alexdima]

Do you have any idea about why MirrorModel.version is causing issues with the esm build?

Yes, TypeScript is sometimes incorrect when invoking Find all references and this was one of those cases, where finding all references on ICommonModel.version would not return MirrorTextModel.version. As far as I am concerned, this is a TypeScript bug that also manifests itself when doing a rename. For example, renaming ICommonModel.version to ICommonModel.versionX will introduce a compilation error. I have reported the problem to TS, but the issue was closed microsoft/TypeScript#25831

To resolve the problem, I added another interface and an explicit implements ... heritage clause to help TS find all references correctly.

[mjbvz]

Done with 66fd0cb

Thanks everyone!