diff --git a/VSDiagnostics/VSDiagnostics/VSDiagnostics.Test/Tests/General/OnPropertyChangedWithoutNameOfOperatorTests.cs b/VSDiagnostics/VSDiagnostics/VSDiagnostics.Test/Tests/General/OnPropertyChangedWithoutNameOfOperatorTests.cs index d28ffc6..b212f74 100644 --- a/VSDiagnostics/VSDiagnostics/VSDiagnostics.Test/Tests/General/OnPropertyChangedWithoutNameOfOperatorTests.cs +++ b/VSDiagnostics/VSDiagnostics/VSDiagnostics.Test/Tests/General/OnPropertyChangedWithoutNameOfOperatorTests.cs @@ -404,5 +404,278 @@ protected void OnPropertyChanged(string propertyName, bool someBoolean) VerifyDiagnostic(original, string.Format(OnPropertyChangedWithoutNameOfOperatorAnalyzer.Rule.MessageFormat.ToString(), "IsEnabled")); VerifyFix(original, expected); } + + [TestMethod] + public void OnPropertyChangedWithoutNameOfOperator_WithPartialClass() + { + var original = @" +using System; +using System.Text; + +namespace ConsoleApplication1 +{ + partial class MyClass : INotifyPropertyChanged + { + private bool _isEnabled; + public bool IsEnabled + { + get { return _isEnabled; } + set + { + _isEnabled = value; + OnPropertyChanged(""IsEnabled""); + } + } + } + + partial class MyClass + { + public event PropertyChangedEventHandler PropertyChanged; + + protected void OnPropertyChanged(string propertyName) + { + PropertyChangedEventHandler handler = PropertyChanged; + + if (handler != null) + { + handler(this, new PropertyChangedEventArgs(propertyName)); + } + } + } +}"; + + var expected = @" +using System; +using System.Text; + +namespace ConsoleApplication1 +{ + partial class MyClass : INotifyPropertyChanged + { + private bool _isEnabled; + public bool IsEnabled + { + get { return _isEnabled; } + set + { + _isEnabled = value; + OnPropertyChanged(nameof(IsEnabled)); + } + } + } + + partial class MyClass + { + public event PropertyChangedEventHandler PropertyChanged; + + protected void OnPropertyChanged(string propertyName) + { + PropertyChangedEventHandler handler = PropertyChanged; + + if (handler != null) + { + handler(this, new PropertyChangedEventArgs(propertyName)); + } + } + } +}"; + + VerifyDiagnostic(original, string.Format(OnPropertyChangedWithoutNameOfOperatorAnalyzer.Rule.MessageFormat.ToString(), "IsEnabled")); + VerifyFix(original, expected); + } + + [TestMethod] + public void OnPropertyChangedWithoutNameOfOperator_ParenthesizedExpression() + { + var original = @" +using System; +using System.Text; + +namespace ConsoleApplication1 +{ + class MyClass : INotifyPropertyChanged + { + private bool _isEnabled; + public bool IsEnabled + { + get { return _isEnabled; } + set + { + _isEnabled = value; + OnPropertyChanged((""IsEnabled"")); + } + } + + public event PropertyChangedEventHandler PropertyChanged; + + protected void OnPropertyChanged(string propertyName) + { + PropertyChangedEventHandler handler = PropertyChanged; + + if(handler != null) + { + handler(this, new PropertyChangedEventArgs(propertyName)); + } + } + } +}"; + + var expected = @" +using System; +using System.Text; + +namespace ConsoleApplication1 +{ + class MyClass : INotifyPropertyChanged + { + private bool _isEnabled; + public bool IsEnabled + { + get { return _isEnabled; } + set + { + _isEnabled = value; + OnPropertyChanged((nameof(IsEnabled))); + } + } + + public event PropertyChangedEventHandler PropertyChanged; + + protected void OnPropertyChanged(string propertyName) + { + PropertyChangedEventHandler handler = PropertyChanged; + + if(handler != null) + { + handler(this, new PropertyChangedEventArgs(propertyName)); + } + } + } +}"; + + VerifyDiagnostic(original, string.Format(OnPropertyChangedWithoutNameOfOperatorAnalyzer.Rule.MessageFormat.ToString(), "IsEnabled")); + VerifyFix(original, expected); + } + + [TestMethod] + public void OnPropertyChangedWithoutNameOfOperator_WithPartialClass_AndDifferentProperty() + { + var original = @" +using System; +using System.Text; + +namespace ConsoleApplication1 +{ + partial class MyClass : INotifyPropertyChanged + { + private bool _isEnabled; + public bool IsEnabled + { + get { return _isEnabled; } + set + { + _isEnabled = value; + OnPropertyChanged(""OtherBoolean""); + } + } + } + + partial class MyClass + { + public event PropertyChangedEventHandler PropertyChanged; + + public bool OtherBoolean { get; set; } + + protected void OnPropertyChanged(string propertyName) + { + PropertyChangedEventHandler handler = PropertyChanged; + + if (handler != null) + { + handler(this, new PropertyChangedEventArgs(propertyName)); + } + } + } +}"; + + var expected = @" +using System; +using System.Text; + +namespace ConsoleApplication1 +{ + partial class MyClass : INotifyPropertyChanged + { + private bool _isEnabled; + public bool IsEnabled + { + get { return _isEnabled; } + set + { + _isEnabled = value; + OnPropertyChanged(nameof(OtherBoolean)); + } + } + } + + partial class MyClass + { + public event PropertyChangedEventHandler PropertyChanged; + + public bool OtherBoolean { get; set; } + + protected void OnPropertyChanged(string propertyName) + { + PropertyChangedEventHandler handler = PropertyChanged; + + if (handler != null) + { + handler(this, new PropertyChangedEventArgs(propertyName)); + } + } + } +}"; + + VerifyDiagnostic(original, string.Format(OnPropertyChangedWithoutNameOfOperatorAnalyzer.Rule.MessageFormat.ToString(), "OtherBoolean")); + VerifyFix(original, expected); + } + + [TestMethod] + public void OnPropertyChangedWithoutNameOfOperator_ParenthesizedExpression_WithNameof() + { + var original = @" +using System; +using System.Text; + +namespace ConsoleApplication1 +{ + class MyClass : INotifyPropertyChanged + { + private bool _isEnabled; + public bool IsEnabled + { + get { return _isEnabled; } + set + { + _isEnabled = value; + OnPropertyChanged(((nameof(IsEnabled)))); + } + } + + public event PropertyChangedEventHandler PropertyChanged; + + protected void OnPropertyChanged(string propertyName) + { + PropertyChangedEventHandler handler = PropertyChanged; + + if(handler != null) + { + handler(this, new PropertyChangedEventArgs(propertyName)); + } + } + } +}"; + VerifyDiagnostic(original); + } } } \ No newline at end of file diff --git a/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/OnPropertyChangedWithoutNameOfOperator/OnPropertyChangedWithoutNameOfOperatorAnalyzer.cs b/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/OnPropertyChangedWithoutNameOfOperator/OnPropertyChangedWithoutNameOfOperatorAnalyzer.cs index d4ac4d3..bc1cc09 100644 --- a/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/OnPropertyChangedWithoutNameOfOperator/OnPropertyChangedWithoutNameOfOperatorAnalyzer.cs +++ b/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/OnPropertyChangedWithoutNameOfOperator/OnPropertyChangedWithoutNameOfOperatorAnalyzer.cs @@ -1,5 +1,7 @@ using System; +using System.Collections.Generic; using System.Collections.Immutable; +using System.Globalization; using System.Linq; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CSharp; @@ -23,7 +25,9 @@ public class OnPropertyChangedWithoutNameOfOperatorAnalyzer : DiagnosticAnalyzer VSDiagnosticsResources.OnPropertyChangedWithoutNameOfOperatorAnalyzerTitle; internal static DiagnosticDescriptor Rule - => new DiagnosticDescriptor(DiagnosticId.OnPropertyChangedWithoutNameofOperator, Title, Message, Category, Severity, true); + => + new DiagnosticDescriptor(DiagnosticId.OnPropertyChangedWithoutNameofOperator, Title, Message, Category, + Severity, true); public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); @@ -54,26 +58,48 @@ private void AnalyzeSymbol(SyntaxNodeAnalysisContext context) } var invokedProperty = invocation.ArgumentList.Arguments.FirstOrDefault(); - var argumentLiteralExpression = invokedProperty.Expression as LiteralExpressionSyntax; - if (argumentLiteralExpression == null) + if (invokedProperty == null) { return; } - var invocationArgument = argumentLiteralExpression.Token.ValueText; + // We use the descendent nodes in case it's wrapped in another level. For example: OnPropertyChanged(((nameof(MyProperty)))) + if (invokedProperty.DescendantNodesAndSelf().OfType().Any(x => x.IsNameofInvocation())) + { + return; + } + + var invocationArgument = context.SemanticModel.GetConstantValue(invokedProperty.Expression); + if (!invocationArgument.HasValue) + { + return; + } + + // Get all the properties defined in this type + // We can't just get all the descendents of the classdeclaration because that would pass by some of a partial class' properties + var classDeclaration = invocation.Ancestors().OfType().FirstOrDefault(); + if (classDeclaration == null) + { + return; + } + + var classSymbol = context.SemanticModel.GetDeclaredSymbol(classDeclaration); + if (classSymbol == null) + { + return; + } - var properties = - invocation.Ancestors() - .OfType() - .FirstOrDefault() - .ChildNodes() - .OfType(); - foreach (var property in properties) + foreach (var property in classSymbol.GetMembers().OfType()) { - if (string.Equals(property.Identifier.ValueText, invocationArgument, StringComparison.OrdinalIgnoreCase)) + if (string.Equals(property.Name, (string) invocationArgument.Value, StringComparison.OrdinalIgnoreCase)) { - context.ReportDiagnostic(Diagnostic.Create(Rule, invokedProperty.GetLocation(), - property.Identifier.ValueText)); + var location = invokedProperty.Expression.DescendantNodesAndSelf().Last().GetLocation(); + var data = ImmutableDictionary.CreateRange(new[] + { + new KeyValuePair("parameterName", property.Name), + new KeyValuePair("startLocation", location.SourceSpan.Start.ToString(CultureInfo.InvariantCulture)), + }); + context.ReportDiagnostic(Diagnostic.Create(Rule, location, data, property.Name)); } } } diff --git a/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/OnPropertyChangedWithoutNameOfOperator/OnPropertyChangedWithoutNameOfOperatorCodeFix.cs b/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/OnPropertyChangedWithoutNameOfOperator/OnPropertyChangedWithoutNameOfOperatorCodeFix.cs index 0b5b37e..e330543 100644 --- a/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/OnPropertyChangedWithoutNameOfOperator/OnPropertyChangedWithoutNameOfOperatorCodeFix.cs +++ b/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/OnPropertyChangedWithoutNameOfOperator/OnPropertyChangedWithoutNameOfOperatorCodeFix.cs @@ -1,5 +1,4 @@ -using System; -using System.Collections.Immutable; +using System.Collections.Immutable; using System.Composition; using System.Linq; using System.Threading.Tasks; @@ -7,7 +6,6 @@ using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.CodeFixes; using Microsoft.CodeAnalysis.CSharp; -using Microsoft.CodeAnalysis.CSharp.Syntax; namespace VSDiagnostics.Diagnostics.General.OnPropertyChangedWithoutNameOfOperator { @@ -23,38 +21,27 @@ public 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 argumentDeclaration = - root.FindNode(diagnosticSpan).AncestorsAndSelf().OfType().FirstOrDefault(); context.RegisterCodeFix( - CodeAction.Create(VSDiagnosticsResources.OnPropertyChangedWithoutNameOfOperatorCodeFixTitle, - x => UseNameOfAsync(context.Document, root, argumentDeclaration), + CodeAction.Create(VSDiagnosticsResources.OnPropertyChangedWithoutNameOfOperatorCodeFixTitle, + x => UseNameOfAsync(context.Document, root, diagnostic), OnPropertyChangedWithoutNameOfOperatorAnalyzer.Rule.Id), diagnostic); } - private Task UseNameOfAsync(Document document, SyntaxNode root, ArgumentSyntax argumentDeclaration) + private Task UseNameOfAsync(Document document, SyntaxNode root, Diagnostic diagnostic) { - var properties = - argumentDeclaration.Ancestors() - .OfType() - .First() - .ChildNodes() - .OfType(); - foreach (var property in properties) - { - if (string.Equals(property.Identifier.ValueText, - ((LiteralExpressionSyntax) argumentDeclaration.Expression).Token.ValueText, - StringComparison.OrdinalIgnoreCase)) - { - root = root.ReplaceNode(argumentDeclaration.Expression, - SyntaxFactory.ParseExpression($"nameof({property.Identifier.ValueText})")); - var newDocument = document.WithSyntaxRoot(root); - return Task.FromResult(newDocument.Project.Solution); - } - } + var propertyName = diagnostic.Properties["parameterName"]; + var startLocation = int.Parse(diagnostic.Properties["startLocation"]); - return null; + // We have to use LastOrDefault because encompassing nodes will have the same start location + // For example in our scenario of OnPropertyChanged("test"), the ArgumentSyntaxNode will have + // the same start location as the following LiteralExpressionNode + // We are interested in the inner-most node therefore we need to take the last one with that start location + var nodeToReplace = root.DescendantNodesAndSelf().LastOrDefault(x => x.SpanStart == startLocation); + + var newRoot = root.ReplaceNode(nodeToReplace, SyntaxFactory.ParseExpression($"nameof({propertyName})")); + var newDocument = document.WithSyntaxRoot(newRoot); + return Task.FromResult(newDocument.Project.Solution); } } } \ No newline at end of file diff --git a/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/UseAliasesInsteadOfConcreteType/UseAliasesInsteadOfConcreteTypeAnalyzer.cs b/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/UseAliasesInsteadOfConcreteType/UseAliasesInsteadOfConcreteTypeAnalyzer.cs index a4a2989..e08b4e1 100644 --- a/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/UseAliasesInsteadOfConcreteType/UseAliasesInsteadOfConcreteTypeAnalyzer.cs +++ b/VSDiagnostics/VSDiagnostics/VSDiagnostics/Diagnostics/General/UseAliasesInsteadOfConcreteType/UseAliasesInsteadOfConcreteTypeAnalyzer.cs @@ -37,11 +37,7 @@ private void AnalyzeSymbol(SyntaxNodeAnalysisContext context) // A nameof() expression cannot contain aliases // There is no way to distinguish between a self-defined method 'nameof' and the nameof operator so we have to ignore all invocations that call into 'nameof' var surroundingInvocation = identifier.Ancestors().OfType().FirstOrDefault(); - var invocationIdentifier = surroundingInvocation?.Expression.DescendantNodesAndSelf() - .OfType() - .FirstOrDefault(); - - if (invocationIdentifier != null && invocationIdentifier.Identifier.ValueText == "nameof") + if (surroundingInvocation != null && surroundingInvocation.IsNameofInvocation()) { return; } diff --git a/VSDiagnostics/VSDiagnostics/VSDiagnostics/Utilities/Extensions.cs b/VSDiagnostics/VSDiagnostics/VSDiagnostics/Utilities/Extensions.cs index 3843138..0c31237 100644 --- a/VSDiagnostics/VSDiagnostics/VSDiagnostics/Utilities/Extensions.cs +++ b/VSDiagnostics/VSDiagnostics/VSDiagnostics/Utilities/Extensions.cs @@ -201,5 +201,20 @@ public static T ElementAtOrDefault(this IEnumerable list, int index, T @de { return index >= 0 && index < list.Count() ? list.ElementAt(index) : @default; } + + // TODO: tests + public static bool IsNameofInvocation(this InvocationExpressionSyntax invocation) + { + if (invocation == null) + { + throw new ArgumentNullException(nameof(invocation)); + } + + var identifier = invocation.Expression.DescendantNodesAndSelf() + .OfType() + .FirstOrDefault(); + + return identifier != null && identifier.Identifier.ValueText == "nameof"; + } } } \ No newline at end of file