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.

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

enable strictFunctionTypes in VS Code codebase #81574

Closed
mjbvz opened this issue Sep 27, 2019 · 31 comments
Closed

enable strictFunctionTypes in VS Code codebase #81574

mjbvz opened this issue Sep 27, 2019 · 31 comments
Assignees
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc.
Milestone

Comments

@mjbvz
Copy link
Collaborator

mjbvz commented Sep 27, 2019

Problem

For an example service defined in VS Code core:

export class MyService {
  constructor(
    private readonly value: string,
    @ITextModelService private readonly textModelResolverService: ITextModelService
  ) { }
}

It is currently possible to use the instantiation service to create an instance of MyService that passes undefined for value:

instantiationService.createInstance(MyService, undefined);

This does not produce any compile errors. It does bypass our strict null checking however and can cause unexpected runtime behavior.

Proposed fix

Enable strictFunctionTypes in our code base

There are about 800 or so errors related to this in our code base today. While some of these errors are simple, many are rather cryptic and some may prove difficult to fix

@mjbvz mjbvz added the debt Code quality issues label Sep 27, 2019
@mjbvz mjbvz added this to the October 2019 milestone Sep 27, 2019
@mjbvz mjbvz self-assigned this Sep 27, 2019
@mjbvz

This comment has been minimized.

mjbvz added a commit that referenced this issue Sep 28, 2019
As part of the fix for #81574, I discovered that a large number of callers were already passing `undefined` for this value. This fix just marks the type as possibly undefined as well
mjbvz added a commit that referenced this issue Sep 28, 2019
For #81574

`IInstantiationService.createInstance` currently does not do a good job at making sure you don't pass `undefined` as a parameter (see #81574)

This change attempt to fix that by making the typings more explicit. The fix in turn revealed a lot of places where possible type errors were slipping through
@alexr00 alexr00 self-assigned this Sep 30, 2019
mjbvz added a commit that referenced this issue Sep 30, 2019
mjbvz added a commit that referenced this issue Oct 4, 2019
For #81574

`IInstantiationService.createInstance` currently does not do a good job at making sure you don't pass `undefined` as a parameter (see #81574)

This change attempt to fix that by making the typings more explicit. The fix in turn revealed a lot of places where possible type errors were slipping through
mjbvz added a commit that referenced this issue Oct 4, 2019
mjbvz added a commit that referenced this issue Oct 4, 2019
@Tyriar Tyriar self-assigned this Oct 4, 2019
Tyriar added a commit that referenced this issue Oct 4, 2019
Tyriar added a commit that referenced this issue Oct 4, 2019
Tyriar added a commit that referenced this issue Oct 4, 2019
Tyriar added a commit that referenced this issue Oct 4, 2019
Tyriar added a commit that referenced this issue Oct 5, 2019
mjbvz added a commit that referenced this issue Oct 5, 2019
For #81574

`IInstantiationService.createInstance` currently does not do a good job at making sure you don't pass `undefined` as a parameter (see #81574)

This change attempt to fix that by making the typings more explicit. The fix in turn revealed a lot of places where possible type errors were slipping through
Tyriar added a commit that referenced this issue Oct 5, 2019
mjbvz added a commit that referenced this issue Oct 5, 2019
@bpasero bpasero self-assigned this Oct 5, 2019
mjbvz added a commit that referenced this issue Oct 5, 2019
For #81574

`IInstantiationService.createInstance` currently does not do a good job at making sure you don't pass `undefined` as a parameter (see #81574)

This change attempt to fix that by making the typings more explicit. The fix in turn revealed a lot of places where possible type errors were slipping through
Tyriar added a commit that referenced this issue Feb 11, 2020
sbatten added a commit that referenced this issue Feb 11, 2020
mjbvz added a commit that referenced this issue Feb 11, 2020
mjbvz added a commit that referenced this issue Feb 11, 2020
For #81574

Let TS infer the types for us here
@joaomoreno
Copy link
Member

Pushed a bunch of fixes. We're down to 23 errors.

@joaomoreno joaomoreno removed their assignment Feb 12, 2020
jrieken added a commit that referenced this issue Feb 12, 2020
@jrieken
Copy link
Member

jrieken commented Feb 12, 2020

Down to 9 errors - This is some left for @bpasero in glob.ts and there is some more tweaking needed for the Event and CancellationToken story... Getting closer

@jrieken
Copy link
Member

jrieken commented Feb 12, 2020

Almost done. This is what's left 👇

Screenshot 2020-02-12 at 11 10 34

@bpasero bpasero assigned chrmarti and unassigned bpasero Feb 12, 2020
@bpasero
Copy link
Member

bpasero commented Feb 12, 2020

I own the simple pieces of glob, not this crazy stuff. Assigning to @chrmarti. I tried to fix it and had to give up, this is just beyond my skills.

E.g I really do not think we should add properties to functions like here 7852430

@roblourens
Copy link
Member

roblourens commented Feb 14, 2020

I don't understand the preferences editor one. It's on a line using the IConstructorSignature type helper, and the bottom line is Type 'BrandedService' is missing the following properties from type 'IInstantiationService': createInstance, invokeFunction, createChild

like it thinks a BrandedService is being referenced as an IInstantiationService. But it should be the other way around, the IInstantiationService in the constructor's argument list should just have to fulfill BrandedService.

@mjbvz
Copy link
Collaborator Author

mjbvz commented Feb 14, 2020

@roblourens I can't decipher this either but have added a workaround: 91fc43d

@chrmarti
Copy link
Contributor

@mjbvz Fixed the last ones, back to you.

@chrmarti chrmarti assigned mjbvz and unassigned chrmarti Feb 18, 2020
@mjbvz mjbvz closed this as completed in 2158e77 Feb 18, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Apr 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues engineering VS Code - Build / issue tracking / etc.
Projects
None yet
Development

No branches or pull requests