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.

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

Clean up rulesets #31554

Merged
merged 2 commits into from Dec 5, 2018

Conversation

Projects
None yet
4 participants
@tmat
Member

tmat commented Dec 5, 2018

Remove unused rulesets. We only need Shipping and NonShipping. Apply them correctly based on IsShipping flag. Previously we applied them in BeforeCommonTargets where the flag was not initialized yet unless the project set it.

@tmat tmat requested review from dotnet/roslyn-compiler as code owners Dec 5, 2018

@tmat

This comment has been minimized.

Member

tmat commented Dec 5, 2018

@mavasani PTAL

@tmat

This comment has been minimized.

Member

tmat commented Dec 5, 2018

@dotnet/roslyn-ide Please review

Show resolved Hide resolved build/Targets/Settings.props Outdated
<RuleSet Name="Common diagnostic rules for all analyzer projects" Description="Enables/disable rules specific to all analyzer projects." ToolsVersion="14.0">
<Include Path=".\Roslyn_BuildRules.ruleset" Action="Default" />
<Rules AnalyzerId="Microsoft.CodeAnalysis.Analyzers" RuleNamespace="Microsoft.CodeAnalysis.Analyzers.MetaAnalyzers">

This comment has been minimized.

@mavasani

mavasani Dec 5, 2018

Contributor

We have analyzer project specific rulesets because these MetaAnalyzers are only useful for projects which contain a diagnostic analyzer/code fix. If we are removing these rulesets because we think it is fine to load and execute these analyzers for all projects, we should also remove this configuration that disabled metadata analyzers for non-analyzer projects.

This comment has been minimized.

@tmat

tmat Dec 5, 2018

Member

I'm removing this file because it is not used. No project sets AnalyzerProject msbuild property, which would cause this file to be used. Should it actually be used?

This comment has been minimized.

@tmat

tmat Dec 5, 2018

Member

There is also this comment in the ruleset file:

#26420: Enable rule RS1022 for Roslyn.sln

This comment has been minimized.

@mavasani

mavasani Dec 5, 2018

Contributor

Ah, I remember setting it specifically for all the projects where we have analyzers/fixers (Features and probably Workspaces) when I first added this ruleset. Seems like it got lost during some refactoring, I think we should enable these analyzers - feel free to file a separate tracking issue OR do it as part of this PR by removing this group and see if build does not break.

This comment has been minimized.

@mavasani

mavasani Dec 5, 2018

Contributor

Seems it got broken somewhere after that commit. We still need to retain suppression for RS1022

This comment has been minimized.

@tmat

tmat Dec 5, 2018

Member

Instead of having special ruleset for projects that declare analyzers, isn't it better to have the applicable rules in the common Shipping ruleset, even if they are no-ops for projects that do not declare analyzers? That way we don't need to worry about manually tracking which project has analyzers and which does not.

This comment has been minimized.

@mavasani

mavasani Dec 5, 2018

Contributor

Yes, that seems fine - we should remove the bulk meta-analyzer suppression here and only retain the suppressions that we really want (for example RS1022 that you mentioned above)

This comment has been minimized.

@tmat

This comment has been minimized.

@mavasani

mavasani Dec 10, 2018

Contributor

Thanks!

@tmat tmat force-pushed the tmat:Rulesets branch from f853dcc to d2d1740 Dec 5, 2018

@tmat tmat force-pushed the tmat:Rulesets branch from 58b61b8 to 41a465c Dec 5, 2018

@tmat

This comment has been minimized.

Member

tmat commented Dec 5, 2018

@sharwell @mavasani Fixed a few more code issues I missed before.

@@ -11,7 +11,7 @@ internal abstract class AbstractPackage : AsyncPackage
protected async Task LoadComponentsInUIContextOnceSolutionFullyLoadedAsync(CancellationToken cancellationToken)
{
await KnownUIContexts.SolutionExistsAndFullyLoadedContext;

This comment has been minimized.

@tmat

tmat Dec 5, 2018

Member

Should this also get ConfigureAwait? not sure how UIContext works.

This comment has been minimized.

@tmat

tmat Dec 5, 2018

Member

@jasonmalinowski Any opinion?

@tmat tmat merged commit 6a9a257 into dotnet:master Dec 5, 2018

2 of 3 checks passed

roslyn-integration-CI #20181205.18 failed
Details
license/cla All CLA requirements met.
Details
roslyn-CI #20181205.20 succeeded
Details

@tmat tmat deleted the tmat:Rulesets branch Dec 5, 2018

@jinujoseph jinujoseph added this to the 16.0.P2 milestone Dec 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment