From 174e4d98d25c2b7da83d7b2a6f79ff5cf5e977e5 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 13 Nov 2024 14:21:18 -0800 Subject: [PATCH 1/2] Fix issue where we were recommending removing the GetEnumerator method used in a foreach --- ...rpRemoveUnusedMembersDiagnosticAnalyzer.cs | 3 ++ .../RemoveUnusedMembersTests.cs | 30 ++++++++++++++- ...ctRemoveUnusedMembersDiagnosticAnalyzer.cs | 38 ++++++++++++------- ...icRemoveUnusedMembersDiagnosticAnalyzer.vb | 3 ++ .../SemanticFacts/CSharpSemanticFacts.cs | 24 +++++------- 5 files changed, 70 insertions(+), 28 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/RemoveUnusedMembers/CSharpRemoveUnusedMembersDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/RemoveUnusedMembers/CSharpRemoveUnusedMembersDiagnosticAnalyzer.cs index 55ed47da767b2..d0c61fa9d6eb2 100644 --- a/src/Analyzers/CSharp/Analyzers/RemoveUnusedMembers/CSharpRemoveUnusedMembersDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/RemoveUnusedMembers/CSharpRemoveUnusedMembersDiagnosticAnalyzer.cs @@ -7,6 +7,7 @@ using System.Threading; using Microsoft.CodeAnalysis.CSharp.Syntax; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.LanguageService; using Microsoft.CodeAnalysis.RemoveUnusedMembers; using Microsoft.CodeAnalysis.Shared.Extensions; @@ -20,6 +21,8 @@ internal sealed class CSharpRemoveUnusedMembersDiagnosticAnalyzer TypeDeclarationSyntax, MemberDeclarationSyntax> { + protected override ISemanticFacts SemanticFacts => CSharpSemanticFacts.Instance; + protected override IEnumerable GetTypeDeclarations(INamedTypeSymbol namedType, CancellationToken cancellationToken) { return namedType.DeclaringSyntaxReferences diff --git a/src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs b/src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs index c06801cfe9adc..52566516aeaee 100644 --- a/src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs +++ b/src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs @@ -15,7 +15,6 @@ namespace Microsoft.CodeAnalysis.Editor.CSharp.UnitTests.RemoveUnusedMembers; -using static Microsoft.CodeAnalysis.CSharp.UsePatternCombinators.AnalyzedPattern; using VerifyCS = CSharpCodeFixVerifier< CSharpRemoveUnusedMembersDiagnosticAnalyzer, CSharpRemoveUnusedMembersCodeFixProvider>; @@ -3243,4 +3242,33 @@ class C await VerifyCS.VerifyCodeFixAsync(code, fixedCode); } + + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/31582")] + public async Task TestForeach() + { + await new VerifyCS.Test + { + TestCode = """ + using System; + using System.Collections; + + static class Program + { + static void Main(string[] args) + { + foreach (var i in 1..10) + Console.WriteLine(i); + } + + static IEnumerator GetEnumerator(this Range range) + { + for (int i = range.Start.Value; i < range.End.Value; i++) + yield return i; + } + } + """, + LanguageVersion = LanguageVersion.CSharp13, + ReferenceAssemblies = ReferenceAssemblies.Net.Net90, + }.RunAsync(); + } } diff --git a/src/Analyzers/Core/Analyzers/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs index 814efb24c087c..e947de09d2524 100644 --- a/src/Analyzers/Core/Analyzers/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/RemoveUnusedMembers/AbstractRemoveUnusedMembersDiagnosticAnalyzer.cs @@ -13,6 +13,7 @@ using Microsoft.CodeAnalysis.CodeQuality; using Microsoft.CodeAnalysis.CodeStyle; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.LanguageService; using Microsoft.CodeAnalysis.Operations; using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Shared.Extensions; @@ -25,8 +26,11 @@ internal abstract class AbstractRemoveUnusedMembersDiagnosticAnalyzer< TDocumentationCommentTriviaSyntax, TIdentifierNameSyntax, TTypeDeclarationSyntax, - TMemberDeclarationSyntax> - : AbstractCodeQualityDiagnosticAnalyzer + TMemberDeclarationSyntax>() + : AbstractCodeQualityDiagnosticAnalyzer( + [s_removeUnusedMembersRule, s_removeUnreadMembersRule], + // We want to analyze references in generated code, but not report unused members in generated code. + GeneratedCodeAnalysisFlags.Analyze) where TDocumentationCommentTriviaSyntax : SyntaxNode where TIdentifierNameSyntax : SyntaxNode where TTypeDeclarationSyntax : TMemberDeclarationSyntax @@ -56,11 +60,7 @@ internal abstract class AbstractRemoveUnusedMembersDiagnosticAnalyzer< new LocalizableResourceString(nameof(AnalyzersResources.Private_member_0_can_be_removed_as_the_value_assigned_to_it_is_never_read), AnalyzersResources.ResourceManager, typeof(AnalyzersResources)), hasAnyCodeStyleOption: false, isUnnecessary: true); - protected AbstractRemoveUnusedMembersDiagnosticAnalyzer() - : base([s_removeUnusedMembersRule, s_removeUnreadMembersRule], - GeneratedCodeAnalysisFlags.Analyze) // We want to analyze references in generated code, but not report unused members in generated code. - { - } + protected abstract ISemanticFacts SemanticFacts { get; } protected abstract IEnumerable GetTypeDeclarations(INamedTypeSymbol namedType, CancellationToken cancellationToken); protected abstract SyntaxList GetMembers(TTypeDeclarationSyntax typeDeclaration); @@ -86,7 +86,8 @@ protected virtual void HandleNamedTypeSymbolStart(SymbolStartAnalysisContext con private sealed class CompilationAnalyzer { - private readonly object _gate; + private readonly object _gate = new(); + /// /// State map for candidate member symbols, with the value indicating how each symbol is used in executable code. /// @@ -114,7 +115,6 @@ private CompilationAnalyzer( Compilation compilation, AbstractRemoveUnusedMembersDiagnosticAnalyzer analyzer) { - _gate = new object(); _analyzer = analyzer; _taskType = compilation.TaskType(); @@ -204,6 +204,7 @@ private void RegisterActions(CompilationStartAnalysisContext compilationStartCon symbolStartContext.RegisterOperationAction(AnalyzeInvocationOperation, OperationKind.Invocation); symbolStartContext.RegisterOperationAction(AnalyzeNameOfOperation, OperationKind.NameOf); symbolStartContext.RegisterOperationAction(AnalyzeObjectCreationOperation, OperationKind.ObjectCreation); + symbolStartContext.RegisterOperationAction(AnalyzeLoopOperation, OperationKind.Loop); // We bail out reporting diagnostics for named types if it contains following kind of operations: // 1. Invalid operations, i.e. erroneous code: @@ -211,8 +212,9 @@ private void RegisterActions(CompilationStartAnalysisContext compilationStartCon // is still editing code and fixing unresolved references to symbols, such as overload resolution errors. // 2. Dynamic operations, where we do not know the exact member being referenced at compile time. // 3. Operations with OperationKind.None. - symbolStartContext.RegisterOperationAction(_ => hasUnsupportedOperation = true, OperationKind.Invalid, OperationKind.None, - OperationKind.DynamicIndexerAccess, OperationKind.DynamicInvocation, OperationKind.DynamicMemberReference, OperationKind.DynamicObjectCreation); + symbolStartContext.RegisterOperationAction( + _ => hasUnsupportedOperation = true, + OperationKind.Invalid, OperationKind.None, OperationKind.DynamicIndexerAccess, OperationKind.DynamicInvocation, OperationKind.DynamicMemberReference, OperationKind.DynamicObjectCreation); symbolStartContext.RegisterSymbolEndAction(symbolEndContext => OnSymbolEnd(symbolEndContext, hasUnsupportedOperation)); @@ -369,6 +371,18 @@ memberReference.Parent is IIncrementOrDecrementOperation || } } + private void AnalyzeLoopOperation(OperationAnalysisContext operationContext) + { + var operation = operationContext.Operation; + if (operation is not IForEachLoopOperation loopOperation) + return; + + var symbols = _analyzer.SemanticFacts.GetForEachSymbols(operation.SemanticModel!, loopOperation.Syntax); + OnSymbolUsage(symbols.CurrentProperty, ValueUsageInfo.Read); + OnSymbolUsage(symbols.GetEnumeratorMethod, ValueUsageInfo.Read); + OnSymbolUsage(symbols.MoveNextMethod, ValueUsageInfo.Read); + } + private void AnalyzeInvocationOperation(OperationAnalysisContext operationContext) { var targetMethod = ((IInvocationOperation)operationContext.Operation).TargetMethod.OriginalDefinition; @@ -380,9 +394,7 @@ private void AnalyzeInvocationOperation(OperationAnalysisContext operationContex // If the invoked method is a reduced extension method, also mark the original // method from which it was reduced as "used". if (targetMethod.ReducedFrom != null) - { OnSymbolUsage(targetMethod.ReducedFrom, ValueUsageInfo.Read); - } } private void AnalyzeNameOfOperation(OperationAnalysisContext operationContext) diff --git a/src/Analyzers/VisualBasic/Analyzers/RemoveUnusedMembers/VisualBasicRemoveUnusedMembersDiagnosticAnalyzer.vb b/src/Analyzers/VisualBasic/Analyzers/RemoveUnusedMembers/VisualBasicRemoveUnusedMembersDiagnosticAnalyzer.vb index 21a7da1fd0061..fdb5b329bf038 100644 --- a/src/Analyzers/VisualBasic/Analyzers/RemoveUnusedMembers/VisualBasicRemoveUnusedMembersDiagnosticAnalyzer.vb +++ b/src/Analyzers/VisualBasic/Analyzers/RemoveUnusedMembers/VisualBasicRemoveUnusedMembersDiagnosticAnalyzer.vb @@ -4,6 +4,7 @@ Imports System.Threading Imports Microsoft.CodeAnalysis.Diagnostics +Imports Microsoft.CodeAnalysis.LanguageService Imports Microsoft.CodeAnalysis.RemoveUnusedMembers Imports Microsoft.CodeAnalysis.VisualBasic.Syntax @@ -16,6 +17,8 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.RemoveUnusedMembers TypeBlockSyntax, StatementSyntax) + Protected Overrides ReadOnly Property SemanticFacts As ISemanticFacts = VisualBasicSemanticFacts.Instance + Protected Overrides Sub HandleNamedTypeSymbolStart(context As SymbolStartAnalysisContext, onSymbolUsageFound As Action(Of ISymbol, ValueUsageInfo)) ' Mark all methods with handles clause as having a read reference ' to ensure that we consider the method as "used". diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SemanticFacts/CSharpSemanticFacts.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SemanticFacts/CSharpSemanticFacts.cs index c76a5a8def8fa..47261897ffdd0 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SemanticFacts/CSharpSemanticFacts.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Services/SemanticFacts/CSharpSemanticFacts.cs @@ -166,22 +166,18 @@ private static void AppendAliasNames(IEnumerable } } - public ForEachSymbols GetForEachSymbols(SemanticModel semanticModel, SyntaxNode forEachStatement) + public ForEachSymbols GetForEachSymbols(SemanticModel semanticModel, SyntaxNode node) { - if (forEachStatement is CommonForEachStatementSyntax csforEachStatement) - { - var info = semanticModel.GetForEachStatementInfo(csforEachStatement); - return new ForEachSymbols( - info.GetEnumeratorMethod, - info.MoveNextMethod, - info.CurrentProperty, - info.DisposeMethod, - info.ElementType); - } - else - { + if (node is not CommonForEachStatementSyntax forEachStatement) return default; - } + + var info = semanticModel.GetForEachStatementInfo(forEachStatement); + return new ForEachSymbols( + info.GetEnumeratorMethod, + info.MoveNextMethod, + info.CurrentProperty, + info.DisposeMethod, + info.ElementType); } public SymbolInfo GetCollectionInitializerSymbolInfo(SemanticModel semanticModel, SyntaxNode node, CancellationToken cancellationToken) From 13cdd19899c7dffa39053f664deb8c5e528c861e Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 13 Nov 2024 14:21:40 -0800 Subject: [PATCH 2/2] Work item --- .../Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs b/src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs index 52566516aeaee..d7ab7dbffa2a7 100644 --- a/src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs +++ b/src/Analyzers/CSharp/Tests/RemoveUnusedMembers/RemoveUnusedMembersTests.cs @@ -3243,7 +3243,7 @@ class C await VerifyCS.VerifyCodeFixAsync(code, fixedCode); } - [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/31582")] + [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/57470")] public async Task TestForeach() { await new VerifyCS.Test