Quick action "Move type to new file" does not fix header or remove unneeded using statements

[vsfeedback]

This issue has been moved from a ticket on Developer Community.

---

[severity:It's more difficult to complete my work]
When using the "Move type to new file" quick action to move a type to a new file, the copyright header from the old file is copied into the new file as well as the usings statements -- even if they aren't needed in the new file.

This presents two problems:
The copyright header has to be cleaned up manually since it is no longer compliant with SA1638 after the move.
Then the usings statements may need to be cleaned up if they are not needed.

While both of these things can be accomplished by subsequent quick actions after the move, this is unnecessary work that should just be handled automatically by the quick action without requiring manual intervention.

It's been a while but I do not recall ReSharper having this issue with its equivalent refactor action.

---

Original Comments

Feedback Bot on 11/23/2021, 09:53 PM:

We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.

Feedback Bot on 11/24/2021, 09:40 AM:

Thank you for sharing your feedback! Our teams prioritize action on product issues with broad customer impact. See details at: https://docs.microsoft.com/en-us/visualstudio/ide/report-a-problem?view=vs-2019#faq. In case you need answers to common questions or need assisted support, be sure to use https://visualstudio.microsoft.com/vs/support/. We’ll keep you posted on any updates to this feedback.

Feedback Bot on 12/1/2021, 10:47 AM:

This issue is currently being investigated. Our team will get back to you if either more information is needed, a workaround is available, or the issue is resolved.

---

Original Solutions

(no solutions)

[ryzngard]

@sharwell The two issues here seem counter to my understanding of what we do with refactorings:

1. Usings in new document are removed if not used. The original document may only have usings that remain in the new document removed if they aren't needed. See https://github.com/dotnet/roslyn/blob/b0cd71f82fbada617a7cb5354f52bbc478c1ff1d/src/EditorFeatures/CSharpTest/CodeActions/MoveType/MoveTypeTests.MoveToNewFile.cs#L797-999

2. We always have kept the header for moving a type. Is it preferable we apply header styling if applicable to the new file? I don't ever see a time where we remove the header here... that won't help fix SA1638 for this user.

Thoughts on closing this as by design or are there changes we'd like here?

[ghost]

Just to contrast with what R# does -- when I move a type to a new file it handles these things for me automatically. It will generate a new header in the new file meeting StyleCop rules and will automatically coalesce using statements in the new file to be only what's needed. (I don't remember if it does the same in the original file WRT usings.) There's a lot less busywork involved -- that's for sure since I don't need to worry about manually fixing these things every time I do a move. (It also doesn't help that VS doesn't open the new file so I might move and forget to change until my build starts getting a bunch of warnings from unresolved StyleCop errors.)

[ryzngard]

and will automatically coalesce using statements in the new file to be only what's needed.

To be clear, we do that. We just limit what usings are removed from the original file.

it will generate a new header in the new file meeting StyleCop rules

We have an editorconfig setting for file header which I think we could use. If that's not set up though likely we would leave it as the same header. At most... maybe search the header string for the file name and update to the new one. I'm not fully sure though.

[davidwengier]

If that's not set up though likely we would leave it as the same header

Thats how the new document formatting service works, so I suspect this whole issue is fixed if "Move type to new file" calls INewDocumentFormattingService after its created the document. I'm surprised I didn't do that already

[ryzngard]

There was a lot of contention for that... apparently there's very specific requirements around usings etc. We could create a blank document and do it then port the new code though?

#58977 was my attempt that got pushback

[davidwengier]

I don't remember that PR, but the only pushback I can see on it is irrelevant: the argument against "formatting a document" is about not calling Formatter.Format. The new document formatting service, which you were calling, is not that, and is absolutely used in other code actions, like Extract Base, without issue.

[ryzngard]

Do we need to format past that? Maybe I misunderstood what the pushback was. I assumed if we used the new document service we'd have to format. Doubly so if we did things like enforce namespace rules. If we don't have to format I'm happy to reopen with just the new document service call

[davidwengier]

The new document format does a subset of things, to match what the code action engine does, after each provider. I don't know the specifics, I just did what Sam told me to do :)

Namespace rules are correctly handled, as that refactoring now just de-dents rather than doing a proper format.

[CyrusNajmabadi]

Fixed with #76270. As andrew mentioned, we already properly clean up usings/imports. The linked pr ensures we fixup filenames in a header as well.