-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add 'priority' concept to our code actions, and code refactoring/fix providers. #68511
Changes from 58 commits
7b9dab4
8b175b0
72795ac
be0075a
f00a244
38a8d57
0ac048a
8906650
1a6240e
657bf1f
5342a43
ff293b1
b92df55
ff2b894
cb2ae1b
741e7ad
81a4645
704b40e
8b8f3cc
38e021c
aff8526
939e28b
7398b34
b33d117
1ca450b
4e24505
3ac6bc1
974cf63
ce1286b
9abc31e
9334477
0624db5
bdecd94
685ed9d
37a2b3f
178a730
e2a04c7
bdbf75e
82a9e30
11d72c1
0d59cca
d95498b
39ed273
923eac7
b9cb2ab
7da1751
71b1862
c7f16db
dcb6052
138d3a3
81458d9
6d5c41b
605ee29
773e21c
76e235b
024a103
8ced823
72f86df
ff02e51
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,6 @@ | |
using System.Collections.Immutable; | ||
using System.Diagnostics; | ||
using System.Linq; | ||
using Microsoft.CodeAnalysis.CodeActions; | ||
using Microsoft.CodeAnalysis.Diagnostics; | ||
|
||
namespace Microsoft.CodeAnalysis.CodeStyle | ||
|
@@ -40,7 +39,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 bool IsHighPriority => false; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the internal IBuiltInAnalyzer api. it currently only ever needs to know if something is high pri or not. It exists at a layer that can't reference Workspaces. So it was much simpler to just make this a boolean. |
||
public sealed override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } | ||
|
||
protected static DiagnosticDescriptor CreateDescriptorWithId( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,12 +45,15 @@ protected AbstractResolveConflictMarkerCodeFixProvider( | |
{ | ||
FixableDiagnosticIds = ImmutableArray.Create(diagnosticId); | ||
_syntaxKinds = syntaxKinds; | ||
|
||
#if !CODE_STYLE | ||
// Backdoor that allows this provider to use the high-priority bucket. | ||
this.CustomTags = this.CustomTags.Add(CodeAction.CanBeHighPriorityTag); | ||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how we allow for only our own internal providers to use the 'high pri' bucket for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. both the fix provider, and the fixes have to be marked 'high pri'. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Anyone who retursn .High pri from ComputeRequestPriority or who has a fix with .High pri needs to do soemthing like this. You'll see that in 4 features across thsi PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this needs to be wrapped around There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why is this required for both providers and code actions? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see that we are requiring providers to have this custom tag for the clamping support. I can't recall our exact discussion about this in past, but I still feel CodeStyle Nuget package should be able to supply high priority code actions. Which means we need a couple of things:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a comment https://github.com/dotnet/roslyn/pull/68511/files?diff=unified&w=1#r1264893170, which may be a good middle ground here, without requiring any additional public API or assembly name checks for clamping support. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because a slow 3rd party high PRI provider would be detrimental to all our 1st party providers. This was the whole idea of keeping that to us for the time being. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
See comment above for the reason for providers. We also want to limit code actions so that they do not show up ahead of our 1st party, high PRI, actions as the would break muscle memory. Like 'add using' is just that special (billions of invocations, orders of mag higher than the rest) |
||
} | ||
|
||
public override ImmutableArray<string> FixableDiagnosticIds { get; } | ||
|
||
#if !CODE_STYLE | ||
|
||
/// <summary> | ||
/// 'Fix merge conflict markers' gets special privileges. A core user scenario around them is that a user does | ||
/// a source control merge, gets conflicts, and then wants to open and edit them in the IDE very quickly. | ||
|
@@ -59,11 +62,9 @@ protected AbstractResolveConflictMarkerCodeFixProvider( | |
/// them if they bring up the lightbulb on a <c><<<<<<<</c> line, it should run ahead of | ||
/// normal fix providers else so the user can quickly fix the conflict and move onto the next conflict. | ||
/// </summary> | ||
private protected override CodeActionRequestPriority ComputeRequestPriority() | ||
protected override CodeActionRequestPriority ComputeRequestPriority() | ||
=> CodeActionRequestPriority.High; | ||
|
||
#endif | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. providing priority is no longer something CODE_STYLE cannot do. |
||
|
||
public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
{ | ||
var cancellationToken = context.CancellationToken; | ||
|
@@ -298,11 +299,14 @@ private static void RegisterCodeFixes( | |
|
||
static CodeAction CreateCodeAction(string title, Func<CancellationToken, Task<Document>> action, string equivalenceKey) | ||
{ | ||
#if CODE_STYLE | ||
return CodeAction.Create(title, action, equivalenceKey); | ||
#else | ||
return CodeAction.DocumentChangeAction.Create(title, action, equivalenceKey, CodeActionPriority.High); | ||
var codeAction = CodeAction.Create(title, action, equivalenceKey, CodeActionPriority.High); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. normal CodeAction methods to create a code action with priority. |
||
|
||
#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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. however, because it is high pri, the feature has to use this backdoor for that to be ok. it will be clamped otherwise. |
||
|
||
return codeAction; | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -65,8 +65,6 @@ internal SuggestedAction( | |
CodeAction = codeAction; | ||
} | ||
|
||
internal virtual CodeActionPriority Priority => CodeAction.Priority; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. never used. removed. |
||
|
||
public virtual bool TryGetTelemetryId(out Guid telemetryId) | ||
{ | ||
telemetryId = CodeAction.GetTelemetryId(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).