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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update class attributes to use `nameof` #86

Merged
merged 3 commits into from Dec 4, 2018

Conversation

Projects
None yet
7 participants
@dotMorten
Copy link
Contributor

commented Nov 26, 2018

There are several class attributes that uses string constants to point to members in a class. To ensure there are no typos and properties exist, this PR updates those attributes to instead use nameof to reference those members. This is being addressed for the attributes DefaultEvent, DefaultProperty, ComplexBindingProperties and DefaultBindingProperty,

To ensure all were found, I used a code analyzer (posted below) to find and identify them all by searching for string constants that matches a member name. In addition I used "Find all references" on each attribute type to double-check the changes, and a last double-check by searching project-wide for the use of attributes that starts with a double-quote like for instance DefaultEvent(" .

In this process, I did find two string names that were pointing to a member that doesn't exist on that class,. I left this as a string in this PR, and I will log it as a separate bug:

It sort of also proves why the use of nameof is important 馃槑

For reference, here's the analyzer used to identify string literals that match member names (credit goes to @adityatogani94 for this):

    [DiagnosticAnalyzer(LanguageNames.CSharp)]
    public class NameOfAnalyzer : DiagnosticAnalyzer
    {
        public const string DiagnosticId = "NAMEOF0001";
        private static readonly LocalizableString Title = "Use the 'nameof' operator";
        private static readonly LocalizableString MessageFormat = "Any string literal that shares its name with a member function should be replaced by a nameof operator";
        private static readonly LocalizableString Description = "Replace with nameof operator";
        private const string Category = "Maintainability";

        private static DiagnosticDescriptor Rule = new DiagnosticDescriptor(DiagnosticId, Title, MessageFormat, Category, DiagnosticSeverity.Warning, isEnabledByDefault: true, description: Description);

        public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get { return ImmutableArray.Create(Rule); } }

        public override void Initialize(AnalysisContext context)
        {
            context.RegisterSyntaxNodeAction(AnalyzeSymbol, SyntaxKind.StringLiteralExpression);
        }

        private static void AnalyzeSymbol(SyntaxNodeAnalysisContext context)
        {
            var diagnostic = Diagnostic.Create(Rule, context.Node.GetLocation());
            var literal = (context.Node as LiteralExpressionSyntax).Token.ValueText;
            var container = context.Node.FirstAncestorOrSelf<ClassDeclarationSyntax>();
            
            if (container == null)
            {
                return;
            }

            var containersymbol = context.SemanticModel.GetDeclaredSymbol(container);

            if(!containersymbol.MemberNames.Contains(literal))
            {
                return;
            }

            context.ReportDiagnostic(diagnostic);
        }
    }

Code fixer:

    [ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(NameOfCodeFix)), Shared]
    public class NameOfCodeFix : CodeFixProvider
    {
        private const string title = "Use the 'nameof' operator";

        public sealed override ImmutableArray<string> FixableDiagnosticIds
        {
            get { return ImmutableArray.Create(NameOfAnalyzer.NameOfAnalyzer.DiagnosticId); }
        }

        public sealed override FixAllProvider GetFixAllProvider()
        {
            return WellKnownFixAllProviders.BatchFixer;
        }

        public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context)
        {
            var root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false);
            var diagnostic = context.Diagnostics.First();
            var diagnosticSpan = diagnostic.Location.SourceSpan;
            var literal = root.FindToken(diagnosticSpan.Start).Parent.FirstAncestorOrSelf<LiteralExpressionSyntax>();

            context.RegisterCodeFix(
                    CodeAction.Create("Use NameOf", c => UseNameOfAsync(context.Document, literal, c)),
                    context.Diagnostics);
        }

        private async Task<Document> UseNameOfAsync(Document document, LiteralExpressionSyntax literal, CancellationToken cancellationToken)
        {
            var name = literal.Token.ValueText;
            var nameof = SyntaxFactory.IdentifierName("nameof");
            var newNode = SyntaxFactory.InvocationExpression(nameof).WithArgumentList(SyntaxFactory.ArgumentList(
                             SyntaxFactory.SingletonSeparatedList<ArgumentSyntax>(
                               SyntaxFactory.Argument(
                                    SyntaxFactory.IdentifierName(name)))));
            var root = await document.GetSyntaxRootAsync(cancellationToken);
            var newRoot = root.ReplaceNode(literal, newNode);
            return document.WithSyntaxRoot(newRoot);
        }
    }

@dotMorten dotMorten requested a review from dotnet/dotnet-winforms as a code owner Nov 26, 2018

@dotMorten dotMorten changed the title Update class attributes to yse `nameof` Update class attributes to use `nameof` Nov 26, 2018

@karelz

karelz approved these changes Nov 27, 2018

Copy link
Member

left a comment

Thanks @dotMorten!

Roll back change
This was pointing to a property that doesn't exist and would fix in a separate PR.

@karelz karelz added the * NO-MERGE * label Nov 28, 2018

@AdamYoblick AdamYoblick closed this Dec 4, 2018

@AdamYoblick AdamYoblick reopened this Dec 4, 2018

@richlander richlander removed the * NO-MERGE * label Dec 4, 2018

@shanselman shanselman merged commit 567f837 into dotnet:master Dec 4, 2018

4 checks passed

WIP Ready for review
Details
dotnet-winforms-github CI #20181126.30 succeeded
Details
license/cla All CLA requirements met.
Details
wf CI Build #20181203.45 succeeded
Details

@dotMorten dotMorten deleted the dotMorten:name_of branch Dec 4, 2018

@psmulovics

This comment has been minimized.

Copy link

commented Dec 4, 2018

I actually think the analyzer would be a value too. @dotMorten any plans to make that part of roslyn itself?

@dotMorten

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

@psmulovics I never really worked on the Roslyn parts, so no I didn't have any such plans. Also note the analyzer can have a lot false positives, so it's mostly one you use ad-hoc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.