Analyzer reports unused constructor for MEF components.

vsfeedback commented 3 years ago

This issue has been moved from a ticket on Developer Community.

Create a class with an Export attribute and a private constructor with an ImportingConstructor attribute on it. This denotes a MEF component that will invoke the private ctor instead of the default (parameterless) one. However, the VS code analyzer still "dims" the ctor and reports it as unused.

    [Export(ExportContractNames.ProjectTreeProviders.PhysicalViewRootGraft, typeof(IProjectTreeProvider))]
    internal class GlobalDesignerProjectTreeProvider : ProjectTreeProviderBase
    {
        [ImportingConstructor]
        private GlobalDesignerProjectTreeProvider(
            IProjectThreadingService threadingService,
            UnconfiguredProject project)
            : base(threadingService, project, configuredProjectExportsRequired: true)
        {
        }

Original Comments

Mila Zhou [MSFT] on 8/13/2020, 11:06 PM:

Could you please tell me what type of project you are? Web or others? Some screenshots will be better.

Feedback Bot on 8/17/2020, 02:55 AM:

We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.

Feedback Bot on 8/17/2020, 09:33 AM:

Thank you for sharing your feedback! Our teams prioritize action on product issues with broad customer impact. See details at: https://docs.microsoft.com/en-us/visualstudio/ide/report-a-problem?view=vs-2019#faq. In case you need answers to common questions or need assisted support, be sure to use https://visualstudio.microsoft.com/vs/support/. We’ll keep you posted on any updates to this feedback.

Original Solutions

kirk solved on 8/14/2020, 06:48 AM, 0 votes:

It probably happens in any C# scenario, but the one above is a C# class library project targeting .NET 4.7.

sharwell commented 3 years ago

The solution used by dotnet/roslyn for this is to use the following together:

The [ImportingConstructor] constructor is declared public
The constructor also has an [Obsolete] attribute applied to it to prevent direct use

I believe the ideal strategy is to replace (2) with one or more analyzers dedicated to enforcing construction rules for MEF parts without the need to use specially-crafted [Obsolete] attributes.

mavasani commented 3 years ago

@sharwell The feedback has no mention of which diagnostic ID this is for? There is no CA rule to report unused private members, and IDE0051/IDE0052 in Roslyn already handles the MEF case.

sharwell commented 3 years ago

Are we sure the MEF case is handled? I would have expected ImportingConstructorAttribute to be included here: https://github.com/dotnet/roslyn/blob/e69b451b362312c18602beadd5c8b9e93833140d/src/Analyzers/Core/Analyzers/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs#L92

mavasani commented 3 years ago

Ah probably I misremembered then... thanks

sharwell commented 3 years ago

I lean towards marking this By Design. The diagnostic will not be reported if the constructor is made public.

kfertitta commented 3 years ago

Hi @sharwell ,

Thanks for your comments on this. I was the original one reporting the issue via the VS Feedback link. While the public ctor would obviously workaround the issue, it's less than ideal for many of our scenarios. We use obfuscation in a variety of places and a public ctor precludes that. Is there really no other way to address this?

sharwell commented 3 years ago

@kfertitta For uncommon scenarios where patterns adopted by a specific project make it impossible to avoid a warning through the expected approach, it's fairly straightforward to implement a DiagnosticSuppressor and include it in the project build. Here's a few examples of cases where I added targeted suppressions for rules:

https://github.com/dotnet/machinelearning/pull/4803 https://github.com/dotnet/roslyn-analyzers/pull/3391 https://github.com/dotnet/roslyn-analyzers/pull/3774

kfertitta commented 3 years ago

@sharwell Thanks much for the quick reply. I'll definitely take a look at those. Just some follow-up to make sure I'm understanding the philosophy here. Is there something different about how we're using MEF and importing ctors than mainstream usage? If a class should only be activated via MEF, then why wouldn't you always "want" your importing ctors to be private (or perhaps protected)? I feel like I may have missed something obvious, so I'll "pre-apologize". We do tons of VS extensibility, so we're very heavy MEF users.

mavasani commented 3 years ago

Note that even marking these internal will avoid the issue.

jmarolf commented 3 years ago

I still feel like this is not very helpful to the user. If MEF is involved saying anything about a constructor or method being unused is speculative at best.

mavasani commented 3 years ago

Yes, I would be fine with modifying the analyzer here to ignore MEF import/export member symbols from analysis.

kfertitta commented 3 years ago

Yes, @jmarolf captures my sentiment best as well.

sharwell commented 3 years ago

@jmarolf there are a wide variety of reflection-based cases where that sentiment holds. Given the accessibility change has proven reliable as a design improvement to avoid this warning, I don't see a lot of value in implementing this special case knowing there is no way we can cover so many others.

dotnet / roslyn