diff --git a/src/Analyzers/CSharp/Analyzers/MakeStructMemberReadOnly/CSharpMakeStructMemberReadOnlyAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/MakeStructMemberReadOnly/CSharpMakeStructMemberReadOnlyAnalyzer.cs index 98ff07fff2c12..597ac9492bf3f 100644 --- a/src/Analyzers/CSharp/Analyzers/MakeStructMemberReadOnly/CSharpMakeStructMemberReadOnlyAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/MakeStructMemberReadOnly/CSharpMakeStructMemberReadOnlyAnalyzer.cs @@ -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.Collections.Immutable; using System.Diagnostics.CodeAnalysis; +using System.Linq; using System.Threading; using Microsoft.CodeAnalysis.CodeStyle; using Microsoft.CodeAnalysis.CSharp.CodeStyle; @@ -11,6 +13,7 @@ using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Operations; +using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Extensions; using Roslyn.Utilities; @@ -18,7 +21,8 @@ namespace Microsoft.CodeAnalysis.CSharp.MakeStructMemberReadOnly; [DiagnosticAnalyzer(LanguageNames.CSharp)] internal sealed class CSharpMakeStructMemberReadOnlyDiagnosticAnalyzer() - : AbstractBuiltInCodeStyleDiagnosticAnalyzer(IDEDiagnosticIds.MakeStructMemberReadOnlyDiagnosticId, + : AbstractBuiltInCodeStyleDiagnosticAnalyzer( + IDEDiagnosticIds.MakeStructMemberReadOnlyDiagnosticId, EnforceOnBuildValues.MakeStructMemberReadOnly, CSharpCodeStyleOptions.PreferReadOnlyStructMember, new LocalizableResourceString(nameof(CSharpAnalyzersResources.Make_member_readonly), CSharpAnalyzersResources.ResourceManager, typeof(CSharpAnalyzersResources)), @@ -39,8 +43,13 @@ protected override void InitializeWorker(AnalysisContext context) if (!ShouldAnalyze(context, out var option)) return; + var methodToDiagnostic = PooledDictionary.GetInstance(); + context.RegisterOperationBlockAction( - context => AnalyzeBlock(context, option.Notification.Severity)); + context => AnalyzeBlock(context, option.Notification.Severity, methodToDiagnostic)); + + context.RegisterSymbolEndAction( + context => ProcessResults(context, option.Notification.Severity, methodToDiagnostic)); }, SymbolKind.NamedType); static bool ShouldAnalyze(SymbolStartAnalysisContext context, [NotNullWhen(true)] out CodeStyleOption2? option) @@ -88,11 +97,42 @@ static bool ShouldAnalyze(SymbolStartAnalysisContext context, [NotNullWhen(true) return true; } + + void ProcessResults( + SymbolAnalysisContext context, ReportDiagnostic severity, PooledDictionary methodToDiagnostic) + { + var cancellationToken = context.CancellationToken; + + // No need to lock the dictionary here. Processing only is called once, after all mutation work is done. + foreach (var (method, diagnostic) in methodToDiagnostic) + { + if (method.IsInitOnly && method.AssociatedSymbol is IPropertySymbol owningProperty) + { + // Iff we have an init method that we want to mark as readonly, we can only do so if there is no + // `get` accessor, or if the `get` method is already `readonly` or would determined we want to + // mark as `readonly`. + var getMethodIsReadOnly = + owningProperty.GetMethod is null || + owningProperty.GetMethod.IsReadOnly || + methodToDiagnostic.ContainsKey(owningProperty.GetMethod); + + // Skip marking this property as readonly for this init method if it would conflict with the get method. + if (!getMethodIsReadOnly) + continue; + } + + // normal case + context.ReportDiagnostic(diagnostic); + } + + methodToDiagnostic.Free(); + } }); private void AnalyzeBlock( OperationBlockAnalysisContext context, - ReportDiagnostic severity) + ReportDiagnostic severity, + Dictionary methodToDiagnostic) { var cancellationToken = context.CancellationToken; @@ -115,12 +155,16 @@ private void AnalyzeBlock( return; } - context.ReportDiagnostic(DiagnosticHelper.Create( - Descriptor, - location, - severity, - additionalLocations: ImmutableArray.Create(additionalLocation), - properties: null)); + // Called concurrently. Make sure we write to this dictionary safely. + lock (methodToDiagnostic) + { + methodToDiagnostic[owningMethod] = DiagnosticHelper.Create( + Descriptor, + location, + severity, + additionalLocations: ImmutableArray.Create(additionalLocation), + properties: null); + } } private static (Location? location, Location? additionalLocation) GetDiagnosticLocation( @@ -133,7 +177,17 @@ private static (Location? location, Location? additionalLocation) GetDiagnosticL || owningMethod.IsStatic || owningMethod.IsImplicitlyDeclared) { - return (null, null); + return default; + } + + // An init accessor in a readonly property is already readonly. No need to analyze it. Note: there is no way + // to tell this symbolically. We have to check to the syntax here. + if (owningMethod.IsInitOnly && + owningMethod.AssociatedSymbol is IPropertySymbol { DeclaringSyntaxReferences: [var reference, ..] } && + reference.GetSyntax(cancellationToken) is PropertyDeclarationSyntax property && + property.Modifiers.Any(SyntaxKind.ReadOnlyKeyword)) + { + return default; } var methodReference = owningMethod.DeclaringSyntaxReferences[0]; @@ -153,7 +207,7 @@ private static (Location? location, Location? additionalLocation) GetDiagnosticL declaration = declaration.GetRequiredParent(); if (nameToken is null) - return (null, null); + return default; return (nameToken.Value.GetLocation(), declaration.GetLocation()); } diff --git a/src/Analyzers/CSharp/CodeFixes/MakeStructMemberReadOnly/CSharpMakeStructMemberReadOnlyCodeFixProvider.cs b/src/Analyzers/CSharp/CodeFixes/MakeStructMemberReadOnly/CSharpMakeStructMemberReadOnlyCodeFixProvider.cs index 7139865b35353..a0f158a6e019d 100644 --- a/src/Analyzers/CSharp/CodeFixes/MakeStructMemberReadOnly/CSharpMakeStructMemberReadOnlyCodeFixProvider.cs +++ b/src/Analyzers/CSharp/CodeFixes/MakeStructMemberReadOnly/CSharpMakeStructMemberReadOnlyCodeFixProvider.cs @@ -20,14 +20,10 @@ namespace Microsoft.CodeAnalysis.CSharp.MakeStructMemberReadOnly; [ExportCodeFixProvider(LanguageNames.CSharp, Name = PredefinedCodeFixProviderNames.MakeStructMemberReadOnly), Shared] -internal sealed class CSharpMakeStructMemberReadOnlyCodeFixProvider : SyntaxEditorBasedCodeFixProvider +[method: ImportingConstructor] +[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] +internal sealed class CSharpMakeStructMemberReadOnlyCodeFixProvider() : SyntaxEditorBasedCodeFixProvider { - [ImportingConstructor] - [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] - public CSharpMakeStructMemberReadOnlyCodeFixProvider() - { - } - public override ImmutableArray FixableDiagnosticIds { get; } = ImmutableArray.Create(IDEDiagnosticIds.MakeStructMemberReadOnlyDiagnosticId); diff --git a/src/Analyzers/CSharp/Tests/MakeStructMemberReadOnly/MakeStructMemberReadOnlyTests.cs b/src/Analyzers/CSharp/Tests/MakeStructMemberReadOnly/MakeStructMemberReadOnlyTests.cs index e1a258e0f34b2..166915324fb74 100644 --- a/src/Analyzers/CSharp/Tests/MakeStructMemberReadOnly/MakeStructMemberReadOnlyTests.cs +++ b/src/Analyzers/CSharp/Tests/MakeStructMemberReadOnly/MakeStructMemberReadOnlyTests.cs @@ -2106,4 +2106,143 @@ public void RemoveBit(int candidate) LanguageVersion = LanguageVersion.CSharp12, }.RunAsync(); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70864")] + public async Task TestInitAccessor1() + { + await new VerifyCS.Test + { + TestCode = """ + using System; + + public record struct TypeMapCapacity + { + private readonly int _MaxSize; + + public readonly int? MaxSize + { + get => _MaxSize; + + // missing, since the property is already readonly + init + { + } + } + } + """, + LanguageVersion = LanguageVersion.CSharp12, + ReferenceAssemblies = ReferenceAssemblies.Net.Net70, + }.RunAsync(); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70864")] + public async Task TestInitAccessor2() + { + await new VerifyCS.Test + { + TestCode = """ + using System; + + public record struct TypeMapCapacity + { + private readonly int _MaxSize; + + public int? MaxSize + { + readonly get => _MaxSize; + + [|init|] + { + } + } + } + """, + FixedCode = """ + using System; + + public record struct TypeMapCapacity + { + private readonly int _MaxSize; + + public readonly int? MaxSize + { + get => _MaxSize; + + init + { + } + } + } + """, + LanguageVersion = LanguageVersion.CSharp12, + ReferenceAssemblies = ReferenceAssemblies.Net.Net70, + }.RunAsync(); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70864")] + public async Task TestInitAccessor3() + { + await new VerifyCS.Test + { + TestCode = """ + using System; + + public record struct TypeMapCapacity + { + private int _MaxSize; + + public int? MaxSize + { + get => _MaxSize++; + + init + { + } + } + } + """, + LanguageVersion = LanguageVersion.CSharp12, + ReferenceAssemblies = ReferenceAssemblies.Net.Net70, + }.RunAsync(); + } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/70864")] + public async Task TestInitAccessor4() + { + await new VerifyCS.Test + { + TestCode = """ + using System; + + public record struct TypeMapCapacity + { + private readonly int _MaxSize; + + public int? MaxSize + { + [|init|] + { + } + } + } + """, + FixedCode = """ + using System; + + public record struct TypeMapCapacity + { + private readonly int _MaxSize; + + public readonly int? MaxSize + { + init + { + } + } + } + """, + LanguageVersion = LanguageVersion.CSharp12, + ReferenceAssemblies = ReferenceAssemblies.Net.Net70, + }.RunAsync(); + } }