Skip to content

Do not offer to remove the primary field from an inline array #76052

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 25, 2024
Merged

Do not offer to remove the primary field from an inline array #76052

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f1a8f1
Do not offer to remove the primary field from an inline array
CyrusNajmabadi Nov 23, 2024
8844029
Do not offer to make primary field from an inline array readonluy
CyrusNajmabadi Nov 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 62 additions & 6 deletions src/Analyzers/CSharp/Tests/MakeFieldReadonly/MakeFieldReadonlyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// 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;
Expand All @@ -19,14 +18,26 @@
namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.MakeFieldReadonly;

[Trait(Traits.Feature, Traits.Features.CodeActionsMakeFieldReadonly)]
public class MakeFieldReadonlyTests : AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor
public sealed class MakeFieldReadonlyTests(ITestOutputHelper logger)
: AbstractCSharpDiagnosticProviderBasedUserDiagnosticTest_NoEditor(logger)
{
private static readonly ParseOptions s_strictFeatureFlag = CSharpParseOptions.Default.WithFeatures([KeyValuePairUtil.Create("strict", "true")]);

public MakeFieldReadonlyTests(ITestOutputHelper logger)
: base(logger)
{
}
private const string s_inlineArrayAttribute = """
namespace System.Runtime.CompilerServices
{
[AttributeUsage(AttributeTargets.Struct, AllowMultiple = false)]
public sealed class InlineArrayAttribute : Attribute
{
public InlineArrayAttribute (int length)
{
Length = length;
}

public int Length { get; }
}
}
""";

internal override (DiagnosticAnalyzer, CodeFixProvider) CreateDiagnosticProviderAndFixer(Workspace workspace)
=> (new CSharpMakeFieldReadonlyDiagnosticAnalyzer(), new CSharpMakeFieldReadonlyCodeFixProvider());
Expand Down Expand Up @@ -2267,4 +2278,49 @@ static C()
}
""");
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69143")]
public async Task DoNotAddReadonlyToInlineArrayInstanceMember()
{
await TestMissingInRegularAndScriptAsync($$"""
using System;
using System.Runtime.CompilerServices;

{{s_inlineArrayAttribute}}

[InlineArray(4)]
struct S
{
private int [|i|];
}
""");
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/75995")]
public async Task AddReadonlyToInlineArrayStaticMember1()
{
await TestInRegularAndScriptAsync($$"""
using System;
using System.Runtime.CompilerServices;

{{s_inlineArrayAttribute}}

[InlineArray(4)]
struct S
{
private static int [|j|];
}
""", $$"""
using System;
using System.Runtime.CompilerServices;

{{s_inlineArrayAttribute}}

[InlineArray(4)]
struct S
{
private static readonly int j;
}
""");
}
}
29 changes: 29 additions & 0 deletions src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3339,4 +3339,33 @@ public void M(
""",
}.RunAsync();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69143")]
public async Task KeepInlineArrayInstanceMember()
{
await new VerifyCS.Test
{
TestCode = """
using System.Runtime.CompilerServices;

[InlineArray(4)]
struct S
{
private int i;
private static int [|j|];
}
""",
FixedCode = """
using System.Runtime.CompilerServices;

[InlineArray(4)]
struct S
{
private int i;
}
""",
LanguageVersion = LanguageVersion.CSharp13,
ReferenceAssemblies = ReferenceAssemblies.Net.Net90,
}.RunAsync();
}
}
96 changes: 54 additions & 42 deletions ...Analyzers/Core/Analyzers/MakeFieldReadonly/AbstractMakeFieldReadonlyDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,31 +14,25 @@

namespace Microsoft.CodeAnalysis.MakeFieldReadonly;

internal abstract class AbstractMakeFieldReadonlyDiagnosticAnalyzer<
TSyntaxKind, TThisExpression>
: AbstractBuiltInCodeStyleDiagnosticAnalyzer
internal abstract class AbstractMakeFieldReadonlyDiagnosticAnalyzer<TSyntaxKind, TThisExpression>()
: AbstractBuiltInCodeStyleDiagnosticAnalyzer(
IDEDiagnosticIds.MakeFieldReadonlyDiagnosticId,
EnforceOnBuildValues.MakeFieldReadonly,
CodeStyleOptions2.PreferReadonly,
new LocalizableResourceString(nameof(AnalyzersResources.Add_readonly_modifier), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)),
new LocalizableResourceString(nameof(AnalyzersResources.Make_field_readonly), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)))
where TSyntaxKind : struct, Enum
where TThisExpression : SyntaxNode
{
protected AbstractMakeFieldReadonlyDiagnosticAnalyzer()
: base(
IDEDiagnosticIds.MakeFieldReadonlyDiagnosticId,
EnforceOnBuildValues.MakeFieldReadonly,
CodeStyleOptions2.PreferReadonly,
new LocalizableResourceString(nameof(AnalyzersResources.Add_readonly_modifier), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)),
new LocalizableResourceString(nameof(AnalyzersResources.Make_field_readonly), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)))
{
}

protected abstract ISyntaxKinds SyntaxKinds { get; }
protected abstract bool IsWrittenTo(SemanticModel semanticModel, TThisExpression expression, CancellationToken cancellationToken);

public override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticDocumentAnalysis;
public sealed override DiagnosticAnalyzerCategory GetAnalyzerCategory() => DiagnosticAnalyzerCategory.SemanticDocumentAnalysis;

// We need to analyze generated code to get callbacks for read/writes to non-generated members in generated code.
protected override GeneratedCodeAnalysisFlags GeneratedCodeAnalysisFlags => GeneratedCodeAnalysisFlags.Analyze;
protected sealed override GeneratedCodeAnalysisFlags GeneratedCodeAnalysisFlags => GeneratedCodeAnalysisFlags.Analyze;

protected override void InitializeWorker(AnalysisContext context)
protected sealed override void InitializeWorker(AnalysisContext context)
{
context.RegisterCompilationStartAction(context =>
{
Expand All @@ -47,9 +41,11 @@ protected override void InitializeWorker(AnalysisContext context)
// 'written' : Indicates if there are any writes to the field outside the constructor and field initializer.
var fieldStateMap = new ConcurrentDictionary<IFieldSymbol, (bool isCandidate, bool written)>();

var threadStaticAttribute = context.Compilation.ThreadStaticAttributeType();
var dataContractAttribute = context.Compilation.DataContractAttribute();
var dataMemberAttribute = context.Compilation.DataMemberAttribute();
var compilation = context.Compilation;
var threadStaticAttribute = compilation.ThreadStaticAttributeType();
var dataContractAttribute = compilation.DataContractAttribute();
var dataMemberAttribute = compilation.DataMemberAttribute();
var inlineArrayAttribute = compilation.InlineArrayAttributeType();

// We register following actions in the compilation:
// 1. A symbol action for field symbols to ensure the field state is initialized for every field in
Expand Down Expand Up @@ -142,10 +138,10 @@ bool ShouldAnalyze(SymbolStartAnalysisContext context, INamedTypeSymbol namedTyp
// Check if we have at least one candidate field in analysis scope.
foreach (var member in namedType.GetMembers())
{
if (member is IFieldSymbol field
&& IsCandidateField(field, threadStaticAttribute, dataContractAttribute, dataMemberAttribute)
&& GetDiagnosticLocation(field) is { } location
&& context.ShouldAnalyzeLocation(location))
if (member is IFieldSymbol field &&
IsCandidateField(field) &&
GetDiagnosticLocation(field) is { } location &&
context.ShouldAnalyzeLocation(location))
{
return true;
}
Expand All @@ -158,21 +154,44 @@ bool ShouldAnalyze(SymbolStartAnalysisContext context, INamedTypeSymbol namedTyp
return false;
}

static bool IsCandidateField(IFieldSymbol symbol, INamedTypeSymbol? threadStaticAttribute, INamedTypeSymbol? dataContractAttribute, INamedTypeSymbol? dataMemberAttribute)
=> symbol is
bool IsCandidateField(IFieldSymbol symbol)
{
if (symbol is not
{
DeclaredAccessibility: Accessibility.Private,
IsReadOnly: false,
IsConst: false,
IsImplicitlyDeclared: false,
Locations.Length: 1,
IsFixedSizeBuffer: false,
} &&
symbol.Type.IsMutableValueType() == false &&
!symbol.GetAttributes().Any(
})
{
return false;
}

if (symbol.Type.IsMutableValueType() != false)
return false;

if (symbol.GetAttributes().Any(
static (a, threadStaticAttribute) => SymbolEqualityComparer.Default.Equals(a.AttributeClass, threadStaticAttribute),
threadStaticAttribute) &&
!IsDataContractSerializable(symbol, dataContractAttribute, dataMemberAttribute);
threadStaticAttribute))
{
return false;
}

if (IsDataContractSerializable(symbol, dataContractAttribute, dataMemberAttribute))
return false;

// The private instance field inside an inline-array is not allowed to be readonly.
if (!symbol.IsStatic && symbol.ContainingType.GetAttributes().Any(
static (a, inlineArrayAttribute) => SymbolEqualityComparer.Default.Equals(a.AttributeClass, inlineArrayAttribute),
inlineArrayAttribute))
{
return false;
}

return true;
}

static bool IsDataContractSerializable(IFieldSymbol symbol, INamedTypeSymbol? dataContractAttribute, INamedTypeSymbol? dataMemberAttribute)
{
Expand All @@ -186,7 +205,7 @@ static bool IsDataContractSerializable(IFieldSymbol symbol, INamedTypeSymbol? da
// Method to update the field state for a candidate field written outside constructor and field initializer.
void UpdateFieldStateOnWrite(IFieldSymbol field)
{
Debug.Assert(IsCandidateField(field, threadStaticAttribute, dataContractAttribute, dataMemberAttribute));
Debug.Assert(IsCandidateField(field));
Debug.Assert(fieldStateMap.ContainsKey(field.OriginalDefinition));

fieldStateMap[field.OriginalDefinition] = (isCandidate: true, written: true);
Expand All @@ -195,35 +214,28 @@ void UpdateFieldStateOnWrite(IFieldSymbol field)
// Method to get or initialize the field state.
(bool isCandidate, bool written) TryGetOrInitializeFieldState(IFieldSymbol fieldSymbol, AnalyzerOptions options, CancellationToken cancellationToken)
{
if (!IsCandidateField(fieldSymbol, threadStaticAttribute, dataContractAttribute, dataMemberAttribute))
{
if (!IsCandidateField(fieldSymbol))
return default;
}

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

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

// Method to compute the initial field state.
(bool isCandidate, bool written) ComputeInitialFieldState(
IFieldSymbol field,
AnalyzerOptions options,
INamedTypeSymbol? threadStaticAttribute,
INamedTypeSymbol? dataContractAttribute,
INamedTypeSymbol? dataMemberAttribute,
CancellationToken cancellationToken)
{
Debug.Assert(IsCandidateField(field, threadStaticAttribute, dataContractAttribute, dataMemberAttribute));
Debug.Assert(IsCandidateField(field));

var option = GetCodeStyleOption(field, options, out var location);
if (option == null
|| !option.Value
|| ShouldSkipAnalysis(location.SourceTree!, options, context.Compilation.Options, option.Notification, cancellationToken))
|| ShouldSkipAnalysis(location.SourceTree!, options, compilation.Options, option.Notification, cancellationToken))
{
return default;
}
Expand Down
32 changes: 19 additions & 13 deletions ...yzers/Core/Analyzers/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ private sealed class CompilationAnalyzer
/// Here, 'get' accessor is used in an increment operation, but the result of the increment operation isn't used and 'P' itself is not used anywhere else, so it can be safely removed
/// </summary>
private readonly HashSet<IPropertySymbol> _propertiesWithShadowGetAccessorUsages = [];
private readonly INamedTypeSymbol? _taskType, _genericTaskType, _debuggerDisplayAttributeType, _structLayoutAttributeType;
private readonly INamedTypeSymbol? _taskType;
private readonly INamedTypeSymbol? _genericTaskType;
private readonly INamedTypeSymbol? _debuggerDisplayAttributeType;
private readonly INamedTypeSymbol? _structLayoutAttributeType;
private readonly INamedTypeSymbol? _inlineArrayAttributeType;
private readonly INamedTypeSymbol? _eventArgsType;
private readonly INamedTypeSymbol? _iNotifyCompletionType;
private readonly DeserializationConstructorCheck _deserializationConstructorCheck;
Expand All @@ -121,6 +125,7 @@ private CompilationAnalyzer(
_genericTaskType = compilation.TaskOfTType();
_debuggerDisplayAttributeType = compilation.DebuggerDisplayAttributeType();
_structLayoutAttributeType = compilation.StructLayoutAttributeType();
_inlineArrayAttributeType = compilation.InlineArrayAttributeType();
_eventArgsType = compilation.EventArgsType();
_iNotifyCompletionType = compilation.GetBestTypeByMetadataName(typeof(INotifyCompletion).FullName!);
_deserializationConstructorCheck = new DeserializationConstructorCheck(compilation);
Expand Down Expand Up @@ -450,36 +455,37 @@ private void OnSymbolEnd(SymbolAnalysisContext symbolEndContext, bool hasUnsuppo
{
var cancellationToken = symbolEndContext.CancellationToken;
if (hasUnsupportedOperation)
{
return;
}

if (symbolEndContext.Symbol.GetAttributes().Any(static (a, self) => a.AttributeClass == self._structLayoutAttributeType, this))
{
// Bail out for types with 'StructLayoutAttribute' as the ordering of the members is critical,
// and removal of unused members might break semantics.
var namedType = (INamedTypeSymbol)symbolEndContext.Symbol;

// Bail out for types with 'StructLayoutAttribute' as the ordering of the members is critical,
// and removal of unused members might break semantics.
if (namedType.GetAttributes().Any(static (a, self) => a.AttributeClass == self._structLayoutAttributeType, this))
return;
}

// Report diagnostics for unused candidate members.
var first = true;

using var _1 = PooledHashSet<ISymbol>.GetInstance(out var symbolsReferencedInDocComments);
using var _2 = ArrayBuilder<string>.GetInstance(out var debuggerDisplayAttributeArguments);

var entryPoint = symbolEndContext.Compilation.GetEntryPoint(cancellationToken);

var namedType = (INamedTypeSymbol)symbolEndContext.Symbol;
var isInlineArray = namedType.GetAttributes().Any(static (a, self) => a.AttributeClass == self._inlineArrayAttributeType, this);

foreach (var member in namedType.GetMembers())
{
if (SymbolEqualityComparer.Default.Equals(entryPoint, member))
{
continue;
}

// The instance field in an InlineArray is required and cannot be removed.
if (isInlineArray && member is IFieldSymbol { IsStatic: false })
continue;

// Check if the underlying member is neither read nor a readable reference to the member is taken.
// If so, we flag the member as either unused (never written) or unread (written but not read).
if (TryRemove(member, out var valueUsageInfo) &&
!valueUsageInfo.IsReadFrom())
if (TryRemove(member, out var valueUsageInfo) && !valueUsageInfo.IsReadFrom())
{
Debug.Assert(IsCandidateSymbol(member));
Debug.Assert(!member.IsImplicitlyDeclared);
Expand Down
4 changes: 1 addition & 3 deletions ...nalyzers/Core/CodeFixes/RemoveUnusedMembers/AbstractRemoveUnusedMembersCodeFixProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,12 @@
// 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;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Editing;
Expand All @@ -22,7 +20,7 @@ namespace Microsoft.CodeAnalysis.RemoveUnusedMembers;
internal abstract class AbstractRemoveUnusedMembersCodeFixProvider<TFieldDeclarationSyntax> : SyntaxEditorBasedCodeFixProvider
where TFieldDeclarationSyntax : SyntaxNode
{
public override ImmutableArray<string> FixableDiagnosticIds
public sealed override ImmutableArray<string> FixableDiagnosticIds
=> [IDEDiagnosticIds.RemoveUnusedMembersDiagnosticId];

/// <summary>
Expand Down
Loading
Loading