Skip to content

Commit 92bacf8

Browse files
sharwellsharwell
committed
Simplify code and add tests based on code review feedback
1 parent 2971297 commit 92bacf8

File tree

3 files changed

+183
-13
lines changed

3 files changed

+183
-13
lines changed

‎src/Analyzers/CSharp/Tests/MakeFieldReadonly/MakeFieldReadonlyTests.cs‎

Lines changed: 87 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1817,6 +1817,93 @@ public class MyClass
18171817
}
18181818
</Document>
18191819
</Project>
1820+
</Workspace>");
1821+
}
1822+
1823+
[WorkItem(40644, "https://github.com/dotnet/roslyn/issues/40644")]
1824+
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)]
1825+
public async Task ShouldWarnForDataMemberFieldsInNonDataContractClasses()
1826+
{
1827+
await TestInRegularAndScript1Async(
1828+
@"
1829+
<Workspace>
1830+
<Project Language=""C#"" AssemblyName=""Assembly1"" CommonReferencesNet45=""true"">
1831+
<Document>
1832+
public class MyClass
1833+
{
1834+
[System.Runtime.Serialization.DataMember]
1835+
private bool [|isReadOnly|];
1836+
}
1837+
</Document>
1838+
</Project>
1839+
</Workspace>",
1840+
@"
1841+
<Workspace>
1842+
<Project Language=""C#"" AssemblyName=""Assembly1"" CommonReferencesNet45=""true"">
1843+
<Document>
1844+
public class MyClass
1845+
{
1846+
[System.Runtime.Serialization.DataMember]
1847+
private readonly bool isReadOnly;
1848+
}
1849+
</Document>
1850+
</Project>
1851+
</Workspace>");
1852+
}
1853+
1854+
[WorkItem(40644, "https://github.com/dotnet/roslyn/issues/40644")]
1855+
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)]
1856+
public async Task ShouldWarnForPrivateNonDataMemberFieldsInDataContractClasses()
1857+
{
1858+
await TestInRegularAndScript1Async(
1859+
@"
1860+
<Workspace>
1861+
<Project Language=""C#"" AssemblyName=""Assembly1"" CommonReferencesNet45=""true"">
1862+
<Document>
1863+
[System.Runtime.Serialization.DataContractAttribute]
1864+
public class MyClass
1865+
{
1866+
[System.Runtime.Serialization.DataMember]
1867+
private bool isReadOnly;
1868+
1869+
private bool [|isReadOnly2|];
1870+
}
1871+
</Document>
1872+
</Project>
1873+
</Workspace>",
1874+
@"
1875+
<Workspace>
1876+
<Project Language=""C#"" AssemblyName=""Assembly1"" CommonReferencesNet45=""true"">
1877+
<Document>
1878+
[System.Runtime.Serialization.DataContractAttribute]
1879+
public class MyClass
1880+
{
1881+
[System.Runtime.Serialization.DataMember]
1882+
private bool isReadOnly;
1883+
1884+
private readonly bool isReadOnly2;
1885+
}
1886+
</Document>
1887+
</Project>
1888+
</Workspace>");
1889+
}
1890+
1891+
[WorkItem(40644, "https://github.com/dotnet/roslyn/issues/40644")]
1892+
[Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)]
1893+
public async Task ShouldNotWarnForPublicImplicitDataMemberFieldsInDataContractClasses()
1894+
{
1895+
await TestMissingAsync(
1896+
@"
1897+
<Workspace>
1898+
<Project Language=""C#"" AssemblyName=""Assembly1"" CommonReferencesNet45=""true"">
1899+
<Document>
1900+
[System.Runtime.Serialization.DataContractAttribute]
1901+
public class MyClass
1902+
{
1903+
public bool [|isReadOnly|];
1904+
}
1905+
</Document>
1906+
</Project>
18201907
</Workspace>");
18211908
}
18221909
}

‎src/Analyzers/Core/Analyzers/MakeFieldReadonly/MakeFieldReadonlyDiagnosticAnalyzer.cs‎

Lines changed: 14 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66

77
using System.Collections.Concurrent;
88
using System.Diagnostics;
9-
using System.Linq;
109
using System.Threading;
1110
using Microsoft.CodeAnalysis;
1211
using Microsoft.CodeAnalysis.CodeStyle;
@@ -45,6 +44,8 @@ protected override void InitializeWorker(AnalysisContext context)
4544
var fieldStateMap = new ConcurrentDictionary<IFieldSymbol, (bool isCandidate, bool written)>();
4645

4746
var threadStaticAttribute = compilationStartContext.Compilation.ThreadStaticAttributeType();
47+
var dataContractAttribute = compilationStartContext.Compilation.DataContractAttribute();
48+
var dataMemberAttribute = compilationStartContext.Compilation.DataMemberAttribute();
4849

4950
// We register following actions in the compilation:
5051
// 1. A symbol action for field symbols to ensure the field state is initialized for every field in
@@ -112,7 +113,7 @@ void OnSymbolEnd(SymbolAnalysisContext symbolEndContext)
112113
}
113114
}
114115

115-
static bool IsCandidateField(IFieldSymbol symbol, INamedTypeSymbol threadStaticAttribute, Compilation compilation) =>
116+
static bool IsCandidateField(IFieldSymbol symbol, INamedTypeSymbol threadStaticAttribute, INamedTypeSymbol dataContractAttribute, INamedTypeSymbol dataMemberAttribute) =>
116117
symbol.DeclaredAccessibility == Accessibility.Private &&
117118
!symbol.IsReadOnly &&
118119
!symbol.IsConst &&
@@ -123,21 +124,21 @@ static bool IsCandidateField(IFieldSymbol symbol, INamedTypeSymbol threadStaticA
123124
!symbol.GetAttributes().Any(
124125
static (a, threadStaticAttribute) => SymbolEqualityComparer.Default.Equals(a.AttributeClass, threadStaticAttribute),
125126
threadStaticAttribute) &&
126-
!IsDataContractSerializable(symbol, compilation);
127+
!IsDataContractSerializable(symbol, dataContractAttribute, dataMemberAttribute);
127128

128-
static bool IsDataContractSerializable(IFieldSymbol symbol, Compilation compilation)
129+
static bool IsDataContractSerializable(IFieldSymbol symbol, INamedTypeSymbol dataContractAttribute, INamedTypeSymbol dataMemberAttribute)
129130
{
130-
var dataMemberAttribute = compilation.DataMemberAttribute();
131-
var dataContractAttribute = compilation.DataContractAttribute();
131+
if (dataContractAttribute is null || dataMemberAttribute is null)
132+
return false;
132133

133-
return symbol.GetAttributes().Any(x => x.AttributeClass == dataMemberAttribute)
134-
&& symbol.ContainingType.GetAttributes().Any(x => x.AttributeClass == dataContractAttribute);
134+
return symbol.GetAttributes().Any(static (x, dataMemberAttribute) => SymbolEqualityComparer.Default.Equals(x.AttributeClass, dataMemberAttribute), dataMemberAttribute)
135+
&& symbol.ContainingType.GetAttributes().Any(static (x, dataContractAttribute) => SymbolEqualityComparer.Default.Equals(x.AttributeClass, dataContractAttribute), dataContractAttribute);
135136
}
136137

137138
// Method to update the field state for a candidate field written outside constructor and field initializer.
138139
void UpdateFieldStateOnWrite(IFieldSymbol field)
139140
{
140-
Debug.Assert(IsCandidateField(field, threadStaticAttribute, compilationStartContext.Compilation));
141+
Debug.Assert(IsCandidateField(field, threadStaticAttribute, dataContractAttribute, dataMemberAttribute));
141142
Debug.Assert(fieldStateMap.ContainsKey(field));
142143

143144
fieldStateMap[field] = (isCandidate: true, written: true);
@@ -146,7 +147,7 @@ void UpdateFieldStateOnWrite(IFieldSymbol field)
146147
// Method to get or initialize the field state.
147148
(bool isCandidate, bool written) TryGetOrInitializeFieldState(IFieldSymbol fieldSymbol, AnalyzerOptions options, CancellationToken cancellationToken)
148149
{
149-
if (!IsCandidateField(fieldSymbol, threadStaticAttribute, compilationStartContext.Compilation))
150+
if (!IsCandidateField(fieldSymbol, threadStaticAttribute, dataContractAttribute, dataMemberAttribute))
150151
{
151152
return default;
152153
}
@@ -156,14 +157,14 @@ void UpdateFieldStateOnWrite(IFieldSymbol field)
156157
return result;
157158
}
158159

159-
result = ComputeInitialFieldState(fieldSymbol, options, threadStaticAttribute, compilationStartContext.Compilation, cancellationToken);
160+
result = ComputeInitialFieldState(fieldSymbol, options, threadStaticAttribute, dataContractAttribute, dataMemberAttribute, cancellationToken);
160161
return fieldStateMap.GetOrAdd(fieldSymbol, result);
161162
}
162163

163164
// Method to compute the initial field state.
164-
static (bool isCandidate, bool written) ComputeInitialFieldState(IFieldSymbol field, AnalyzerOptions options, INamedTypeSymbol threadStaticAttribute, Compilation compilation, CancellationToken cancellationToken)
165+
static (bool isCandidate, bool written) ComputeInitialFieldState(IFieldSymbol field, AnalyzerOptions options, INamedTypeSymbol threadStaticAttribute, INamedTypeSymbol dataContractAttribute, INamedTypeSymbol dataMemberAttribute, CancellationToken cancellationToken)
165166
{
166-
Debug.Assert(IsCandidateField(field, threadStaticAttribute, compilation));
167+
Debug.Assert(IsCandidateField(field, threadStaticAttribute, dataContractAttribute, dataMemberAttribute));
167168

168169
var option = GetCodeStyleOption(field, options, cancellationToken);
169170
if (option == null || !option.Value)

‎src/Analyzers/VisualBasic/Tests/MakeFieldReadonly/MakeFieldReadonlyTests.vb‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -998,5 +998,87 @@ End Class
998998

999999
Await TestMissingAsync(initialMarkup)
10001000
End Function
1001+
1002+
<Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)>
1003+
<WorkItem(40644, "https://github.com/dotnet/roslyn/issues/40644")>
1004+
Public Async Function ShouldWarnForDataMemberFieldsInNonDataContractClasses() As Task
1005+
Dim initialMarkup =
1006+
<Workspace>
1007+
<Project Language="Visual Basic" CommonReferencesNet45="true">
1008+
<Document>
1009+
Class Test
1010+
&lt;System.Runtime.Serialization.DataMember&gt;
1011+
Private [|id|] As String
1012+
End Class
1013+
</Document>
1014+
</Project>
1015+
</Workspace>.ToString()
1016+
Dim expectedMarkup =
1017+
<Workspace>
1018+
<Project Language="Visual Basic" CommonReferencesNet45="true">
1019+
<Document>
1020+
Class Test
1021+
&lt;System.Runtime.Serialization.DataMember&gt;
1022+
Private ReadOnly id As String
1023+
End Class
1024+
</Document>
1025+
</Project>
1026+
</Workspace>.ToString()
1027+
1028+
Await TestInRegularAndScript1Async(initialMarkup, expectedMarkup)
1029+
End Function
1030+
1031+
<Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)>
1032+
<WorkItem(40644, "https://github.com/dotnet/roslyn/issues/40644")>
1033+
Public Async Function ShouldWarnForPrivateNonDataMemberFieldsInDataContractClasses() As Task
1034+
Dim initialMarkup =
1035+
<Workspace>
1036+
<Project Language="Visual Basic" CommonReferencesNet45="true">
1037+
<Document>
1038+
&lt;System.Runtime.Serialization.DataContract&gt;
1039+
Class Test
1040+
&lt;System.Runtime.Serialization.DataMember&gt;
1041+
Private id As String
1042+
1043+
Private [|id2|] As String
1044+
End Class
1045+
</Document>
1046+
</Project>
1047+
</Workspace>.ToString()
1048+
Dim expectedMarkup =
1049+
<Workspace>
1050+
<Project Language="Visual Basic" CommonReferencesNet45="true">
1051+
<Document>
1052+
&lt;System.Runtime.Serialization.DataContract&gt;
1053+
Class Test
1054+
&lt;System.Runtime.Serialization.DataMember&gt;
1055+
Private id As String
1056+
1057+
Private ReadOnly id2 As String
1058+
End Class
1059+
</Document>
1060+
</Project>
1061+
</Workspace>.ToString()
1062+
1063+
Await TestInRegularAndScript1Async(initialMarkup, expectedMarkup)
1064+
End Function
1065+
1066+
<Fact, Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)>
1067+
<WorkItem(40644, "https://github.com/dotnet/roslyn/issues/40644")>
1068+
Public Async Function ShouldNotWarnForPublicImplicitDataMemberFieldsInDataContractClasses() As Task
1069+
Dim initialMarkup =
1070+
<Workspace>
1071+
<Project Language="Visual Basic" CommonReferencesNet45="true">
1072+
<Document>
1073+
&lt;System.Runtime.Serialization.DataContract&gt;
1074+
Class Test
1075+
Public [|id|] As String
1076+
End Class
1077+
</Document>
1078+
</Project>
1079+
</Workspace>.ToString()
1080+
1081+
Await TestMissingAsync(initialMarkup)
1082+
End Function
10011083
End Class
10021084
End Namespace

0 commit comments

Comments
 (0)