Fody/PropertyChanged

VS 2019 Private member On_PropertyName_Changed is unused

virzak opened this issue · 10 comments

virzak commented

This isn't a bug, but an annoying warning.

image

Happens since On_PropertyName_Changed is not being referenced at development time. Or is this a question to the roslyn team? here is a similar issue dotnet/roslyn/issues/31581

ltrzesniewski commented

That looks like a ReSharper warning to me, not a Roslyn one (or maybe they show it exactly the same way?)

Anyway, I don't really see what PropertyChanged.Fody could do to prevent this. If that's a ReSharper warning, just add the [UsedImplicitly] attribute if you have JetBrains.Annotations, or [SuppressMessage("ReSharper", "UnusedMember.Local")] if you don't.

SimonCropp commented

Make the member public

virzak commented

@SimonCropp aren't there situations where it is best to keep On_PropertyName_Changed private?

Looks like this issue is happening in plenty other projects and the warning could be disabled programmatically soon dotnet/roslyn/issues/30172

SimonCropp commented

aren't there situations where it is best to keep On_PropertyName_Changed private

not that i know of. this is not a case where u are exposing an API to an external consumer. So encapsulation doesnt really matter. you are exposing an instance to your view to bind to, if u have code that incorrectly uses a viewmodel in other ways, u have bigger problems than a public method.

Also making it public means u can test it and verify the outcome

virzak commented

I read it when I reported previous issue #247. I see what you're saying.

SimonCropp commented

@virzak i really dont see any way this can be fixed at the fody level. since u have a few workarounds, shall we close this one?

virzak commented

Definitely

virzak commented

It is possible to suppress this warning programmartically using an analyzer.

Not sure if anyone else likes keeping these non-public.
The code is pretty simple.

It would be nice to get this analyzer along with PropertyChanged. If you're interested I can submit a PR.

using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using System;
using System.Collections.Immutable;
using System.Linq;
using System.Text.RegularExpressions;

namespace DiagnosticSuppressorTest
{
    [DiagnosticAnalyzer(LanguageNames.CSharp)]
    public class FodyPropertyChangedSuppressor : DiagnosticSuppressor
    {
        public override ImmutableArray<SuppressionDescriptor> SupportedSuppressions { get; } = new[]
        {
            new SuppressionDescriptor("SUPP2953", "IDE0051", "OnChanged suppression")
        }.ToImmutableArray();

        readonly Regex OnChangedPattern = new Regex("^On(.+)Changed$", RegexOptions.Compiled);

        public override void ReportSuppressions(SuppressionAnalysisContext context)
        {
            foreach (var diagnostic in context.ReportedDiagnostics)
            {
                var methodNode = diagnostic.Location.SourceTree.GetRoot(context.CancellationToken).FindNode(diagnostic.Location.SourceSpan);
                if (methodNode == null || !(methodNode is MethodDeclarationSyntax methodDec))
                    continue;

                // Determine if the method matches On_PropertyName_Changed pattern
                var matches = OnChangedPattern.Matches(methodDec.Identifier.Text);
                if (matches.Count == 0 || matches.Count > 1)
                    continue;

                var expectedPropertyName = matches[0].Groups[1].Value;

                var classNode = methodNode.Parent;

                var classModel = context.GetSemanticModel(classNode.SyntaxTree);
                var classDeclaredSymbol = classModel.GetDeclaredSymbol(classNode, context.CancellationToken);

                if (!(classDeclaredSymbol is INamedTypeSymbol classSymbol))
                    continue;

                // Check if parent class implements INotifyPropertyChanged
                bool isINotifyPropertyChanged = false;
                foreach (var inheditedInterface in classSymbol.Interfaces)
                {
                    if (inheditedInterface.Name.Equals("INotifyPropertyChanged", StringComparison.InvariantCulture))
                    {
                        isINotifyPropertyChanged = true;
                        break;
                    }
                }

                // Check if parent class has AddINotifyPropertyChangedInterfaceAttribute
                bool hasAddINotifyPropertyChangedInterface = classSymbol.GetAttributes()
                    .Any(a => a.AttributeClass.Name.Equals("AddINotifyPropertyChangedInterfaceAttribute", StringComparison.InvariantCulture));

                if (!hasAddINotifyPropertyChangedInterface && !isINotifyPropertyChanged)
                    continue;

                var relatedPropertyFound = false;

                // Iterate through all locations (possible partial classes)
                foreach (var loc in classSymbol.Locations)
                {
                    var locnode = loc.SourceTree.GetRoot(context.CancellationToken).FindNode(loc.SourceSpan);
                    
                    SyntaxList<MemberDeclarationSyntax> members;
                    if (locnode is ClassDeclarationSyntax declaringClass)
                    {
                        members = declaringClass.Members;
                    }
                    else if (locnode is StructDeclarationSyntax declaringStruct)
                    {
                        members = declaringStruct.Members;
                    }
                    else
                        continue;

                    // Iterate through member of (partial) class
                    foreach (var member in members)
                    {
                        // Bail out if not a property
                        if (!(member is PropertyDeclarationSyntax property))
                            continue;

                        // Check to see if property name is what's between On and Changed
                        if (property.Identifier.Text.Equals(expectedPropertyName, StringComparison.InvariantCulture))
                        {
                            relatedPropertyFound = true;
                        }
                    }

                    if (relatedPropertyFound)
                        context.ReportSuppression(Suppression.Create(SupportedSuppressions[0], diagnostic));
                }
            }
        }
    }
}
virzak commented

Now available as a NuGet package

  • 🎉1