Move static members to a New Type by Soreloser2 · Pull Request #55204 · dotnet/roslyn

Soreloser2 · 2021-07-28T23:53:19Z

Part of #55127.
This initial feature will include a text box to input the desired class name, and will generate a new static class in a new file (in the same folder and namespace as the file the action was invoked from).

MoveStaticMembersDemo.mp4

…ot working bc of a few bugs

No behavior created after dialog exited Still working on destination type/filename selection formatting

…ox, assuming new class (c# only, no automated tests yet)

JoeRobich

Good start. Can you add some screenshots or a gif to the PR description?

JoeRobich · 2021-07-29T00:25:25Z

src/Features/Core/Portable/MoveStaticMembers/MoveStaticMembersWithDialogCodeAction.cs

+            var optionsService = _service ?? _document.Project.Solution.Workspace.Services.GetRequiredService<IMoveStaticMembersOptionsService>();
+            return optionsService.GetMoveMembersToTypeOptions(_document, _selectedType, _selectedMember);


This never sets _ service so you don't get any benefit of caching the lookup.

Suggested change

var optionsService = _service ?? _document.Project.Solution.Workspace.Services.GetRequiredService<IMoveStaticMembersOptionsService>();

return optionsService.GetMoveMembersToTypeOptions(_document, _selectedType, _selectedMember);

_service ??= _document.Project.Solution.Workspace.Services.GetRequiredService<IMoveStaticMembersOptionsService>();

return _service.GetMoveMembersToTypeOptions(_document, _selectedType, _selectedMember);

src/Features/Core/Portable/MoveStaticMembers/MoveStaticMembersWithDialogCodeAction.cs

JoeRobich · 2021-07-29T00:30:25Z

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/MoveStaticMembersDialog.xaml

@@ -0,0 +1,105 @@
+<vs:DialogWindow


We should run the dialog through our accessibility check tool. Reach out to me or @ryzngard this week to talk you through it.

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/MoveStaticMembersDialog.xaml.cs

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/MoveStaticMembersDialogViewModel.cs

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/StaticMemberSelection.xaml

JoeRobich · 2021-07-29T00:37:40Z

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/StaticMemberSelection.xaml

@@ -0,0 +1,94 @@
+<UserControl x:Class="Microsoft.VisualStudio.LanguageServices.Implementation.MoveStaticMembers.StaticMemberSelection"


We should investigate the effort to unify the member selector controls and selectively hide unnecessary components.

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/StaticMemberSelectionViewModel.cs

src/VisualStudio/Core/Def/Implementation/PullMemberUp/MainDialog/PullMemberUpDialog.xaml

Cosifne · 2021-07-29T05:01:45Z

...ortable/CodeRefactorings/MoveStaticMembers/CSharpMoveStaticMembersCodeRefactoringProvider.cs

+        /// Test purpose only.
+        /// </summary>
+        [SuppressMessage("RoslynDiagnosticsReliability", "RS0034:Exported parts should have [ImportingConstructor]", Justification = "Used incorrectly by tests")]
+        public CSharpMoveStaticMembersCodeRefactoringProvider(IMoveStaticMembersOptionsService service) : base(service)


I wonder do we have a new MEF framework to get rid of the test accessor constructor? @sharwell might know more about this.

Cosifne · 2021-07-29T05:07:08Z

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/MoveStaticMembersDialog.xaml

@@ -0,0 +1,105 @@
+<vs:DialogWindow
+    xmlns:vs="clr-namespace:Microsoft.VisualStudio.PlatformUI;assembly=Microsoft.VisualStudio.Shell.15.0"
+    xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"


Dialog in VS has to meet some standards. (I don't if any folks in UX has mentioned to you)
Please check this and make sure your xaml meets it
https://devdivdesignguide.azurewebsites.net/windows/dialog-and-window-patterns/dialogs/

Also plz share a screenshot : )

Cosifne · 2021-07-29T05:08:52Z

src/Features/Core/Portable/MoveStaticMembers/MoveStaticMembersOptions.cs

+
+namespace Microsoft.CodeAnalysis.MoveStaticMembers
+{
+    internal class MoveStaticMembersOptions


readonly struct?

Cosifne · 2021-07-29T05:17:04Z

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/StaticMemberSelectionViewModel.cs

+        private readonly ImmutableDictionary<ISymbol, Task<ImmutableArray<ISymbol>>> _symbolToDependentsMap;
+        private readonly ImmutableDictionary<ISymbol, SymbolViewModel<ISymbol>> _symbolToMemberViewMap;
+
+        public StaticMemberSelectionViewModel(


Not necessarily needed in this PR. But please make sure after all the UI details completes, add some ViewModel test.
exmaple: https://sourceroslyn.io/#Microsoft.VisualStudio.LanguageServices.UnitTests/PullMemberUp/PullMemberUpViewModelTest.vb,a6d70fd2561a2e76

...ortable/CodeRefactorings/MoveStaticMembers/CSharpMoveStaticMembersCodeRefactoringProvider.cs

Youssef1313 · 2021-07-29T06:42:21Z

src/Features/Core/Portable/MoveStaticMembers/MoveStaticMembersWithDialogCodeAction.cs

+            var fileBanner = syntaxFacts.GetFileBanner(root);
+
+            // add annotations to the symbols that we selected so we can find them later to pull up
+            var memberNodes = moveOptions.SelectedMembers.SelectAsArray(symbol => symbol.Locations.Single().FindNode(cancellationToken));


What guarantees that .Single() won't throw?

…as the semantic model cannot get the symbol for VB declared fields from the node. Fields are added as part of the dialog box however.

…oveStaticMembers Merge main to features/MoveStaticMembers

…pendents

src/VisualStudio/Core/Def/Implementation/MoveStaticMembers/MoveStaticMembersDialog.xaml.cs

src/VisualStudio/Core/Def/ServicesVSResources.resx

src/EditorFeatures/CSharpTest/MoveStaticMembers/CSharpMoveStaticMembersTests.cs

Youssef1313 · 2021-08-04T18:11:41Z

src/EditorFeatures/CSharpTest/MoveStaticMembers/CSharpMoveStaticMembersTests.cs

+
+namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.MoveStaticMembers
+{
+    public class CSharpMoveStaticMembersTests : AbstractCSharpCodeActionTest


Better to use the new testing framework. We want to get rid of these legacy tests.

Youssef1313 · 2021-08-04T18:13:09Z

...ortable/CodeRefactorings/MoveStaticMembers/CSharpMoveStaticMembersCodeRefactoringProvider.cs

+        /// <summary>
+        /// Test purpose only.
+        /// </summary>
+        [SuppressMessage("RoslynDiagnosticsReliability", "RS0034:Exported parts should have [ImportingConstructor]", Justification = "Used incorrectly by tests")]
+        public CSharpMoveStaticMembersCodeRefactoringProvider(IMoveStaticMembersOptionsService? service) : base(service)
+        {
+        }


I'm not sure if this is okay.

Youssef1313 · 2021-08-04T18:14:44Z

src/Features/Core/Portable/MoveStaticMembers/AbstractMoveStaticMembersRefactoringProvider.cs

+
+            var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
+
+            var action = new MoveStaticMembersWithDialogCodeAction(document, span, _service, selectedType, selectedMember: selectedMembers[0]);


Can't we get the service via document.GetRequiredLanguageService<IMoveStaticMembersOptionsService>()?

src/EditorFeatures/CSharpTest/MoveStaticMembers/CSharpMoveStaticMembersTests.cs

Youssef1313 · 2021-08-04T18:31:00Z

src/EditorFeatures/TestUtilities/MoveStaticMembers/TestMoveStaticMembersService.cs

+            // just return all the selected members
+            return new MoveStaticMembersOptions(
+                _filename,
+                string.Join(".", selectedType.ContainingNamespace.ToDisplayString(), _destinationType),


You're doing string.Join, then splitting it back in the constructor. Why not pass them separately?

My idea was that the user could include a additional nested namespace(s) when they defined the type. For example, if they were moving a member from a class called SomeNamespace.InnerNamespace.Class1, and wanted to move it to a separate utility namespace, they could write Utilities.Class1Helpers in the destination selection box, and the new created class would be fully qualified as SomeNamespace.InnerNamespace.Utilities.Class1Helpers.

I think splitting the extra namespaces from the destination box (Utilities in the example) and adding them on to the containing namespace, then passing in both separately would work as well.

…ling due to the way AddMembers reuses syntax. Fixed a few bugs

…er2/roslyn into MoveStaticMembersNewType

src/EditorFeatures/TestUtilities/MoveStaticMembers/TestMoveStaticMembersService.cs

src/Features/VisualBasic/Portable/CodeRefactorings/NodeSelectionHelpers.vb

Soreloser2 · 2021-08-06T17:45:07Z

Tests are not passing because Pull Members Up is not pulling entire methods in VB. Fixing at #55450.

JoeRobich · 2021-08-09T17:02:31Z

/azp run

azure-pipelines · 2021-08-09T17:03:18Z

Azure Pipelines successfully started running 3 pipeline(s).

JoeRobich · 2021-08-09T22:24:31Z

Although GitHub checks haven't updated. Integration CI is passing https://dev.azure.com/dnceng/public/_build/results?buildId=1283655&view=results

JoeRobich · 2021-08-10T17:33:51Z

src/EditorFeatures/CSharpTest/MoveStaticMembers/CSharpMoveStaticMembersTests.cs

+        {
+            return false;
+        }
+        public static int TestMethodInt()


The lack of whitespace here may be an issue with the member puller.

JoeRobich · 2021-08-10T17:35:58Z

src/EditorFeatures/CSharpTest/MoveStaticMembers/CSharpMoveStaticMembersTests.cs

+{
+    static class Class1Helpers
+    {
+


We should open an issue for the leading whitespace in these classes.

I believe it is an extension of #54847. We can add a formatter annotation to the new class in this case, but this will still be an issue when moving to existing types

JoeRobich

Would be handy to test extracting from other types like static and abstract classes. Tests for extracting from records as well. These can come in a follow up.

Youssef1313 · 2021-08-10T17:41:12Z

...rp/Portable/CodeRefactorings/MoveStaticMembers/CSharpMoveStaticMembersRefactoringProvider.cs

+        /// <summary>
+        /// Test purpose only.
+        /// </summary>
+        [SuppressMessage("RoslynDiagnosticsReliability", "RS0034:Exported parts should have [ImportingConstructor]", Justification = "Used incorrectly by tests")]
+        public CSharpMoveStaticMembersRefactoringProvider(IMoveStaticMembersOptionsService? service) : base(service)
+        {
+        }


This constructor is incorrect based on dotnet/roslyn-analyzers#5358:

MEF-exported types should have at most one public constructor, which should either have no parameters or be marked [ImportingConstructor]

FYI @sharwell :)

I'll be meeting with him to change the test framework, it will be addressed in a follow-up PR 👍

* Move static members to a New Type (#55204) * WIP Dialog work * Rename + refactor * WIP rename, add xaml elements * WIP dialog/VM work * WIP, Reverted Pull Member up change, updated destination selection, not working bc of a few bugs * Refactoring provider now shows up and launches dialog No behavior created after dialog exited Still working on destination type/filename selection formatting * Rename/Refactor name to MoveStaticMembers. Feature working with textbox, assuming new class (c# only, no automated tests yet) * Accessibility, theming, resources, and sizing retweaks * Added VB refactoring provider. Currently does not trigger on fields, as the semantic model cannot get the symbol for VB declared fields from the node. Fields are added as part of the dialog box however. * Addressing some of the feedback * Fixed fields not being able to trigger dialog in VB * Remove unnecessary usings * Fixed nullable error * Added test classes for view model and C# impl, made VB able to get dependents * Finished Writing impl tests for C# and VB. VB tests are currently failing due to the way AddMembers reuses syntax. Fixed a few bugs * Fixed resources and removed nullable disable annotation * Adress small feedback points * Small test fix. Works with generator changes (sepearate PR) Co-authored-by: msftbot[bot] <48340428+msftbot[bot]@users.noreply.github.com> * Test framework movement (#55586) * WIP Dialog work * Rename + refactor * WIP rename, add xaml elements * WIP dialog/VM work * WIP, Reverted Pull Member up change, updated destination selection, not working bc of a few bugs * Refactoring provider now shows up and launches dialog No behavior created after dialog exited Still working on destination type/filename selection formatting * Rename/Refactor name to MoveStaticMembers. Feature working with textbox, assuming new class (c# only, no automated tests yet) * Accessibility, theming, resources, and sizing retweaks * Added VB refactoring provider. Currently does not trigger on fields, as the semantic model cannot get the symbol for VB declared fields from the node. Fields are added as part of the dialog box however. * Addressing some of the feedback * Fixed fields not being able to trigger dialog in VB * Remove unnecessary usings * Fixed nullable error * Added test classes for view model and C# impl, made VB able to get dependents * Finished Writing impl tests for C# and VB. VB tests are currently failing due to the way AddMembers reuses syntax. Fixed a few bugs * Fixed resources and removed nullable disable annotation * Adress small feedback points * Small test fix. Works with generator changes (sepearate PR) * WIP untested, refactoring references to moved members * Added tests for refactoring and extensions for C#. Will add tests for refactoring in VB, but unsure about extensions since we do not generate a module. * Make module now for VB, add refactor tests * Updated C# tests to a new framework. May need to rewrite VB test file in C# to set some readonly properties * Changed VB tests to new Framework. Revealed an error in the way vb members are moved * Fixed VB tests by removing shared modifier when moving to a module * Fixed more tests to account for member spacing innaccuracy * Refactored tests, added vb extension tests * Added tests for file banner and moving from a static class/module * Added tests to enure operator and static constructors are not able to be selected or trigger refactoring * Make service field readonly * Fixed some code warnings and reverted to before member refactoring, removing a few tests in the process Co-authored-by: msftbot[bot] <48340428+msftbot[bot]@users.noreply.github.com> * Fix api change * Update FeatureResources xlf * Changed tests to use new default accessibility modifier for new documents * Move Static Members: Refactor moved member references (#55581) * WIP Dialog work * Rename + refactor * WIP rename, add xaml elements * WIP dialog/VM work * WIP, Reverted Pull Member up change, updated destination selection, not working bc of a few bugs * Refactoring provider now shows up and launches dialog No behavior created after dialog exited Still working on destination type/filename selection formatting * Rename/Refactor name to MoveStaticMembers. Feature working with textbox, assuming new class (c# only, no automated tests yet) * Accessibility, theming, resources, and sizing retweaks * Added VB refactoring provider. Currently does not trigger on fields, as the semantic model cannot get the symbol for VB declared fields from the node. Fields are added as part of the dialog box however. * Addressing some of the feedback * Fixed fields not being able to trigger dialog in VB * Remove unnecessary usings * Fixed nullable error * Added test classes for view model and C# impl, made VB able to get dependents * Finished Writing impl tests for C# and VB. VB tests are currently failing due to the way AddMembers reuses syntax. Fixed a few bugs * Fixed resources and removed nullable disable annotation * Adress small feedback points * Small test fix. Works with generator changes (sepearate PR) * WIP untested, refactoring references to moved members * Added tests for refactoring and extensions for C#. Will add tests for refactoring in VB, but unsure about extensions since we do not generate a module. * Make module now for VB, add refactor tests * Updated C# tests to a new framework. May need to rewrite VB test file in C# to set some readonly properties * Changed VB tests to new Framework. Revealed an error in the way vb members are moved * Fixed VB tests by removing shared modifier when moving to a module * Fixed more tests to account for member spacing innaccuracy * Refactored tests, added vb extension tests * Added tests for file banner and moving from a static class/module * Added tests to enure operator and static constructors are not able to be selected or trigger refactoring * Make service field readonly * Added support for alias refactoring + tests * Refactored references on source type. Added simplifier annotation to remove unnecessary qualifications. * Now correctly refactors references using symbol annotations and expansions for extension methods, should address conflicting imports issues * Fixed simplifier incorrectly simplifying references inside moved members * Added more tests, generic class refactoring currently isn't working * Added support for generic class member access, copying only the type arguments we moved from the methods * Fixed tests by adding accesibility modifier * Addressed feedback, fixed multiple extension method bug by using docEditor * Changed new type to create the same kind (class/Module) as the source type, refactored tests Co-authored-by: msftbot[bot] <48340428+msftbot[bot]@users.noreply.github.com> * Move Static Members: Show destination name (#55650) * Now shows the fully qualified name when creating a new type. Discovered an issue with duplicating a root namespace, not fixed here. * Reformatted new type name message * Write tests that utilize root namespace. One is failing as it depends on reference refactoring, and is not merged with the reference refactoring branch currently * Changed way file names are generated so added namespaces aren't included, refactored options getting for tests * Moved destination selection box a little bit to the right to align with message * Changed tests to reflect module changes * Removed theming (#56013) * Move static members Root Namespace + OK button enabled fixes (#56035) * Added tests and changed root namespace filtering. Removes crash bug. * Disabled "OK" button when destination is invalid. Added ability to select no members (will create a new empty class) Co-authored-by: msftbot[bot] <48340428+msftbot[bot]@users.noreply.github.com> Co-authored-by: Joey Robichaud <jorobich@microsoft.com> Co-authored-by: Joey Robichaud <joseph.robichaud@microsoft.com> Co-authored-by: dotnet bot <dotnet-bot@dotnetfoundation.org>

Soren Dahl added 7 commits July 14, 2021 11:19

WIP Dialog work

952d435

Rename + refactor

3eb8edb

WIP rename, add xaml elements

44c49b6

WIP dialog/VM work

a36baf8

WIP, Reverted Pull Member up change, updated destination selection, n…

ddfbb13

…ot working bc of a few bugs

Refactoring provider now shows up and launches dialog

1cb6b2f

No behavior created after dialog exited Still working on destination type/filename selection formatting

Rename/Refactor name to MoveStaticMembers. Feature working with textb…

5b188e6

…ox, assuming new class (c# only, no automated tests yet)

Soreloser2 requested a review from a team as a code owner July 28, 2021 23:53

ghost added the Area-IDE label Jul 28, 2021

Soreloser2 mentioned this pull request Jul 28, 2021

Move Static Members to Another Type #55129

Closed

Soreloser2 added the Feature - Move Static Members label Jul 29, 2021

JoeRobich reviewed Jul 29, 2021

View reviewed changes

Cosifne reviewed Jul 29, 2021

View reviewed changes

...ortable/CodeRefactorings/MoveStaticMembers/CSharpMoveStaticMembersCodeRefactoringProvider.cs Outdated Show resolved Hide resolved

Youssef1313 reviewed Jul 29, 2021

View reviewed changes

Soren Dahl and others added 8 commits July 29, 2021 12:25

Accessibility, theming, resources, and sizing retweaks

b87c671

Added VB refactoring provider. Currently does not trigger on fields, …

f215bef

…as the semantic model cannot get the symbol for VB declared fields from the node. Fields are added as part of the dialog box however.

Addressing some of the feedback

dfe55f5

Fixed fields not being able to trigger dialog in VB

50ff662

Remove unnecessary usings

5944640

Fixed nullable error

9af7a12

Merge pull request dotnet#55324 from dotnet/merges/main-to-features/M…

bc4bfef

…oveStaticMembers Merge main to features/MoveStaticMembers

Added test classes for view model and C# impl, made VB able to get de…

75cd384

…pendents

Soreloser2 requested a review from a team as a code owner August 3, 2021 23:53

Merge branch 'features/MoveStaticMembers' into MoveStaticMembersNewType

0fcc387

JoeRobich reviewed Aug 4, 2021

View reviewed changes

Youssef1313 reviewed Aug 4, 2021

View reviewed changes

src/EditorFeatures/CSharpTest/MoveStaticMembers/CSharpMoveStaticMembersTests.cs Show resolved Hide resolved

Youssef1313 reviewed Aug 4, 2021

View reviewed changes

Soren Dahl added 3 commits August 4, 2021 17:18

Finished Writing impl tests for C# and VB. VB tests are currently fai…

4f6c03f

…ling due to the way AddMembers reuses syntax. Fixed a few bugs

Merge branch 'MoveStaticMembersNewType' of https://github.com/Sorelos…

30b1a79

…er2/roslyn into MoveStaticMembersNewType

Fixed resources and removed nullable disable annotation

dc1edd4

JoeRobich reviewed Aug 5, 2021

View reviewed changes

src/EditorFeatures/TestUtilities/MoveStaticMembers/TestMoveStaticMembersService.cs Outdated Show resolved Hide resolved

src/Features/VisualBasic/Portable/CodeRefactorings/NodeSelectionHelpers.vb Outdated Show resolved Hide resolved

Soren Dahl added 2 commits August 5, 2021 11:28

Adress small feedback points

08ca13c

Small test fix. Works with generator changes (sepearate PR)

40dedaf

JoeRobich force-pushed the features/MoveStaticMembers branch from 71183e2 to 845e2f4 Compare August 9, 2021 17:02

JoeRobich reviewed Aug 10, 2021

View reviewed changes

JoeRobich approved these changes Aug 10, 2021

View reviewed changes

Soreloser2 merged commit 6fbd753 into dotnet:features/MoveStaticMembers Aug 10, 2021

Youssef1313 reviewed Aug 10, 2021

View reviewed changes

		var optionsService = _service ?? _document.Project.Solution.Workspace.Services.GetRequiredService<IMoveStaticMembersOptionsService>();
		return optionsService.GetMoveMembersToTypeOptions(_document, _selectedType, _selectedMember);

		@@ -0,0 +1,94 @@
		<UserControl x:Class="Microsoft.VisualStudio.LanguageServices.Implementation.MoveStaticMembers.StaticMemberSelection"


		var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();

		var action = new MoveStaticMembersWithDialogCodeAction(document, span, _service, selectedType, selectedMember: selectedMembers[0]);

Conversation

Soreloser2 commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeRobich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Soreloser2 commented Aug 6, 2021

Uh oh!

JoeRobich commented Aug 9, 2021

Uh oh!

azure-pipelines bot commented Aug 9, 2021

Uh oh!

JoeRobich commented Aug 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoeRobich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Soreloser2 commented Jul 28, 2021 •

edited

Loading