Improve discoverability of code refactorings

[mavasani]

A bunch of recent customer surveys done by @kendrahavens identified that quite a few customers find our refactorings to be not discoverable. The refactorings that they requested were already implemented by us, but they needed Kendra to point out where to put the cursor or how to change the selection span for these to show up in the light bulb menu. The same concern does not apply to code fixes due to visual cue from the squiggle/suggestion dots in the UI, and pressing Ctrl + dot anywhere on the line shows up the fix, ordering being based on the distance from the diagnostic span. Note that the primary reason why we don't show up all the refactorings available on a given line are to avoid overloading the light bulb menu, which is already quite noisy. We need to fine tune the experience here to find a balance between discoverability of actions and overloading the light bulb menu.

We have talked about adding 2 different discoverability enhancements to address these concerns:

1. Show additional actions in light bulb menu Improve the discoverability of the refactorings in the light bulb, by showing additional, but likely less relevant actions, that are applicable for positions near the current cursor or selection span. Most relevant refactorings would still be show at the top of the menu and these additional actions will either be shown at the bottom of the menu or nested within a separate menu towards the bottom. Internal discussions have led to bunch of different implementation suggestions on how to achieve this (discussed below), but the primary conclusion being that we need to somehow associate a fix span/ideal span with each registered code action within a refactoring, so the engine can prioritize the ordering and/or nesting of these refactorings based on promixity of this span with current cursor position or selection span. Possible implementation approaches that came out:

   1. Implementation 1: Convert all the IDE refactorings that are not selection based into a pair of diagnostic analyzer reporting hidden diagnostics + code fix. The diagnostic span would be the ideal span for the code actions.

      PROS:

      1. All refactorings available on any position in a line show up in the light bulb, solving the discoverability concerns.
      2. We get other useful analyzer/fixer features for free: fix all, ability to bump up the severity to see squiggles/error list entries across a wider scope, etc.

      CONS:

      1. Likely to overload the light bulb menu with lot more actions now showing up. If so, one option might be to move all the code actions registered for hidden diagnostics whose span does not intersect with current position into a nested menu near the bottom of the light bulb menu.
      2. This approach adds quite a bit of implementation cost as we need to refactor code in each code refactoring into a pair of diagnostic analyzer/code fix.

   2. Implementation 2: Currently, the refactoring service executes naively for identifying available actions for current cursor position or span. It passes in the entire line span or selection span into each refactoring, and then treats all registered refactorings to be on par with each other. This forces our refactorings to then be implemented in a restrictive manner, so they are not offered everywhere on the line and do not overload the light bulb menu. This whole setup relies on the assumption that users are already aware about where to put their cursor or what span to select to get the relevant refactorings, which does not seem to be true as mentioned at the start of this post. This proposal tries to remove this assumption by making the following changes:

      1. Change the code refactoring service to perform multiple passes instead of just one. It first identifies the current token/node at the position or selection span and identifies available actions. These would be the most relevant refactorings that get offered at the top level of the light bulb menu. Then the service walks up the parent node and asks available actions for the parent and siblings of previous token/node. These actions would automatically be assigned a lower priority and will be shown under a nested menu near the bottom. These additional nearby actions would serve as a discoverability point for beginner users, while also not polluting the menu for advanced users.
      2. Change all the refactorings so they only register an action if the input text span exactly matches the token/node that is most relevant to it.

      PROS:

      1. We do not alter the existing light bulb menu significantly for advanced/experienced users, while adding a new discoverability point for beginner users to discover new potential actions in nearby locations.
      2. The implementation for each refactoring is greatly simplified and unified as they only work when input span exactly matches it's fixed span.

      CONS:

      1. We might end up with a perf hit due to the code refactoring service doing multiple passes. We would need perf measurements to identify if this indeed a concern as most refactorings would just bail out upfront.
      2. We need to experiment/decide if a nested menu is indeed a good discoverability point as beginner users might not know that they need to dive into a nested menu at the bottom.

   3. Implementation 3: Allow refactorings to specify a span in RegisterRefactoring callback. This would serve as the virtual diagnostic span for the code action. We would need to change all the refactorings to then operate on all the nodes/tokens of interest in the input context span and register refactorings for each node/token with their span. The code refactoring service would be changed to prioritize refactorings whose registered span is a close match to current position/span, and show the rest of refactorings in a nested menu or towards the bottom of the menu.

      PROS: Get the similar user experience as prior approaches, with potentially lesser implementation cost then approach i. and avoid multiple passes that are needed in approach ii.

      CONS: Adds implementation complexity in each refactoring of identifying multiple nodes/tokens of interest and then register each action with a span.

   4. Implementation 4: Enforce common helpers that each code refactoring ought to use to determine it's applicability span and bail out if that is not the case. We need to ensure we polish and/or extend the existing helpers, make them public as appropriate and audit all existing or just the problematic refactorings to ensure they are using these helpers.

2. New UI for viewing available actions in a broader scope: Create a separate tool window to show available code actions within a given scope (document/project/solution, with document being the default). Few open questions:

   1. Should the refactorings shown in this window be opt-in to avoid polluting it with common refactorings that show up everywhere?
   2. Should the tool window automatically be opened and brought to focus when user invokes light bulb and/or applies a code action? If not, how would we make this UI discoverable for users?
   3. Should the actions list in the window be ordered such that the actions closer to current cursor are near the top? We would potentially start with a simple UI, that only works for document scope to start with, and iterate on improving it to work with broader scopes.

[mavasani]

Tagging @heejaechang @jinujoseph @vatsalyaagrawal, who were involved in the internal discussions in this area.

Tagging @sharwell - can we please bring this up in today's design meeting?

Tagging @dotnet/roslyn-ide @CyrusNajmabadi for additional views/concerns that must be discussed at the design meeting.

[JoeRobich]

I am a fan of 1.iii. We always have a lot of concern about what we show to the user and right now that decision is made in each individual refactoring. If we allowed the refactoring to say "this is what I will operate on", we could implement a smarter system for determining which refactorings are the most important. It could use more information than just the current cursor or selection location. We could allow recently used, frequently used, etc... to influence which refactorings get the most visibility.

[mavasani]

Note that even before we get into any of the implementation of approaches in 1, we should discuss if we want to do 1. (Show additional actions in light bulb) or 2. (New UI) or both to improve discoverability.

Thanks @JoeRobich, good points. I think there are merits and demerits in all approaches that we should discuss. Heejae suggested approach 1.iii. and sounds reasonsable to me. I personally prefer 1.ii. from an API contract perspective as that simplifies each refactoring considerably: the refactoring just does a FindNode or FindToken on input span and bails out if it does not match the syntax kind that it can refactor. All registered code actions are implicitly associated with input span, which will be guaranteed to be non-zero. Approach 1.i. has the biggest advantages of unifying our code fix/refactoring story, and also brings value add from getting already implemented analyzer/fixer features.

[CyrusNajmabadi]

My primary concern is that we spent a lot of effort dealing with teh feedback of:

I see too many items in the lightbulb, and so many of them aren't useful or aren't relevant to where i am right now. Why is there so much crap here that i have to deal with.

I've very worried toward slipping back toward that place esp after all that effort went in.

[CyrusNajmabadi]

A bunch of recent customer surveys done by @kendrahavens identified that quite a few customers find our refactorings to be not discoverable.

Why don't we write docs and list all our features and where they can be used? IntelliJ and Resharper does this. It's not clear to me why we don't just list somewhere (and update when new VS releases come out):

1. Here's all the refactorings/fixes we offer.
2. Here's how you can use them.

[CyrusNajmabadi]

Note: i bring this up because i've heard several times in gitter people going: oh wait, VS has that feature? Nice! I wish i'd known that.

I think we're pushing too hard on hte idea that these features need to be 'in your face'. I think it's critical that they're not. This certainly hurts discoverability, but i think it's not necessary to presume the feature has to be discoverable directly from the produce. It can be discoverable through other means.

[jmarolf]

@CyrusNajmabadi I think we need to be very careful to not be in your face about things, but they way spans work for refactorings today is a real problem.

Most of our refactoring placement is extremely finicky. Many times people have been told we have a foreach to linq refactoring. They go and try it out by placing their cursor on the same line as their foreach statement and invoke the litebulb. Nothing happens. They then move on and assume they must have mis-understood or the feature is just broken. I have heard this feedback a lot.

We need to be careful to not swing the pendulum too far back in the direction of over-showing refactorings, but its clear what we have today doesn't work for the majority of people. Even if we tell them they are unlikely to discover our features.

[jmarolf]

It can be discoverable through other means.

We can do better on docs, and some of that is happening in parallel, but I also don't think a blog post or reading docs.microsoft.com should be a requirement to use our features. Ideally you should be able to understand and use them without taking time out of your day to go searching for docs.

[CyrusNajmabadi]

Most of our refactoring placement is extremely finicky. Many times people have been told we have a foreach to linq refactoring. They go and try it out by placing their cursor on the same line as their foreach statement and invoke the litebulb. Nothing happens. They then move on and assume they must have mis-understood or the feature is just broken. I have heard this feedback a lot.

Of course. I totally agree with thsi feedback. I even helped work on creating helpers for this sort of thing. For example: RefactoringSelectionIsValidAsync

I think we absolutely should have public helpers here to allow the refactoring author specify the range they want their feature to be available, and do appropriate computations to make the experience good for the user.

For example, with teh 'caret is on line' case, a normal refactoring should have been written to:

1. FindToken.
2. Determine if token was appropriate for feature (removing teh common filter that the cursor must be in the normal-span of the token).
3. extend range to encompass what we traditionally think of as the user area for that token. This is generally eating the whitespace backward to the start of line (but not going into preceding lines). Check if the cursor is in that span

I personally just think we need to audit our features and have them stop all doing this sort of thing in an adhoc manner. We can write these simple helpers and improve the scenarios pretty simply.

[CyrusNajmabadi]

We can do better on docs, and some of that is happening in parallel, but I also don't think a blog post or reading docs.microsoft.com should be a requirement to use our features. Ideally you should be able to understand and use them without taking time out of your day to go searching for docs.

I personally disagree. I think it's actually quite reasonable to have docs for this sort of thing, or else you're held hostage to trying to make teh features discoverable through every means a user may try. Say we do any/all of the above. And a user still doesn't figure it out. Maybe they think "to convert 'for' to 'foreach' i'll just add each and expect the IDE to offer the fix". Do we then solve that case for them? I think it's perfectly fine to have reasonably intuitive (i.e. not extremely restrictive) approaches for the spans here, along with documentation to help people get full sense of what is available and where they could use it.

[CyrusNajmabadi]

We need to be careful to not swing the pendulum too far back in the direction of over-showing refactorings, but its clear what we have today doesn't work for the majority of people. Even if we tell them they are unlikely to discover our features.

I agree with that. However, my overall point is that this isn't something that requires some large investment. A dev could literally walk through most or all of our refactorings in a couple of hours and address the deficiencies here.

It's not like we don't have experience with thsi. We've tweaked refactorings several times. For example, we discovered it was common users to do the following to select a bunch of members:

class C
{
     int foo;[|
     int bar;
     int baz;|]
}

This originally didn't work because the start of the span was on a field that wasn't being selected. But we realized this was a desirable coding pattern and we made it work. I think we can do the same for all the refactorings (ideally with shared code for consistency) without a larger piece of work here (i.e. converting to analyzers). }

[CyrusNajmabadi]

Basically, my suggestoin to start would be the very cheap:

Option 4: Go audit and fix these problematic refactorings. While you're doing that, try to come up with a reasonable set of rules we can publish, ideally along with helper functions for this.

See where this gets us. It's extremely cheap and likely effective. If we find out after this that it's still insufficient, and even with these changes and with docs, that people can't figure it out then we invest in a more dramatic solution.

[mavasani]

I think we absolutely should have public helpers here to allow the refactoring author specify the range they want their feature to be available, and do appropriate computations to make the experience good for the user.

I feel this is the crux of the problem here. We expect to throw different selection spans or cursor position at the refactorings, and expect each refactoring to decide if the refactoring is applicable based on the context or selection. It seems like all this should be the job of the code refactoring engine, not individual refactoring. Refactoring implementation should be exactly identical to a how a code fix would be implemented - just do FindToken or FindNode on input span and bail out if the found token/node is not one that it refactors. The engine should be the one who decides if the registered refactoring is the most applicable refactoring based on cursor/selection, or down the priority list. This way we guard against the refactorings from overpolluting the light bulb and also push down the less relevant/close proximity refactorings down into a nested menu for discoverability.

Basically, we have 2 separate sources for light bulb actions - code fixes and refactorings. Both of them choose different model where former uses a concrete span for communication between the engine and the extension, and have a very intuitive location where it will be offered. The later chose a model where the input span is a hint for extension to do further processing to determine applicability, and when multiple refactorings register actions, the engine has no way to determine the most applicable one. We haven't had any problems for discoverability of code fixes, but have had issues with refactorings as mentioned by @jmarolf and also found by @kendrahavens when discussing with customers. Yes, we can take the approach to find tune refactorings that customer complain about, but this does not prevent future refactorings from going down the same path and hitting same issues. I think fixing the engine/API would help us solve the issue for long term.

[CyrusNajmabadi]

Yes, we can take the approach to find tune refactorings that customer complain about, but this does not prevent future refactorings from going down the same path and hitting same issues. I think fixing the engine/API would help us solve the issue for long term.

I think it will. We commonly and continually refine and do better.

Note that i don't think teh refactoring engine has enough information to decide what to do. For example, it's very important that some refactorings be made available on certain parts of a construct, but not the whole construct. We'd need an api to signify that and we'd still need refactorings to use that properly. In many cases, it's really case specific and the refactoring has to apply the right smarts no matter what.

We haven't had any problems for discoverability of code fixes,

Yes we have. Definitely fixes that are currently in the "no UI mode". It was a large enough deal that i had to go through and fixup many of them to address issues here (both showing up too much, and not showing up enough). That's precisely what drove me to open several PRs on a framework for how a feature should work with all severity levels, because we were having so many problems here. That framework was rejected at the time, even though this was an area i was trying to improve :)

[CyrusNajmabadi]

and when multiple refactorings register actions, the engine has no way to determine the most applicable one.

I think it's worse than that. I think many refactorings simply don't register when appropriate. Nothing about the engine changing will affect that (unless you literally ask to refactor every char on a line and you aggregate the results somehow). This will invariably require changing the refactorings for whatever new system you have. So i proprose just skipping that middle step and fixing up the refactorings that are problematic.

Note: we've done this numerous times very successfully. There are complains about new refactorings, and that doesn't surprise me since we didn't focus on this during the reviews (since they were enormous) and we didn't invest any time on them fixing them up after the fact. That's different from almost every other refactoring we've written.

We normally would spend a fair amount of tiem thinking about "where are the places this should (and, importantly) not show up?", and we would rapidly tweak the refactoring soon after release.

Now, we've released the refactoring, spent little (if any) time on doing any fixups here, and are extrapolating out now that we have a significant problem that needs addressing.

I don't buy it. I want to see the outcome of just taking the simple and cheap approach that we've always had, before deciding it's time to throw the baby out with the bathwater.

[CyrusNajmabadi]

Implementation 1: Convert all the IDE refactorings that are not selection based into a pair of diagnostic analyzer reporting hidden diagnostics + code fix. The diagnostic span would be the ideal span for the code actions.

Let's look at that a second. Consider "generate equals and hashcode". One thing we heard from people was that they expected to be able to go to a blank line in a class and say "i want to generate these guys here". How does it work to convert this to a diagnostic+span? Would there be a span for every blank line?

[CyrusNajmabadi]

Change the code refactoring service to perform multiple passes instead of just one. It first identifies the current token/node at the position or selection span and identifies available actions.

I'm not sure what this means. tokens/nodes are a c#/vb-ism. How does this work for TypeScript/JavaScript/F#?

Determining the token from a positoin is also well defined for C#/vb. but it's unclear waht it means to 'determine a node'. That is often very refactoring specific, with lots of necessarily logic to figure out what is sensible on a per-construct basis.

[CyrusNajmabadi]

Of all the approaches, impl3 make the most sense to me. But i don't see it being much different from what i'm proposing. Namely, that the refactoring is in charge. :)

It lists this as a CON:

CONS: Adds implementation complexity in each refactoring of identifying multiple nodes/tokens of interest and then register each action with a span.

But htat's not a con for me. Refactorings must think about this. For example, we know from direct experience that code-fixes that specify too broad a range of applicability are really unpleasant. The same is true for refactorings. We often do not want them diving into certain children (like lambdas) because of how noisy and unintuitive the experience seems.

The driving scenarios here (for/foreach/linq) def suffer from this, and we'd want to customize the approach for these features. So i would def drive this by trying to actually fix these actual user issues and seeing what we can grok out of it. Right now i think we're examining potential options without actually knowing if they would fit the very scenarios we're trying to fix.

[mavasani]

Consider "generate equals and hashcode". One thing we heard from people was that they expected to be able to go to a blank line in a class and say "i want to generate these guys here". How does it work to convert this to a diagnostic+span?

This diagnostic span would potentially be the entire type declaration. The refactoring will only show up in top level menu if you are directly touching node or token in the type declaration, and will show up in a nested menu if you need to walk a step up in parent chain.

I'm not sure what this means. tokens/nodes are a c#/vb-ism. How does this work for TypeScript/JavaScript/F#?

The goal here is to improve the experience for C#/VB and all languages that have documents based on syntax (SupportsSyntaxTree). We would leave the current approach of delegating to refactoring to make these choices for other languages.

But htat's not a con for me. Refactorings must think about this. For example, we know from direct experience that code-fixes that specify too broad a range of applicability are really unpleasant.

That seems unrelated. Diagnostics giving too broad spans have visual effect of huge squiggles that is unpleasant. Hidden diagnostics are never affected by this issue. Going back to your issue with refactorings must think about applicable spans, yes approach impl3 does retain the refactorings as being in charge of determining applicability, and only enhances them to communicate back to the engine that this is my exact fix span for this code action. I personally prefer impl1 and impl2 more due to the fact that refactorings are much less in charge and engine decides the applicability and priority based on current cursor/selection. I am still fine with impl3 as at least the refactoring is forced to associate a span with each registered code action, and then the engine can make the decision of whether to show the refactoring in top level or nest it at the bottom of the menu or drop it completely.

[CyrusNajmabadi]

Note: i'm very much not opposed to the idea of a larger-scale fix here (in case that's how my earlier posts came across). What i'm trying to emphasize though is that we should relaly be certain that is necessary, as opposed to it being the case that we have a couple of refactorings that we simply shipped quickly, and which we could improve with 5 mins of effort :)

[CyrusNajmabadi]

The goal here is to improve the experience for C#/VB and all languages that have documents based on syntax (SupportsSyntaxTree). We would leave the current approach of delegating to refactoring to make these choices for other languages.

Note: i reallllly don't like that idea. If we have an idea for how to make this better, it should be better for all langs :) Note that i think this is definitely possible.

[CyrusNajmabadi]

This diagnostic span would potentially be the entire type declaration. The refactoring will only show up in top level menu if you are directly touching node or token in the type declaration, and will show up in a nested menu if you need to walk a step up in parent chain.

But is that the experience we want? i.e. if someone liked being able to generate these members by going to a specific gap between members, now they need to go into this sub-menu (which i'm guessing will rarely be looked at).

Furthermore, it would be really unpleasant if we had this submenu and it basically showed everything that had a span that crossed it. Basically any 'type' or 'member' refactoring would always show up in this submenu inside a member, no matter how off it seemed. That seems like exactly the type of noisiness we were working toward moving away from :)

[CyrusNajmabadi]

Again, please don't take this as me being ultra negative. I'm strongly in favor of a good user experience here, and driving that through tech. But i want to think about waht we want the user experience to be and then solve the tech problem. Not: create a tech solution, but potentially have it cause a poor experience.

Thanks!

[mavasani]

i'm very much not opposed to the idea of a larger-scale fix here (in case that's how my earlier posts came across). What i'm trying to emphasize though is that we should relaly be certain that is necessary, as opposed to it being the case that we have a couple of refactorings that we simply shipped quickly, and which we could improve with 5 mins of effort

Thanks @CyrusNajmabadi. The primary reason we started discussing discoverability of code actions was an outcome of customer surveys for many customers across multiple refactorings, plus internal feedback from customers indicating that they either did not find out we had xyz refactoring or even if they did, they had trouble identifying how to invoke them. @kendrahavens @vatsalyaagrawal and @jinujoseph can give more background here on the reasons why discoverability of refactorings has come up as a top issue recently. If the issue is just related to 1-2 refactorings, I would be more then happy to avoid additional work from new API, engine changes or creating a new UI/tool window for code actions :)

[CyrusNajmabadi]

That seems unrelated. Diagnostics giving too broad spans have visual effect of huge squiggles that is unpleasant. Hidden diagnostics are never affected by this issue.

They definitely are. For example, we used hidden diagnostics for things like "convert a block back to an expression" (or vice versa) depending on what your actual option was.

it took a lot of fine-tuning of that to not make it feel noisy. Here's why:

Even if there's no squiggle, if you're on the invisible-diagnostic-span:

1. it causes the lightbulb. which makes people want to go see what's up. Note that this is a problem, and hidden diagnostics really should cause the brush/screwdriver, not the lightbulb.
2. it does cause noise when you're bringing up the lightbulb for something else. Even de-prioritized (which we do do), we found people complaining: why are these items always in the menu, it's just adds more stuff i need to look at, which i don't care about.

This is not speculative mind you. This is directly from having feedback and our direct experiences using this.

Again, this is why i was trying to come up with solutions on this earlier. Indeed, one solution was to stop using diagnostic analyzers here because they gave such a problematic experience for the invisible-level dianostics. :)

[CyrusNajmabadi]

Thanks @CyrusNajmabadi. The primary reason we started discussing discoverability of code actions was an outcome of customer surveys for many customers across multiple refactorings, plus internal feedback from customers indicating that they either did not find out we had xyz refactoring or even if they did, they had trouble identifying how to invoke them

I 100% agree this problem exists :D

Refactoring discoverability is a big deal, and we often gloss over it. That's often been the case in th past because of me (and i own that). The primary reason why though was that it was often the case that we could simply get it out there, and then we'd immediately be trying to use it, and it would become clear almost immediately: yeah, this isn't showing up where i would expect. We'd then fix the issue, and that refactoring would generally be ok.

Note: i am very sympathetic to the desire to not have to do the above. I'm just very skeptical that there's a global solution that isn't full of it's own drawbacks (see my points about 'generate equals/hashcode'). As such, i'd like us to:

1. determine if we think this is a problem with just these refactorings, and we can just quickly address them (like we have in the past).
2. if we think it's endemic to all refactorings, and it's too much cost to handle each one individually, then we can design something new. But it can't be "in the blind". We need to take a healthy number of features and try it out against this new system to ensure we're not making the experience worse in a substantive way.

Finally, i'm extremely loathe to make a C#/VB only solution. Such solutions just mean that when someone goes over to TS/JS they go: "why does this not feel right?" That was something we specifically worked on this roslyn extensibility layers to avoid. For example, TS/JS had their own complete intellisense impl (including the interaction model), and people were commonly griping that they'd switch between languages and things just felt wrong. If the interaction model (and that's what lightbulbs/spans/etc. are) is different, it's going to be bad for users :)

[CyrusNajmabadi]

@kendrahavens @vatsalyaagrawal and @jinujoseph can give more background here on the reasons why discoverability of refactorings has come up as a top issue recently.

Sure, i'd be happy to hear. But note that i'm 100% on board with this being a problem. I definitely had to fix my fair share of this over the years, so i'm really sympathetic for the desire to ahve a 'one size fits all' solution that just nips this in the bud now and for the future.

So you don't need to convince me of the problem. You need to convince me of the solution** :)

--

** Actually, you don't ever have to convince me. It's your product. Do what you want :) I'll just weigh in on if i think it's the best choice. And if you decide to go a different direction, that's totally ok. I'm just happy if you consider and evaluate the feedback, even if you don't eventually take it :)

[CyrusNajmabadi]

BTW, i was serious about the 'helpers' idea. I think it was jnm2 and i who thought up of: https://github.com/dotnet/roslyn/blob/master/src/Features/Core/Portable/CodeRefactoringHelpers.cs to help out here. Basically, we did say: most refactorings want to just specify a span of applicability, with spans of in-applicabilty carved out. And that applicability span should be expanded in a uniform fashion to fit the intuition of the user (i.e. it should expect through beginning whitespace on the same line).

I'd honestly be curious if we could just use these helpers for these features (whch i think i proposed at the time, but was rejected on**), and tweak them a bit to see if it just solves the problem. We could then make the tweaked version public, and/or just directly include it on the RefactoringContext object. I think this is effectively 'impl3' for you, with some small variants to address real-word tweaks we need to make.

Note that the helpers are 'node-centric'. But really, those are only to get the spans of those nodes. We could have the same span-centric helpers, with the node-centric ones as VB/C# specific overloads/extensions.

--

** IIRC, the reasoning back then was: well, we'll start with these spans, and fix them up later if there's a probem. Well... there's a problem, but we're possibly avoiding the easy 5 min solution (2-days if we want to expose these helpers in a language agnostic-way).

[mavasani]

BTW, i was serious about the 'helpers' idea. I

Thanks, I will update my original comment to add another option about taking the helpers approach, where we ensure we polish and/or extend the existing helpers, make them public as appropriate and audit existing or just the problematic refactorings to ensure they are using these helpers. We are hoping to discuss this issue in the next week's design meeting to get entire team's view on this.

[CyrusNajmabadi]

Awesome! Thanks for including me in the discussion!

[jinujoseph]

@CyrusNajmabadi Marking this for next design meeting. We will keep you updated with what team decided about this

[AdamSpeight2008]

Why can't we have new tool window similar to Errors but have it filled with code fixes instead?

[CyrusNajmabadi]

@AdamSpeight2008 these are not code fixes. Code fixes are already exposed in many discoverable ways to the user (both in the editor, and in the task-list). Refactorings are problematic in that there's much less of an indication to let the user know they're available.

[canton7]

I quite like proposal 2, with some specifics:

As a developer who's become aware that there are many refactorings out there that I have no idea about, what I'd like (in an ideal world) is a tool window which shows me all of the refactorings available which affect the code at my current cursor position, and also the enclosing scope, right the way out to the enclosing method (preferably sorted in order of how specific they are to my current cursor position). If the cursor's on a method, show all of the refactorings on the method. Ditto a type. So the contents of the tool window would change depending on where the cursor was.

By "which affect the code at my current cursor position", I mean all refactorings which will directly alter the code under my cursor, or alter the code in any of the enclosing scopes. For example, if I put my cursor somewhere inside the body of a for loop, the refactoring to change it to a foreach loop will be displayed. If I select a span of text, then perhaps only refactorings which directly affect all of the code within the span should be shown?

I singled out method-level and class-level refactorings as exceptions, as under the above scheme those would be shown wherever in a method my cursor was, which would probably be excessive. I think asking the developing to put their cursor on a method or class name to get this is not a big ask.

This means I can browse through all of the refactorings which are relevant to a particular bit of code, both to see what's available (out of curiosity), and to look for a specific refactoring which does what I want. I can imagine some fancy UI which highlights the points in my code which will trigger the refactoring, which I can use to learn how to trigger my favourite ones without using the tool window.

We might take a perf hit when loading the tool window, but that's probably OK. I won't be using it for refactorings that I use a lot, only to explore what's available.

You still have the discoverability issue that people won't know that the tool window exists. But at least when they find it (and it could be prominent in the View menu, or even enabled by default), the rest of the refactorings are fairly easy to discover.

Now I understand of course that anything of this scale is a lot of work, and I'm not the one doing it. Please don't take this as a list of demands. I just wanted to share my thoughts as a developer who was until recently completely unaware of the number of refactorings, and still doesn't know how best to discover them all.

[mavasani]

@canton7 - That is very good feedback, thanks! We will surely consider it as part of design discussions.

As a developer who's become aware that there are many refactorings out there that I have no idea about, what I'd like (in an ideal world) is a tool window which shows me all of the refactorings available which affect the code at my current cursor position, and also the enclosing scope, right the way out to the enclosing method (preferably sorted in order of how specific they are to my current cursor position). If the cursor's on a method, show all of the refactorings on the method. Ditto a type. So the contents of the tool window would change depending on where the cursor was.

Funny, this is exactly how I have written my prototype for this tool window. It shows refactorings across the document, but the ordering is context specific based on the cursor position, so the ones that are likely applicable for your current context are near the top.

[CyrusNajmabadi]

I very much am intrigued by the idea of "affected if the refactoring would touch the code i'm in". That would def work great for a bunch of cases, but would also require opt-in from teh refactoring itself.

For example, you meantioned 'convert for to foreach'. However, that refactoring doesn't really touch the body of the loop. So if you were in the loop, naively, this wouldn't show up. However, i don't think it would be hard for a refactoring to have a 'context span' that indicate what span it felt it was "relevant to".

Another example of this are refactorings like "generate overrides". Right now, there really is no 'span' that that relates to. but it would be sensible in this new model to say that is applied to the enture span of the containing class/struct.

I def think this is at least something worth looking into! Maybe we'll run into some insurmountable problems. or maybeit will turn out this sort of simple reformalization would be totally sufficient.

[CyrusNajmabadi]

Oh, and thanks very much for the feedback!

[kendrahavens]

To be more specific, half the users I interviewed struggled to find codefixes even when they were looking for them.

• Convert foreach to LINQ - user may select entire IEnumerable
• Convert local function to method - user would select entire method
• Pull members up - user selected all members or one entire line, user may hover over members expecting something to appear while not noticing the screwdriver in the margin, many selected several and right-clicked expecting the context menu to have adapted to their selection range. (ReSharper adapts it's right-click menu based on selection. Even participants who did not use ReSharper used right-click, but that's a separate discoverability issue I want to improve.)

All of these (and many more) already have documentation: aka.ms/refactor A few users did google what they weren't familiar with and quickly found the docs so I think we are covered there.

I included several more codefixes in my study and I'd say, in general, they were hard to find.

[AdamSpeight2008]

@CyrusNajmabadi I don't think we need a context span. Wouldn't a intersection of the visual span (the span of code that it viewable in the editor, and the code refactoring be sufficient.

[CyrusNajmabadi]

@CyrusNajmabadi I don't think we need a context span. Wouldn't a intersection of the visual span (the span of code that it viewable in the editor, and the code refactoring be sufficient.

Let's consider that for a second. Say i have teh following:

void Foo()
{
    foreach (...)
    {
         // ...
    }
}

// big collapsed region here:

class C : $$Bar
{
    // ...
}

It doesn't make sense to me that if i were on $$Bar and i brought up the refactorings menu that it would offer convert foreach to for. Furthermore, if this really is "does the refactoring intersect my viewabl area " then you'll just have tons (literally tons) of optoins in the menu, all of which are confusing. For example, imagine you have:

void Foo()
{
    foreach (...)
    {
         // ...
    }

    foreach (...)
    {
         // ...
    }

    foreach (...)
    {
         // ...
    }

    $$foreach (...)
    {
         // ...
    }
}

If i'm on the last foreach, do we really want to see 4 options in the list to convert foreach to for jsut because they were all on screen? That seems like it would be hugely problematic for several reasons. First, it's just overwhelming. Instead of the list having <5 items, it oculd have dozens**. Second, each item would have to be looked at carefully (including the preview) to even know what it was referring to.

--

** Note, data shows that once you get past like 3-4 items, it basically becomes useless for a user. They just stop looking as there's just overload for them in terms of all the options.

[CyrusNajmabadi]

(the span of code that it viewable in the editor, and the code refactoring be sufficient.

Note: regardless of how good an experience this would be, it's also necessary to clear something up. "and the code refactoring" doesn't really have meaning. Today code-refactorings have no way of expressing their "span". They are simply queried with a specific position and they respond "yes, i'm available".

This is also problematic from the perspective of intersecting the visible-area with the refactorings. This would mean every position in the visible area would need to be queried for a refactoring. And this would be a lot of refactorings, and would be hugely expensive.

Today we have written refactorings when the expectation that they are only called rarely. i.e. when the cursor changes, and only for that position. This has affected deeply how we designed and implemented them. if this changes to being called literally hundreds or thousands of times (which is how many positions there are on a normal screen), that completely blows our perf equation out of the water.

[AdamSpeight2008]

My point is you don't need a new context. There are ways using the existing method. Take block structure context for example.

            For Each symbol In options.PreprocessorSymbols
                ' The values in options have already been verified
                result(symbol.Key) = CConst.CreateChecked(symbol.Value)
            Next

only trigger Convert To 'For' code-refactoring availability on the For Each statement. It should be available any within the for each block. Or preferably anywhere on the line containing that block.
This could then be extend out to the next outer nested block, just to clarify this would only be the block and not the contents of said block. This technique is already use to find the matching block start in vb compiler code. Especially in for loop, exit and continue construct.

We aren't asking at every position, just on each syntax node.

[CyrusNajmabadi]

It should be available any within the for each block.

What makes that happen? Note that it's very intentional that we don't just offer it inside some construct just because there's an applicable construct higher up the tree. We had this before with several refactorings and there was lots of feedback about how noisy this is. For example, people would have this:

foreach (...)
{
       // deep nesting
       (some, lambda) =>
       {
             // deep nesting
             $$
       }
}

Having convert foreach to for show up there is a negative as it will clutter up the list with somethin that users aren't intending to ever trigger 99.999% of the time.

We aren't asking at every position, just on each syntax node.

Taht isn't a concept in the lightbulb or refactoring formalization of the world. Consider, for example, that TS and F# have no syntax nodes that can be queried. This would have to be an approach that could work for them.

[mavasani]

A typical example of the shortcomings of the current design is https://github.com/dotnet/roslyn/issues/35875.

@chborl mentioned offline: The “Generate Constructor” refactoring originally required the entire field declaration statement being selected. It was made this restrictive so it wouldn’t be too noisy. A month or two ago I changed it to only require a portion of the identifier of the statement to be selected. This improved discoverability a lot, but still… The new issue above asks that we not require a selection at all.

[CyrusNajmabadi]

@mavasani I totally agree. I think all the "generate from member" features shoudl be available from at least from anywhere on hte 'header' of a field/prop :)

Fortunately, it's easy to tweak this and get the behavior we want for a whole set of features at once as they all share a unified "am i on the right location" logic.

[sharwell]

Design review conclusion:

• There are two types of users
  • One that wants to see a specific set of things
  • One that wants all options presented
• No matter which way we go, the common patterns and helpers from implementation 4 will be important
• Start with implementation 3, which allows refactorings to opt-in to the prioritization mechanisms we already implemented for diagnostics

We are planning this as one of our team's summer intern projects.

---

In addition to the above, I suggested it would be nice to have the ability to start typing with the light bulb menu visible to filter the list using the algorithms we already have for completion. With a few predictable characters, users will be able to reliably trigger refactorings even if items get added and/or rearranged in the future.

[kendrahavens]

FYI, we may want to increase the range that "Convert anonymous type to class" is offered: https://developercommunity.visualstudio.com/content/idea/651687/creating-a-class-from-annonomous-type.html

[CyrusNajmabadi]

@kendrahavens from reading that, it sounds more like they don't really know how to invoke lightbulbs, or that they're looking for a specific keystroke to perform this specific refactorings.

[petrroll]

Common helpers:

Current situation:

The initial work for implementation 4 helpers has been mostly done, a common set of helpers in IRefactoringHelpersService has been created and 18 refactorings (including all mentioned in #35180) have been moved to it.

In addition to decreased complexity (potentially easier maintanance) within the individual Refactorings it also brought consistency for when individual Refactorings are offered, especially but not limited to when a user uses seletion.

Note: Existing helpers were considered but due to their nature (I already have a node, is current seletion applicable) they weren't enough. A strong need to have a helper "I have selection, tell me if there's an applicable node of type X conforming to predicate Y & potentially return it" has been identified.

Note: The helper service is also exposed via a set of extension methods with simpler API.

The common set of rules for when a Refactoring should be offered:

The general idea behind the helpers is based on the assumption that Refactorings work on nodes. For example ConvertLINQToFor works on LINQ query node, ConvertForToForeach on For node. With that assumption the helper takes current selection/caret location and tries to find out if a desired node is selected (in case of selection) / logically near (in case of caret location). If it is, it returns such node. Being near means for example being on the edge of a the node or selecting the whole node. Specific rules are explained below.

Note: The phrase "logically near" is important as explained below.

Extractions:

Since the SyntaxTree sometimes doesn't map to how a user might think about the code a set of extractions is done for each node that is considered by rules below. For example a user might select a whole declaration [|var a = from a select b;|] in cases when they want to have Refactorings on the right-side expression. This also handles overselection to ; and bunch of other cases.

In essence it makes [|var a = from a select b;|] equivalent to var [|a = from a select b|];, var a = [|from a select b|];, and var a = [|from a select b;|]when it comes to Refactorings on from a select b.

Note: The current set of extractions is very much work in progress and should be expanded upon.

Non-empty selections:

Note: For non-empty selections we assume the user wanted to work specifically on selected thing.

Note: For selections we first trim all whitespace from the beginning & end to handle arbitrary over-selection.

Note: If selection is just whitepsace -> return nothing. Don't want to colapse it to caret location (hard to say whether beginning or end should be used and wouldn't return anything as selection)

• A token whose direct parent is wanted node is selected. While arbitrary it covers most reasonable scenarios. E.g. selecting for token when you want to do Refactorings on for construct or identifier token (name) for a method. The rationale behind going just one-up and just from Tokens is that a selection was used -> the user knew what they wanted.
• The smallest node that is larger than whole selection is wanted node. This enables "user selected the whole node" while being forgiving if slight underselection happens. It also handles leading comments correctly (uses FullSpan) If multiple "smallest" (the same sized) nodes exist -> all are considered.
• A special care is done to handle Attributes, if only (multiple) attribute lists are selected ([|[A][B]|] out var a) this rule would normally trigger but doesn't because the user specifically selected only attributes.

Empty selections (just caret location):

Note: If location is within whitespace -> move location towards an edge of a token in whose FullSpan we're. Essentially if we're in whitespace after a some code -> treat it as if we're just at the end of the code. If we're in whitespace before -> treat it as we're in the beginning ($$ for(...) {} -> $$for(...)).

• Caret is on left/right edge of the whole node. E.g. for(...) {}$$.
• When attributes are present both locations before 1st attribute and before the node are applicable e.g. $$[A][B] $$void myMethod() { ...
• Caret is within/touching a Token whose direct parent is wanted node (same justification as for selection)
• Caret is within a header of a node. This one is defined on type-by-type basis.
• Method: public I.C([Test]int a = 42) {}
• Local declaration: var a =3,b =5,c =7 + 3 ;
• Foreach: foreach (var a in b) { }
• Property declaration: [Test] public int a { get; set; } = 3;
• ... Note: Generally, the notion on header is not strictly defined and should be refined based on feedback. Also, not all types have defined header as of right now so it's a point of future work.
• Wanted node is Expression / Expression-like and caret is anywhere within such expression as long as it is on the first line of the wanted node.
• Rather crude heuristic to enable e.g. add argument name / tuple element name / await Refactorings even if caret is within the (argument) expression.
• The only first line limitation is to not-enable the Refactoring when a user is too deep e.g. in multiline lambda where it wouldn't make any sense (there were issues with this previously).
• This way of checking not-too-deep is very much a first shot and should be reiterated upon (e.g. climbing only a few nodes up / something more clever, ...). It is very important to tune this shared helper so that the experience is consistent instead of making tweaks to individual Refactorings (as was done in past (some Refactorings didn't have the first-line check -> issues)). Note: Expressions and ArgumentNode is currently treated as Expression-like. Feel free to extend this list if needed.

Present:

• Consistent experience for users (over/under selections, attributes, ... are handled the same way)
• Reusing one codebase for determining if a node is applicable for selection.
• Selection now works for all (moved) Refactorings. Overselection to ; in assignments/declarations is also handled correctly.

Future:

• Ideally all Refactorings that work on individual Nodes (e.g. not GenerateXXX) should be moved over.
• Header rules & extractions should be refined based on feedback.
• Create similar helper for selection of multiple nodes (i.e. user selects multiple declarations, want's to move them all) & enable support for such multi-node interaction within relevant refactorings.
  • Should be fairly simple, get first node based on Start location, update location after the first, … continue.
  • Must handle over/underselection consistently (strip whitespace, ...).
  • Should be able to leverage extractions.

[petrroll]

Ordering of available Refactorings/CodeActions:

When a user invokes CodeActions menu (ctrl+.) individual elements (Refactorings & CodeFixes) are gathered and presented to user. These CodeActions are sorted in some way. Following paragraphs describes how.

Current situation:

Note: All sorts are stable -> consecutive sorts don't change order of two elements unless the sorting criteria forces a change for them.

Note: Both Refactorings & CodeFixes (from Analyzer+CodeFix combo) are present in CodeActions menu. Our scope is limited to better ordering Refactorings, however (ordering for CodeFixes already works reasonable well).

1. Refactorings & CodeFixes are gathered in order in which their providers are returned.
   • The order is determined by ExtensionOrderAttribute that provides explicit relative ordering.
2. Order Refactorings by their Priority first and the distance of current selection and their ApplicableToSpan.
3. If selection is empty -> treat all Refactorings as low priority (rationale) & append ref. after code fixes. Otherwise (non-empty selection) don't change priorities and append fixes after Refactorings.
   • We might want to revisit this rule.
4. Order both together by their Priority first and distance of their ApplicableToSpan to current selection/caret location second.
5. Do inlining, grouping, filtering, … return

Most Refactorings are Medium priority, with the exception of WrapXXX, MoveXXX, ... which are Low priority.

The big issue is that ApplicableToSpan is always set to current selection for all available Refactorings. That means it is useless to order them against each other, and since most have medium priority, the sorting usually falls back to the static ordering defined by ExtensionOrderAttribute.

It also means that Refactorings' distance of ApplicableToSpan to current selection is zero so priority being equal (e.g. between most Refactorings and medium priority codefixes when there's a selection (see 3.)) Refactorings always win. While that's not bad necessarily it's an (IMHO) unintended consequence and should be thought through.

Note: ApplicableToSpan for CodeFixes is equal to their diagnostics spans.

TL;DR:

Generally, current ordering of CodeFixes can be summarized as follows:

• Selection empty: CodeFixes first by priority and ApplicableToSpan, Refactorings mostly by their ExtensionOrderAttribute.
• Selection non-empty: All by priority, Refactorings first within a priority group, Refactorings within each other by ExtensionOrderAttribute.