Skip to content
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.

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

Pull methodName Up refactoring no longer available since release 1.38.2 #2381

Open
JeremyCaron opened this issue Apr 12, 2022 · 7 comments
Open

Comments

@JeremyCaron
Copy link

JeremyCaron commented Apr 12, 2022

Issue Description

The "Pull methodName Up" refactoring is no longer available on methods. This is one of the refactorings I use the most and have had to revert to 1.24.1 to use it.

Steps to Reproduce

Create an empty class that implements an empty interface. Add a method to the class and Ctrl-. to get the refactorings list. It should show "Pull methodName Up" but does not.

Expected Behavior

"Pull methodName Up" refactoring option should be available in the list.

Actual Behavior

Logs

OmniSharp log

Starting OmniSharp server at 4/11/2022, 9:14:31 PM Target: c:\code\allocate.co\allocate-web-api\allocate-web-api.sln

OmniSharp server started with .NET 6.0.100
.
Path: c:\Users\Jeremy.vscode\extensions\ms-dotnettools.csharp-1.24.2-win32-x64.omnisharp\1.38.2-net6.0\OmniSharp.dll
PID: 6864

Starting OmniSharp on Windows 10.0.19043.0 (x64)
info: OmniSharp.Services.DotNetCliService
Checking the 'DOTNET_ROOT' environment variable to find a .NET SDK
info: OmniSharp.Services.DotNetCliService
Using the 'dotnet' on the PATH.
info: OmniSharp.Services.DotNetCliService
DotNetPath set to dotnet
info: OmniSharp.MSBuild.Discovery.MSBuildLocator
Located 1 MSBuild instance(s)
1: .NET Core SDK 6.0.100 17.0.0 - "C:\Program Files\dotnet\sdk\6.0.100"
info: OmniSharp.MSBuild.Discovery.MSBuildLocator
Registered MSBuild instance: .NET Core SDK 6.0.100 17.0.0 - "C:\Program Files\dotnet\sdk\6.0.100"
info: OmniSharp.WorkspaceInitializer
Invoking Workspace Options Provider: OmniSharp.Roslyn.CSharp.Services.CSharpFormattingWorkspaceOptionsProvider, Order: 0
info: OmniSharp.MSBuild.ProjectSystem

... loading lots of projects - snipped...

Received response for /quickinfo but could not find request.

C# log

Installing C# dependencies... Platform: win32, x86_64

Downloading package 'OmniSharp for Windows (.NET 6 / x64)' (39624 KB).................... Done!
Validating download...
Integrity Check succeeded.
Installing package 'OmniSharp for Windows (.NET 6 / x64)'

Finished

Environment information

VSCode version: 1.66.1
C# Extension: 1.24.2

Dotnet Information .NET SDK (reflecting any global.json): Version: 6.0.100 Commit: 9e8b04bbff

Runtime Environment:
OS Name: Windows
OS Version: 10.0.19043
OS Platform: Windows
RID: win10-x64
Base Path: C:\Program Files\dotnet\sdk\6.0.100\

Host (useful for support):
Version: 6.0.0
Commit: 4822e3c3aa

.NET SDKs installed:
5.0.301 [C:\Program Files\dotnet\sdk]
6.0.100 [C:\Program Files\dotnet\sdk]

.NET runtimes installed:
Microsoft.AspNetCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.AspNetCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
Microsoft.NETCore.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.NETCore.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
Microsoft.WindowsDesktop.App 5.0.7 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
Microsoft.WindowsDesktop.App 6.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

To install additional .NET runtimes or SDKs:
https://aka.ms/dotnet-download

Visual Studio Code Extensions
Extension Author Version
bar olback 0.2.0
Bookmarks alefragnani 13.2.4
codestream CodeStream 12.15.0
copilot GitHub 1.12.5517
csharp ms-dotnettools 1.24.2
csharpextensions jchannon 1.3.1
csharpsortusings jongrant 0.0.3
debugger-for-chrome msjsdiag 4.13.0
docomment k--kato 0.1.30
dotnet-core-commands matijarmk 1.0.6
dotnet-core-essentials KishoreIthadi 0.0.8
dotnet-test-explorer formulahendry 0.7.7
gitlab-workflow GitLab 3.42.0
gitlens eamodio 12.0.5
jupyter ms-toolsai 2022.3.1000901801
markdown-pdf yzane 1.4.4
markdown-preview-enhanced shd101wyy 0.6.2
msbuild-project-tools tintoy 0.4.3
namespace adrianwilczynski 1.1.2
npm-intellisense christian-kohler 1.4.1
open-html-in-browser peakchen90 2.1.9
powershell ms-vscode 2021.12.0
python ms-python 2022.4.1
remote-containers ms-vscode-remote 0.231.5
remote-wsl ms-vscode-remote 0.66.0
sort-lines Tyriar 1.9.1
theme-cobalt2 wesbos 2.2.5
todo-tree Gruntfuggly 0.0.215
vetur octref 0.35.0
vscode-coverage-gutters ryanluker 2.9.1
vscode-docker ms-azuretools 1.21.0
vscode-duplicate mrmlnc 1.2.1
vscode-eslint dbaeumer 2.2.2
vscode-markdownlint DavidAnson 0.47.0
vscode-npm-script eg2 0.3.24
vscode-nuget-package-manager jmrog 1.1.6
vscode-pylance ms-python 2022.4.0
@JeremyCaron JeremyCaron changed the title Version 1.24.2 broke the "Pull <methodName> Up" refactoring on a method (1.24.3 is still broken) Version 1.24.2 broke the "Pull [methodName] Up" refactoring on a method (1.24.3 is still broken) Apr 12, 2022
@filipw
Copy link
Member

filipw commented Apr 12, 2022

I will need to check this, however it was never intended to work in the first place.
We even have code and tests to explicitly disable it that has been in place for 3 years now.

actionName != "PullMemberUpWithDialogCodeAction"; // Blacklisted because doesn't give additional value over non UI generate type (when defaults used.)

[Fact]
// Theres generate type without options that gives ~same result with default ui. No point to bring this.
public async Task Blacklists_pull_members_up_with_UI()
{
const string code =
@"
public class Class1: BaseClass
{
public string Foo[||] { get; set; }
}
public class BaseClass {}";
var response = await FindRefactoringNamesAsync(code);
Assert.DoesNotContain("Pull 'Foo' up -> Pull members up to base type...", response);
}

It might have worked "by accident" and got removed with the recent changes to how we interact with Roslyn.

@filipw filipw transferred this issue from dotnet/vscode-csharp Apr 12, 2022
@filipw filipw changed the title Version 1.24.2 broke the "Pull [methodName] Up" refactoring on a method (1.24.3 is still broken) Pull methodName Up refactoring no longer available since release 1.38.2 Apr 12, 2022
@filipw
Copy link
Member

filipw commented Apr 12, 2022

possibly related to #2343 or #2363

@JeremyCaron
Copy link
Author

JeremyCaron commented Apr 12, 2022

I don't know who wrote the comment that this provides no additional value, but they could not be more wrong. If my class already implements an interface then extracting or generating a new one does nothing for me, I need to update the existing one.

@tony-dwire-paradigm
Copy link

tony-dwire-paradigm commented Jun 20, 2023

I also use to use this all the time and thought it was just a bug in my setup that it disappeared. I would like to echo @JeremyCaron 's point that it makes no sense to generate a whole new interface for a type that already has one. I realize if there are multiple levels of interfaces or multiple interfaces on the type it is harder to pick, but the vast common case (at least in my 12 years of C#) is a class with a single interface that we want to pull a method up to.

@JoeRobich
Copy link
Member

Testing with C# 1.24.1 and 1.24.0, I was unable to get this refactoring be offered with the following test file and the cursor at various places on the public void Run() line.

namespace TestConsole
{
    public interface IPerson
    {
    }

    public class Person : IPerson
    {
        public void Run()
        {
        }
    }
}

After looking at the Roslyn code for pull member up, I am unsure how this would have ever worked. The RefactoringProvider and CodeAction require a workspace service that has no default implementation and has not had an implementation in O#. To support this feature, we would need to expose the internal IPullMemberUpOptionsService from Roslyn through the ExternalAccess layer and provide an implementation on our end.

The open questions are what to return since we have no UI. Would a member go to a base class over an interface when both are specified? If there are multiple interfaces implemented by the class, would it go to the first specified? etc...

@JeremyCaron
Copy link
Author

Thanks, I'll try rolling back too. I might have had the version numbers wrong though? filipw changed the title to say 1.38.2 on Apr 12, 2022

@JoeRobich
Copy link
Member

filipw changed the title to say 1.38.2

1.38.2 is the O# version used by C# ext 1.24.2 where you saw the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants