Simplify and fix fading options. #81346
Conversation
| : base(IDEDiagnosticIds.RemoveUnnecessaryDiscardDesignationDiagnosticId, | ||
| EnforceOnBuildValues.RemoveUnnecessaryDiscardDesignation, | ||
| option: null, | ||
| fadingOption: null, |
There was a problem hiding this comment.
i always disliked how these options were being handled. now is a good opportunity to rip it all out since it broke anyways.
| { IDEDiagnosticIds.RemoveUnreachableCodeDiagnosticId, FadingOptions.FadeOutUnreachableCode }, | ||
| { IDEDiagnosticIds.RemoveUnnecessaryImportsDiagnosticId, FadingOptions.FadeOutUnusedImports }, | ||
| { IDEDiagnosticIds.RemoveUnnecessaryImportsGeneratedCodeDiagnosticId, FadingOptions.FadeOutUnusedImports }, | ||
| }.ToImmutableDictionary(); |
There was a problem hiding this comment.
the end result of all that goo was populating a dictionary with these values. So..................................................... just populate the dictionary once, in the place where it is read.
Maybe there's a cleaner way to do this. But i can't think of anything, so here we go.
|
|
||
| // DiagnosticId supports fading, check if the corresponding VS option is turned on. | ||
| if (!SupportsFadingOption(diagnosticData, globalOptionService)) | ||
| if (s_diagnosticToFadingOption.TryGetValue(diagnosticData.Id, out var fadingOption)) |
There was a problem hiding this comment.
Would logging or a Debug.Assert here help us avoid regressions? Perhaps if the Id begins with IDE ?
unfortunately i dont think there is currently an easy way to test this. would need to setup the infrastructure to allow the lsp tests to spawn and use the remote process. |
| public static readonly PerLanguageOption2<bool> FadeOutUnusedMembers = new("dotnet_fade_out_unused_members", defaultValue: true); | ||
| public static readonly PerLanguageOption2<bool> FadeOutUnreachableCode = new("dotnet_fade_out_unreachable_code", defaultValue: true); | ||
|
|
||
| // When adding a new fading option, be sure to update ProtocolConversions.ConvertDiagnostic |
There was a problem hiding this comment.
should we just define the map here instead?
3 participants
Have validated this fixes things.
@dibarbet i looked into a pull diagnostics test that actually used the remote workspace... and i gave up. if you can think of a cheap way to write a test that hits this and actually goes through the remote codepath (so it would fail prior to this fix and work after the fix) lmk.