[WIP] Rework ordering of refactorings & fixes with/out selection

[petrroll]

Previously having empty selection caused refactorings to have
low-priority in codeactions menu. It was introduced in time when only
very few refactorings accepted selection and when there were very little
non-selection based refactorings. Nowadays the distinction between some
codefixes and refactorings is blurry and as such should both be treated
the same way, regardless of selection.

[petrroll]

Before:

After:

[petrroll]

Related: #35525
CC: @kendrahavens

[CyrusNajmabadi]

Nowadays the distinction between some
codefixes and refactorings is blurry and as such should both be treated
the same way, regardless of selection.

i think we need more examples. Because my gut is telling me this is a bad idea. Specifically, codefixes can have UI affordances (squiggles, fading, dots, etc.), while refactorings don't. I'm def very wary of potentially prioritizing something that is effectively invisible on the line versus something visible and nearby.

Note: this applies to fixes that are visible. If a codefix has no UI affordance (i.e. is 'refactoring-only') then it should be bucketed together.

[petrroll]

I'm def very wary of potentially prioritizing something that is effectively invisible on the line versus something visible and nearby.

I agree with that. But if we want to prioritize "visible" codefixes then we should prioritize codefixes as whole. And not when there's selection / isn't, IMO (and also, as you say, we should do that only for non-invisible ones).

I can see how the "when user selects he want's to refactor" made sense back when there were only few (and obviously) selection based refactorings. But it simply doesn't hold true anymore. Especially given most of refactorings previously didn't work with selection.

[petrroll]

I.e. simply bumping priority for Codefixes that fix "Warning" (High), or visible info "?high?" would be a way more consistent solution IMO.

[CyrusNajmabadi]

Yes. I'd prefer a larger rethinking here. I personally agree that refactoring vs fix isn't the relevant axis.

However the code we have now serves an important purpose. I don't think we should get rid of it until we have implemented other rules (like what I mentioned above) to handle those sorts of cases

[petrroll]

Glossary:

• CodeAction: An action that changes code.
• CodeRefactoring: CodeAction produced by a CodeRefactoring.
• CodeFix: CodeAction produced by a CodeRefactoring.
• Explicitly suggested CodeAction: A CodeFix that was explicitly suggested by a user. E.g. as a fix for visibly shown diagnostics (green/red squiggles, three dots, faded-in text (well-known tag Unnecessary)).
  • Right there're no "Explicitly suggested CodeAction"s that would be CodeRefactorings, all are CodeActions.
  • Suggestions: three dots, warning: green, error: red, unnecessary: grayed out

Problem scope:

Explicitly suggested CodeActions should be ordered higher than all other CodeActions, regardless if they're CodeFixes or CodeRefactorings. The rationale is that there might be a number of CodeActions and if we suggested one explicitely there's a high chance a user might really want to perform it. Generally, we only explicitly suggest things a user set (editorconfig, ...) or that really should be done (fixes for warnings, ...).

Current situation:

All CodeActions are ordered by their priority first, and by the distance of the span they change to current caret/selection second. If there's no selection all Refactorings get changed to low priority, essentially causing CodeFixes being top, Refactorings bottom.

Due to the fact that most refactorings have not historically been offered for selection (fixing that as part of: #35525) it can be assumed that users didn't/don't use it except for situations when they wanted to invoke the few ones that worked with selection explicitly, such as extract method and introduce local variable.

That in combination with the fact that all explicitly suggested CodeActions are Codefixes means that the current behavior is (given current user behaviour) not all that far from potentially desired state. Since users don't use selection, the priority of refactorings gets set to low, and codefixes (some/?most of which ore explicitly suggested) get ordered to be on top.

Issues with current situation:

With recent fix of "not offering refactorings on selection" and adding more and more hidden diagnostics + codefix (essentially a refactoring), however, the UX is starting to be more problematic (as shown in first comment's screenshots).

Firstly, users will now be able to use selection for all refactorings which will lead to inconsistent experience where e.g. selection of a keyword leads to substantially different ordering to e.g. having caret in the keyword.

Secondly, adding more hidden diagnostics + codefix combos (like add discard on 2nd before screenshot above) leads to it being on top for caret location (because it's Codefix) even though it's not a explicitly suggested (i.e. there's no reason to believe it should be offered more prominently than any of refactorings) one and is "quite far from current caret location".

[petrroll]

Proposed solution:

First, remove any current priority changes based on selection/refactoring.vs.codefix due to abovementioned problems.

Then, extend CodeFixService so that it modifies priority of CodeActions for CodeFixes based on their diagnostic's severity. Currently most A CodeActions are Medium priority, only few (add missing usings, inline rename) are high, and few are low (wrap arguments, ...).

The priority field is not public and thus all 3rd party are guaranteed to be medium priority.

EDIT: The priority field gets translated to SuggestedActionSetPriority which is part of shipped public API.

Option1:

Plainly change priority based on their diagnostic's severity (1.1):

• Errors/Warnings: High
• Suggestions/Unnecessary: Medium
• Else: Low (?)

Or maybe (1.2):

• Previous low-else: None
• Previous medium-else: Low
• Suggestions/unnecessary: Medium
• Errors/Warning: High
• Previous high-else: High

Issues:

• Removes priority ordering for Diagnostics generally (all that produce warnings are equal now, ...). (?isn't this good -> simpler mental model?) (solved by 1.2 to a certain degree)
• How do refactorings fir into this? Should they all (but a few like extract method) be low priority with a few (that are now low) None? (solved by 1.2 to a certain degree)
• Loose the ability to have always top codeaction regardless of errors/warnings. For 1.2 loose the ability to have bottom. as well. (see below)
• 1.2: None is already used to make sure supression fixes are last: http://index/?query=%60SuggestedActionSetPriority&rightProject=Microsoft.CodeAnalysis.EditorFeatures.Wpf&file=Suggestions%5CSuggestedActionsSource.cs&line=590

Benefits:

• Simple, understandable, doesn't require extension of CodeActionPriority
• Doesn't require changes of external SuggestedActionSetPriority

Option2:

Extend CodeActionPriority to be essentially unbounded (or just add enough levels) and bump priority of explicitely suggested codeactions by certain amount.

• Errors/Warnings: +4
• Suggestions/Unnecessary: +2
• Else: +0

Issues:

• Requires change of semantics of CodeActionPriority (even though it's still backwards compatible).
• Slightly harder to reason about.

Benefits:

• Simple to implement.
• Maintains current relative ordering while achieving the goal of having explicit CodeActions higher.

[petrroll]

Further observations:

• Location dynamic priority: E.g. CSharpRemoveUnreachableCodeCodeFixProvider has dynamic priority: Medium on first unreachable line, low on subsequent.
• Severity dynamic priority: E.g: UseExpressionBodyCodeFixProvider, AbstractAddAccessibilityModifiersCodeFixProvider Low if DiagnosticSeverity.Hidden, otherwise Medium.
• Nothing is currently None priority (?).
• CodeActionPriority gets translated to SuggestedActionSetPriority
  • Only used for sorting, 1:1 mapping.

[petrroll]

Additional notes:

• Option 2 is problematic due to requirement to change external SuggestedActionSetPriority from VSPlatform repo.
  • Is part of public API
• Option 1.1 looses a lot of information & might potentially be problematic. In reality, however, it should be fine.
• Both 1.X have the issue that we need at least 5 levels, ideally 6 to fully order things while current system only has 3/4:
  • Always lowest: (supressions)
  • Non-explicit: previous low
  • Non-explicit: previous medium, explicit suggestion/unnecessary
  • Explicit warning/error
  • Always highest: (add using/rename)

Going between 1.1 and 1.2 should be very simple, it's just a difference in mapping.

[petrroll]

cc @CyrusNajmabadi, thoughts? Will have design meeting next week and would love your input beforehand.

[CyrusNajmabadi]

My main thoughts are that I would drive this through specific problems we are solving. I.e.: this list of fixes is suboptimal.

Figure out why, then tweak as necessary.

I would not really want full scale changes without clear examples of how it improve things.

[petrroll]

Ok, I'll try to produce more examples.

That said, I don't thing there're small tweaks possible. The current experience gets really surprising once selection is allowed more broadly and with more and more invisible diagnostics+codefix (add _) it gets super-weird.

[petrroll]

Current usage of priorities:

None: 
NestedSuppressionCodeAction : always

Low: 
CSharpRemoveUnreachableCodeCodeFixProvider : if > first line

AbstractAddAccessibilityModifiersCodeFixProvider : if hidden
UseExpressionBodyCodeFixProvider : if hidden
AbstractUseAutoPropertyCodeFixProvider : if hidden

CSharpIsAndCastCheckWithoutNameCodeFixProvider : always
AbstractAddImportFeatureService : always
AbstractMoveDeclarationNearReferenceCodeRefactoringProvider : always
AbstractMoveDeclarationNearReferenceService : always
AddImportFixData : always

AbstractSyntaxWrapper: always : explicit
WrapItemsAction : always : explicit
ParentInstallPackageCodeAction : always : explicit

AbstractAddImportFeatureService : low if other project : explicit : else high

High: 
AbstractAddImportFeatureService : low if other project : else high
? Rename : afterRenamingSmthWithoutRenameAction

Medium:
Else / third party: 

[petrroll]

Instead of explicitely changing priority in the service we might want to just go with the change in this PR + change the way CodeFixes report their priority (?add helper that takes DiagnosticSeverity and returns CodeActionPriority).

In that case we need to deal with the fact that all 3rd party (including refactorings) is Medium which leaves us only very little room for higher priority. Ideally we would want:

• Keep Refactorings Medium (3rd party)
• Have (most) Refactorings >= hidden CodeFixes (currently Low/Medium)
• Have ability to have always top codeactions (AddImport): High
• Have ability to have always bottom codeactions (Suppresion): None
• Have > Refactorings (Medium) for explicit CodeFixes (?warnings, suggestions, unnecessary)
  • Needs to be > Medium but < High ~> Issue
• Have > Refactorings (Medium) for super explicit CodeFixes (errors, ?warnings)
  • Could be High

[petrroll]

After proposed explicit CodeAction boost (Warning > suggestion > CodeRefactoring, Hidden > Supress):

Just with this PR:

Previous:

[petrroll]

Changing CodeFixes so that they report priority based on Diagnostics:

Issues:

• Most CodeFixes CodeActions classes don't have constructor that enables setting Priority (easy solution, but some boilerplate).
• Additional logic in every codefix (might be easy to forget, ?analyzer).

Benefits:

• Some CodeFixes need more elaborate Priority selection: e.g. RemoveUnreachable firstLine x rest, AddImport
• Doesn't impose change on other users of CodeAction service while providing them with helper.
• Better controllable than constant boosting / translation based on diagostics' severity.

[petrroll]

Results from design meeting:

• Want to boost explicitly offered CodeActions, with following tiers:
  • Suggestions/Unnecessary
  • Warnings/Errors
• Current high needs to remain top
• Current low/medium distinction should remain as well
• Explicitly boost any CodeFix for which we're at the beginning of its applicableToSpan -> user wanted to fix that one particular thing.

Implementation:

• The SuggestedActionsSource will handle boosting without explicitly changing either of the Priority enums, they'll remain as they are to provide the baseline priority information.
• Any external interfacing will use the original un-boosted-priority (interfacing with IntelliSense defined Priority, ...).

Notes:

• CodeFixes should just set their initial priority & not do boosting based on their set severenity (i.e. don't do what if hidden do [WIP] Rework ordering of refactorings & fixes with/out selection #38271 (comment))

[petrroll]

Initial implementation done:

• CodeActionSets are ordered:
  • Priority first
  • SuggestedActionSetPriorityBoost (from diagnostics severity) second
  • applicableToSpan third
  • Static ordering of providers last
• SuggestedActionSetPriorityBoost:
  • DiagnosticSeverity.Hidden && WellKnownDiagnosticTags.Unnecessary => SuggestedActionSetPriorityBoost.Suggestion,
  • DiagnosticSeverity.Hidden => SuggestedActionSetPriorityBoost.None,
  • DiagnosticSeverity.Info => SuggestedActionSetPriorityBoost.Suggestion,
  • DiagnosticSeverity.Warning => SuggestedActionSetPriorityBoost.ExplicitRecommendation,
  • DiagnosticSeverity.Error => SuggestedActionSetPriorityBoost.ExplicitRecommendation,

Remarks:

• Since most CodeFixes/Refactorings CodeSuggestions are Medium priority anything with SuggestedActionSetPriorityBoost (i.e. explicitely suggested) will get higher.
• Since Priority is still the first criteria, anything with High or None will have guaranteed positions (top/bottom).

TODO (this PR):

• Switch from tuple to struct to cleanup the code.
• Document the feature properly including guidlines on cond. changing priority in codefixes (e.g. if hidden from [WIP] Rework ordering of refactorings & fixes with/out selection #38271 (comment)).
• Consider adding special ordering after priority that will prioritize CodeSuggestionSets for which the cursor is at the beginning of their applicableToSpan (more important than SuggestedActionSetPriorityBoost, maybe even Priority (?)) and (maybe) deprioritizes those for which the cursor is completely outside. As per @sharwell suggestion.
• Fix order-based integration tests, add integration tests for new ordering.

TODO (future):

• LSP issues around ordering: general case of LSP strips applicableToSpan for CodeRefactorings / CodeFixes #38097. LSP communication will/would strip away the link to diagnostics (because it transfers everything as Refactoring) and such this improvement wouldn't apply. It might be beneficial to simply leave ordering on the server side and let client just present the results in whatever order they come in.

Current issue:
VS platform re-orders all LightBulb actions based on CodeSuggestionSet.Priority and CodeSuggestionSet.TextSpan again, and so any changes of order on our side that are out of sync with them get thrown away.

[petrroll]

Alternative approach to: 243d101

Since VS Platform needs to do reordering due to potentially other sources of LightBulb actions we need to translate PriorityBoost directly to SuggestedActionSetPriority (defined here). As discussed above (#38271 (comment)) that is impossible due to there being only one level above currently default Medium.

Therefore to enable feature proposed in this PR we'd need to extend SuggestedActionSetPriority with at least two more levels between Medium and High to accommodate:
SuggestedActionSetPriorityBoost.Suggestion (suggestions/unnecessary) and SuggestedActionSetPriorityBoost.ExplicitRecommendation (warnings/errors).

Such extension would, however, need to make sure that current/legacy High priority remains the Highest priority in the new system as to not break 3rd party expectation. Other back-compat implications of extending enum might apply.

Note: in addition to doing that updating VS's SuggestedActionSetComparer so that it takes into account current user selection instead of just caret position would also be beneficial. I.e. to make it consistent with changes in Roslyn done in here: https://github.com/dotnet/roslyn/pull/37941/files#diff-c115e9b22618dafba7dd3f8d6e2c4621