Skip to content

Commit

Permalink
Merge pull request #67227 from CyrusNajmabadi/strictChecks
Browse files Browse the repository at this point in the history 
Update 'make field readonly' to understand a special strict behaviour around generic assignment.
  • Loading branch information
@CyrusNajmabadi
CyrusNajmabadi authored Mar 8, 2023
2 parents fa8b720 + df0cba2 commit cb3c823
Show file tree
Hide file tree
Showing 4 changed files with 243 additions and 89 deletions.
78 changes: 78 additions & 0 deletions src/Analyzers/CSharp/Tests/MakeFieldReadonly/MakeFieldReadonlyTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Analyzers.MakeFieldReadonly;
using Microsoft.CodeAnalysis.CSharp.MakeFieldReadonly;
using Microsoft.CodeAnalysis.Diagnostics;
Expand All @@ -18,6 +20,8 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.MakeFieldReadonly
[Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)]
public class MakeFieldReadonlyTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest
{
private static readonly ParseOptions s_strictFeatureFlag = CSharpParseOptions.Default.WithFeatures(new[] { new KeyValuePair<string, string>("strict", "true") });

public MakeFieldReadonlyTests(ITestOutputHelper logger)
: base(logger)
{
Expand Down Expand Up @@ -2188,5 +2192,79 @@ public class C
}
""");
}

[Fact, WorkItem(47197, "https://github.com/dotnet/roslyn/issues/47197")]
public async Task StrictFeatureFlagAssignment1()
{
await TestInRegularAndScriptAsync(
"""
using System;
using System.Collections.Generic;
class C<T>
{
private static IEqualityComparer<T> [|s_value|];
static C()
{
C<T>.s_value = null;
}
}
""",
"""
using System;
using System.Collections.Generic;
class C<T>
{
private static readonly IEqualityComparer<T> s_value;
static C()
{
C<T>.s_value = null;
}
}
""", parseOptions: s_strictFeatureFlag);
}

[Fact, WorkItem(47197, "https://github.com/dotnet/roslyn/issues/47197")]
public async Task StrictFeatureFlagAssignment2()
{
await TestMissingInRegularAndScriptAsync(
"""
using System;
using System.Collections.Generic;
class C<T>
{
private static IEqualityComparer<T> [|s_value|];
static C()
{
C<string>.s_value = null;
}
}
""", new TestParameters(parseOptions: s_strictFeatureFlag));
}

[Fact, WorkItem(47197, "https://github.com/dotnet/roslyn/issues/47197")]
public async Task StrictFeatureFlagAssignment3()
{
await TestMissingInRegularAndScriptAsync(
"""
using System;
using System.Collections.Generic;
class C<T>
{
private static IEqualityComparer<T> [|s_value|];
static C()
{
C<string>.s_value = null;
}
}
""");
}
}
}
74 changes: 43 additions & 31 deletions ...Analyzers/Core/Analyzers/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,13 @@ void AnalyzeOperation(OperationAnalysisContext operationContext)

// Ignore fields that are not candidates or have already been written outside the constructor/field initializer.
if (!isCandidate || written)
{
return;
}

// Check if this is a field write outside constructor and field initializer, and update field state accordingly.
if (IsFieldWrite(fieldReference, operationContext.ContainingSymbol))
{
UpdateFieldStateOnWrite(fieldReference.Field);
}
if (!IsFieldWrite(fieldReference, operationContext.ContainingSymbol))
return;

UpdateFieldStateOnWrite(fieldReference.Field);
}

void OnSymbolEnd(SymbolAnalysisContext symbolEndContext)
Expand All @@ -113,7 +111,7 @@ void OnSymbolEnd(SymbolAnalysisContext symbolEndContext)
var members = ((INamedTypeSymbol)symbolEndContext.Symbol).GetMembers();
foreach (var member in members)
{
if (member is IFieldSymbol field && fieldStateMap.TryRemove(field, out var value))
if (member is IFieldSymbol field && fieldStateMap.TryRemove(field.OriginalDefinition, out var value))
{
var (isCandidate, written) = value;
if (isCandidate && !written)
Expand Down Expand Up @@ -160,9 +158,9 @@ static bool IsDataContractSerializable(IFieldSymbol symbol, INamedTypeSymbol? da
void UpdateFieldStateOnWrite(IFieldSymbol field)
{
Debug.Assert(IsCandidateField(field, threadStaticAttribute, dataContractAttribute, dataMemberAttribute));
Debug.Assert(fieldStateMap.ContainsKey(field));
Debug.Assert(fieldStateMap.ContainsKey(field.OriginalDefinition));

fieldStateMap[field] = (isCandidate: true, written: true);
fieldStateMap[field.OriginalDefinition] = (isCandidate: true, written: true);
}

// Method to get or initialize the field state.
Expand All @@ -173,13 +171,13 @@ void UpdateFieldStateOnWrite(IFieldSymbol field)
return default;
}

if (fieldStateMap.TryGetValue(fieldSymbol, out var result))
if (fieldStateMap.TryGetValue(fieldSymbol.OriginalDefinition, out var result))
{
return result;
}

result = ComputeInitialFieldState(fieldSymbol, options, threadStaticAttribute, dataContractAttribute, dataMemberAttribute, cancellationToken);
return fieldStateMap.GetOrAdd(fieldSymbol, result);
return fieldStateMap.GetOrAdd(fieldSymbol.OriginalDefinition, result);
}

// Method to compute the initial field state.
Expand Down Expand Up @@ -209,41 +207,55 @@ private static bool IsFieldWrite(IFieldReferenceOperation fieldReference, ISymbo
// Check if the underlying member is being written or a writable reference to the member is taken.
var valueUsageInfo = fieldReference.GetValueUsageInfo(owningSymbol);
if (!valueUsageInfo.IsWrittenTo())
{
return false;
}

// Writes to fields inside constructor are ignored, except for the below cases:
// 1. Instance reference of an instance field being written is not the instance being initialized by the constructor.
// 2. Field is being written inside a lambda or local function.

// Check if we are in the constructor of the containing type of the written field.
var isInConstructor = owningSymbol.IsConstructor();
var isInInstanceConstructor = owningSymbol.IsConstructor();
var isInStaticConstructor = owningSymbol.IsStaticConstructor();

// If we're not in a constructor, this is definitely a write to the field that would prevent it from
// becoming readonly.
if (!isInInstanceConstructor && !isInStaticConstructor)
return true;

var field = fieldReference.Field;
if ((isInConstructor || isInStaticConstructor) &&
field.ContainingType == owningSymbol.ContainingType)
{
// For instance fields, ensure that the instance reference is being initialized by the constructor.
var instanceFieldWrittenInCtor = isInConstructor &&
fieldReference.Instance?.Kind == OperationKind.InstanceReference &&
!fieldReference.IsTargetOfObjectMemberInitializer();

// Follow 'strict' C#/VB behavior. Has to be opted into by user with feature-flag "strict", but is
// actually what the languages specify as correct. Specifically the actual types of the containing
// type and field-reference-containing type must be the same (not just their OriginalDefinition
// types).
//
// https://github.com/dotnet/roslyn/blob/8770fb62a36157ed4ca38a16a0283d27321a01a7/src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs#L1201-L1203
// https//github.com/dotnet/roslyn/blob/93d3aa1a2cf1790b1a0fe2d120f00987d50445c0/src/Compilers/VisualBasic/Portable/Binding/Binder_Expressions.vb#L1868-L1871
if (!field.ContainingType.Equals(owningSymbol.ContainingType))
return true;

if (isInStaticConstructor)
{
// For static fields, ensure that we are in the static constructor.
var staticFieldWrittenInStaticCtor = isInStaticConstructor && field.IsStatic;

if (instanceFieldWrittenInCtor || staticFieldWrittenInStaticCtor)
if (!field.IsStatic)
return true;
}
else
{
// For instance fields, ensure that the instance reference is being initialized by the constructor.
Debug.Assert(isInInstanceConstructor);
if (fieldReference.Instance?.Kind != OperationKind.InstanceReference ||
fieldReference.IsTargetOfObjectMemberInitializer())
{
// Finally, ensure that the write is not inside a lambda or local function.
if (fieldReference.TryGetContainingAnonymousFunctionOrLocalFunction() is null)
{
// It is safe to ignore this write.
return false;
}
return true;
}
}

return true;
// Finally, ensure that the write is not inside a lambda or local function.
if (fieldReference.TryGetContainingAnonymousFunctionOrLocalFunction() is not null)
return true;

return false;
}

private static CodeStyleOption2<bool> GetCodeStyleOption(IFieldSymbol field, AnalyzerOptions options)
Expand Down
64 changes: 64 additions & 0 deletions src/Analyzers/VisualBasic/Tests/MakeFieldReadonly/MakeFieldReadonlyTests.vb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ Namespace Microsoft.CodeAnalysis.Editor.VisualBasic.UnitTests.MakeFieldReadonly
Public Class MakeFieldReadonlyTests
Inherits AbstractVisualBasicDiagnosticProviderBasedUserDiagnosticTest

Private Shared ReadOnly s_strictFeatureFlag As ParseOptions = VisualBasicParseOptions.Default.WithFeatures({New KeyValuePair(Of String, String)("strict", "true")})

Friend Overrides Function CreateDiagnosticProviderAndFixer(workspace As Workspace) As (DiagnosticAnalyzer, CodeFixProvider)
Return (New VisualBasicMakeFieldReadonlyDiagnosticAnalyzer(), New VisualBasicMakeFieldReadonlyCodeFixProvider())
End Function
Expand Down Expand Up @@ -1123,5 +1125,67 @@ End Module
Await TestInRegularAndScript1Async(initialMarkup, expectedMarkup)
End Function

<Fact, WorkItem(47197, "https://github.com/dotnet/roslyn/issues/47197")>
Public Async Function StrictFeatureFlagAssignment1() As Task
Await TestInRegularAndScriptAsync(
"
imports System
imports System.Collections.Generic

class C(Of T)
private shared [|s_value|] as IEqualityComparer(Of T)

shared sub new()
C(Of T).s_value = nothing
end sub
end class
",
"
imports System
imports System.Collections.Generic

class C(Of T)
private shared ReadOnly s_value as IEqualityComparer(Of T)

shared sub new()
C(Of T).s_value = nothing
end sub
end class
", parseOptions:=s_strictFeatureFlag)
End Function

<Fact, WorkItem(47197, "https://github.com/dotnet/roslyn/issues/47197")>
Public Async Function StrictFeatureFlagAssignment2() As Task
Await TestMissingInRegularAndScriptAsync(
"
imports System
imports System.Collections.Generic

class C(Of T)
private shared [|s_value|] as IEqualityComparer(Of T)

shared sub new()
C(Of string).s_value = nothing
end sub
end class
", New TestParameters(parseOptions:=s_strictFeatureFlag))
End Function

<Fact, WorkItem(47197, "https://github.com/dotnet/roslyn/issues/47197")>
Public Async Function StrictFeatureFlagAssignment3() As Task
Await TestMissingAsync(
"
imports System
imports System.Collections.Generic

class C(Of T)
private shared [|s_value|] as IEqualityComparer(Of T)

shared sub new()
C(Of string).s_value = nothing
end sub
end class
")
End Function
End Class
End Namespace
Loading

0 comments on commit cb3c823

Please sign in to comment.