Repository navigation
TypeScript 3.5 compile errors in the VS Code codebase #31380
Description
Activity
Specific issues that seem like breaking changes:
-
A few style properties in
dom.d.tsare no longer nullable:[15:39:42] Error: /Users/matb/projects/vscode/src/vs/workbench/browser/parts/editor/editorDropTarget.ts(91,3): Type 'string | null' is not assignable to type 'string'. -
TS 3.5 regression casting
objectto{ [key: string]: unknown };#31381 -
Issues around switch statement for keys that write to an object in the case body (still trying to put together a minimal repo for this one)
[15:39:42] Error: /Users/matb/projects/vscode/src/vs/workbench/services/themes/common/fileIconThemeData.ts(121,7): Type 'any' is not assignable to type 'never'. -
TS 3.5 new error when using keyof with assignment to copy property #31382
-
I fixed many of these errors in microsoft/vscode@fd1ac75
microsoft/vscode@fd1ac75#diff-fed321a94e979ed6f2aaaf1001dbac36L17
Caused by #27089. We might consider reverting the lib change because this code is manifestly correct, but our lack of ability to narrow
TtoObjectish<T>from thetypeofcheck at the top of the function means we incorrectly reject the call.microsoft/vscode@fd1ac75#diff-4414fa1e065d93e77c72eec3db97c187L105
We previously inferred
countto be{} | undefined, but now infer it to beunknown. The{} | undefinedtype was "better" because it could narrow awayundefinedin the!count ||expression, then{}could be "safely" coerced with+count.Again, this is a case of manifestly correct code where we need type facts here to express the fact that
undefinedandnullwere removed from the type by the truthiness check.There's a case to be made that our rules around unary
+are inconsistent enough to be useless.+{}is really not any more or less suspect than+undefinedand maybe we should just allow it for everything, same as"" + unknownmicrosoft/vscode@fd1ac75#diff-11b5b53dd82e487bffdf3b724fad9d83L82
We prevented a hypothetical call of
merge(0, 0, false)which would throw under ES5 (#27089)Again, this is a case of manifestly correct code where we need type facts here to express the fact that undefined and null were removed from the type by the truthiness check.
unknown & not undefined -> not undefined❤️Type facts as they exist today can only remove types from a union, not "add negated types to an intersection", so it's moreso a shortcoming in the type facts system as it exists than some specific fact being missing. Simply put: The type to narrow to in the case where
x !== undefinedsimply cannot be expressed today. You could get real close by convertingunknowninto{} | undefined | nulland narrowing that, but{}carries some object-like baggage that could cause interesting behavior down the line.microsoft/vscode@fd1ac75#diff-6c07482b4bc75dfd16bc0d2239065809R162
Context: VS Code has
suppressImplicitAnyIndexErrorsenabled.The relevant calling code looks like this (simplified):
return asJson(context).then(result => { return result && result['experiments']; });
Most other uses of
asJsonsupply the type parameter to provide the expected JSON shape. The intent, I guess, is to fall back to something likeanywhen that's not provided. Going back tounknownis really quite bad here becauseresult's type is supposed to meaningfully include| null.It's not clear how to proceed here since the code was quite unsafe to begin with. We should get a time machine and ban uninferrable type parameters.
microsoft/vscode@fd1ac75#diff-1e9db0aff31fe7167a26aef6c4176a19R23
The failing example looks like this:
let watcher = new ConfigWatcher<{}>(testFile); let config = watcher.getConfig(); assert.ok(config); assert.equal(Object.keys(config), 0);
This is a hairy variant of the
foo && Object.keys(foo)problem since they're usingassert.okto validate the truthiness ofconfig. However, the error is entirely correct - iftestFile's contents were5, this would throw under ES5 as specified in the target.ConfigWatcher<T>should have a constraint.Reacted by Jordan Harbandmicrosoft/vscode@fd1ac75#diff-641fa4cc338f106854e4b4a33964acbdR84
This is an extremely real error.
Consider these three assignments:
const k1: IDraggedEditor = { resource: URI.parse(draggedEditor.resource), backupResource: draggedEditor.backupResource ? URI.parse(draggedEditor.backupResource) : undefined, viewState: draggedEditor.viewState, isExternal: false }; const k2: IDraggedResource = { resource: URI.parse(draggedEditor.resource), backupResource: draggedEditor.backupResource ? URI.parse(draggedEditor.backupResource) : undefined, viewState: draggedEditor.viewState, isExternal: false }; const k3: IDraggedEditor | IDraggedResource = { resource: URI.parse(draggedEditor.resource), backupResource: draggedEditor.backupResource ? URI.parse(draggedEditor.backupResource) : undefined, viewState: draggedEditor.viewState, isExternal: false };k3should not be a legal initialization unless at least one ofk1andk2are, but both of those are also errors. The provided object literal is not a match for either object --backupResourceis extraneous inIDraggedEditor, andviewStatehas the wrong type inIDraggedResource.Reacted by Daniel Rosenwassermicrosoft/vscode@fd1ac75#diff-b7604f54db2a2f61d2bf5af1fcc5047fL391
I can't tell what's going on here; this doesn't error locally for me. The type is extremely complex.
Re #31380 (comment), why can't the types be narrowed correctly?
Let's take that discussion to the PR please
Reacted by Jordan Harbandmicrosoft/vscode@fd1ac75#diff-1ba53e60093143b29593e77afffaacb6L1036
microsoft/vscode@fd1ac75#diff-0b973b67f859d3b8fa68baec97534643R98
More instances of indexing in to a non-null-narrowed
{} | undefinedwhich is now anunknowninstead.microsoft/vscode@fd1ac75#diff-0f1abf8ff5283dac48ea3231f2dddfa0R96
An interesting case. This is a typical unsound method declaration where the derived class needs a specialized object from a parallel class hierarchy and would be broken by a call through a base type alias. Method bivariance allows this, but until a subsequent change this was disallowed due to differing definitions of the return type of
getTelemetryDescriptorThe change appears to be unnecessary due to the change (presumably made chronologically later as Matt was fixing stuff) to
getTelemetryDescriptorat microsoft/vscode@fd1ac75#diff-d6d4527bd487a54f95c554d21f5e05ebUpdate: Also microsoft/vscode@fd1ac75#diff-022291e875d46b01fc7f4e6ba724f466
microsoft/vscode@fd1ac75#diff-00578f406c825a8136fefeb491625f65L722
Bullet 2 from Matt's comment; #31381
Matt Bierner (@mjbvz) Regarding this one
[15:39:42] Error: /Users/matb/projects/vscode/src/vs/workbench/services/themes/common/fileIconThemeData.ts(121,7): Type 'any' is not assignable to type 'never'.This is an effect of #30769. The type
FileIconThemeData["id" | "label" | "description" | "settingsId" | "extensionData" | "styleSheetContent" | "hasFileIcons" | "hidesExplorerArrows" | "hasFolderIcons" | "watch"]reduces toneverwhen it is the target of an assignment because the properties are of disjoint types, i.e. there exists no type for which it is safe to perform this assignment.microsoft/vscode@fd1ac75#diff-00578f406c825a8136fefeb491625f65L722
In 3.4,
promisehas typePromise<{} | null>, which is allowed to be used for aPromise<ITaskSummary | null>becauseITaskSummaryonly has optional properties. The type of this variable comes from three places in the function body that follows -Promise.resolve(null)(Promise<null>), a call tonew Promise((c, e) => ...that producesPromise<{}>, and an ultimately-irrelevant call tothis.taskService.run(task)that returnsPromise<ITaskSummary>. It's unsafe because we can't really say what's going in toc(at line 736 (and indeed, this seems to be an incorrect valueundefined).In 3.5, the
Promise<{}>becomesPromise<unknown>and the whole thing becomesPromise<unknown>, andunknowncan't beITaskSummary | nullThe better fix is probably to annotate the untyped
Promiseconstructor call. I'm still confused why TS is allowing anundefinedto to through this codepath.microsoft/vscode@fd1ac75#diff-4a12537872685c49f4dc5c42f1537c45
I don't understand what this change is doing. The only manifestation of the type parameter in
ITreeContextMenuEvent<RenderableMatch | null>iselement: T | null;, so including| nullhere should not have changed anything. There's no error locally when I revert thismicrosoft/vscode@fd1ac75#diff-203c27e756d1503a7f55c19fd8440959R268
A simplified version:
enum E { f, g } type A = { q?: E; x?: string; y?: string; } function set(a: A, key: keyof A, value: string) { a[key] = value; }
This is a correct error; it happens that no one called
set(a, "q", "foo"), but the code doesn't appear to guard against this at all.A preferable fix would be to change the signature of
set / fillPropertyto reject keys that didn't have correspondingstringproperty typesThank you for taking a look. For #31380 (comment), is that TS error or a VS Code error?
I believe the only one that I think is really blocking us then is the d.ts change: #27089
is that TS error or a VS Code error?
It's a VS Code error IMO. The object has a field that is either surplus in one constituent of the union, or an incorrect type in the other constituent.
Closing this as I believe the errors have all be addressed or are by-design
Inferring
Tas unknown results in the errorBuild:Property 'hasOwnProperty' does not exist on type 'T
in the following:
public foo<T>(target: T): void { target.hasOwnProperty; }
Not sure if you are seeing this as a bug. It is certainly rather inconvenient because
hasOwnPropertydoes exist.Pass
undefinedforTand be amazed at howhasOwnPropertydoes not exist. ❤️That's exactly the class of error the constraint change is supposed to catch - you might want to say
T extends object😉Wesley Wigham (@weswigham) good point. Althought the temptation is to write
(target as {}).hasOwnPropertyas the relation to undefined is not very clear - unless you're a smart arse 😉Thanks
Reacted by poseidonCore- locked as resolved and limited conversation to collaborators
on Oct 21, 2025
TypeScript Version: 3.5.0-dev.20190512
Repo
Build VS Code with TypeScript@next:
git clone https://github.com/microsoft/vscode.git cd vscode yarn add typescript@next yarn run watchPotential issues
There are 68 compile errors on
TS@next, compared to zero when compiling withTS@3.4.5:Some of these seem related to #30637 but other ones seem suspect (or should be noted in the breaking changes)