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. :)

[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.

[petrroll]

Ordering of available Refactorings/CodeActions:

Proposed solution:

The proposals above revolve around giving the service more information so that it is able to order Refactorings CodeActions better. Using this-refactoring-is-going-to-work-on-this-span type of information has even been proposed as part of 1.impl3 proposal. And since the higher-level service (SuggestedActionsSourceProvider) already knows how to handle that type of information it is the logical option.

Since most refactorings operate on nodes & we have a standardized way of getting the nodes (see Common helpers above) there's also a convenient source of ApplicableToSpan information, just use the Span of retrieved/used node.

Therefore the proposal is to:

• Extend CodeRefactoringService so that individual Refactorings can communicate their ApplicableToSpan when registering a CodeAction.
• Update SuggestedActionsSource so that it uses ApplicableToSpan provided by Refactorings (instead of hard-setting it to current selection).
• Update individual Refactorings so that they report the span of their Node (returned by helpers) as their ApplicableToSpan

Issues:

Implementation issues:

Some Refactorings might not necessarily operate on nodes. For example generateXXX Refactorings can generate new nodes anywhere in root of a type. For these refactorings, however, we can simply specify their ApplicableToSpan as the whole enclosing type.

That will cause the distance to current carret / selection being larger then for all other available refactorings which is the desired state. Generally, in conjunction with using Priority this shouldn't be pose a real problem.

Migration issues:

• How to handle Refactorings that haven't been updated yet (don't report their ApplicableToSpan eg.: 3rd party, …)?
  • If we use current selection (what is done today) as a fallback they'd always be prioritized (closest) -> undesirable.
  • Probably don't want to do the opposite (largest span -> always lowest within the same priority) either
  • Remains open question

Limitations of using ApplicableToSpan:

Current implementation of sorting by ApplicableToSpan has several limitations:

• Doesn't take whole selection into account, only caret position (usually the beginning of a selection)
  • Can be extended to use Selection (compare starts & ends) -> CodeFixes would benefit from this as well
• Only compares the closer edge of ApplicableToSpan
  • Can also be fixed (sum of distances to both edges) -> CodeFixes would benefit from this as well
  • Is $$var a = [|b + c|]; closer or further than $$var a = [|b|] + c;

Note: Generally, AoRN a lot of CodeFixes (and potentially Refactorings if their ApplicableToSpan was used properly) would have the same "distance" of their ApplicableToSpan to current selection due to the fact that the comparer doesn't use all available information.

Alternatives:

There're two types of alternatives. Firstly, those who work very similarly to proposed solution via providing more info to the SuggestedActionsSource service so that it can order actions better. Secondly, those that implement multiple-pass approach within either SuggestedActionsSource or CodeRefactoringService and order based on how far the a pass is from current selection.

Similar alternatives:

A different type of information than an ApplicableToSpan could be shared by Refactorings. For example the refactorings could calculate distance to given span themselves / return how they had to "climb" from given selection to get desire node (e.g Enum).

Both of these approches would require non-trivial changes within SuggestedActionsSource so that it is able to use the retrieved information for sorting. It would also create a divergence between how CodeFixes and Refactorings are sorted.

At the same time, both would enable fine-grained per-Refactoring tweaks to priority. For example two refactorings with the same priority that work on the same Node might want to have very different notion of "climbing to get the node is this expected" and might want to return different distance. That said, these situations should be far and in between and generally non-impactful. And if they arose they could still be mitigated via returning a modified Span instead of the span of the Node a Refactoring actually works on.

Returning any "structured" (e.g. Enum) information would create compatibility issues with other languages (F#, …) that might have different notions of SyntaxTrees and extractions. It would laso require bigger changes within all Refactorings even though that could be mitigated via extending abovementioned helpers (they could report either the structured "how did I climb to get the node" info or the distance).

Moving the computaiton distance to individual refactorings would also require changes within individual refactorings. And while it would enable some of the abovementioned fine-grained tuning, the costs of having to change SuggestedActionsSource doesn't seem worth it. Also it would make the exchanged information an arbitrary "metric" which might be harder to understand and consistently treat than an ApplicableToSpan.

Different alternatives

The other proposed alternative (2.impl2) was to limit Refactorings to register only on very conservative set of tokens & then do multiple passes with changed "current selection". In essence the CodeRefactoringService try to ask all refactorings “Are you applicable” for multiple locations around current caret / selection.

This approach has several advantages:

• Shifts more responsability from individual Refactorings to shared service.
• Transparently enables situations where the current selection is nowhere in/near desired node but still close enough.
  • Essentially, it enables capturing arbitrarily large surrounding while preserving sensible ordering.
  • The size of surrounding can be dynamic (larger if no CodeFixes for current specific location, …)

It, however, has even more disadvantages:

• Multiple passes are hard to do for languages without SyntaxTree notion (e.g. F#, TypeScript) within the system because it's hard to define where the other passes should be / in which order.
  • Even with syntax-tree it's not easy to define order of multiple-passes.
• Requires significant changes to both CodeRefactoringService (needs to learn how to do multiple passes) and SuggestedActionsSource (how to digest "node retrieved in n-th pass" information).
• The requirement that Refactoring should only register for conservative Tokens might be hard to do. To identify if the Token is suitable a lot of work mentioned in helpers design (e.g. the notion of headers) would still need to be done. Essentially this approach wouldn't simplify individual Refactorings that much.
  • It would also remove the ability to fine-tune within individual Refactorings.

Conclusion:

While alternatives exist the chosen approach is simple and solves most current issues. Since it has very little risks & costs (in comparison to abovementioned alternatives) it should be implemented first. If it proves to be insufficient more involved approach (e.g. multiple passes) can be reconsidered without sunken cost.

Other languages impact

Since we're reusing already existing facilities within the SuggestedActionsSource and just slightly extending CodeRefactoringService other languages shouldn't be impacted. Since the newly exchanged information is TextSpan (language agnostic) they should also be easily able to leverage these new ordering capabilities the same way as VB/C# Refactorings.

Refactorings for other languages will not be able to reuse abovementioned helpers to get relevant nodes for current selection, however, as the helpers are very SyntaxTree dependent and assume a lot of C#/VB invariants.

[petrroll]

Relevant: #33818