dotnet / roslyn

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

Quick actions list has a suggestion before a fix - breaks hotkey flow #28550

Closed vsfeedback closed 3 weeks ago

vsfeedback commented 6 years ago

Depending on where in a line the cursor is, the quick actions suggestions can contain a "suggestion" at the top of the list, when what I really want is the error on the same line to be at the top of the list.

This breaks a workflow of the keyboard shortcut, followed by enter to "fix" the resolution problem. In my case, the suggestion is IDE0024, and the error is CS0103 (I'm missing a using directive).

It's variable depending where in the line my cursor is placed.

This issue has been moved from https://developercommunity.visualstudio.com/content/problem/16055/quick-actions-list-has-a-suggestion-before-a-fix-b.html VSTS ticketId: 418477 These are the original issue comments:

Cyrus Najmabadi [MSFT] on 2/1/2017, 04:17 PM (528 days ago):

Hi Craig,

Can you provide a little more information? With your example provided, what line/column is your caret at? Can you provide a screenshot of what you see in the list when you bring up the lightbulb list as well?

Thanks!

Craig Popham on 2/1/2017, 04:44 PM (528 days ago):

Hi Cyrus,

Sure - I've attached two screenshots showing the behaviour. You're right - it varies based on caret position.

The first case (see attachment "repro-happy.png") happens when my caret is anywhere on the red underlined symbol (on line 20). This is the "good" case.

repro-happy.png

However, if the caret is anywhere else on the line (20) (see attachment "repro-sad.png"), then I get the "tip" for IDE0024 first. In my specific case, my caret is at the end of the line, immediately after the semicolon, because I've just finished typing the rest of the line. In previous versions of VS, the "fix" for CS0246 would have been first, but it is now second after the tip. This is a pretty minor annoyance in the grand scheme of things, but it makes the quick actions panel less useful from the keyboard.

repro-sad.png

I realise this behaviour might actually be by design, so maybe it's more appropriate for a suggestion than a bug?

I can reproduce this with other suggestions (e.g. IDE0030 - null propagation), and the same behaviour occurs - when the caret is over the invalid expression the tip is suppressed, but in the rest of the statement both tip and fix are shown in the list, with the tip being selected in preference.

I suppose what I'm really expecting could be more generally expressed as "if my caret is anywhere in a statement, and there are both style suggestions, and a fix available via quick actions for that statement, then the fix should be the automatically selected item, in preference to the style suggestion".

These are the original issue solutions:

Cyrus Najmabadi [MSFT] solved on 2/1/2017, 04:49 PM (528 days ago), 0 votes:

Awesome. Thanks for the clarification. We agree that this is not desirable behavior. We will have to think of a good solution here as certain layering makes adjustments here difficult.

CyrusNajmabadi commented 6 years ago

Tagging @sharwell . This is an issue where there is a large hidden-diagnostic for "use expression body". As this is effectively a 'refactoring', it would go in the bucket that it should be prioritized after real suggestions with real adornments (ellipses/squiggles) in the editor.

I think you're tracking these right?

sharwell commented 3 years ago

Potentially related to #28261?

jmarolf commented 3 years ago

Problem

Dues to leaky abstractions the way a refactoring is implemented (either as an analyzer/codefix pair or using the refactoring API) can have a significant impact on where it appears in the lightbulb menu relative to the user's cursor.

Background

The algorithm for displaying the lightbulb is rather complex because of several previous designs that needed to be inter-woven into the lightbulb menu when Roslyn was being developed. There are essentially two "Modes" one for if you cursor is on the line where the light bulb is and one without.

If there is no cursor on the line we show things in "priority" order. There are some fixes that are marked as "High priority" and others that are marked as "low priority". Examples of high priority fixes would be "add usings" examples of "low priority" fixes would be secerity configuration.

If the cursor is on the current line we attempt to order fixes based on the which span is closest to the cursor. This means that things like refactorings which do not have spans are always ordered after code fixes. There is a bit of complexity here about ordering when spans overlap (which is what the original issue is about). If the spans are "identical" or there is no span then items are ordered according to the "priority" order above.

image

this was what was around before the lightbulb menu and it operated similarly to how Microsoft Word's spell check works: the user hovers their mouse over a squiggle, there is a little "tag" to click on, and the tag suggests fixes/refactorings. For keyboard users, they could place the cursor on the squiggle and bring up the list with Ctrl+.. This keyboard behavior is what needed to be preserved as we moved the the lightbulb menu in 2015.

Proposal

CyrusNajmabadi commented 3 years ago

Change the lightbulb service to know if some codefixes are actually "refactorings" and consider these lower priority than regular code fixes using our existing priorty system.

I actually thought we had this :)
If we're not respecting it somewhere, i do agree that that's a bug more than anything else :)

jmarolf commented 3 years ago

@CyrusNajmabadi I think this is something we can accomplish today without adding/changing any APIs. There is just not been a wider effort to ensure all code fixes and refactorings that we offer have a priority set of them. I think if we do that everywhere, then this issue is fixed in principle.

CyrusNajmabadi commented 3 years ago

@CyrusNajmabadi I think this is something we can accomplish today without adding/changing any APIs.

Agreed :)

vatsalyaagrawal commented 3 years ago

Design meeting conclusion:

  1. Fix the individual heuristic
  2. Call for volunteers - There are multiple axis which can be used to define sort order. Come up with concrete proposals for improvements in this area.
CyrusNajmabadi commented 3 weeks ago

Closing out due to lack of feedback.