Resolve compiler errors that appear due to useDefineForClassFields

[trevorade]

These changes go a ways towards addressing #186726

These changes DO NOT include setting useDefineForClassFields to true so backsliding is still possible.

These changes were done by hand over a period of about five focused hours. There were 500 errors when I started.

The process I used was:

• Enable useDefineForClassFields in the base tsconfig file (NOT IN THIS PR)
• Run npm run watch
• Identify an error and open the file with an error
• For any field initialization that references a parameter property:
  • Move the initialization into the constructor
  • Explicitly add the type via a plugin
  • Repeat for any remaining + new errors
• Look for usages of waitForState and recomputeInitiallyAndOnChange and inline any impacted derived fields

To truly enable useDefineForClassFields, more changes will more than likely need to be made. For now, I was interested in tackling the compilation errors.

All tests pass with these changes.

If the VSCode team would like to take over this PR, that's fine.

The motivation for making these changes is that, at Google, we build this codebase internally. We are going to enable the compilation errors related to useDefineForClassFields in our internal typescript compiler settings (without actually turning on those transpilation changes, similar to microsoft/TypeScript#59623). So we would much prefer to fix these issues in your codebase rather than maintain a huge 200 file patch :P

[trevorade]

@microsoft-github-policy-service agree company="Google"

[trevorade]

@jrieken, would you be willing to take a look at this?

[jrieken]

@trevorade Thanks for reaching out and starting this. This is a large PR and something we need to coordinate across the team. Usually, we have reservations against PRs that touch very many code owner areas but this seems mostly mechanical and exception-worthy.

I will circle this with the team and see how folks respond. Generally, I am positive about this change and it seems this must happen anyways sooner or later.

To truly enable useDefineForClassFields, more changes will more than likely need to be made

Do you have a feel for what that would be?

@mjbvz You initial concern in #186726 (comment) seems to have fixed, right? Any other insights this that you have?

[trevorade]

@trevorade Thanks for reaching out and starting this. This is a large PR and something we need to coordinate across the team. Usually, we have reservations against PRs that touch very many code owner areas but this seems mostly mechanical and exception-worthy.

I will circle this with the team and see how folks respond. Generally, I am positive about this change and it seems this must happen anyways sooner or later.

Thx. This is exactly what I expected. That seeing such a large PR from a new contributor would raise some eyebrows and require some discussion.

Once you guys are fully comfortable with this PR, I will re-sync, again fix all new errors, and move forward with submit.

It would really be best if the codebase had some method to prevent backsliding but I'm unaware of any short of enabling useDefineForClassFields.

To truly enable useDefineForClassFields, more changes will more than likely need to be made

Do you have a feel for what that would be?

When you actually enable the flag, the code emitted changes quite a bit more. You start using true class fields rather than object properties throughout. While testing with this flag enabled, I wasn't able to actually run VSCode due to some errors. I forget exactly what. Easiest way to test that out would be to patch this PR and turn on the flag to experiment.

Anyways, my point is that I would expect that someone would want to do rigorous testing once enabling this flag. This PR is safer as it only fixes all the compiler errors without actually making the change.

@mjbvz You initial concern in #186726 (comment) seems to have fixed, right? Any other insights this that you have?

Matt was correct that there were additional runtime errors to consider. In the case of moving only the initializers that reference parameter properties, there's still the issue of other non-constructor initializers that access other fields initialized in the constructor from a function. This can be safe if the function is evaluated after the constructor or unsafe if evaluated immediately (i.e. via waitForState or recomputeInitiallyAndOnChange). I found a few instances of this in relation to this PR which I fixed here (from observing errors and then from analyzing all instances of waitForState or recomputeInitiallyAndOnChange for safety (i.e. usage in a field initializer versus inside a method).

I can only be as confident as your tests though. I would hope that someone in the VSCode team could patch this PR and test things out a bit to check for anything that may not be as well covered in tests.

Beyond what I fixed, it's possible there are other code execution order changes that will be introduced via enabling useDefineForClassFields that need to be fixed.

[jrieken]

but I'm unaware of any short of enabling useDefineForClassFields.

We can prevent erosion by adding another tsconfig which enables useDefineForClassFields and do a separate check with that. Will slow down CI by a tiny bit, for the time being, but something that we have done successfully in the past

[trevorade]

Here's a concrete example of a new runtime error from enabling useDefineForClassFields:

StorageMainService has parameter property logService which is referenced within the private method createApplicationStorage which is then called directly by the inline field initializer for applicationStorage

In general, it's not safe to execute functions/methods that reference this of a class instance that has not completed initializing. This is not a TS-specific issue. You can have these same bugs in JS.

[trevorade]

How's the team feeling about this?

If you're willing to go forward with this, we can coordinate on a time when I can sync, merge conflicts, verify all the compiler errors are resolved, re-upload, and then you can merge the PR.

[jrieken]

We are scared :-) I trust your abilities but in theory there can be issues because the init-order of properties is changing with this PR. Generally our code is sane but you never know.

Therefore we wanna go the slightly slower path and make it a team effort where each owner needs to tackle this. We have created #243049 to fix current violations and have CI checks that ensures new files/changes are future proof.

It depends on the owner but IMO individual PRs for the spelled out files are very likely to be accepted and welcomed. Thanks for kicking this off and sorry for closing this large work piece. Happy Coding

[trevorade]

Totally understood. I hope that teams steal this PR as a starting point. This PR is a bunch of rote work to just move around initialization into the constructor where needed. There's work to do after this PR for sure but this is a lot of the actual work you will have to do.

If you want to recreate this PR individually, your call.

I also added a note to your "Debt" issue with regards to the local tsc patch to report these errors early. I saw a previous PR Ron made to report these errors that was incomplete and I wasn't sure if you were using that version. See the screenshots in that issue for the tsc patch we use at google to report these errors without actually turning on useDefineForClassFields.

[a-stewart]

If it helps at all, as far as I am aware Trevor has applied the same patches to our internal version of VS Code too and it has been running with them for a while without any observed issues. We'll make sure to report back here in the event that we do discover anything though.

[tmm1]

This is quite a patch to maintain. I hope it can be revisited soon.

[jrieken]

@tmm1 we are on it #243049