Refactoring: Strict TypeScript Fix #1208

[nx10]

What problem did you solve?

WIP PR to refactor the entire TypeScript codebase to use stricter type checks. See #1208

We should work bottom-up and fix files with many dependents earlier as null-checks may propagate.

TODO

Rough checklist to track progress:

• html\help\script.ts
• html\help\webviewMessages.d.ts
• html\httpgd\index.ts
• html\httpgd\webviewMessages.d.ts
• src\api.d.ts
• src\apiImplementation.ts
• src\completions.ts
• src\cppProperties.ts
• src\extension.ts
• src\languageService.ts
• src\lineCache.ts
• src\lintrConfig.ts
• src\preview.ts
• src\rGitignore.ts
• src\rstudioapi.ts
• src\rTerminal.ts
• src\selection.ts
• src\session.ts
• src\tasks.ts
• src\util.ts
• src\workspaceViewer.ts
• src\helpViewer\cran.ts
• src\helpViewer\helpProvider.ts
• src\helpViewer\index.ts
• src\helpViewer\packages.ts
• src\helpViewer\panel.ts
• src\helpViewer\treeView.ts
• src\helpViewer\webviewMessages.d.ts
• src\liveShare\index.ts
• src\liveShare\shareCommands.ts
• src\liveShare\shareSession.ts
• src\liveShare\shareTree.ts
• src\liveShare\virtualDocs.ts
• src\plotViewer\httpgdTypes.d.ts
• src\plotViewer\index.ts
• src\plotViewer\webviewMessages.d.ts
• src\rmarkdown\draft.ts
• src\rmarkdown\index.ts
• src\rmarkdown\knit.ts
• src\rmarkdown\manager.ts
• src\rmarkdown\preview.ts
• src\test\runTest.ts
• src\test\suite\extendSelection.test.ts
• src\test\suite\index.ts
• src\test\suite\lineCache.test.ts
• src\test\suite\syntax.test.ts

[ManuelHentschel]

I'm quite familiar with the help and plot viewer related code, so I could do those files. Should I just push the corresponding changes directly to this branch?

[nx10]

I'm quite familiar with the help and plot viewer related code, so I could do those files. Should I just push the corresponding changes directly to this branch?

Yes, go for it. I am done for today and will continue tomorrow. I have just finished all code files in the src/ root but src\session.ts (I don't really understand the request file mechanism used there so help is appreciated).

[nx10]

For reference: There was/is a pretty large number of unchecked null-ish values. (for example when the R-path is not set) I try to return early or abort gracefully where possible but this PR will need extensive testing of all vscode-r functionality after it is finished. On the plus side this will hopefully fix a lot of hard to track down bugs presently and in the future.

[nx10]

We should probably refactor all occurrences of

config().get<T>(section: string): T | undefined

to

config().get<T>(section: string, defaultValue: T): T

[nx10]

It's done! We should definitely test this well before merge, but I will open the PR for review now.

[renkun-ken]

LGTM

[renkun-ken]

From https://code.visualstudio.com/api/references/vscode-api#WorkspaceConfiguration, it seems config().get() will at least return the default value if the config entry is found. As long as the extension is properly activated, the config entries should be found anyway. If we always specify a default value, then it might be hard to maintain the settings if we want to change the default, since there might be many get() for a certain entry.

Take vscode-python for example, none of the calls to pythonSettings.get() (e.g. https://github.com/microsoft/vscode-python/blob/a4de2b8a98b1e3ba56f81cc23bd163e6862c059b/src/client/common/configSettings.ts#L255) specify the default value since they all access to setting entries defined by the extension. One that specifies the default value is https://github.com/microsoft/vscode-python/blob/a4de2b8a98b1e3ba56f81cc23bd163e6862c059b/src/client/common/configSettings.ts#L182 where it accesses an external entry.

Do you think if it makes sense to follow the pattern that we don't specify the default value if the entry is defined by the extension so that it should automatically follow the definition in package.json?

[nx10]

Take vscode-python for example, none of the calls to pythonSettings.get() (e.g. https://github.com/microsoft/vscode-python/blob/a4de2b8a98b1e3ba56f81cc23bd163e6862c059b/src/client/common/configSettings.ts#L255) specify the default value since they all access to setting entries defined by the extension. One that specifies the default value is https://github.com/microsoft/vscode-python/blob/a4de2b8a98b1e3ba56f81cc23bd163e6862c059b/src/client/common/configSettings.ts#L182 where it accesses an external entry.

Interesting, it seems like they opted to do comparisons to avoid T | undefined.

Do you think if it makes sense to follow the pattern that we don't specify the default value if the entry is defined by the extension so that it should automatically follow the definition in package.json?

I was under the impression adding a default value to the call just adds another fallback after there was no default found on any other level. I.e.: config().get(x) !== undefined ? config().get(x) : defaultValue
Edit: ...and thus providing type-safety e.g. even in case of typos in the field name

[renkun-ken]

It feels quite redundant and burdensome to maintain the default values of each setting not only at where it is defined in package.json but also all places that use the setting.

even in case of typos in the field name

Thought about this case too. And specifying a default when there is a typo seems to hide the problem rather than make it easier to spot.

[nx10]

So what is your opinion on how to proceed?

We could also opt for a helper function that either ignores the undefined or checks and errors:

function config_get<T>(section: string): T {
    return config().get<T>(section) as T;
}

function config_get<T>(section: string): T {
    const x = config().get<T>(section);
    if (x === undefined) {
        throw 'No config entry for ' + section;
        return undefined as T; // a bit scary
    }
    return x;
}

Edit: Only using default values when needed is also fine by me, I am just wondering what the most developer friendy way ist.

[renkun-ken]

Only using default values when needed is also fine by me, I am just wondering what the most developer friendy way ist.

Me too. Please feel free to use whatever you think makes good sense or common practice.

[nx10]

Please feel free to use whatever you think makes good sense or common practice.

Yes it's probably the best to leave this for now revisit it if it actually presents itself as a problem.

From my side we are good to go with this PR.

[renkun-ken]

Thanks!