From 393ab415cb69908b0914e721ddc897ef8205cda7 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 20 Apr 2018 17:14:20 -0700 Subject: [PATCH 1/4] Fix issue where a write wasn't detected in make-readonly. --- .../MakeFieldReadonlyTests.cs | 47 ++++++++ .../MakeFieldReadonlyTests.vb | 24 ++++ .../Extensions/ExpressionSyntaxExtensions.cs | 112 +++++++++++++----- 3 files changed, 152 insertions(+), 31 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs b/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs index f7a871c01ea77..2b13b677bdeed 100644 --- a/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs +++ b/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs @@ -6,6 +6,7 @@ using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.Diagnostics; using Microsoft.CodeAnalysis.Test.Utilities; +using Roslyn.Test.Utilities; using Xunit; namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.MakeFieldReadonly @@ -779,5 +780,51 @@ partial struct MyClass { }", partial struct MyClass { }"); } + + [WorkItem(26262, "https://github.com/dotnet/roslyn/issues/26262")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)] + public async Task FieldAssignedInCtor_InParens() + { + await TestInRegularAndScriptAsync( +@"class MyClass +{ + private int [|_goo|]; + MyClass() + { + (_goo) = 0; + } +}", +@"class MyClass +{ + private readonly int _goo; + MyClass() + { + (_goo) = 0; + } +}"); + } + + [WorkItem(26262, "https://github.com/dotnet/roslyn/issues/26262")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)] + public async Task FieldAssignedInCtor_QualifiedWithThis_InParens() + { + await TestInRegularAndScriptAsync( +@"class MyClass +{ + private int [|_goo|]; + MyClass() + { + (this._goo) = 0; + } +}", +@"class MyClass +{ + private readonly int _goo; + MyClass() + { + (this._goo) = 0; + } +}"); + } } } diff --git a/src/EditorFeatures/VisualBasicTest/MakeFieldReadonly/MakeFieldReadonlyTests.vb b/src/EditorFeatures/VisualBasicTest/MakeFieldReadonly/MakeFieldReadonlyTests.vb index 85fe0781517d8..fecbd19062ed3 100644 --- a/src/EditorFeatures/VisualBasicTest/MakeFieldReadonly/MakeFieldReadonlyTests.vb +++ b/src/EditorFeatures/VisualBasicTest/MakeFieldReadonly/MakeFieldReadonlyTests.vb @@ -599,6 +599,30 @@ End Class") End Class") End Function + + + Public Async Function CopyPassedAsByRefParameter() As Task + Await TestInRegularAndScriptAsync( +"Class C + Private [|_goo|] As Integer = 0 + Sub Goo() + ' Note: the parens cause a copy, so this is not an actual write into _goo + Bar((_goo)) + End Sub + Sub Bar(ByRef value As Integer) + End Sub +End Class", +"Class C + Private ReadOnly _goo As Integer = 0 + Sub Goo() + ' Note: the parens cause a copy, so this is not an actual write into _goo + Bar((_goo)) + End Sub + Sub Bar(ByRef value As Integer) + End Sub +End Class") + End Function + Public Async Function PassedAsByRefParameterInCtor() As Task Await TestInRegularAndScriptAsync( diff --git a/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs b/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs index a5226d2c33486..0d4d696984a0c 100644 --- a/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs +++ b/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs @@ -6,7 +6,6 @@ using System.Linq; using System.Threading; using Microsoft.CodeAnalysis.CodeStyle; -using Microsoft.CodeAnalysis.CSharp.CodeStyle.TypeStyle; using Microsoft.CodeAnalysis.CSharp.Extensions.ContextQuery; using Microsoft.CodeAnalysis.CSharp.Simplification; using Microsoft.CodeAnalysis.CSharp.Symbols; @@ -325,13 +324,20 @@ public static bool IsInRefContext(this ExpressionSyntax expression) public static bool IsInInContext(this ExpressionSyntax expression) => (expression?.Parent as ArgumentSyntax)?.RefKindKeyword.Kind() == SyntaxKind.InKeyword; - public static bool IsOnlyWrittenTo(this ExpressionSyntax expression) + private static ExpressionSyntax GetExpressionToAnalyzeForWrites(ExpressionSyntax expression) { - if (expression.IsRightSideOfDotOrArrow()) + while (expression.IsRightSideOfDotOrArrow() || expression.IsParentKind(SyntaxKind.ParenthesizedExpression)) { expression = expression.Parent as ExpressionSyntax; } + return expression; + } + + public static bool IsOnlyWrittenTo(this ExpressionSyntax expression) + { + expression = GetExpressionToAnalyzeForWrites(expression); + if (expression != null) { if (expression.IsInOutContext()) @@ -364,16 +370,13 @@ public static bool IsAttributeNamedArgumentIdentifier(this ExpressionSyntax expr public static bool IsWrittenTo(this ExpressionSyntax expression) { + expression = GetExpressionToAnalyzeForWrites(expression); + if (expression.IsOnlyWrittenTo()) { return true; } - if (expression.IsRightSideOfDotOrArrow()) - { - expression = expression.Parent as ExpressionSyntax; - } - if (expression.IsInRefContext()) { return true; @@ -692,20 +695,13 @@ public static bool TryReduceExplicitName( var memberAccess = (MemberAccessExpressionSyntax)expression; return memberAccess.TryReduce(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken); } - - if (expression is TypeSyntax typeName) + else if (expression is NameSyntax name) { - // First, see if we can replace this type with var if that's what the user prefers. - // That always overrides all other simplification. - if (typeName.IsReplaceableByVar(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken)) - { - return true; - } - - if (expression is NameSyntax name) - { - return name.TryReduce(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken); - } + return name.TryReduce(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken); + } + else if (expression is TypeSyntax typeName) + { + return typeName.IsReplaceableByVar(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken); } return false; @@ -2277,21 +2273,75 @@ private static bool IsReplaceableByVar( OptionSet optionSet, CancellationToken cancellationToken) { - var typeStyle = CSharpUseImplicitTypeHelper.Instance.AnalyzeTypeName( - simpleName, semanticModel, optionSet, cancellationToken); + replacementNode = null; + issueSpan = default; - if (!typeStyle.IsStylePreferred || !typeStyle.CanConvert()) + if (!optionSet.GetOption(SimplificationOptions.PreferImplicitTypeInLocalDeclaration)) { - replacementNode = null; - issueSpan = default; return false; } - replacementNode = SyntaxFactory.IdentifierName("var") - .WithLeadingTrivia(simpleName.GetLeadingTrivia()) - .WithTrailingTrivia(simpleName.GetTrailingTrivia()); - issueSpan = simpleName.Span; - return true; + // If it is already var + if (simpleName.IsVar) + { + return false; + } + + var candidateReplacementNode = SyntaxFactory.IdentifierName("var") + .WithLeadingTrivia(simpleName.GetLeadingTrivia()) + .WithTrailingTrivia(simpleName.GetTrailingTrivia()); + var candidateIssueSpan = simpleName.Span; + + // If there exists a Type called var , fail. + var checkSymbol = semanticModel.GetSpeculativeSymbolInfo(simpleName.SpanStart, candidateReplacementNode, SpeculativeBindingOption.BindAsTypeOrNamespace).Symbol; + if (checkSymbol != null && checkSymbol.IsKind(SymbolKind.NamedType) && ((INamedTypeSymbol)checkSymbol).TypeKind == TypeKind.Class && checkSymbol.Name == "var") + { + return false; + } + + // If the simpleName is the type of the Variable Declaration Syntax belonging to LocalDeclaration, For Statement or Using statement + if (simpleName.IsParentKind(SyntaxKind.VariableDeclaration) && + ((VariableDeclarationSyntax)simpleName.Parent).Type == simpleName && + simpleName.Parent.Parent.IsKind(SyntaxKind.LocalDeclarationStatement, SyntaxKind.ForStatement, SyntaxKind.UsingStatement)) + { + if (simpleName.Parent.IsParentKind(SyntaxKind.LocalDeclarationStatement) && + ((LocalDeclarationStatementSyntax)simpleName.Parent.Parent).Modifiers.Any(n => n.Kind() == SyntaxKind.ConstKeyword)) + { + return false; + } + + var variableDeclaration = (VariableDeclarationSyntax)simpleName.Parent; + + // Check the Initialized Value to see if it is allowed to be in the Var initialization + if (variableDeclaration.Variables.Count != 1 || + !variableDeclaration.Variables.Single().Initializer.IsKind(SyntaxKind.EqualsValueClause)) + { + return false; + } + + var variable = variableDeclaration.Variables.Single(); + var initializer = variable.Initializer; + var identifier = variable.Identifier; + + if (EqualsValueClauseNotSuitableForVar(identifier, simpleName, initializer, semanticModel, cancellationToken)) + { + return false; + } + + replacementNode = candidateReplacementNode; + issueSpan = candidateIssueSpan; + return true; + } + + if (simpleName.IsParentKind(SyntaxKind.ForEachStatement) && + ((ForEachStatementSyntax)simpleName.Parent).Type == simpleName) + { + replacementNode = candidateReplacementNode; + issueSpan = candidateIssueSpan; + return true; + } + + return false; } private static bool EqualsValueClauseNotSuitableForVar( From 899800e3fb4737a012217446321519ed050397b2 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 20 Apr 2018 17:17:02 -0700 Subject: [PATCH 2/4] Simplify code. --- .../CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs b/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs index 0d4d696984a0c..36df9b12eeae6 100644 --- a/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs +++ b/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs @@ -326,11 +326,13 @@ public static bool IsInInContext(this ExpressionSyntax expression) private static ExpressionSyntax GetExpressionToAnalyzeForWrites(ExpressionSyntax expression) { - while (expression.IsRightSideOfDotOrArrow() || expression.IsParentKind(SyntaxKind.ParenthesizedExpression)) + if (expression.IsRightSideOfDotOrArrow()) { expression = expression.Parent as ExpressionSyntax; } + expression = expression.WalkUpParentheses(); + return expression; } From a46a554edb24786b264eb9ee08fd180a3243f2a8 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 20 Apr 2018 17:19:20 -0700 Subject: [PATCH 3/4] Fix merge. --- .../Extensions/ExpressionSyntaxExtensions.cs | 106 +++++------------- 1 file changed, 30 insertions(+), 76 deletions(-) diff --git a/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs b/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs index 36df9b12eeae6..aec8e1541a174 100644 --- a/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs +++ b/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs @@ -6,6 +6,7 @@ using System.Linq; using System.Threading; using Microsoft.CodeAnalysis.CodeStyle; +using Microsoft.CodeAnalysis.CSharp.CodeStyle.TypeStyle; using Microsoft.CodeAnalysis.CSharp.Extensions.ContextQuery; using Microsoft.CodeAnalysis.CSharp.Simplification; using Microsoft.CodeAnalysis.CSharp.Symbols; @@ -364,12 +365,6 @@ public static bool IsOnlyWrittenTo(this ExpressionSyntax expression) return false; } - public static bool IsAttributeNamedArgumentIdentifier(this ExpressionSyntax expression) - { - var nameEquals = expression?.Parent as NameEqualsSyntax; - return nameEquals.IsParentKind(SyntaxKind.AttributeArgument); - } - public static bool IsWrittenTo(this ExpressionSyntax expression) { expression = GetExpressionToAnalyzeForWrites(expression); @@ -398,6 +393,12 @@ public static bool IsWrittenTo(this ExpressionSyntax expression) return false; } + public static bool IsAttributeNamedArgumentIdentifier(this ExpressionSyntax expression) + { + var nameEquals = expression?.Parent as NameEqualsSyntax; + return nameEquals.IsParentKind(SyntaxKind.AttributeArgument); + } + public static bool IsOperandOfIncrementOrDecrementExpression(this ExpressionSyntax expression) { if (expression != null) @@ -697,13 +698,20 @@ public static bool TryReduceExplicitName( var memberAccess = (MemberAccessExpressionSyntax)expression; return memberAccess.TryReduce(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken); } - else if (expression is NameSyntax name) - { - return name.TryReduce(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken); - } - else if (expression is TypeSyntax typeName) + + if (expression is TypeSyntax typeName) { - return typeName.IsReplaceableByVar(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken); + // First, see if we can replace this type with var if that's what the user prefers. + // That always overrides all other simplification. + if (typeName.IsReplaceableByVar(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken)) + { + return true; + } + + if (expression is NameSyntax name) + { + return name.TryReduce(semanticModel, out replacementNode, out issueSpan, optionSet, cancellationToken); + } } return false; @@ -2275,75 +2283,21 @@ private static bool IsReplaceableByVar( OptionSet optionSet, CancellationToken cancellationToken) { - replacementNode = null; - issueSpan = default; - - if (!optionSet.GetOption(SimplificationOptions.PreferImplicitTypeInLocalDeclaration)) - { - return false; - } - - // If it is already var - if (simpleName.IsVar) - { - return false; - } - - var candidateReplacementNode = SyntaxFactory.IdentifierName("var") - .WithLeadingTrivia(simpleName.GetLeadingTrivia()) - .WithTrailingTrivia(simpleName.GetTrailingTrivia()); - var candidateIssueSpan = simpleName.Span; + var typeStyle = CSharpUseImplicitTypeHelper.Instance.AnalyzeTypeName( + simpleName, semanticModel, optionSet, cancellationToken); - // If there exists a Type called var , fail. - var checkSymbol = semanticModel.GetSpeculativeSymbolInfo(simpleName.SpanStart, candidateReplacementNode, SpeculativeBindingOption.BindAsTypeOrNamespace).Symbol; - if (checkSymbol != null && checkSymbol.IsKind(SymbolKind.NamedType) && ((INamedTypeSymbol)checkSymbol).TypeKind == TypeKind.Class && checkSymbol.Name == "var") + if (!typeStyle.IsStylePreferred || !typeStyle.CanConvert()) { + replacementNode = null; + issueSpan = default; return false; } - // If the simpleName is the type of the Variable Declaration Syntax belonging to LocalDeclaration, For Statement or Using statement - if (simpleName.IsParentKind(SyntaxKind.VariableDeclaration) && - ((VariableDeclarationSyntax)simpleName.Parent).Type == simpleName && - simpleName.Parent.Parent.IsKind(SyntaxKind.LocalDeclarationStatement, SyntaxKind.ForStatement, SyntaxKind.UsingStatement)) - { - if (simpleName.Parent.IsParentKind(SyntaxKind.LocalDeclarationStatement) && - ((LocalDeclarationStatementSyntax)simpleName.Parent.Parent).Modifiers.Any(n => n.Kind() == SyntaxKind.ConstKeyword)) - { - return false; - } - - var variableDeclaration = (VariableDeclarationSyntax)simpleName.Parent; - - // Check the Initialized Value to see if it is allowed to be in the Var initialization - if (variableDeclaration.Variables.Count != 1 || - !variableDeclaration.Variables.Single().Initializer.IsKind(SyntaxKind.EqualsValueClause)) - { - return false; - } - - var variable = variableDeclaration.Variables.Single(); - var initializer = variable.Initializer; - var identifier = variable.Identifier; - - if (EqualsValueClauseNotSuitableForVar(identifier, simpleName, initializer, semanticModel, cancellationToken)) - { - return false; - } - - replacementNode = candidateReplacementNode; - issueSpan = candidateIssueSpan; - return true; - } - - if (simpleName.IsParentKind(SyntaxKind.ForEachStatement) && - ((ForEachStatementSyntax)simpleName.Parent).Type == simpleName) - { - replacementNode = candidateReplacementNode; - issueSpan = candidateIssueSpan; - return true; - } - - return false; + replacementNode = SyntaxFactory.IdentifierName("var") + .WithLeadingTrivia(simpleName.GetLeadingTrivia()) + .WithTrailingTrivia(simpleName.GetTrailingTrivia()); + issueSpan = simpleName.Span; + return true; } private static bool EqualsValueClauseNotSuitableForVar( From 388efee57b16aedcab25703c971a79ee7dcc1ef3 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 20 Apr 2018 18:26:57 -0700 Subject: [PATCH 4/4] Do not offer MakeReadOnly if variable is assigned through deconstruction. --- .../MakeFieldReadonlyTests.cs | 78 +++++++++++++++++++ .../Extensions/ExpressionSyntaxExtensions.cs | 59 ++++++++++++++ 2 files changed, 137 insertions(+) diff --git a/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs b/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs index 2b13b677bdeed..5bcb9ded0752d 100644 --- a/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs +++ b/src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs @@ -824,6 +824,84 @@ await TestInRegularAndScriptAsync( { (this._goo) = 0; } +}"); + } + + [WorkItem(26264, "https://github.com/dotnet/roslyn/issues/26264")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)] + public async Task FieldAssignedInMethod_InDeconstruction() + { + await TestMissingAsync( +@"class C +{ + [|int i;|] + int j; + + void M() + { + (i, j) = (1, 2); + } +}"); + } + + [WorkItem(26264, "https://github.com/dotnet/roslyn/issues/26264")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)] + public async Task FieldAssignedInMethod_InDeconstruction_InParens() + { + await TestMissingAsync( +@"class C +{ + [|int i;|] + int j; + + void M() + { + ((i, j), j) = ((1, 2), 3); + } +}"); + } + + [WorkItem(26264, "https://github.com/dotnet/roslyn/issues/26264")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)] + public async Task FieldAssignedInMethod_InDeconstruction_WithThis_InParens() + { + await TestMissingAsync( +@"class C +{ + [|int i;|] + int j; + + void M() + { + ((this.i, j), j) = (1, 2); + } +}"); + } + + [WorkItem(26264, "https://github.com/dotnet/roslyn/issues/26264")] + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)] + public async Task FieldUsedInTupleExpressionOnRight() + { + await TestInRegularAndScriptAsync( +@"class C +{ + [|int i;|] + int j; + + void M() + { + (j, j) = (i, i); + } +}", +@"class C +{ + readonly int i; + int j; + + void M() + { + (j, j) = (i, i); + } }"); } } diff --git a/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs b/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs index aec8e1541a174..c99238edd1575 100644 --- a/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs +++ b/src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Diagnostics; using System.Linq; using System.Threading; using Microsoft.CodeAnalysis.CodeStyle; @@ -360,11 +361,69 @@ public static bool IsOnlyWrittenTo(this ExpressionSyntax expression) return true; } } + + if (IsExpressionOfArgumentInDeconstruction(expression)) + { + return true; + } } return false; } + /// + /// If this declaration or identifier is part of a deconstruction, find the deconstruction. + /// If found, returns either an assignment expression or a foreach variable statement. + /// Returns null otherwise. + /// + /// copied from SyntaxExtensions.GetContainingDeconstruction + /// + private static bool IsExpressionOfArgumentInDeconstruction(ExpressionSyntax expr) + { + if (!expr.IsParentKind(SyntaxKind.Argument)) + { + return false; + } + + while (true) + { + var parent = expr.Parent; + if (parent == null) + { + return false; + } + + switch (parent.Kind()) + { + case SyntaxKind.Argument: + if (parent.Parent?.Kind() == SyntaxKind.TupleExpression) + { + expr = (TupleExpressionSyntax)parent.Parent; + continue; + } + + return false; + case SyntaxKind.SimpleAssignmentExpression: + if (((AssignmentExpressionSyntax)parent).Left == expr) + { + return true; + } + + return false; + case SyntaxKind.ForEachVariableStatement: + if (((ForEachVariableStatementSyntax)parent).Variable == expr) + { + return true; + } + + return false; + + default: + return false; + } + } + } + public static bool IsWrittenTo(this ExpressionSyntax expression) { expression = GetExpressionToAnalyzeForWrites(expression);