Configure severity level of .editorconfig rule from editor with Additional Files#29332
Conversation
…le directly from the editor using Additional Files
| } | ||
| else | ||
| { | ||
| properties["OptionCurrent"] = option.Value.ToString().ToLowerInvariant(); |
There was a problem hiding this comment.
so. in general, i'm pretty concerned about this sort of duplication happening all over the place. It's not at all clear, IMO, for features to know tey need to do this sort of this. It's also not clear why this wouldn't just be specified on the option itself. Then the option could just be used to create all this data.
There was a problem hiding this comment.
still have this concern.
| var additionalLocations = ImmutableArray.Create(member.GetLocation()); | ||
|
|
||
| var properties = ImmutableDictionary.CreateBuilder<string, string>(); | ||
| var ruleName = CodeStyleOptions.RequireAccessibilityModifiers.StorageLocations.OfType<EditorConfigStorageLocation<CodeStyleOption<AccessibilityModifiersRequired>>>().FirstOrDefault(); |
There was a problem hiding this comment.
I really dislike all of this. It makes things so leaky.
Ths is similar to my issue with how we did codecleanup in the first place. Instead of keeping concerns separated, we made it so that different systems have a deep and leaky knowledge of how other parts of the ysstem work. This is extremely brittle and difficult to maintain.
There was a problem hiding this comment.
Please add a comment mentioning the issue that @CyrusNajmabadi filed at every place in the PR where we are introducing tight-coupling or hard coding options.
| { IDEDiagnosticIds.UseExpressionBodyForOperatorsDiagnosticId, CSharpCodeStyleOptions.PreferExpressionBodiedOperators }, | ||
| { IDEDiagnosticIds.UseExpressionBodyForPropertiesDiagnosticId, CSharpCodeStyleOptions.PreferExpressionBodiedProperties }, | ||
| { IDEDiagnosticIds.UseExpressionBodyForIndexersDiagnosticId, CSharpCodeStyleOptions.PreferExpressionBodiedIndexers }, | ||
| { IDEDiagnosticIds.UseExpressionBodyForAccessorsDiagnosticId, CSharpCodeStyleOptions.PreferExpressionBodiedAccessors } |
There was a problem hiding this comment.
This continues the tendancy for us to have tight coupling between features scattered all over the codebase. I very much do not like this.
| { IDEDiagnosticIds.UseDeconstructionDiagnosticId, CodeStyleOptions.PreferDeconstructedVariableDeclaration }, | ||
| { IDEDiagnosticIds.MakeFieldReadonlyDiagnosticId, CodeStyleOptions.PreferReadonly }, | ||
| { IDEDiagnosticIds.UseConditionalExpressionForAssignmentDiagnosticId, CodeStyleOptions.PreferConditionalExpressionOverAssignment }, | ||
| { IDEDiagnosticIds.UseConditionalExpressionForReturnDiagnosticId, CodeStyleOptions.PreferConditionalExpressionOverReturn } |
There was a problem hiding this comment.
More tight coupling. How would anyone know they would ever need to update these areas of the code when changing any sort of feature/analyzer/fixer/etc.?
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
Overall, i think more thought needs to be put into our optiosn and feaures so that they can naturally fit into a system like 'cleanup' or 'configure editorconfig severity'.
Right now we've implemented these features for C# only, and in a way where the code is scattered around and hard to maintain.
I think we need a suitable design here that removes all that tight and brittle coupling.
--
One way to validate that we've done the right thing would be to make it so that we decided on one feature we wanted to be part of Cleanup/Configure, but we didn't include it in the core PRs. THen, later on, we'd want to light things up for that. To do so, we should not have to touch the core Cleanup/Configure code.
Another way to think about it is that the core code should exist at a layer that knows nothing about the features themselves. The features should just be provided through plugins and they should light up those systems automatically without having to change the core layer.
|
As an example. Imagine we wanted to light stuff up for VB or C# specific features. Could we do that without adding specific code that had to know about those features' IDs/Options themselves? If not, that's a problem, because we're effectively saying: the only way to Cleanup/Configure something is for us to hardcode knowledge about it. That violates core concepts we have with Analyzers/Fixers/Refactorings in that they need to be something that can just be added to the system independently without anything else having to know or care about them. |
| } | ||
| </Document> | ||
| <AdditionalDocument FilePath="".editorconfig""># Core EditorConfig Options | ||
| root = true |
There was a problem hiding this comment.
Can we trim this file to only have the entries relevant to this test?
| } | ||
| </Document> | ||
| <AdditionalDocument FilePath="".editorconfig""># Core EditorConfig Options | ||
| root = true |
There was a problem hiding this comment.
Same comment here and for other tests which have this large original .editorconfig file.
| private readonly ConditionalWeakTable<AnalyzerReference, ProjectCodeFixProvider>.CreateValueCallback _createProjectCodeFixProvider; | ||
|
|
||
| private readonly ImmutableDictionary<LanguageKind, Lazy<ISuppressionFixProvider>> _suppressionProvidersMap; | ||
| private readonly ImmutableDictionary<LanguageKind, Lazy<ImmutableArray<ISuppressionFixProvider>>> _suppressionProvidersMap; |
There was a problem hiding this comment.
Can you rename ISuppressionFixProvider to something like ISuppressionOrConfigurationFixProvider?
There was a problem hiding this comment.
Similarly, rename the field _suppressionProvidersMap and methods and local variables in this file to reflect this.
| { | ||
| await AppendFixesOrSuppressionsAsync<ISuppressionFixProvider>( | ||
| document, span, diagnostics, result, provider, | ||
| hasFix: d => provider.CanBeSuppressedOrUnsuppressed(d), |
There was a problem hiding this comment.
Rename CanBeSuppressedOrUnsuppressed to CanBeConfigured?
| return null; | ||
| } | ||
|
|
||
| return new WrapperCodeFixProvider(lazySuppressionProvider.Value, diagnosticIds); |
There was a problem hiding this comment.
Please add a comment here and a doc comment to the method that it explicitly looks for an AbstractSuppressionCodeFixProvider
| { | ||
| var solution = project.Solution; | ||
| TextDocument editorconfig = null; | ||
| FindOrGenerateEditorconfig(); |
There was a problem hiding this comment.
Change signature to return the editorconfig?
| string mostRecentHeader = null; | ||
|
|
||
| var name = string.Empty; | ||
| FindRuleName(); |
There was a problem hiding this comment.
Change signature to string FindRuleName() to return the name?
There was a problem hiding this comment.
Nit: Return null if cannot find it.
| TextDocument editorconfig = null; | ||
| FindOrGenerateEditorconfig(); | ||
|
|
||
| editorconfig.TryGetText(out var text); |
| FindOrGenerateEditorconfig(); | ||
|
|
||
| editorconfig.TryGetText(out var text); | ||
| var result = editorconfig.GetTextAsync().Result; |
There was a problem hiding this comment.
Can you instead make this an async method internal static async Task<Solution> ConfigureEditorConfigAsync(...) and use var result = await editorconfig.GetTextAsync().ConfigureAwait(false) here?
|
|
||
| var rulePattern = new Regex(@"([\w ]+)=([\w ]+):([\w ]+)"); | ||
| var headerPattern = new Regex(@"\[\*([^#\[]*)\]"); | ||
| if (rulePattern.IsMatch(curLineText)) |
There was a problem hiding this comment.
You should probably add a few unit tests where the header or rule entry has typo.
|
Pushed in a few more refactorings. Any feedback is greatly appreciated! |
|
Some questions about the feature overall:
Overall i have a general concern about this bloating up the context menu too much. Is it possible you could have a few suggestions here and each suggestion will have a corresponding "configure severity" option shown along with it? Should we instead potentially expose this on hte preview dialog like we do for 'Fix all'. i.e. here:
Can we add the "configure severity as a drop down here instead? That way hte main list stays uncluttered, but config about a specific fix is all centralized? |
|
Question about:
Do we need "Refactoring Only" as that's another severity we support? |
| var expectedRoot = await expectedDocument.GetSyntaxRootAsync(); | ||
| Assert.Equal(expectedRoot.ToFullString(), root.ToFullString()); | ||
| } | ||
| foreach (var additionalDoc in project.AdditionalDocuments) |
There was a problem hiding this comment.
nit. blank line above this.
| Diagnostic diagnostic, | ||
| ImmutableArray<CodeAction> nestedActions) | ||
| : base(FeaturesResources.Configure_severity_level_via_editorconfig_file, | ||
| ImmutableArray<CodeAction>.CastUp(nestedActions), isInlinable: false) |
There was a problem hiding this comment.
this CastUp seems unnecessary.
| { | ||
| var solution = project.Solution; | ||
| var editorconfig = FindOrGenerateEditorconfig(); | ||
| var result = await editorconfig.GetTextAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
pass cancellationToken to GetTextAsync.
| Diagnostic diagnostic, | ||
| Project project, | ||
| Dictionary<string, Option<CodeStyleOption<bool>>> languageSpecificOptions, | ||
| Dictionary<string, Option<CodeStyleOption<ExpressionBodyPreference>>> expressionOptions, |
There was a problem hiding this comment.
this seems like a layering violation. This is a generic system for configuring editorconfig. Why does it have any knowledge of specific enums like ExpressionBodyPreference? Would we expect that every time we added a new optoin, we'd need to come fix up this code to be aware of it?
| // First we check if the rule already exists in the file; if it does, we replace it | ||
| if (CheckIfRuleExistsAndReplaceInFile()) | ||
| { | ||
| return solution; |
There was a problem hiding this comment.
if hte rule exists... why would we offer the feature in the first place? That concerns be. That means that even if the user has a rule for this feature, we'll be bloating the lightbulb menu for all future invocations? I don't think that's an ok user experience.
There was a problem hiding this comment.
I don't understand this pattern. All codepaths end up returning 'solution'. so what actually causes anything to change?
There was a problem hiding this comment.
-
Based on customer interviews Kasey and I conducted, some customers indicated that they wanted to change the severity level of rules depending on what stage of the code development process they were in. So for example, in order for people to pay greater attention to fixing a diagnostic, they may now want a rule that was previously configured as a warning to now be configured as an error.
-
The change is caused by calling solution.WithAdditionalDocument(…) and ultimately returning the solution returned from the method.
There was a problem hiding this comment.
sorry, my point wasn't clear. What i was trying to say was: this makes it look as if you're returning the 'solution' untouched. That's because you actually mutate it in non-local ways because of your use of local functions. That makes the code actually look like it's doing something else.
| { | ||
| doc = curDoc; | ||
| } | ||
| } |
There was a problem hiding this comment.
var doc = project.AdditionalDocuments.FirstOrDefault(d => d.Name...).
| # will not correctly take effect until additional steps are taken to enable it. See the | ||
| # following page for additional information: | ||
| # | ||
| # https://github.com/dotnet/roslyn/blob/master/docs/analyzers/Enable%20Editorconfig%20Configuration.md"; |
| # following page for additional information: | ||
| # | ||
| # https://github.com/dotnet/roslyn/blob/master/docs/analyzers/Enable%20Editorconfig%20Configuration.md"; | ||
| solution = solution.AddAdditionalDocument(id, ".editorconfig", editorconfigDefaultFileContent); |
There was a problem hiding this comment.
oh... i really dislike this. it's not clear at all from reading the code in the main method that you will be overwriting this local in a nested function. Please refactor this code to not do that :)
| solution = solution.AddAdditionalDocument(id, ".editorconfig", editorconfigDefaultFileContent); | ||
| doc = solution.GetAdditionalDocument(id); | ||
| } | ||
| return doc; |
| if (diagnosticToEditorConfigDotNet.ContainsKey(diagnostic.Id)) | ||
| { | ||
| diagnosticToEditorConfigDotNet.TryGetValue(diagnostic.Id, out var value); | ||
| var storageLocation = value.StorageLocations.OfType<EditorConfigStorageLocation<CodeStyleOption<bool>>>().FirstOrDefault(); |
There was a problem hiding this comment.
i've seen this pattern all over hte place. It's not good. we are leaking out inner details of the options.
| { | ||
| ruleName = storageLocation.KeyName; | ||
| } | ||
|
|
There was a problem hiding this comment.
unnecessary blank line.
| if (storageLocation != null) | ||
| { | ||
| ruleName = storageLocation.KeyName; | ||
| } |
There was a problem hiding this comment.
this is the third copy of this code in this method AFAICT.
| var ruleName = CodeStyleOptions.RequireAccessibilityModifiers.StorageLocations.OfType<EditorConfigStorageLocation<CodeStyleOption<AccessibilityModifiersRequired>>>().FirstOrDefault(); | ||
| if (ruleName != null) | ||
| { | ||
| properties["OptionName"] = ruleName.KeyName; |
There was a problem hiding this comment.
all these magic strings need to be declared in a single place. It's far too easy for someone to mispell something.
| } | ||
| } | ||
| return ruleFound; | ||
| } |
There was a problem hiding this comment.
TODO(cyrusn): review this in depth.
| } | ||
| else if (preference == ExpressionBodyPreference.Never) | ||
| { | ||
| option = "false"; |
There was a problem hiding this comment.
more copying of all these details all over hte place. This is really not good. We are effectively blasting out impl detials about specific features all over the IDE codebase in all sorts of disparate areas. I do not like this at all.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
TODO(cyrus): Review this in depth.
|
|
||
| namespace Microsoft.CodeAnalysis.CodeFixes.Suppression | ||
| { | ||
| class ConfigureSeverityLevelCodeActionError : CodeAction |
There was a problem hiding this comment.
why do you need this type, instead of whoever creates this creating a SolutionChangeCodeAction?
|
|
||
| public override string Title | ||
| { | ||
| get { return "Error"; } |
| get { return "Suggestion"; } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
you have four classes that entirely duplicate each other.
- you don't need the classes at all, you could just create a simple subclass of SolutionChangeAction
- the only thing you'd need to do is slightly customize based on what configuration value you're changing to.
This would reduce 95% of this code.
| <ItemGroup Label="Project References"> | ||
| <ProjectReference Include="..\..\..\Compilers\Core\Portable\CodeAnalysis.csproj" /> | ||
| <ProjectReference Include="..\..\..\Workspaces\Core\Portable\Workspaces.csproj" /> | ||
| <ProjectReference Include="..\..\..\Workspaces\CSharp\Portable\CSharpWorkspace.csproj" /> |
There was a problem hiding this comment.
this is a layering violation. this must be removed.
| using Microsoft.CodeAnalysis.Options; | ||
| using Microsoft.CodeAnalysis.LanguageServices; | ||
| using System.Linq; | ||
| using System.Collections.Immutable; |
There was a problem hiding this comment.
System namespace go above Microsoft namespace.
|
The concerns i brought up here still apply: #29345 Namely that we are very significantly adding severe technical debt here with this approach. To put it in perspective: If i were to create a new IDE feature i would have no idea what subset of the IDE i would now have to update for it to properly work with all these new subsystems we're building. It's already hard enough to figure out everythgin that needs updating when you add an option+analyzer+refactoring+fixer. It's now become even harder. You have to know exactly what needs updating in code cleanup. You have to know exactly what needs updating in .editorconfig generation. And now you have to know exactly what needs updating in this suppression area. This is not healthy. It not only deeply isolates IDE features as the only features "blessed" play in these spaces, but it also means people have to deeply understand the interrelationship of all of these subsystems in order to do any work at all. I know it is not palatable to hear, but i strongly do not believe it's acceptable for this to go in in the current form unless it is into a feature branch. I definitely applaud the work here and i think there is huge value in bringing these experiences to users. However, that work must be done in a manner that preserves hte health of hte codebase and does not introduce large amounts of coupling, brittleness, and silo'ed knowledge, like what we see here. |
|
Thank you for the helpful feedback, Cyrus! I realize that this feature has a long way to go, most especially due to architectural limitations and open-ended design questions. The hard-coding and tight coupling is something that will most definitely have to be addressed before the feature is considered for release. This current iteration will only go into the .editorconfig feature branch and a design meeting addressing your concerns will likely occur in the future. I appreciate the feedback! |
That works for me :)
Sounds good. I'd definitely like to know the results of that! Thanks! |
|
|
||
| ```csharp | ||
| <None Include=".editorconfig" /> | ||
| ``` |
There was a problem hiding this comment.
nit: this is not C#.
|
@allisonchou @CyrusNajmabadi - I have a PR out to revive this work with .editorconfig as an |
Added support to configure the severity level of an .editorconfig rule directly through the editor. If a user does not currently have an .editorconfig file at the project level, one will be generated for them.
Example if user already has pre-existing .editorconfig file (marked as Additional File):
Example if user does not have a pre-existing .editorconfig file:
Note: This feature is currently in its very early iterations and is expected to see large changes in the future.
Some things of note:
Due to technical limitations, this feature currently only works if an .editorconfig file is marked as an Additional File at the project-level. If there is an existing .editorconfig file at the project-level but it is not marked as an Additional File, then the file will not be recognized. In future iterations of this feature, we likely want to consider a solution such as automatically marking an .editorconfig file as an Additional File upon the file being added to the project/solution, or allowing an .editorconfig file to recognized as a regular document and getting rid of the Additional Document workaround altogether.
Adding an .editorconfig and marking it as an Additional File is currently not supported by Roslyn (see issue #4655). Therefore, while we can currently add the .editorconfig file for the user, the user must mark it as an Additional File manually.