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.

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

Suppressions for CA1811:AvoidUncalledPrivateCode should be respected by IDE0051 #33314

Closed
davkean opened this issue Feb 12, 2019 · 4 comments
Closed
Assignees
Labels
Area-Analyzers Feature Request Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.
Milestone

Comments

@davkean
Copy link
Member

davkean commented Feb 12, 2019

CPS code base has a bunch of "unused" things that look like this:

        /// <summary>
        /// Wires up this service to event notifications that it needs to respond to.
        /// </summary>
        [ConfiguredProjectAutoLoad]
        [AppliesTo(ProjectCapabilities.Cps + " & " + ProjectCapabilities.ProjectReferences)]
        [SuppressMessage("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode", Justification = "Called by MEF")]
        private void WireUp()
        {
            this.ShellServices.ProjectRenamedInSolution += this.ShellServices_ProjectRenamedInSolution;
            this.ShellServices.ProjectRemovedFromSolution += this.ShellServices_ProjectRemovedFromSolution;
        }

For these we get IDE0051:

Severity	Code	Description	Project	File	Line	Category	Suppression State
Message	IDE0051	Private member 'ProjectReferenceMaintenanceService.WireUp' is unused.	Microsoft.VisualStudio.ProjectSystem.VS.Implementation	E:\CPS\src\Microsoft.VisualStudio.ProjectSystem.VS.Implementation\ProjectReferenceMaintenanceService.cs	106	Code Quality	Active

IDE0051 is exactly the same rule as CA1811, except that it has been removed to the Roslyn tree and renamed. In FxCop days, when we merged rules, we had a mapping table so that existing suppressions would be respected by the new rule. Roslyn should have the same thing.

CPS code base has over 200 instances of this.

@sharwell sharwell added Area-Analyzers Feature Request Need Design Review The end user experience design needs to be reviewed and approved. labels Feb 12, 2019
@mavasani
Copy link
Contributor

Few things to note:

  1. IDE0051 reports all unused private members, I believe CA1811 is only for unused methods.
  2. CA1811 implementation, or in fact any other CAxxxx rule port has strict compat requirements and so needs to follow the old heuristics. IDE0051 does not have such compat requirements.
  3. This is a classical case for Allow an analyzer to suppress diagnostics #20242, which would be solved by exposing a DiagnosticSuppressor or DiagnosticFilter API (Add new analyzer API (DiagnosticSuppressor) to suppress reported comp… #31603) - FxCop analyzers package would basically include a filter that suppresses all IDE0051 instances.

@mavasani
Copy link
Contributor

Tagging @tmat and @jinujoseph

@davkean
Copy link
Member Author

davkean commented Feb 12, 2019

AvoidUncalledPrivateCode would fire on accessors and would respect suppressions against the property, like so:

        /// <summary>
        /// Gets or sets the configured project.
        /// </summary>
        [Import]
        [SuppressMessage("Microsoft.Performance", "CA1811:AvoidUncalledPrivateCode", Justification = "MEF")]
        private ConfiguredProject ConfiguredProject { get; set; }

@jinujoseph jinujoseph added this to the Backlog milestone Feb 12, 2019
@sharwell
Copy link
Member

sharwell commented Mar 8, 2019

One of the primary motivations for changing the diagnostic ID is to decouple ongoing analyzer work from the back-compat expectations of FXCop Analyzers. In addition to the implementation complexity, linking the diagnostic IDs for the purpose of suppressions undermines this goal, so this is a Won't Fix item. We are still interested in resolving bugs related to the analysis and reporting of diagnostics, and those can be filed as separate issues.

Should the suppression analyzer proposal move forward (#20242), it would be possible for an independent extension to perform the requested analysis and suppressions.

@sharwell sharwell closed this as completed Mar 8, 2019
@sharwell sharwell added Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on. and removed Need Design Review The end user experience design needs to be reviewed and approved. labels Mar 8, 2019
@sharwell sharwell moved this to Complete in IDE: Design review Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Analyzers Feature Request Resolution-Won't Fix A real bug, but Triage feels that the issue is not impactful enough to spend time on.
Projects
Archived in project
Development

No branches or pull requests

4 participants