Add 'priority' concept to our code actions, and code refactoring/fix providers. #68511
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -40,7 +40,7 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context) | |||
| // helpful, but shouldn't interfere with anything else the uesr is doing. | |||
| var priority = IsSubsequentSection(diagnostic) | |||
| ? CodeActionPriority.Low | |||
| : CodeActionPriority.Medium; | |||
| : CodeActionPriority.Default; | |||
There was a problem hiding this comment.
Lowest/Low/Default/High/Highest matches editor naming for this sort of thing (including the lightbulb priority class namign. so i aligned on that for our public API).
There was a problem hiding this comment.
i also preferred 'Default' (versus 'Normal' or 'Medium') as it prevented the need to have a duplicate 'Default' member, or to explain what the 'Default' value for these enums was. By just naming it that way, it all simplified (though it does impact the diff here).
| private protected override CodeActionRequestPriority ComputeRequestPriority() | ||
| => CodeActionRequestPriority.High; | ||
| private protected override CodeActionRequestPriorityInternal ComputeRequestPriorityInternal() | ||
| => CodeActionRequestPriorityInternal.High; |
There was a problem hiding this comment.
features that return 'high' use the 'internal' priority system. For now, only we allow a blessed set to be high pri. That set is exactly:
- Add using.
- Rename
- Fix formatting
- Fix git merge-conflicts
| @@ -40,7 +40,7 @@ protected AbstractBuiltInCodeStyleDiagnosticAnalyzer(ImmutableArray<DiagnosticDe | |||
| Debug.Assert(!supportedDiagnostics.Any(descriptor => descriptor.CustomTags.Any(t => t == WellKnownDiagnosticTags.Unnecessary)) || this is AbstractBuiltInUnnecessaryCodeStyleDiagnosticAnalyzer); | |||
| } | |||
|
|
|||
| public virtual CodeActionRequestPriority RequestPriority => CodeActionRequestPriority.Normal; | |||
| public virtual CodeActionRequestPriorityInternal RequestPriority => CodeActionRequestPriorityInternal.Normal; | |||
There was a problem hiding this comment.
we could consider makign the diagnostic-analyzer priority part available as well.
| @@ -64,8 +64,6 @@ internal abstract partial class SuggestedAction : ForegroundThreadAffinitizedObj | |||
| CodeAction = codeAction; | |||
| } | |||
|
|
|||
| internal virtual CodeActionPriority Priority => CodeAction.Priority; | |||
There was a problem hiding this comment.
never used. removed.
| // Note: CodeActionRequestPriority.Lowest is reserved for IConfigurationFixProvider. | ||
| Contract.ThrowIfFalse(priority is CodeActionRequestPriority.Low or CodeActionRequestPriority.Normal or CodeActionRequestPriority.High); | ||
| return priority; | ||
| Debug.Assert(priority is CodeActionRequestPriority.Low or CodeActionRequestPriority.Default or CodeActionRequestPriority.High, "Provider returned invalid priority"); |
There was a problem hiding this comment.
note: this is just an assert to catch if we do something wrong ourselves. We dont' want to throw here as our approach is to just clamp. that way someone coudl request a higher pri than currently supported, and we could potentially allow it in the future.
|
@mavasani this is ready for another review. Thanks for the suggestion to just have a single public enum for both CodeActionPriority and CodeActionRequestPriority (and to not have any sort of internal matching enum for each). It def cleaned things up. |
|
|
||
| #if !CODE_STYLE | ||
| // Backdoor that allows this provider to use the high-priority bucket. | ||
| codeAction.CustomTags = codeAction.CustomTags.Add(CodeAction.CanBeHighPriorityTag); | ||
| #endif |
There was a problem hiding this comment.
however, because it is high pri, the feature has to use this backdoor for that to be ok. it will be clamped otherwise.
|
@mavasani ptal. |
There was a problem hiding this comment.
Overall, this approach LGTM.
I believe we should allow CodeStyle layer to also specify the high priority custom tag. So basically replace your current code patterns:
#if !CODE_STYLE
// Backdoor that allows this provider to use the high-priority bucket.
this.CustomTags = this.CustomTags.Add(CodeAction.CanBeHighPriorityTag);
#endifwith
// Backdoor that allows this provider to use the high-priority bucket.
#if !CODE_STYLE
this.CustomTags = this.CustomTags.Add(CodeAction.CanBeHighPriorityTag);
#else
// Reflection based logic to add `CanBeHighPriorityTag` to `CustomTags` property.
#endifand at this point, we probably don't need to do this via CustomTags property, but can have a new internal bool property to indicate "please don't clamp me". It's your call on choosing the most preferred implementation though.
I'll try this today. |
There was a problem hiding this comment.
We discussed this offline. This PR is not regressing any existing functionality, as code action priority has always been an internal concept which was never available in the CodeStyle layer. We would eventually like CodeStyle layer to also be able to supply these priorities, but that would be a new feature request. Given that default VS customers who do not explicitly reference MS.CA.CSharp.CodeStyle NuGet package are not affected here, adding this feature would be a slightly lower priority. If we do decide to take it up, reflection would be a possible approach.
ghost
added this to the 
Fixes #42431.
This is already a concept we have internally to support prioritizing important work, as well as determining the order to show things in. Not having this has hampered our efforts to move things into the CodeStyle layer (which can only access the public API), as well as allowing analyzer-packs (like roslyn-analyzers) to take advantage of this for their own analyzers.
Note: this PR intentionally does not expose a way for 2nd/3rd parties to provide high priority work. We do not want to do that yet as we are concerned about slow analyzers/providers being added in that then detrimentally impact features like 'add using'. That said, we have designed the code such that a 3rd party could try to go to a higher priority class. Currently, they will be clamped back down to the normal priority class. But if we feel we are ok with allowing this in the future, we can then just change thigns on our end with the 3rd party not having to change their code.