diff --git a/src/Analyzers/CSharp/Tests/UseSystemHashCode/UseSystemHashCodeTests.cs b/src/Analyzers/CSharp/Tests/UseSystemHashCode/UseSystemHashCodeTests.cs index e02cdbfdf1e767485783c3d9afed39b6a259cf75..3eac11a3f1b4919998eae2c9ff6c793aeca40525 100644 --- a/src/Analyzers/CSharp/Tests/UseSystemHashCode/UseSystemHashCodeTests.cs +++ b/src/Analyzers/CSharp/Tests/UseSystemHashCode/UseSystemHashCodeTests.cs @@ -1260,6 +1260,57 @@ public override int GetHashCode() hash.Add(i); return hash.ToHashCode(); } +}"); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseSystemHashCode)] + public async Task TestNotOnSingleReturnedMember() + { + await TestMissingAsync( +@"namespace System { public struct HashCode { } } + +class C +{ + int j; + + public override int $$GetHashCode() + { + return j; + } +}"); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseSystemHashCode)] + public async Task TestNotOnSingleMemberWithInvokedGetHashCode() + { + await TestMissingAsync( +@"namespace System { public struct HashCode { } } + +class C +{ + int j; + + public override int $$GetHashCode() + { + return j.GetHashCode(); + } +}"); + } + + [Fact, Trait(Traits.Feature, Traits.Features.CodeActionsUseSystemHashCode)] + public async Task TestNotOnSimpleBaseReturn() + { + await TestMissingAsync( +@"namespace System { public struct HashCode { } } + +class C +{ + int j; + + public override int $$GetHashCode() + { + return base.GetHashCode(); + } }"); } } diff --git a/src/Analyzers/Core/Analyzers/UseSystemHashCode/Analyzer.cs b/src/Analyzers/Core/Analyzers/UseSystemHashCode/Analyzer.cs index 664007f961077a1033775faecaf09fdcf6cef4eb..a3a4630c7651fc015b2f441ff18c6b55b442e90b 100644 --- a/src/Analyzers/Core/Analyzers/UseSystemHashCode/Analyzer.cs +++ b/src/Analyzers/Core/Analyzers/UseSystemHashCode/Analyzer.cs @@ -58,34 +58,23 @@ public static bool TryGetAnalyzer(Compilation compilation, [NotNullWhen(true)] o /// Analyzes the containing GetHashCode method to determine which fields and /// properties were combined to form a hash code for this type. /// - public (bool accessesBase, ImmutableArray members) GetHashedMembers(ISymbol owningSymbol, IOperation? operation) + public (bool accessesBase, ImmutableArray members, ImmutableArray statements) GetHashedMembers(ISymbol owningSymbol, IOperation? operation) { if (!(operation is IBlockOperation blockOperation)) - { return default; - } // Owning symbol has to be an override of Object.GetHashCode. if (!(owningSymbol is IMethodSymbol { Name: nameof(GetHashCode) } method)) - { return default; - } - if (method.Locations.Length != 1 || - method.DeclaringSyntaxReferences.Length != 1) - { + if (method.Locations.Length != 1 || method.DeclaringSyntaxReferences.Length != 1) return default; - } if (!method.Locations[0].IsInSource) - { return default; - } if (!OverridesSystemObject(method)) - { return default; - } // Unwind through nested blocks. This also handles if we're in an 'unchecked' block in C# while (blockOperation.Operations.Length == 1 && @@ -95,9 +84,12 @@ public static bool TryGetAnalyzer(Compilation compilation, [NotNullWhen(true)] o } var statements = blockOperation.Operations.WhereAsArray(o => !o.IsImplicit); - return MatchAccumulatorPattern(method, statements) ?? - MatchTuplePattern(method, statements) ?? - default; + var (accessesBase, members) = + MatchAccumulatorPattern(method, statements) ?? + MatchTuplePattern(method, statements) ?? + default; + + return (accessesBase, members, statements); } private (bool accessesBase, ImmutableArray members)? MatchTuplePattern( diff --git a/src/Analyzers/Core/Analyzers/UseSystemHashCode/UseSystemHashCodeDiagnosticAnalyzer.cs b/src/Analyzers/Core/Analyzers/UseSystemHashCode/UseSystemHashCodeDiagnosticAnalyzer.cs index 954d4be6f54284ea3625f605852b75ed6ba0b3da..8b78df06a91c9cd93789f573a349f89721c438f0 100644 --- a/src/Analyzers/Core/Analyzers/UseSystemHashCode/UseSystemHashCodeDiagnosticAnalyzer.cs +++ b/src/Analyzers/Core/Analyzers/UseSystemHashCode/UseSystemHashCodeDiagnosticAnalyzer.cs @@ -5,6 +5,7 @@ #nullable enable using System.Collections.Immutable; +using System.Diagnostics; using Microsoft.CodeAnalysis.CodeStyle; using Microsoft.CodeAnalysis.Diagnostics; @@ -39,26 +40,37 @@ protected override void InitializeWorker(AnalysisContext context) private void AnalyzeOperationBlock(Analyzer analyzer, OperationBlockAnalysisContext context) { if (context.OperationBlocks.Length != 1) - { return; - } var owningSymbol = context.OwningSymbol; var operation = context.OperationBlocks[0]; - var (accessesBase, hashedMembers) = analyzer.GetHashedMembers(owningSymbol, operation); - if (!accessesBase && hashedMembers.IsDefaultOrEmpty) - { + var (accessesBase, hashedMembers, statements) = analyzer.GetHashedMembers(owningSymbol, operation); + var elementCount = (accessesBase ? 1 : 0) + (hashedMembers.IsDefaultOrEmpty ? 0 : hashedMembers.Length); + + // No members to call into HashCode.Combine with. Don't offer anything here. + if (elementCount == 0) return; - } + + // Just one member to call into HashCode.Combine. Only offer this if we have multiple statements that we can + // reduce to a single statement. It's not worth it to offer to replace: + // + // `return x.GetHashCode();` with `return HashCode.Combine(x);` + // + // But it is work it to offer to replace: + // + // `return (a, b).GetHashCode();` with `return HashCode.Combine(a, b);` + if (elementCount == 1 && statements.Length < 2) + return; + + // We've got multiple members to hash, or multiple statements that can be reduced at this point. + Debug.Assert(elementCount >= 2 || statements.Length >= 2); var syntaxTree = operation.Syntax.SyntaxTree; var cancellationToken = context.CancellationToken; var option = context.Options.GetOption(CodeStyleOptions2.PreferSystemHashCode, operation.Language, syntaxTree, cancellationToken); if (option?.Value != true) - { return; - } var operationLocation = operation.Syntax.GetLocation(); var declarationLocation = context.OwningSymbol.DeclaringSyntaxReferences[0].GetSyntax(cancellationToken).GetLocation(); diff --git a/src/Analyzers/Core/CodeFixes/UseSystemHashCode/UseSystemHashCodeCodeFixProvider.cs b/src/Analyzers/Core/CodeFixes/UseSystemHashCode/UseSystemHashCodeCodeFixProvider.cs index 8a80679b5b5b8e26d528011196d985cde218248d..ef0bf746d86015c6796d6c37312090745e19276d 100644 --- a/src/Analyzers/Core/CodeFixes/UseSystemHashCode/UseSystemHashCodeCodeFixProvider.cs +++ b/src/Analyzers/Core/CodeFixes/UseSystemHashCode/UseSystemHashCodeCodeFixProvider.cs @@ -74,7 +74,7 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context) var method = semanticModel.GetDeclaredSymbol(methodDecl, cancellationToken); var methodBlock = declarationService.GetDeclarations(method)[0].GetSyntax(cancellationToken); - var (accessesBase, members) = analyzer.GetHashedMembers(method, operation); + var (accessesBase, members, _) = analyzer.GetHashedMembers(method, operation); if (accessesBase || !members.IsDefaultOrEmpty) { // Produce the new statements for the GetHashCode method and replace the diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs index c709a5148290e20b8b596875cea327812ed4ca9e..94f7908fc23d8a0d181607748222e720ef4b7133 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Utilities/SpeculationAnalyzer.cs @@ -426,6 +426,11 @@ protected override bool ReplacementChangesSemanticsForNodeLanguageSpecific(Synta // expression type are not broken. var newSwitchStatement = (SwitchStatementSyntax)currentReplacedNode; + var previousReplacedExpression = (ExpressionSyntax)previousReplacedNode; + + // it is never legal to use `default/null` in a switch statement's expression. + if (previousReplacedExpression.WalkDownParentheses().IsKind(SyntaxKind.NullLiteralExpression, SyntaxKind.DefaultLiteralExpression)) + return true; var originalSwitchLabels = originalSwitchStatement.Sections.SelectMany(section => section.Labels).ToArray(); var newSwitchLabels = newSwitchStatement.Sections.SelectMany(section => section.Labels).ToArray();