Replace deprecated uses of RenameDocumentAsync/RenameSymbolAsync by MiYanni · Pull Request #8784 · dotnet/project-system

MiYanni · 2023-01-10T00:49:05Z

Replaces the use of the deprecated overload for RenameDocumentAsync/RenameSymbolAsync. The new overload takes in a DocumentRenameOptions/SymbolRenameOptions. I looked at the code in Roslyn and these options are simply defaulted (new instance with no parameters) in our current use of the deprecated overload. Given that, for each of our classes, I simply made a static default field for the class to use.

Microsoft Reviewers: Open in CodeFlow

MiYanni · 2023-01-10T00:49:34Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/RoslynServices.cs

-                return SyntaxFactsServicesImpl.FirstOrDefault()?.Value;
-            }
-        }
+        private ISyntaxFactsService? SyntaxFactsService => SyntaxFactsServicesImpl.FirstOrDefault()?.Value;


No functional change here.

MiYanni · 2023-01-10T00:50:58Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/RoslynServices.cs

-#pragma warning disable CS0618 // Type or member is obsolete https://github.com/dotnet/project-system/issues/8591
-            return RoslynRenamer.Renamer.RenameSymbolAsync(solution, symbol, newName, solution.Workspace.Options, token);
-#pragma warning restore CS0618 // Type or member is obsolete
+            return Renamer.RenameSymbolAsync(solution, symbol, s_renameOptions, newName, token);


We never updated the options provided by solution.Workspace.Options. Behind the scenes, it is just a new'ed SymbolRenameOptions but there is a translation layer between that class (which is the newer options class) and the older one provided by the Options property (which is also deprecated).

My reading of this is that it should pull settings from the solution. Something like:

Suggested change

return Renamer.RenameSymbolAsync(solution, symbol, s_renameOptions, newName, token);

SymbolRenameOptions options = new(

RenameOverloads: solution.Options.GetOption(RenameOptions.RenameOverloads),

RenameInStrings: solution.Options.GetOption(RenameOptions.RenameInStrings),

RenameInComments: solution.Options.GetOption(RenameOptions.RenameInComments),

RenameFile: false);

return Renamer.RenameSymbolAsync(solution, symbol, options, newName, token);

solution.Workspace.Options ends up reading the OptionSet from the underlying solution, so I think the above would avoid a change of behaviour in this code.

We could consider extending our dialog to display these options too.

drewnoakes

Thanks for picking this up. I'm not sure we can ignore the options though.

drewnoakes · 2023-01-10T07:16:20Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/RoslynServices.cs

-#pragma warning disable CS0618 // Type or member is obsolete https://github.com/dotnet/project-system/issues/8591
-            return RoslynRenamer.Renamer.RenameSymbolAsync(solution, symbol, newName, solution.Workspace.Options, token);
-#pragma warning restore CS0618 // Type or member is obsolete
+            return Renamer.RenameSymbolAsync(solution, symbol, s_renameOptions, newName, token);


My reading of this is that it should pull settings from the solution. Something like:

Suggested change

return Renamer.RenameSymbolAsync(solution, symbol, s_renameOptions, newName, token);

SymbolRenameOptions options = new(

RenameOverloads: solution.Options.GetOption(RenameOptions.RenameOverloads),

RenameInStrings: solution.Options.GetOption(RenameOptions.RenameInStrings),

RenameInComments: solution.Options.GetOption(RenameOptions.RenameInComments),

RenameFile: false);

return Renamer.RenameSymbolAsync(solution, symbol, options, newName, token);

solution.Workspace.Options ends up reading the OptionSet from the underlying solution, so I think the above would avoid a change of behaviour in this code.

We could consider extending our dialog to display these options too.

drewnoakes · 2023-01-10T07:21:55Z

...isualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Rename/FileMoveNotificationListener.cs

-#pragma warning disable CS0618 // Type or member is obsolete https://github.com/dotnet/project-system/issues/8591
-                    Renamer.RenameDocumentActionSet documentAction = await Renamer.RenameDocumentAsync(oldDocument, null!, documentFolders);
-#pragma warning restore CS0618 // Type or member is obsolete
+                    Renamer.RenameDocumentActionSet documentAction = await Renamer.RenameDocumentAsync(oldDocument, s_renameOptions, null, documentFolders);


Similarly I would expect this to be:

Suggested change

Renamer.RenameDocumentActionSet documentAction = await Renamer.RenameDocumentAsync(oldDocument, s_renameOptions, null, documentFolders);

DocumentRenameOptions options = new(

RenameMatchingTypeInStrings: optionSet.GetOption(RenameOptions.RenameInStrings),

RenameMatchingTypeInComments: optionSet.GetOption(RenameOptions.RenameInComments));

Renamer.RenameDocumentActionSet documentAction = await Renamer.RenameDocumentAsync(oldDocument, options, newDocumentName: null, documentFolders);

The exact same code to do this exists in Roslyn but is also marked obsolete... I'm trying to understand why now.
https://github.com/dotnet/roslyn/blob/4d0af36500e90653f5086866acccf2a73567aaa0/src/Workspaces/Core/Portable/Rename/Renamer.cs#L27-L39

[Obsolete] private static SymbolRenameOptions GetSymbolRenameOptions(OptionSet optionSet) => new( RenameOverloads: optionSet.GetOption(RenameOptions.RenameOverloads), RenameInStrings: optionSet.GetOption(RenameOptions.RenameInStrings), RenameInComments: optionSet.GetOption(RenameOptions.RenameInComments), RenameFile: false); [Obsolete] private static DocumentRenameOptions GetDocumentRenameOptions(OptionSet optionSet) => new( RenameMatchingTypeInStrings: optionSet.GetOption(RenameOptions.RenameInStrings), RenameMatchingTypeInComments: optionSet.GetOption(RenameOptions.RenameInComments));

Where can the user set the value of these options for this project system rename operation?

They cannot, as it currently stands. We could add UI for this, as called out above. Would those options default to the solution's setting values? Would changing them also change the solution's settings?

The options don't have values associated with a solution. If there is no way to set these in the Project System UI then use any setting that makes the most sense for this feature.

Inline Rename feature sets these values from its UI and remembers those settings across VS sessions.

I don't think these should necessarily be used by project system though.

In lieu of creating new UI or linking to existing UI/settings, for the purposes of simply removing deprecated API calls, would using a defaulted DocumentRenameOptions/SymbolRenameOptions be adequate to maintain current functionality? If we want to improve the experience, I'm down for that. However, we should make that into a new issue/feature and prioritize it with our other work.

would using a defaulted DocumentRenameOptions/SymbolRenameOptions be adequate to maintain current functionality?

Yup. That should do.

@drewnoakes Do you think it is okay if we use the defaults for these types (DocumentRenameOptions/SymbolRenameOptions) for now, and make an issue to add a feature for exposing these options to users?

would using a defaulted DocumentRenameOptions/SymbolRenameOptions be adequate to maintain current functionality?

Yup. That should do.

@tmat Right now the way we're using the deprecated API means that Roslyn will pick up the OptionSet from document.Project.Solution.Options. Are you saying that will currently always be the same as the options we get by doing new DocumentRenameOptions()?

drewnoakes

My concerns were addressed following the discussion and input from tmat. This is a great addition to VS.

MiYanni · 2023-02-17T01:20:33Z

@drewnoakes Created a feature request to expose the document and symbol rename options: #8858

Replaces the use of the deprecated overload for RenameDocumentAsync.

bbe7b4f

MiYanni requested a review from a team as a code owner January 10, 2023 00:49

MiYanni commented Jan 10, 2023

View reviewed changes

MiYanni changed the title ~~Replace deprecated uses of RenameDocumentAsync~~ Replace deprecated uses of RenameDocumentAsync/RenameSymbolAsync Jan 10, 2023

drewnoakes reviewed Jan 10, 2023

View reviewed changes

drewnoakes approved these changes Feb 17, 2023

View reviewed changes

MiYanni mentioned this pull request Feb 17, 2023

Add options for document and symbol renaming #8858

Open

MiYanni merged commit fa3e9f4 into dotnet:main Feb 17, 2023

ghost added this to the 17.6 milestone Feb 17, 2023

-            return Renamer.RenameSymbolAsync(solution, symbol, s_renameOptions, newName, token);
+            SymbolRenameOptions options = new(
+                RenameOverloads: solution.Options.GetOption(RenameOptions.RenameOverloads),
+                RenameInStrings: solution.Options.GetOption(RenameOptions.RenameInStrings),
+                RenameInComments: solution.Options.GetOption(RenameOptions.RenameInComments),
+                RenameFile: false);
+            return Renamer.RenameSymbolAsync(solution, symbol, options, newName, token);

-                    Renamer.RenameDocumentActionSet documentAction = await Renamer.RenameDocumentAsync(oldDocument, s_renameOptions, null, documentFolders);
+                    DocumentRenameOptions options = new(
+                        RenameMatchingTypeInStrings: optionSet.GetOption(RenameOptions.RenameInStrings),
+                        RenameMatchingTypeInComments: optionSet.GetOption(RenameOptions.RenameInComments));
+                    Renamer.RenameDocumentActionSet documentAction = await Renamer.RenameDocumentAsync(oldDocument, options, newDocumentName: null, documentFolders);

Conversation

MiYanni commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

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!

drewnoakes 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!

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!

tmat Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drewnoakes left a comment

Choose a reason for hiding this comment

Uh oh!

MiYanni commented Feb 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MiYanni commented Jan 10, 2023 •

edited

Loading

tmat Jan 22, 2023 •

edited

Loading