Remove CompilationPair abstraction in diagnostic analyzer subsystem. #80086
Conversation
| namespace A | ||
| { | ||
| using System; |
There was a problem hiding this comment.
this is the correct behavior. The test here is testing " 'usings' should be INSIDE the namespace, and it is an ERROR otherwise". So it was a bug this wasn't moving in in CodeCleanup.
There was a problem hiding this comment.
Note: this differs from teh tests taht validate that nothing change when the option suggestions the change, but the severity is set to 'hidden'. in that case we do not make any change.
CyrusNajmabadi
changed the title
CyrusNajmabadi
changed the title
|
Will have to see if the validation is accurate. But this looks promising: 
|
| @@ -18,7 +18,7 @@ namespace Microsoft.CodeAnalysis.Diagnostics; | |||
| internal sealed partial class DiagnosticAnalyzerService | |||
| { | |||
| /// <summary> | |||
| /// Cached data from a <see cref="ProjectState"/> to the <see cref="CompilationWithAnalyzersPair"/>s | |||
| /// Cached data from a <see cref="ProjectState"/> to the <see cref="CompilationWithAnalyzers"/>s | |||
There was a problem hiding this comment.
Can do in followup.
| CreateCompilationWithAnalyzers(compilation, filteredHostAnalyzers, project.HostAnalyzerOptions, exceptionFilter)); | ||
| return CreateCompilationWithAnalyzers( | ||
| compilation, | ||
| filteredHostAnalyzers.Concat(filteredProjectAnalyzers).Distinct(), |
There was a problem hiding this comment.
filteredHostAnalyzers.Concat(filteredProjectAnalyzers).Distinct(),
Trying to work my way through the WhereAsArray mental gymnastics above, and I'm probably wrong, but it seems like all that reduces to this parameter just being
analyzers.Where(a => !a.IsWorkspaceDiagnosticAnalyzer())
and not needing any of those intermediate calculations
There was a problem hiding this comment.
yup. that seems to be true. Are you ok with that happening in a followup. My plan was to go over all this code and reduce complexity now that we only have one list of analyzers. Thanks! :)
There was a problem hiding this comment.
Definitely ok with any change related to this to be in a followup PR
| projectBuilder.Add(analyzer); | ||
| } | ||
| } | ||
| var builder = ArrayBuilder<DiagnosticAnalyzer>.GetInstance(); |
There was a problem hiding this comment.
Oh, I see, maybe that's a bad idea if the order matters
There was a problem hiding this comment.
Yeah. That part isn't clear to me. So i don't want to change that right now.
| hostAnalyzerBuilder.RemoveRange(projectSuppressors); | ||
| hostAnalyzerBuilder.AddRange(projectSuppressors); | ||
| // We do not want to run SDK 'features' analyzers. We always defer to what is in VS for that. | ||
| if (ShouldRedirectAnalyzers(_project, reference)) |
There was a problem hiding this comment.
sure. can def do in followup.
|
Discussing with Jason if this is the way we want to go. |
|
@CyrusNajmabadi I presume at this point we're taking the other one? |
Will likely take #80141 over this.
This was needed back when we allowed loading Roslyn Feature Analyzers from the SDK. IN that case, we'd have the analyzer from VS and the analyzer from the SDK. Now we've moved to a model where we will always take the VS analyzer to prevent tearing.
Note: option values still always prefer editorconfig, but fallback to VS options if they are not present.
Build: https://devdiv.visualstudio.com/DevDiv/_build?definitionId=8972
PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/666447