dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
19.02k stars 4.03k forks source link

VS2019 code intelligence (refactorings and quick edit) are still FAR from being as useful as having R# #44537

Closed vsfeedback closed 4 years ago

vsfeedback commented 4 years ago

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


I'm going to walk though a series of edits using the built-in helpers to show how poor an experience this really is.

  1. Create a new Key class, like this:

public class Key {}

  1. Somewhere else, write code to use a non-existing ctor on this clas, like:
var eventId = 123L;
var key = new Key(eventId, Color.Red);
  1. Use Ctrl-. (generate code) to generate a ctor. This will modify the Key class to:
public class Key
{
        private long eventId;
        private Color red;

        public Key(long eventId, Color red)
        {
            this.eventId = eventId;
            this.red = red;
        }
}
  1. But I wanted this to be an immutable class with auto-properties, so now I have to click each field and choose "Encapsulate (as property)", which results in:
public class Key
{
        private long eventId;
        private Color red;

        public Key(long eventId, Color red)
        {
            this.EventId = eventId;
            this.Red = red;
        }

        public long EventId { get => eventId; set => eventId = value; }
        public Color Red { get => red; set => red = value; }
}
  1. But this did not give me auto-props, so now we refactor each prop and pick "Use auto property" to get to:
public class Key
{
        public Key(long eventId, Color red)
        {
            this.EventId = eventId;
            this.Red = red;
        }

        public long EventId { get; set; }
        public Color Red { get; set; }
}
  1. Alas, this did not remove the now unnecessary this. prefixes in the ctor, so we need to click each of those assignments and pick "Remove this. qualification" to get:
public class Key
{
        public Key(long eventId, Color red)
        {
            EventId = eventId;
            Red = red;
        }

        public long EventId { get; set; }
        public Color Red { get; set; }
}
  1. Now, I like the properties before the ctor, but the refactoring dumped them at the bottom. Since I want immutability, I also need to manually remove the setters to get to the desired final result:
public class Key
{
        public long EventId { get; }
        public Color Red { get; }

        public Key(long eventId, Color red)
        {
            EventId = eventId;
            Red = red;
        }
}

These refactorings surely satisfy some internal check list of "can we do this or that refactoring", but they are in practise completely useless. It is MUCH faster to just type out whatever change I want manually, without even bothering to use any of these.

I mean, who in their right mind right-clicks on a context menu to remove a SINGLE INSTANCE of a this. qualification? And then repeats that ad nauseum for every property?

In contrast, R# allows you to do all of this in a single step. Refactorings offer options to "repeat for all occurrences in file/project/solution", and customizations (would you like auto-props with this, sir? read-only or read-write?), saving lots of time. And they respect your code formatting rules, so redundant this qualifications automatically dissapear and properties are placed in the file where you expect them to be. Now, THAT is what productivity is all about!

So, please consider making refactorings that actually enhance productivity rather than hampering it.


Original Comments

Visual Studio Feedback System on 3/26/2019, 11:29 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.

Visual Studio Feedback System on 4/3/2019, 00:40 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.

Morten Mertner on 4/7/2019, 03:48 PM:

I'd like to add that I've found some of refactoring dialogs do provide links to apply the change for all occurrences in either document, project or solution. It's hidden well and not easily accessible with the keyboard. Also, I picked "solution" and had to cancel the modal dialog after waiting for a minute or so.

Visual Studio Feedback System on 8/12/2019, 05:28 PM:

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-2017#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.


Original Solutions

(no solutions)

CyrusNajmabadi commented 4 years ago

In contrast, R# allows you to do all of this in a single step. Refactorings offer options to "repeat for all occurrences in file/project/solution"

Roslyn analyzers allow for this as well. Just choose the "document/project/solution" scope when invoking the fix :)

CyrusNajmabadi commented 4 years ago

So a good way we could deal with this is that var key = new Key(eventId, Color.Red); should offer generate constructor (without fields). This would produce:

class Key
{
    public Key(object eventId, Color red)
    {
    }
}

Then, you can just pick a parameter and say to create properties from all of them like so:

image

This would be two steps and would give the flexibility of easily creating hte constructor, and then deciding the shape of the data.

CyrusNajmabadi commented 4 years ago

And they respect your code formatting rules, so redundant this qualifications automatically dissapear and properties are placed in the file where you expect them to be

Can you give an example of when properties aren't placed in the file you expect them to be? Thanks!

CyrusNajmabadi commented 4 years ago

So this can currently be done in two steps like so:

image

And then:

image

This leaves the code in the desired form for the user:

image

This also respects the preferences around this..

Would could consider having direct access to generating a constructor that initializes properties directly. This would take things down from two actions to one.

mnmr commented 4 years ago

Refactorings offer options to "repeat for all occurrences in file/project/solution"

Roslyn analyzers allow for this as well. Just choose the "document/project/solution" scope when invoking the fix :)

That option is not always available, and is a bit hidden at the bottom of the dialog (and at the time required mouse to be activated; not sure if that is still the case)

mnmr commented 4 years ago

Can you give an example of when properties aren't placed in the file you expect them to be?

Sure, see original issue description above ;) .. to clarify: I put my properties after any fields and above the ctor, mostly because they are used somewhat interchangeably since the introduction of auto properties. It may have inserted them at the bottom of the file because I had none before the refactoring, but ideally it would not move it at all: just put the auto prop where the field was.

mikadumont commented 4 years ago

@CyrusNajmabadi

Roslyn analyzers allow for this as well. Just choose the "document/project/solution" scope when invoking the fix :)

Fix all occurrences is easy to miss and I worked with UX on a more discoverable UI below:

image

image

When I presented it in our design meeting I know there was concern for streaming results as they are found in the the preview for projects/solutions.

Maybe we can revisit this and discuss one of the following solutions:

• Load larger scopes asynchronously with a spinner that says loading. • Dynamically loading list where you show first few with scroll bar. • Could do something similar to Team Explorer where it shows files changed and lets you explore each.

CyrusNajmabadi commented 4 years ago

It may have inserted them at the bottom of the file because I had none before the refactoring, but ideally it would not move it at all: just put the auto prop where the field was.

We got pretty vehement feedback not to do that by the VS community. People did not want fields and props intermixed. We should be respecting your current grouping of properties if you have any. However, if you have none, we pick a default location for properties that has been our fairly consistent location (after constructros) for around 20 years now :)

CyrusNajmabadi commented 4 years ago

When I presented it in our design meeting I know there was concern for streaming results as they are found in the the preview for projects/solutions.

There are several concerns here. One is streaming, but the second is just how would this info be presented (consider how fix-all may have thousands of results). If some team can build us this UI, we could certain attempt to plug into it. But roslyn definitely shoudl not build this. For one, that would only help C#/VB. For another, it won't work once we move to nexus.

Do you want to drive a better diff experience for VS/vscode with the platform team? Thanks!

mnmr commented 4 years ago

I just want to point out that there were basically three issues reported here:

  1. Refactorings in VS are too simple. Stuff like “remove this. qualifier” should be bundled in “cleanup code” and not be a stand-alone refactoring (to avoid UI clutter)
  2. It should be possible to choose what a refactoring does, rather than assuming defaults and forcing the user to perform a series of refactorings to get to the desired result
  3. Availability and discoverability of “apply to file/project/solution” is inconsistent

That said, I’ve been using Rider for the past year so am not keeping track of changes to VS.

CyrusNajmabadi commented 4 years ago

Refactorings in VS are too simple. Stuff like “remove this. qualifier” should be bundled in “cleanup code”

It already is.

and not be a stand-alone refactoring (to avoid UI clutter)

I strongly disagree with this. We've heard time and time again that people don't necessarily want to make wholesale changes to lots of code. I know that I (and others on the team) in particular will often just run a single kind of cleanup when we feel its appropriate.

While code-cleanup fits the bill for what people want to do some of the time, it's not the only way that people want to do this sort of thing, and having a quick and easy way to do this in a consistent fashion with other fixes is highly desirable (even if it's not ideal for you).

It should be possible to choose what a refactoring does, rather than assuming defaults and forcing the user to perform a series of refactorings to get to the desired result

A lot of the current design stems from user feedback on fatigue around choice-itis and option-itis. i.e. that they just found it tiresome to have to pick some many options and choices, instead of just having sensible defaults, that would emit reasonable code, which they could tweak after the fact.

Indeed, there was a strong preference shown in actual user studies that these steps be broken up into smaller pieces so that people could chain them together to form solutions they wanted, and could pick and choose individual parts without having to take a large refactoring whole-hog.

THere are very different philosophies at play here, and there's no clear 'right' answer for all user for all needs. We've gone more granular with smaller, but composable, features because that's what a ton of our feedback was versus when we tried larger and more 'kobby/optioned' approaches in the past.

Overall, since doing that, the amount of feedback about people liking this went up, while the amount of feedback about people disliking this went way down. It's certainly understood that it's not for everyone. But it's not trying to be for everyone either :-/

There are some who are absolutely going to want "the single command that just does everything i want". And, in the case of the OP, we may actually just provide that. i.e. having:

Generate constructor (with fields)
Generate constructor (with properties)
Generate constructor only

would be simple to add and would suffice for this case. However, it's not always going to be the case that we make a single "does everything" refactoring, when we think it's fine if it's broken into two simple and orthogonal steps that can be reasonably combined.

Availability and discoverability of “apply to file/project/solution” is inconsistent

Please file specific issues if you encounter that. That simply sounds like a bug which can be addressed.

mnmr commented 4 years ago

@CyrusNajmabadi

Refactorings serve two purposes: 1) modifying code safely, and 2) productively. If VS only offers fine-grained refactorings, it will not be a productivity booster. For instance, it's faster to manually clean out this. prefixes than it is to activate a dialog to do so. If you need to go through 7 steps to achieve the desired end result, any developer that values time spent will not be using them.

I guess there's no harm in offering up the simple refactorings, but if you can't get from A to F (or close) without going through B, C, D and E then they add little value, and people will continue to use extensions or other IDEs to be productive.

I'd suggest to take a look at the dialogs presented by R# for various commonly used refactorings. They allow you to configure the desired result, so you avoid the multi-step process. Remembering the choices made in the dialog makes it fast for subsequent operations of the same kind.

Some refactorings have a specific goal, with little variance in how the end result should look (such as making a class immutable or implementing equality). These should at least be considered as separate refactorings even if multiple steps are involved behind the scenes.

CyrusNajmabadi commented 4 years ago

For instance, it's faster to manually clean out this. prefixes than it is to activate a dialog to do so.

That seems odd, given that it's just opening the lightbulb and selecting the fix-all scope you want. And, you can always just use code-cleanup for that purpose if you want as well. It's unclear why this isn't sufficient for this case.

. If you need to go through 7 steps to achieve the desired end result, any developer that values time spent will not be using them.

Can you give an example where you need 7 steps to achieve the result? in hte case i mentioned, it would only be two steps.

They allow you to configure the desired result, so you avoid the multi-step process. Remembering the choices made in the dialog makes it fast for subsequent operations of the same kind.

Yes, there are several cases where we offer dialogs as well when we think the value it there and the number of manual steps would be too many. That doesn't mean we think dialogs are appropriate for all scenarios. I especially don't think it's necessary for this scenario where:

  1. you can do what is requested in two steps today.
  2. you could open a PR to make this possible in one step using the lightbulb today (which we would likely accept).

I don't see the need for the dialog there.

mnmr commented 4 years ago

Can you give an example where you need 7 steps to achieve the result? in hte case i mentioned, it would only be two steps.

This whole issue started with me describing how it took me 7 steps to go from A to B. If issues didn't languish for a year before being picked up then it'd be easier to know whether it still takes 7 steps.

Yes, there are several cases where we offer dialogs as well when we think the value it there and the number of manual steps would be too many. That doesn't mean we think dialogs are appropriate for all scenarios.

At no point have I been advocating that all refactorings should involve dialogs.

In fact, I'm not here to argue about any one specific refactoring or whether this or that should show a dialog. I've been using VS since 2001, and at no point in time has VS had the best refactoring options. The performance cost of using R# with VS is immense, and it would be nice if you could refactor code (as well) in VS without it.

You seem to believe that that is already the case, and now that I have switched to Rider I don't really feel like spending a lot of time digging out examples to prove otherwise. I am therefore not going to contribute further comments to this issue, and you may close it as you see fit.

CyrusNajmabadi commented 4 years ago

If issues didn't languish for a year before being picked up

Which issue languished for a year? Note: roslyn accepts many contributions. If there are areas you'd like to see improved for your needs, you can def jump in and make changes :)

You seem to believe that that is already the case,

I did not say that.

and now that I have switched to Rider I don't really feel like spending a lot of time digging out examples to prove otherwise.

i asked for examples so i can fix them. I can't fix things if you're not going to be willing to actually give the cases where things take so many steps :-/

CyrusNajmabadi commented 4 years ago

and you may close it as you see fit.

Would you like to contribute a small fix that allows easy generation of Generate constructor (with properties)? That seems like it would fit your use case quite well and would avoid the need to create any sort of new UI/dialog here.

Thanks!

CyrusNajmabadi commented 4 years ago

This whole issue started with me describing how it took me 7 steps to go from A to B.

Right, but my point with post https://github.com/dotnet/roslyn/issues/44537#issuecomment-633811786 is that this can be done with 2 steps. So i'm looking for examples where you have no choice but to take 7 steps, not that you took seven steps which could be done with two.

Thanks! :)

CyrusNajmabadi commented 4 years ago

At no point have I been advocating that all refactorings should involve dialogs.

Right, so it sound like there's some amount of understanding and agreement on this. We've definitely adopted dialogs when we think they're the best/only way to solve a problem effectively. My point with this issue is that there doesn't seem to be a need for a dialog given that:

  1. you should be able to perform the operation you want with two steps. 2 we would accept a PR that made it possible to perform this with one step going through the lightbulb.

Producing a dialog here seems unnecessary given the state we're in, and given hte alternate approaches available. Thanks! :)

mnmr commented 4 years ago

Which issue languished for a year?

This issue. It was first created in March 2019 and migrated to GitHub a week ago. There is a link to the original ticket at the top.

and now that I have switched to Rider I don't really feel like spending a lot of time digging out examples to prove otherwise.

i asked for examples so i can fix them. I can't fix things if you're not going to be willing to actually give the cases where things take so many steps :-/

Well, this took 7 steps for me to accomplish a year ago. I can’t say when the number of steps changed or if it could have been achieved in fewer steps at the time. Since I’m now using a different editor, the window for asking me to dig deeper has sort of closed.

CyrusNajmabadi commented 4 years ago

I have a fix for the issue coming soon. Just doing the VB side now. Thanks for the report!

CyrusNajmabadi commented 4 years ago

This issue. It was first created in March 2019 and migrated to GitHub a week ago.

Ah, i would def recommend just filing on github. It goes directly to us and doesn't potentially get mixed up in the VS process system. Thanks :)

CyrusNajmabadi commented 4 years ago

@mnmr The change has gone in. I hope it helps you out! :)