Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Deprecate CompilationWithAnalyzers.IsDiagnosticAnalyzerSuppressed public API #67671

Merged
merged 10 commits into from
May 15, 2023
Merged

Deprecate CompilationWithAnalyzers.IsDiagnosticAnalyzerSuppressed public API #67671

merged 10 commits into from
May 15, 2023

Conversation

mavasani
Copy link
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

@dotnet-issue-labeler dotnet-issue-labeler bot 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
Contributor Author

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
Contributor Author

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
Contributor Author

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
Contributor Author

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
Member

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
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
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
Contributor Author

mavasani commented May 1, 2023

@dotnet/roslyn-compiler for reviews

@mavasani
Copy link
Contributor Author

mavasani commented May 3, 2023

@dotnet/roslyn-compiler for an additional review

@mavasani
Copy link
Contributor Author

mavasani commented May 9, 2023

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

@mavasani
Copy link
Contributor Author

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

@sharwell sharwell sharwell left review comments

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@jjonescz jjonescz jjonescz approved these changes

@cston cston cston approved these changes

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