Skip to content

Copilot-based TS refactors #192602

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 24 commits into from
Sep 22, 2023
Merged

Copilot-based TS refactors #192602

merged 24 commits into from
Sep 22, 2023

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Sep 8, 2023 •
edited
Loading

This PR adds copilot suggestions to various Typescript refactors and quickfixes. It also adds a separate quickfix for implicit any that lets Copilot infer types from the body of a function, which is less context than the TS codefix. However, it creates new interfaces with reasonable names and uses them where appropriate, which the TS codefix cannot.

The existing refactor/quickfixes are followed by Copilot's inline chat now:

  • Extract to X now asks copilot to provide a name for the new constant/function/type instead of showing the rename UI.
  • Implement X now asks copilot to provide reasonable implementations for inherited interface or abstract methods. This replaces the TS-generated throw new Error("Not implemented").

I think the the UX and performance are both good enough for insider use at this point, so I think it's worth merging behind the typescript.experimental.aiQuickFix flag.

Some future work:

  • Could the codefix could be skipped if there are lots of types around already?
  • Could the codefix prompt by modified to omit "Please generate new interfaces" if there lots of interfaces already declared?
  • Fix code formatting!

Extends #190690

Question and work from that PR that are now resolved:

  • Infer from types now works in JS files.
  • When given enough context, inline chat works well, especially since it will edit code if the AI produces a code sample. I tried showing multiple name suggestions in the chat panel, but I felt like it didn't add enough utility -- and the suggestions were usually all good or all bad, so taking the first suggestion works well enough.

sandersn added 14 commits July 6, 2023 13:11
@sandersn
AI quickFix noImplicitAny: replaces inferFromUsage
5e72826
@sandersn
Offer infer-types quickfix separate from Typescript one
44375f7
@sandersn
Merge branch 'main' into ai-inferFromUsage
a288cbb
@sandersn
Clean up for PR
4cb0bc5
@sandersn
AI name suggestions in Extract Constant refactor
81fad69
@sandersn
Merge branch 'main' into ai-codefixes
d8753fb
@sandersn
Include selected expression in conversatin prompt
ea5af2d
@sandersn
Support extract to function and type alias as well
2267ba8
@sandersn
Support addNameToNamelessParameter, refactor copilot code
2aaf53b 
Not done switching quick fix code to use CompositeCommand instead of
passing a followup to ApplyCodeActionCommand.
@sandersn
Rest of useful codefixes
63c84d3 
Initial hacky versions only
@sandersn
Pretty good ranges based on RefactorInfo
a0a2cb4 
Results are not pretty good yet. They are still below average.
@sandersn
Switch entirely to inline+action-based expansion
9c5b16a 
Except for a couple that still rely only on navtree-based expansion.
Lots of cleanup and standardisation.
@sandersn
Merge branch 'main' into ai-codefixes
7906b2b
@sandersn
Clean up for review
3328d54 
Remove unused, abandoned ideas.
@sandersn sandersn mentioned this pull request Sep 8, 2023
Closed
@sandersn
Copy link
Member Author

sandersn commented Sep 8, 2023

@mjbvz and @jrieken can you take a look at this?

@jrieken jrieken self-assigned this Sep 13, 2023
@jrieken jrieken requested review from mjbvz and jrieken September 13, 2023 07:47
@jrieken
Copy link
Member

jrieken commented Sep 13, 2023

@sandersn There are some eslint failures. You can see them when running yarn eslint (actually the precommit hooks should have reported them...)

I wonder if we should grow the typescript.experimental.aiQuickFix setting, e.g this isn't just quick fixes anymore and also more nuanced. Maybe a new setting like typescript.exprimental.aiCodeActions which value is a complex object would be best, e.g each key would correspond to a feature

@sandersn
Copy link
Member Author

sandersn commented Sep 13, 2023 •
edited
Loading

Ah, that's right. I completely messed up the setup of vscode's formatter, so I skipped the precommit hook. I'll go back and figure that out so the machine can fix the formatting for me.

Edit: I didn't know about typescript.enable.format, but somehow set it to false in the past. Makes it very hard to format documents with it.

@sandersn
Copy link
Member Author

OK, I added a more complex options object. If you just create the top-level object as {}, all the code actions are enabled. Then you can set individual ones to false.

I don't know if this is the usual way to do it, so please let me know if there's a standard I should follow.

jrieken
jrieken reviewed Sep 20, 2023
View reviewed changes
jrieken
jrieken previously approved these changes Sep 21, 2023
View reviewed changes
@vscodenpa vscodenpa added this to the September 2023 milestone Sep 21, 2023
@sandersn
Copy link
Member Author

I realised that the config still effectively defaulted to true, and that the altered descriptions weren't working.
I fixed that and also created custom descriptions, so please take a look at those for wording.

@sandersn
Copy link
Member Author

Linux smoke test case failed:
1) verifies that auto save triggers on shutdown

But I don't see how that could be related to my change.

@jrieken jrieken merged commit 0aecb58 into microsoft:main Sep 22, 2023
@jrieken jrieken mentioned this pull request Sep 22, 2023
Merged
@jrieken
Copy link
Member

jrieken commented Sep 22, 2023

Merging caused a build failure down the line (when doing the product build...) A revert PR is up (#193810) and this needs to be reopened and looked at again

@sandersn
Copy link
Member Author

How can I see the build failure?

yiliang114 pushed a commit to yiliang114/vscode that referenced this pull request Sep 25, 2023
@jrieken @yiliang114
Merge pull request microsoft#192602 from sandersn/ai-codefixes
98b88b3 
Copilot-based TS refactors
@sandersn sandersn deleted the ai-codefixes branch September 26, 2023 13:20
@github-actions github-actions bot locked and limited conversation to collaborators Nov 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jrieken jrieken jrieken approved these changes

@alexr00 alexr00 alexr00 approved these changes

@mjbvz mjbvz Awaiting requested review from mjbvz

Assignees

@jrieken jrieken

@mjbvz mjbvz

Labels
None yet
Projects
None yet
Milestone
September 2023
Development

Successfully merging this pull request may close these issues.
5 participants
@sandersn @jrieken @alexr00 @mjbvz @vscodenpa