Move Static Members: Refactor moved member references#55581
Move Static Members: Refactor moved member references#55581Soreloser2 merged 45 commits intodotnet:features/MoveStaticMembersfrom
Conversation
…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)
…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
…ling due to the way AddMembers reuses syntax. Fixed a few bugs
…er2/roslyn into MoveStaticMembersNewType
…icMembersNewType
… refactoring in VB, but unsure about extensions since we do not generate a module.
… in C# to set some readonly properties
src/Features/Core/Portable/MoveStaticMembers/MoveStaticMembersWithDialogCodeAction.cs
Outdated
Show resolved
Hide resolved
src/Features/Core/Portable/MoveStaticMembers/MoveStaticMembersWithDialogCodeAction.cs
Outdated
Show resolved
Hide resolved
| .WithAdditionalAnnotations(Simplification.Simplifier.Annotation)); | ||
| } | ||
| } | ||
| else if (syntaxFacts.IsIdentifierName(refNode)) |
There was a problem hiding this comment.
what about a generic name. for example. Foo<int>()? do we have tests for this?
| var expression = syntaxFacts.GetExpressionOfMemberAccessExpression(refNode.Parent); | ||
| if (expression != null) | ||
| { | ||
| docEditor.ReplaceNode(expression, (node, generator) => generator.TypeExpression(newType) |
There was a problem hiding this comment.
i'm not certain how it's safe to replace a general expression with a TypeExpression. what if hte expression is (1 + 1)?
There was a problem hiding this comment.
All of the member access expressions in this block should strictly be static non-extension members (also not operators or static constructors, as we don't allow movement of those at all), so the expression for the member access should always be a type name. I don't think expressions like (1 + 1) could ever call a static non-extension method.
…sions for extension methods, should address conflicting imports issues
…arguments we moved from the methods
|
@CyrusNajmabadi Could you take a look at these changes when you get time? I believe I addressed the imports and potential conflict issue by using the simplifier and add imports annotation. There also should be more handling for the generic class case, and tests for both generic classes and methods. |
…t/roslyn into MoveStaticMembersNewType
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| // refactor references across the entire solution | ||
| var memberReferenceLocations = await FindMemberReferencesAsync(moveOptions.SelectedMembers, newDoc.Project.Solution, cancellationToken).ConfigureAwait(false); | ||
| var projectToLocations = memberReferenceLocations.ToLookup(loc => loc.location.Document.Project.Id); | ||
| var reReferencedSolution = await RefactorReferencesAsync(projectToLocations, newDoc.Project.Solution, newType!, typeArgIndices, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
not sure why ! is necessary. you did a GetRequiredDeclaredSymbol. Either cast to INamedTypeSymbol, or do the 'as' check, but then check the result. doing 'as' and tehn suppressing NRT is a bad smell.
| /// Modules cannot be generic however, so for VB we choose class if there are generic type params. | ||
| /// This could be later extended to a language-service feature if there is more complex behavior. | ||
| /// Finds what type kind new type should be. In case of C#, returning both class and module create a class | ||
| /// For VB, we want a module unless there are class type params, in which case we want a class |
There was a problem hiding this comment.
interesting. if we create a module that Widely brings the members into scope in VB. it coudl really impact lots of other locations. is that really what we want?
There was a problem hiding this comment.
That's an interesting point that I hadn't really thought of before. I mostly looked at Module as the the way to make a class static. Right now all the references to moved members are fully qualified (and include the module name), then get simplified as necessary, so the members being referenced would be correctly handled, but I could see how it could introduce a conflict for some other references that don't want to access the moved members, but now suddenly have the module in scope.
I think the best option would be to change the new type creation to only make a module if we're moving members from a module. This way, we still support extension methods, as extensions can't be in a class for VB. This change could replace the logic around generics, and we would no longer need to modify the static/non static status of members when moving from a class to a module, as the source and destination class type should always be the same.
There was a problem hiding this comment.
Seems sound! Note: I don't remember the vb rules around extension methods (they might be required to be in a module), so potentially keep that in mind.
| // organize by project first, so we can solve one project at a time | ||
| var project = solution.GetRequiredProject(projectId); | ||
| var documentToLocations = referencesForProject.ToLookup(reference => reference.location.Document.Id); | ||
| foreach (var (docId, referencesForDoc) in documentToLocations) |
There was a problem hiding this comment.
i recommend grabbing teh compilation above this foreach (and add a GK.keepalive below) to ensure that we don't constantly make/release/make/release the compilatino we're processing.
There was a problem hiding this comment.
ignore this. you never use compilation/semanticmodel below.
| project = doc.Project; | ||
| } | ||
|
|
||
| solution = project.Solution; |
There was a problem hiding this comment.
on interesting. is this intentional? you're updating the solution in place that you're reading new compilations from. that means we can't reuse any semantic info? can you avoid that and build a new solution while only reading the old one?
| foreach (var (docId, referencesForDoc) in documentToLocations) | ||
| { | ||
| var doc = project.GetRequiredDocument(docId); | ||
| doc = await FixReferencesSingleDocumentAsync( |
There was a problem hiding this comment.
get the SemanticModel above this, and GC.KeepAlive below it.
There was a problem hiding this comment.
ignore this. you never use compilation/semanticmodel below.
| // extension methods should be changed into their static class versions with | ||
| // full qualifications, then the qualification changed to the new type | ||
| if (syntaxFacts.IsNameOfAnyMemberAccessExpression(refNode) && | ||
| syntaxFacts.IsAnyMemberAccessExpression(refNode!.Parent) && |
There was a problem hiding this comment.
| syntaxFacts.IsAnyMemberAccessExpression(refNode!.Parent) && | |
| syntaxFacts.IsAnyMemberAccessExpression(refNode?.Parent) && |
| // full qualifications, then the qualification changed to the new type | ||
| if (syntaxFacts.IsNameOfAnyMemberAccessExpression(refNode) && | ||
| syntaxFacts.IsAnyMemberAccessExpression(refNode!.Parent) && | ||
| syntaxFacts.IsInvocationExpression(refNode.Parent!.Parent)) |
There was a problem hiding this comment.
| syntaxFacts.IsInvocationExpression(refNode.Parent!.Parent)) | |
| syntaxFacts.IsInvocationExpression(refNode.Parent?.Parent)) |
| var extensionMethodInvocation = refNode.GetRequiredParent().GetRequiredParent(); | ||
| var expandedExtensionInvocation = await Simplifier.ExpandAsync( | ||
| extensionMethodInvocation, | ||
| doc, |
There was a problem hiding this comment.
oh. you do use semantics. yeah, do what i said above and grab out the compilation/semanticmodel it will ensure that each call to Expand doesn't create/throw-away the semantic model. for this doc.
| } | ||
| } | ||
|
|
||
| return doc.WithSyntaxRoot(root); |
There was a problem hiding this comment.
i skipped all this logic. seems acceptable given the level fo testing :)
| var symbolRefs = await Task.WhenAll(tasks).ConfigureAwait(false); | ||
| return symbolRefs | ||
| .Flatten() | ||
| .SelectMany(refSymbol => refSymbol.Locations.Select(loc => (loc, refSymbol.Definition.IsExtensionMethod()))) |
There was a problem hiding this comment.
i believe we have a property on the FindRefs results stating if it is implicitly referenced or not. you should likely filter those out here.
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
conditionally signing off with the changes i asked for in teh PR :) looks great!
src/Features/Core/Portable/MoveStaticMembers/MoveStaticMembersWithDialogCodeAction.cs
Show resolved
Hide resolved
* 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>
Part of #55127
During the Move Static Members action, this PR now adds support to refactor references to the moved members from other files.
someVar.Foo()) will be expanded to static method calls so that they are now member access expressions (OldClass.Foo(someVar)), and their old class type will be replaced with the new class type (NewClass.Foo(someVar)).OriginalClass.Foo()) will have the old type expression replaced with the new named type (NewClass.Foo()).Foo()) will be converted to member access expressions with the new class type as the expression (NewClass.Foo()).In all cases, we add simplifying annotations, as well as import annotations, so that our new type gets correctly imported if necessary (as long as no conflicts get introduced) and our expanded types and extension methods get reduced back into their simplest forms.
A more expansive example: