Skip to content

Commit

Permalink
Merge pull request dotnet#26303 from CyrusNajmabadi/makeReadOnlyDecon…
Browse files Browse the repository at this point in the history 
…struction

Fix issue where a write wasn't detected in make-readonly #2
  • Loading branch information
@jinujoseph
jinujoseph authored Apr 24, 2018
2 parents 25fbfe4 + 388efee commit 70d9902
Show file tree
Hide file tree
Showing 3 changed files with 223 additions and 9 deletions.
125 changes: 125 additions & 0 deletions src/EditorFeatures/CSharpTest/MakeFieldReadonly/MakeFieldReadonlyTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -779,5 +780,129 @@ 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;
}
}");
}

[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);
}
}");
}
}
}
24 changes: 24 additions & 0 deletions src/EditorFeatures/VisualBasicTest/MakeFieldReadonly/MakeFieldReadonlyTests.vb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,30 @@ End Class")
End Class")
End Function

<WorkItem(26262, "https://github.com/dotnet/roslyn/issues/26262")>
<Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)>
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

<Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)>
Public Async Function PassedAsByRefParameterInCtor() As Task
Await TestInRegularAndScriptAsync(
Expand Down
83 changes: 74 additions & 9 deletions src/Workspaces/CSharp/Portable/Extensions/ExpressionSyntaxExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -325,13 +326,22 @@ 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())
{
expression = expression.Parent as ExpressionSyntax;
}

expression = expression.WalkUpParentheses();

return expression;
}

public static bool IsOnlyWrittenTo(this ExpressionSyntax expression)
{
expression = GetExpressionToAnalyzeForWrites(expression);

if (expression != null)
{
if (expression.IsInOutContext())
Expand All @@ -351,29 +361,78 @@ public static bool IsOnlyWrittenTo(this ExpressionSyntax expression)
return true;
}
}

if (IsExpressionOfArgumentInDeconstruction(expression))
{
return true;
}
}

return false;
}

public static bool IsAttributeNamedArgumentIdentifier(this ExpressionSyntax expression)
/// <summary>
/// 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
/// </summary>
private static bool IsExpressionOfArgumentInDeconstruction(ExpressionSyntax expr)
{
var nameEquals = expression?.Parent as NameEqualsSyntax;
return nameEquals.IsParentKind(SyntaxKind.AttributeArgument);
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);

if (expression.IsOnlyWrittenTo())
{
return true;
}

if (expression.IsRightSideOfDotOrArrow())
{
expression = expression.Parent as ExpressionSyntax;
}

if (expression.IsInRefContext())
{
return true;
Expand All @@ -393,6 +452,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)
Expand Down

0 comments on commit 70d9902

Please sign in to comment.