Skip to content

Deprecate CompilationWithAnalyzers.IsDiagnosticAnalyzerSuppressed public API#67671

Merged
mavasani merged 10 commits intodotnet:mainfrom
mavasani:Issue67592
May 15, 2023
Merged

Deprecate CompilationWithAnalyzers.IsDiagnosticAnalyzerSuppressed public API#67671
mavasani merged 10 commits intodotnet:mainfrom
mavasani:Issue67592

Conversation

@mavasani
Copy link
Copy Markdown
Contributor

@mavasani mavasani commented Apr 6, 2023 •
edited
Loading

Fixes #67592
Details in above issue description

NOTE: Pending API review for #67592 API deprecation has been approved

@ghost ghost added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 6, 2023
@mavasani mavasani mentioned this pull request Apr 6, 2023
Closed
mavasani
mavasani commented Apr 14, 2023
View reviewed changes
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.cs
return IsDiagnosticAnalyzerSuppressed(analyzer, options, isCompilerAnalyzer, severityFilter,
isEnabledWithAnalyzerConfigOptions, getSupportedDiagnosticDescriptors, getSupportedSuppressionDescriptors, cancellationToken);

bool isEnabledWithAnalyzerConfigOptions(DiagnosticDescriptor descriptor)
Copy link
Copy Markdown
Contributor Author

@mavasani mavasani Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing local function moved from below

mavasani
mavasani commented Apr 14, 2023
View reviewed changes
src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs
}
};

return AnalyzerManager.IsDiagnosticAnalyzerSuppressed(analyzer, options, IsCompilerAnalyzer, severityFilter: SeverityFilter.None,
Copy link
Copy Markdown
Contributor Author

@mavasani mavasani Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Retained the existing functionality of this deprecated public API as per the public API review feedback: #67592 (comment)

Copy link
Copy Markdown
Contributor Author

@mavasani mavasani May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharwell - My bad IsCompilerAnalyzer is being accessed here, so needs to be made internal.

mavasani
mavasani commented Apr 14, 2023
View reviewed changes
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs Outdated
}

internal bool ExceptionFilter(Exception ex, CancellationToken cancellationToken)
internal static void HandleAnalyzerException(
Copy link
Copy Markdown
Contributor Author

@mavasani mavasani Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Contains all existing code now refactored to an internal static API so it can be invoked from the deprecated public API

@mavasani mavasani marked this pull request as ready for review April 18, 2023 05:25
@mavasani mavasani requested a review from a team as a code owner April 18, 2023 05:25
@mavasani mavasani mentioned this pull request Apr 26, 2023
Closed
@arunchndr
Copy link
Copy Markdown

arunchndr commented Apr 26, 2023 •
edited
Loading

The change looks good to me. As a sidenote, would it feasible to collate the diagnostics work to a known parent or tag with a label to make it easier for tracking?

@mavasani
Copy link
Copy Markdown
Contributor Author

mavasani commented May 1, 2023

As a sidenote, would it feasible to collate the diagnostics work to a known parent or tag with a label to make it easier for tracking?

Yes, let me create a label and mark all the relevant PRs with it.

@mavasani mavasani added the Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. label May 1, 2023
@mavasani
Copy link
Copy Markdown
Contributor Author

mavasani commented May 1, 2023

@arkalyanms - Tagged PRs with label: Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger.

@mavasani
Copy link
Copy Markdown
Contributor Author

mavasani commented May 1, 2023

@dotnet/roslyn-compiler for reviews

@mavasani
Copy link
Copy Markdown
Contributor Author

mavasani commented May 3, 2023

@dotnet/roslyn-compiler for an additional review

@mavasani
Copy link
Copy Markdown
Contributor Author

mavasani commented May 9, 2023

@dotnet/roslyn-compiler can I get another review? Thanks!

@mavasani
Copy link
Copy Markdown
Contributor Author

mavasani commented May 15, 2023

Thank you @jjonescz

@mavasani mavasani merged commit c253f13 into dotnet:main May 15, 2023
@ghost ghost added this to the Next milestone May 15, 2023
@mavasani mavasani deleted the Issue67592 branch May 15, 2023 15:11
@sharwell sharwell mentioned this pull request May 15, 2023
Merged
@allisonchou allisonchou mentioned this pull request May 16, 2023
Closed
@Cosifne Cosifne modified the milestones: Next, 17.7 P2 May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jjonescz jjonescz jjonescz approved these changes

+3 more reviewers

@sharwell sharwell sharwell left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@cston cston cston approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-Analyzers Performance-Scenario-Diagnostics This issue affects diagnostics computation performance for lightbulb, background analysis, tagger. untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Milestone

17.7 P2

Development

Successfully merging this pull request may close these issues.

Deprecate CompilationWithAnalyzers.IsDiagnosticAnalyzerSuppressed public API

7 participants

@mavasani @arunchndr @sharwell @jjonescz @CyrusNajmabadi @cston @Cosifne