From 78d78e92fc4f3a2619540eeac68ba61c9c57810e Mon Sep 17 00:00:00 2001 From: NewellClark Date: Mon, 17 May 2021 19:18:09 -0400 Subject: [PATCH 01/10] Create files and tests --- .../Core/AnalyzerReleases.Unshipped.md | 1 + .../MicrosoftNetCoreAnalyzersResources.resx | 14 +- .../UseStringEqualsOverStringCompare.Fixer.cs | 24 ++ .../UseStringEqualsOverStringCompare.cs | 42 +++ .../MicrosoftNetCoreAnalyzersResources.cs.xlf | 20 ++ .../MicrosoftNetCoreAnalyzersResources.de.xlf | 20 ++ .../MicrosoftNetCoreAnalyzersResources.es.xlf | 20 ++ .../MicrosoftNetCoreAnalyzersResources.fr.xlf | 20 ++ .../MicrosoftNetCoreAnalyzersResources.it.xlf | 20 ++ .../MicrosoftNetCoreAnalyzersResources.ja.xlf | 20 ++ .../MicrosoftNetCoreAnalyzersResources.ko.xlf | 22 +- .../MicrosoftNetCoreAnalyzersResources.pl.xlf | 20 ++ ...crosoftNetCoreAnalyzersResources.pt-BR.xlf | 20 ++ .../MicrosoftNetCoreAnalyzersResources.ru.xlf | 22 +- .../MicrosoftNetCoreAnalyzersResources.tr.xlf | 20 ++ ...osoftNetCoreAnalyzersResources.zh-Hans.xlf | 20 ++ ...osoftNetCoreAnalyzersResources.zh-Hant.xlf | 20 ++ .../UseStringEqualsOverStringCompareTests.cs | 280 ++++++++++++++++++ .../DiagnosticCategoryAndIdRanges.txt | 2 +- 19 files changed, 623 insertions(+), 4 deletions(-) create mode 100644 src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs create mode 100644 src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs create mode 100644 src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs diff --git a/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md b/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md index 1264f0f948..27991d470e 100644 --- a/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md +++ b/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md @@ -8,6 +8,7 @@ CA1840 | Performance | Info | UseEnvironmentMembers, [Documentation](https://doc CA1841 | Performance | Info | PreferDictionaryContainsMethods, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1841) CA1842 | Performance | Info | DoNotUseWhenAllOrWaitAllWithSingleArgument, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1842) CA1843 | Performance | Info | DoNotUseWhenAllOrWaitAllWithSingleArgument, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1843) +CA2250 | Usage | Hidden | UseStringEqualsOverStringCompare, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2250) ### Removed Rules diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx index 55b025f92e..6949f710be 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/MicrosoftNetCoreAnalyzersResources.resx @@ -1589,4 +1589,16 @@ Replace 'WhenAll' call with argument - + + Use 'string.Equals' + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + Use 'string.Equals' + + \ No newline at end of file diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs new file mode 100644 index 0000000000..769bddc998 --- /dev/null +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs @@ -0,0 +1,24 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Composition; +using System.Text; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeFixes; + +namespace Microsoft.NetCore.Analyzers.Runtime +{ + [ExportCodeFixProvider(LanguageNames.CSharp, LanguageNames.VisualBasic), Shared] + public sealed class UseStringEqualsOverStringCompareFixer : CodeFixProvider + { + public override Task RegisterCodeFixesAsync(CodeFixContext context) + { + return Task.CompletedTask; + } + + public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Empty; + } +} diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs new file mode 100644 index 0000000000..be703b848b --- /dev/null +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs @@ -0,0 +1,42 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Text; +using Analyzer.Utilities; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; + +using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources; + +namespace Microsoft.NetCore.Analyzers.Runtime +{ + [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] + public sealed class UseStringEqualsOverStringCompare : DiagnosticAnalyzer + { + internal const string RuleId = "CA2250"; + + private static readonly LocalizableString s_localizableTitle = new LocalizableResourceString(nameof(Resx.UseStringEqualsOverStringCompareTitle), Resx.ResourceManager, typeof(Resx)); + private static readonly LocalizableString s_localizableMessage = new LocalizableResourceString(nameof(Resx.UseStringEqualsOverStringCompareMessage), Resx.ResourceManager, typeof(Resx)); + private static readonly LocalizableString s_localizableDescription = new LocalizableResourceString(nameof(Resx.UseStringEqualsOverStringCompareDescription), Resx.ResourceManager, typeof(Resx)); + + internal static readonly DiagnosticDescriptor Rule = DiagnosticDescriptorHelper.Create( + RuleId, + s_localizableTitle, + s_localizableMessage, + DiagnosticCategory.Usage, + RuleLevel.IdeHidden_BulkConfigurable, + s_localizableDescription, + isPortedFxCopRule: false, + isDataflowRule: false); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + } + } +} diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf index 3cd6a9d14d..794ce1a5bc 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.cs.xlf @@ -2342,6 +2342,26 @@ Pokud je to možné, zvažte použití řízení přístupu Azure na základě role namísto sdíleného přístupového podpisu (SAS). Pokud i přesto potřebujete používat sdílený přístupový podpis, zadejte SharedAccessProtocol.HttpsOnly. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.de.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.de.xlf index 6a2fce2f48..5c5613b14c 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.de.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.de.xlf @@ -2342,6 +2342,26 @@ Erwägen Sie (sofern möglich) die Verwendung der rollenbasierten Zugriffssteuerung von Azure anstelle einer Shared Access Signature (SAS). Wenn Sie weiterhin eine SAS benötigen, verwenden Sie "SharedAccessProtocol.HttpsOnly". + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.es.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.es.xlf index 8a831cdcde..b40fbd70f8 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.es.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.es.xlf @@ -2342,6 +2342,26 @@ Considere la posibilidad de usar el control de acceso basado en rol de Azure en lugar de una firma de acceso compartido (SAS), si es posible. Si tiene que usar una firma de acceso compartido, especifique SharedAccessProtocol.HttpsOnly. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.fr.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.fr.xlf index fa9bdc5528..4e38dc766a 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.fr.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.fr.xlf @@ -2342,6 +2342,26 @@ Si possible, utilisez le contrôle d'accès en fonction du rôle d'Azure à la place d'une signature d'accès partagé. Si vous devez quand même utiliser une signature d'accès partagé, spécifiez SharedAccessProtocol.HttpsOnly. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.it.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.it.xlf index 3bd218ea1d..e4b454bb3b 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.it.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.it.xlf @@ -2342,6 +2342,26 @@ Se possibile, provare a usare il controllo degli accessi in base al ruolo di Azure, invece della firma di accesso condiviso. Se è necessario usare una firma di accesso condiviso, specificare SharedAccessProtocol.HttpsOnly. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ja.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ja.xlf index a3d2bc2036..90c99c061a 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ja.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ja.xlf @@ -2342,6 +2342,26 @@ 可能な場合は、Shared Access Signature (SAS) の代わりに、Azure のロールベースのアクセス制御を使用することを検討してください。依然として SAS を使用する必要がある場合は、SharedAccessProtocol.HttpsOnly を指定します。 + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ko.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ko.xlf index 5d1a3758a1..f9d8fd35ac 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ko.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ko.xlf @@ -2342,6 +2342,26 @@ 가능한 경우 SAS(공유 액세스 서명) 대신 Azure의 역할 기반 액세스 제어를 사용하는 것이 좋습니다. 계속 SAS를 사용해야 하는 경우 SharedAccessProtocol.HttpsOnly를 지정하세요. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. @@ -2434,4 +2454,4 @@ - + \ No newline at end of file diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pl.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pl.xlf index 44d9d287e9..fcccf6d82e 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pl.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pl.xlf @@ -2342,6 +2342,26 @@ Jeśli to możliwe, rozważ użycie kontroli dostępu opartej na rolach platformy Azure zamiast sygnatury dostępu współdzielonego (SAS). Jeśli nadal chcesz używać sygnatury SAS, określ właściwość SharedAccessProtocol.HttpsOnly. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pt-BR.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pt-BR.xlf index 5983d573c6..2948d07d25 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pt-BR.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.pt-BR.xlf @@ -2342,6 +2342,26 @@ Se possível, considere usar o controle de acesso baseado em função do Azure em vez de uma SAS (Assinatura de Acesso Compartilhado). Se você ainda precisar usar uma SAS, especifique SharedAccessProtocol.HttpsOnly. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ru.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ru.xlf index cf40196173..3d67908d59 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ru.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.ru.xlf @@ -2342,6 +2342,26 @@ Если возможно, попробуйте использовать управление доступом на основе ролей Azure, а не подписанный URL-адрес (SAS). Если все-таки требуется использовать SAS, укажите SharedAccessProtocol.HttpsOnly. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. @@ -2434,4 +2454,4 @@ - + \ No newline at end of file diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.tr.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.tr.xlf index 274516ad9b..426a0a7d1b 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.tr.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.tr.xlf @@ -2342,6 +2342,26 @@ Mümkünse Paylaşılan Erişim İmzası (SAS) yerine Azure'un rol tabanlı erişim denetimini kullanmayı düşünün. Yine de SAS kullanmanız gerekiyorsa, SharedAccessProtocol.HttpsOnly belirtin. + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hans.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hans.xlf index f38b9c26e3..58d3804971 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hans.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hans.xlf @@ -2342,6 +2342,26 @@ 如果可能，请考虑使用 Azure 基于角色的访问控制，而不是共享访问签名(SAS)。如果仍需使用 SAS，请指定 SharedAccessProtocol.HttpsOnly。 + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hant.xlf b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hant.xlf index 4091a82ba8..8add97f4df 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hant.xlf +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/xlf/MicrosoftNetCoreAnalyzersResources.zh-Hant.xlf @@ -2342,6 +2342,26 @@ 如果可行的話，請考慮從共用存取簽章 (SAS) 改為使用 Azure 的角色型存取控制。如果您仍需要使用 SAS，請指定 SharedAccessProtocol.HttpsOnly。 + + Use 'string.Equals' + Use 'string.Equals' + + + + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + + + + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + Use 'string.Equals' instead of comparing the result of 'string.Compare' to 0 + + + + Use 'string.Equals' + Use 'string.Equals' + + Platform compatibility analyzer requires a valid platform name and version. Platform compatibility analyzer requires a valid platform name and version. diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs new file mode 100644 index 0000000000..d63421b9d3 --- /dev/null +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs @@ -0,0 +1,280 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System; +using System.Collections.Generic; +using System.Globalization; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Xunit; + +using VerifyCS = Test.Utilities.CSharpCodeFixVerifier< + Microsoft.NetCore.Analyzers.Runtime.UseStringEqualsOverStringCompare, + Microsoft.NetCore.Analyzers.Runtime.UseStringEqualsOverStringCompareFixer>; +using VerifyVB = Test.Utilities.VisualBasicCodeFixVerifier< + Microsoft.NetCore.Analyzers.Runtime.UseStringEqualsOverStringCompare, + Microsoft.NetCore.Analyzers.Runtime.UseStringEqualsOverStringCompareFixer>; + +namespace Microsoft.NetCore.Analyzers.Runtime.UnitTests +{ + public class UseStringEqualsOverStringCompareTests + { + #region Test Data + private static DiagnosticDescriptor Rule => UseStringEqualsOverStringCompare.Rule; + + private static readonly (string CompareCall, string EqualsCall)[] CS_ComparisonEqualityMethodCallPairs = new[] + { + ("string.Compare(x, y)", "string.Equals(x, y)"), + ("string.Compare(x, y, false)", "string.Equals(x, y, StringComparison.CurrentCulture)"), + ("string.Compare(x, y, true)", "string.Equals(x, y, StringComparison.CurrentCultureIgnoreCase)"), + ("string.Compare(x, y, StringComparison.CurrentCulture)", "string.Equals(x, y, StringComparison.CurrentCulture)"), + ("string.Compare(x, y, StringComparison.Ordinal)", "string.Equals(x, y, StringComparison.Ordinal)"), + ("string.Compare(x, y, StringComparison.OrdinalIgnoreCase)", "string.Equals(x, y, StringComparison.OrdinalIgnoreCase)"), + }; + + private static readonly (string CompareCall, string EqualsCall)[] VB_ComparisonEqualityMethodPairs = new[] + { + ("String.Compare(x, y)", "String.Equals(x, y)"), + ("String.Compare(x, y, false)", "String.Equals(x, y, StringComparison.CurrentCulture)"), + ("String.Compare(x, y, true)", "String.Equals(x, y, StringComparison.CurrentCultureIgnoreCase)"), + ("String.Compare(x, y, StringComparison.CurrentCulture)", "String.Equals(x, y, StringComparison.CurrentCulture)"), + ("String.Compare(x, y, StringComparison.Ordinal)", "String.Equals(x, y, StringComparison.Ordinal)"), + ("String.Compare(x, y, StringComparison.OrdinalIgnoreCase)", "String.Equals(x, y, StringComparison.OrdinalIgnoreCase)"), + }; + + public static IEnumerable CS_ComparisonLeftOfLiteralTestData { get; } = CS_ComparisonEqualityMethodCallPairs + .Select(pair => new object[] { $"{pair.CompareCall} == 0", pair.EqualsCall }); + + public static IEnumerable VB_ComparisonLeftOfLiteralTestData { get; } = VB_ComparisonEqualityMethodPairs + .Select(pair => new object[] { $"{pair.CompareCall} = 0", pair.EqualsCall }); + + public static IEnumerable CS_ComparisonRightOfLiteralTestData { get; } = CS_ComparisonEqualityMethodCallPairs + .Select(pair => new object[] { $"0 == {pair.CompareCall}", pair.EqualsCall }); + + public static IEnumerable VB_ComparisonRightOfLiteralTestData { get; } = VB_ComparisonEqualityMethodPairs + .Select(pair => new object[] { $"0 = {pair.CompareCall}", pair.EqualsCall }); + + public static IEnumerable CS_InvertedComparisonLeftOfLiteralTestData { get; } = CS_ComparisonEqualityMethodCallPairs + .Select(pair => new object[] { $"{pair.CompareCall} != 0", $"!{pair.EqualsCall}" }); + + public static IEnumerable VB_InvertedComparisonLeftOfLiteralTestData { get; } = VB_ComparisonEqualityMethodPairs + .Select(pair => new object[] { $"{pair.CompareCall} <> 0", $"Not {pair.EqualsCall}" }); + + public static IEnumerable CS_InvertedComparisonRightOfLiteralTestData { get; } = CS_ComparisonEqualityMethodCallPairs + .Select(pair => new object[] { $"0 != {pair.CompareCall}", $"!{pair.EqualsCall}" }); + + public static IEnumerable VB_InvertedComparisonRightOfLiteralTestData { get; } = VB_ComparisonEqualityMethodPairs + .Select(pair => new object[] { $"0 <> {pair.CompareCall}", $"Not {pair.EqualsCall}" }); + + public static IEnumerable CS_StringCompareExpressionsTestData { get; } = CS_ComparisonEqualityMethodCallPairs + .Select(pair => new object[] { pair.CompareCall }); + + public static IEnumerable VB_StringCompareExpressionsTestData { get; } = VB_ComparisonEqualityMethodPairs + .Select(pair => new object[] { pair.CompareCall }); + + public static IEnumerable CS_IneligibleStringCompareOverloadTestData + { + get + { + yield return new[] { "string.Compare(x, y, true, System.Globalization.CultureInfo.InvariantCulture)" }; + yield return new[] { "string.Compare(x, y, System.Globalization.CultureInfo.InvariantCulture, System.Globalization.CompareOptions.None)" }; + } + } + + public static IEnumerable VB_IneligibleStringCompareOverloadTestData + { + get + { + yield return new[] { "String.Compare(x, y, true, System.Globalization.CultureInfo.InvariantCulture)" }; + yield return new[] { "String.Compare(x, y, System.Globalization.CultureInfo.InvariantCulture, System.Globalization.CompareOptions.None)" }; + } + } + + #endregion + + [Theory] + [MemberData(nameof(CS_ComparisonLeftOfLiteralTestData))] + [MemberData(nameof(CS_ComparisonRightOfLiteralTestData))] + [MemberData(nameof(CS_InvertedComparisonLeftOfLiteralTestData))] + [MemberData(nameof(CS_InvertedComparisonRightOfLiteralTestData))] + public Task StringCompareResult_CompareToZero_Diagnostic_CS(string testExpression, string fixedExpression) + { + string testCode = $@" +using System; + +public class Testopolis +{{ + public bool Huh(string x, string y) + {{ + return {{|#0:{testExpression}|}}; + }} +}}"; + string fixedCode = $@" +using System; + +public class Testopolis +{{ + public bool Huh(string x, string y) + {{ + return {fixedExpression}; + }} +}}"; + + return VerifyCS.VerifyCodeFixAsync(testCode, VerifyCS.Diagnostic(Rule).WithLocation(0), fixedCode); + } + + [Theory] + [MemberData(nameof(VB_ComparisonLeftOfLiteralTestData))] + [MemberData(nameof(VB_ComparisonRightOfLiteralTestData))] + [MemberData(nameof(VB_InvertedComparisonLeftOfLiteralTestData))] + [MemberData(nameof(VB_InvertedComparisonRightOfLiteralTestData))] + public Task StringCompareResult_CompareToZero_Diagnostic_VB(string testExpression, string fixedExpression) + { + string testCode = $@" +Imports System + +Public Class Testopolis + + Public Function Huh(x As String, y As String) As Boolean + Return {{|#0:{testExpression}|}} + End Function +End Class"; + string fixedCode = $@" +Imports System + +Public Class Testopolis + + Public Function Huh(x As String, y As String) As Boolean + Return {fixedExpression} + End Function +End Class"; + + return VerifyVB.VerifyCodeFixAsync(testCode, VerifyVB.Diagnostic(Rule).WithLocation(0), fixedCode); + } + + [Theory] + [MemberData(nameof(CS_StringCompareExpressionsTestData))] + public Task StringCompareResult_CompareToNonLiteralZero_NoDiagnostic_CS(string expression) + { + string code = $@" +using System; + +public class Testopolis +{{ + private const int Zero = 0; + + public void Method(string x, string y) + {{ + bool a = {expression} == Zero; + bool b = {expression} != Zero; + bool c = Zero == {expression}; + bool d = Zero != {expression}; + }} +}}"; + + return VerifyCS.VerifyAnalyzerAsync(code); + } + + [Theory] + [MemberData(nameof(VB_StringCompareExpressionsTestData))] + public Task StringCompareResult_CompareToNonLiteralZero_NoDiagnostic_VB(string expression) + { + string code = $@" +Imports System + +Public Class Testopolis + Private Const Zero As Integer = 0 + + Public Sub Method(x As String, y As String) As Boolean + Dim a = {expression} = Zero + Dim b = {expression} <> Zero + Dim c = Zero = {expression} + Dim d = Zero <> {expression} + End Sub +End Class"; + + return VerifyVB.VerifyAnalyzerAsync(code); + } + + [Theory] + [MemberData(nameof(CS_StringCompareExpressionsTestData))] + public Task StringCompareResult_CompareToLiteralNonZero_NoDiagnostic_CS(string expression) + { + string code = $@" +using System; + +public class Testopolis +{{ + public void Method(string x, string y) + {{ + bool a = {expression} == 1; + bool b = {expression} != 1; + bool c = 1 == {expression}; + bool d = 1 != {expression}; + }} +}}"; + + return VerifyCS.VerifyAnalyzerAsync(code); + } + + [Theory] + [MemberData(nameof(VB_StringCompareExpressionsTestData))] + public Task StringCompareResult_CompareToLiteralNonZero_NoDiagnostic_VB(string expression) + { + string code = $@" +Imports System + +Public Class Testopolis + Public Sub Method(x As String, y As String) + Dim a = {expression} = 1 + Dim b = {expression} <> 1 + Dim c = 1 = {expression} + Dim d = 1 <> {expression} + End Sub +End Class"; + + return VerifyVB.VerifyAnalyzerAsync(code); + } + + [Theory] + [MemberData(nameof(CS_IneligibleStringCompareOverloadTestData))] + public Task IneligibleStringCompareOverload_NoDiagnostic_CS(string expression) + { + string code = $@" +using System; + +public class Testopolis +{{ + public void Method(string x, string y) + {{ + bool a = {expression} == 0; + bool b = {expression} != 0; + bool c = 0 == {expression}; + bool d = 0 != {expression}; + }} +}}"; + + return VerifyCS.VerifyAnalyzerAsync(code); + } + + [Theory] + [MemberData(nameof(VB_IneligibleStringCompareOverloadTestData))] + public Task IneligibleStringCompareOverload_NoDiagnostic_VB(string expression) + { + string code = $@" +Imports System + +Public Class Testopolis + Public Sub Method(x As String, y As String) + Dim a = {expression} = 0 + Dim b = {expression} <> 0 + Dim c = 0 = {expression} + Dim d = 0 <> {expression} + End Sub +End Class"; + + return VerifyVB.VerifyAnalyzerAsync(code); + } + } +} diff --git a/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt b/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt index 15b72db84b..55a6b04b07 100644 --- a/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt +++ b/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt @@ -14,7 +14,7 @@ Globalization: CA2101, CA1300-CA1310 Mobility: CA1600-CA1601 Performance: HA, CA1800-CA1843 Security: CA2100-CA2153, CA2300-CA2330, CA3000-CA3147, CA5300-CA5403 -Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2249 +Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2250 Naming: CA1700-CA1726 Interoperability: CA1400-CA1418 Maintainability: CA1500-CA1509 From c3408c73478dbc8f2c30910203f638481f69792e Mon Sep 17 00:00:00 2001 From: NewellClark Date: Tue, 18 May 2021 21:39:33 -0400 Subject: [PATCH 02/10] Implement analyzer and fixer Add GetFirstOrDefaultMemberWithParameterTypes to utilities --- .../UseStringEqualsOverStringCompare.Fixer.cs | 44 ++- .../UseStringEqualsOverStringCompare.cs | 331 +++++++++++++++++- .../UseStringEqualsOverStringCompareTests.cs | 4 +- .../IEnumerableOfIMethodSymbolExtensions.cs | 28 ++ 4 files changed, 396 insertions(+), 11 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs index 769bddc998..51d15f4a38 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs @@ -1,24 +1,56 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; -using System.Collections.Generic; using System.Collections.Immutable; using System.Composition; -using System.Text; using System.Threading.Tasks; using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Operations; + +using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources; namespace Microsoft.NetCore.Analyzers.Runtime { [ExportCodeFixProvider(LanguageNames.CSharp, LanguageNames.VisualBasic), Shared] public sealed class UseStringEqualsOverStringCompareFixer : CodeFixProvider { - public override Task RegisterCodeFixesAsync(CodeFixContext context) + public override async Task RegisterCodeFixesAsync(CodeFixContext context) { - return Task.CompletedTask; + var document = context.Document; + var token = context.CancellationToken; + var semanticModel = await document.GetSemanticModelAsync(token).ConfigureAwait(false); + + if (!UseStringEqualsOverStringCompare.RequiredSymbols.TryGetSymbols(semanticModel.Compilation, out var symbols)) + return; + + var root = await document.GetSyntaxRootAsync(token).ConfigureAwait(false); + + if (semanticModel.GetOperation(root.FindNode(context.Span, getInnermostNodeForTie: true), token) is not IBinaryOperation operation) + return; + + var selectors = UseStringEqualsOverStringCompare.GetSelectors(symbols); + + foreach (var selector in selectors) + { + if (selector.IsMatch(operation)) + { + var codeAction = CodeAction.Create( + Resx.UseStringEqualsOverStringCompareCodeFixTitle, + async token => + { + var editor = await DocumentEditor.CreateAsync(document, token).ConfigureAwait(false); + var replacementNode = selector.GetReplacementExpression(operation, editor.Generator); + editor.ReplaceNode(operation.Syntax, replacementNode); + return editor.GetChangedDocument(); + }, Resx.UseStringEqualsOverStringCompareCodeFixTitle); + context.RegisterCodeFix(codeAction, context.Diagnostics); + break; + } + } } - public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Empty; + public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(UseStringEqualsOverStringCompare.RuleId); } } diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs index be703b848b..f932a79a23 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs @@ -1,13 +1,16 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Collections.Generic; using System.Collections.Immutable; -using System.Text; +using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; +using System.Linq; using Analyzer.Utilities; +using Analyzer.Utilities.Extensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; - +using Microsoft.CodeAnalysis.Editing; +using Microsoft.CodeAnalysis.Operations; using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources; namespace Microsoft.NetCore.Analyzers.Runtime @@ -37,6 +40,328 @@ public override void Initialize(AnalysisContext context) { context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); context.EnableConcurrentExecution(); + + context.RegisterCompilationStartAction(context => + { + if (!RequiredSymbols.TryGetSymbols(context.Compilation, out var symbols)) + return; + + var selectors = GetSelectors(symbols); + + context.RegisterOperationAction(context => + { + foreach (var selector in selectors) + { + if (selector.IsMatch(context.Operation)) + { + var diagnostic = context.Operation.CreateDiagnostic(Rule); + context.ReportDiagnostic(diagnostic); + break; + } + } + }, OperationKind.Binary); + }); + } + + internal static ImmutableArray GetSelectors(RequiredSymbols symbols) + { + return ImmutableArray.Create( + new StringStringSelector(symbols), + new StringStringBoolSelector(symbols), + new StringStringStringComparisonSelector(symbols)); + } + + internal sealed class RequiredSymbols + { + // Named-constructor 'TryGetSymbols' inits all properties or doesn't construct an instance. +#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable. + private RequiredSymbols() { } +#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable. + + public static bool TryGetSymbols(Compilation compilation, [NotNullWhen(true)] out RequiredSymbols? symbols) + { + symbols = default; + + var stringType = compilation.GetSpecialType(SpecialType.System_String); + var boolType = compilation.GetSpecialType(SpecialType.System_Boolean); + + if (stringType is null || boolType is null) + return false; + + if (!compilation.TryGetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemStringComparison, out var stringComparisonType)) + return false; + + var compareMethods = stringType.GetMembers(nameof(string.Compare)) + .OfType() + .Where(x => x.IsStatic); + var compareStringString = compareMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType); + var compareStringStringBool = compareMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType, boolType); + var compareStringStringStringComparison = compareMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType, stringComparisonType); + + if (compareStringString is null || + compareStringStringBool is null || + compareStringStringStringComparison is null) + { + return false; + } + + var equalsMethods = stringType.GetMembers(nameof(string.Equals)) + .OfType() + .Where(x => x.IsStatic); + var equalsStringString = equalsMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType); + var equalsStringStringStringComparison = equalsMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType, stringComparisonType); + + if (equalsStringString is null || + equalsStringStringStringComparison is null) + { + return false; + } + + symbols = new RequiredSymbols + { + StringType = stringType, + BoolType = boolType, + StringComparisonType = stringComparisonType, + CompareStringString = compareStringString, + CompareStringStringBool = compareStringStringBool, + CompareStringStringStringComparison = compareStringStringStringComparison, + EqualsStringString = equalsStringString, + EqualsStringStringStringComparison = equalsStringStringStringComparison + }; + return true; + } + + public INamedTypeSymbol StringType { get; init; } + public INamedTypeSymbol BoolType { get; init; } + public INamedTypeSymbol StringComparisonType { get; init; } + public IMethodSymbol CompareStringString { get; init; } + public IMethodSymbol CompareStringStringBool { get; init; } + public IMethodSymbol CompareStringStringStringComparison { get; init; } + public IMethodSymbol EqualsStringString { get; init; } + public IMethodSymbol EqualsStringStringStringComparison { get; init; } + } + + /// + /// Selects an equals or not-equals operation where one argument is a literal zero, and the other argument + /// is an eligible string.Compare invocation. + /// + internal abstract class OperationSelector + { + protected OperationSelector(RequiredSymbols symbols) + { + Symbols = symbols; + } + + protected RequiredSymbols Symbols { get; } + + /// + /// Indicates whether the specified matches the current + /// + /// + /// + public abstract bool IsMatch(IOperation compareResultToLiteralZero); + + /// + /// Creates a replacement expression for the specified matching . Asserts if + /// returns for the specified . + /// + /// An that is matched by the current + /// The to use. + /// The replacement expression to be used by the code fixer. + public abstract SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator); + + /// + /// Tries to get an invocation operation that is being compared to a literal zero. + /// + /// An operation that is potentially a comparison of an invocation with a literal zero. + /// The invocation operation. + /// True if the specified operation is an equals or not-equals operation that compares a literal zero to + /// any invocation operation. + protected static bool TryGetInvocationFromComparisonWithLiteralZero(IOperation compareResultToLiteralZero, [NotNullWhen(true)] out IInvocationOperation? invocation) + { + if (compareResultToLiteralZero is IBinaryOperation binaryOperation && + binaryOperation.OperatorKind is BinaryOperatorKind.Equals or BinaryOperatorKind.NotEquals) + { + if (TryConvertOperands(binaryOperation.LeftOperand, binaryOperation.RightOperand, out invocation) || + TryConvertOperands(binaryOperation.RightOperand, binaryOperation.LeftOperand, out invocation)) + { + return true; + } + } + + invocation = default; + return false; + + // Local functions + + static bool TryConvertOperands(IOperation first, IOperation second, [NotNullWhen(true)] out IInvocationOperation? result) + { + if (first is IInvocationOperation invocation && + second is ILiteralOperation literal && + literal.ConstantValue.HasValue && + literal.ConstantValue.Value is int integer && + integer is 0) + { + result = invocation; + return true; + } + + result = default; + return false; + } + } + + protected static IInvocationOperation GetInvocationFromComparisonWithLiteralZero(IOperation compareResultToLiteralZero) + { + if (!TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation)) + Fail(); + + return invocation; + } + + protected static bool IsNotEqualsOperation(IOperation operation) + { + return operation is IBinaryOperation binaryOperation && + binaryOperation.OperatorKind is BinaryOperatorKind.NotEquals; + } + + [DoesNotReturn] + protected static void Fail() => Debug.Fail($"'{nameof(GetReplacementExpression)}' must only be called when '{nameof(IsMatch)}' is 'true'."); + + protected SyntaxNode CreateStringEqualsMemberAccessExpression(SyntaxGenerator generator) + { + var stringTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringType); + return generator.MemberAccessExpression(stringTypeExpression, nameof(string.Equals)); + } + } + + /// + /// Selects s that satisfy all of the following: + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is an invocation of + /// + /// + private sealed class StringStringSelector : OperationSelector + { + public StringStringSelector(RequiredSymbols symbols) + : base(symbols) + { } + + public override bool IsMatch(IOperation compareResultToLiteralZero) + { + return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && + invocation.TargetMethod.Equals(Symbols.CompareStringString, SymbolEqualityComparer.Default); + } + + public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + { + RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); + + var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); + var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); + var equalsInvocationExpression = generator.InvocationExpression( + equalsMemberAccessExpression, + invocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); + + return IsNotEqualsOperation(compareResultToLiteralZero) ? + generator.LogicalNotExpression(equalsInvocationExpression) : + equalsInvocationExpression; + } + } + + /// + /// Selects s that satisfy all of the following: + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is an invocation of + /// The argument is a literal + /// + /// + private sealed class StringStringBoolSelector : OperationSelector + { + public StringStringBoolSelector(RequiredSymbols symbols) + : base(symbols) + { } + + public override bool IsMatch(IOperation compareResultToLiteralZero) + { + // The 'ignoreCase' bool argument of string.Compare must be a literal. + return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && + invocation.TargetMethod.Equals(Symbols.CompareStringStringBool, SymbolEqualityComparer.Default) && + invocation.Arguments.GetArgumentForParameterAtIndex(2).Value is ILiteralOperation boolLiteral && + boolLiteral.ConstantValue.HasValue && + boolLiteral.ConstantValue.Value is bool; + } + + public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + { + RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); + + var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); + + // 'IsMatch' rejects operations where the 'ignoreCase' argument is not a literal. + var ignoreCaseLiteral = (ILiteralOperation)invocation.Arguments.GetArgumentForParameterAtIndex(2).Value; + + var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); + var stringComparisonTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringComparisonType); + + // Convert 'ignoreCase' boolean argument to equivalent StringComparison value. + var stringComparisonEnumMemberName = (bool)ignoreCaseLiteral.ConstantValue.Value ? + nameof(StringComparison.CurrentCultureIgnoreCase) : + nameof(StringComparison.CurrentCulture); + var stringComparisonEnumMemberAccessExpression = generator.MemberAccessExpression(stringComparisonTypeExpression, stringComparisonEnumMemberName); + + var equalsInvocationExpression = generator.InvocationExpression( + equalsMemberAccessExpression, + invocation.Arguments.GetArgumentForParameterAtIndex(0).Value.Syntax, + invocation.Arguments.GetArgumentForParameterAtIndex(1).Value.Syntax, + stringComparisonEnumMemberAccessExpression); + + return IsNotEqualsOperation(compareResultToLiteralZero) ? + generator.LogicalNotExpression(equalsInvocationExpression) : + equalsInvocationExpression; + } + } + + /// + /// Selects s that satisfy all of the following: + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is an invocation of + /// + /// + private sealed class StringStringStringComparisonSelector : OperationSelector + { + public StringStringStringComparisonSelector(RequiredSymbols symbols) + : base(symbols) + { } + + public override bool IsMatch(IOperation compareResultToLiteralZero) + { + return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && + invocation.TargetMethod.Equals(Symbols.CompareStringStringStringComparison, SymbolEqualityComparer.Default); + } + + public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + { + RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); + + var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); + + var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); + + var equalsInvocationExpression = generator.InvocationExpression( + equalsMemberAccessExpression, + invocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); + + return IsNotEqualsOperation(compareResultToLiteralZero) ? + generator.LogicalNotExpression(equalsInvocationExpression) : + equalsInvocationExpression; + } } } } diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs index d63421b9d3..9a8bdc911d 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs @@ -180,13 +180,13 @@ public void Method(string x, string y) [MemberData(nameof(VB_StringCompareExpressionsTestData))] public Task StringCompareResult_CompareToNonLiteralZero_NoDiagnostic_VB(string expression) { - string code = $@" + var code = $@" Imports System Public Class Testopolis Private Const Zero As Integer = 0 - Public Sub Method(x As String, y As String) As Boolean + Public Sub Method(x As String, y As String) Dim a = {expression} = Zero Dim b = {expression} <> Zero Dim c = Zero = {expression} diff --git a/src/Utilities/Compiler/Extensions/IEnumerableOfIMethodSymbolExtensions.cs b/src/Utilities/Compiler/Extensions/IEnumerableOfIMethodSymbolExtensions.cs index cfbac56c1f..6179856fa8 100644 --- a/src/Utilities/Compiler/Extensions/IEnumerableOfIMethodSymbolExtensions.cs +++ b/src/Utilities/Compiler/Extensions/IEnumerableOfIMethodSymbolExtensions.cs @@ -100,6 +100,34 @@ public static IEnumerable GetMethodOverloadsWithDesiredParameterA return GetMethodOverloadsWithDesiredParameterAtLeadingOrTrailing(methods, selectedOverload, expectedTrailingParameterType, trailingOnly: true); } + /// + /// Gets the in the sequence who's parameters match . + /// + /// The sequence of s to search. + /// The types of the parameters, in order. + /// + /// The first in the sequence who's parameters match , or null if + /// no method was found. + /// + public static IMethodSymbol? GetFirstOrDefaultMemberWithParameterTypes(this IEnumerable? members, params ITypeSymbol[] expectedParameterTypesInOrder) + { + return members?.FirstOrDefault(member => + { + if (member.Parameters.Length != expectedParameterTypesInOrder.Length) + return false; + + for (int i = 0; i < expectedParameterTypesInOrder.Length; ++i) + { + var parameterType = member.Parameters[i].Type; + + if (!expectedParameterTypesInOrder[i].Equals(parameterType)) + return false; + } + + return true; + }); + } + /// /// Given a , this method returns the method symbol which /// matches the expectedParameterTypesInOrder parameter requirement From 6e503b6cb665d2a6798b185c0aa3eea410c64376 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Tue, 18 May 2021 21:45:33 -0400 Subject: [PATCH 03/10] Run msbuild --- .../Microsoft.CodeAnalysis.NetAnalyzers.md | 12 +++++++++++ .../Microsoft.CodeAnalysis.NetAnalyzers.sarif | 20 +++++++++++++++++++ src/NetAnalyzers/RulesMissingDocumentation.md | 2 +- 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md b/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md index 0dd518e5d2..ec3bb411e4 100644 --- a/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md +++ b/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md @@ -1920,6 +1920,18 @@ Calls to 'string.IndexOf' where the result is used to check for the presence/abs |CodeFix|True| --- +## [CA2250](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2250): Use 'string.Equals' + +It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. + +|Item|Value| +|-|-| +|Category|Usage| +|Enabled|True| +|Severity|Hidden| +|CodeFix|True| +--- + ## [CA2300](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2300): Do not use insecure deserializer BinaryFormatter The method '{0}' is insecure when deserializing untrusted data. If you need to instead detect BinaryFormatter deserialization without a SerializationBinder set, then disable rule CA2300, and enable rules CA2301 and CA2302. diff --git a/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif b/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif index be49daf75f..2602c404cc 100644 --- a/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif +++ b/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif @@ -3391,6 +3391,26 @@ ] } }, + "CA2250": { + "id": "CA2250", + "shortDescription": "Use 'string.Equals'", + "fullDescription": "It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero.", + "defaultLevel": "hidden", + "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2250", + "properties": { + "category": "Usage", + "isEnabledByDefault": true, + "typeName": "UseStringEqualsOverStringCompare", + "languages": [ + "C#", + "Visual Basic" + ], + "tags": [ + "Telemetry", + "EnabledRuleInAggressiveMode" + ] + } + }, "CA2300": { "id": "CA2300", "shortDescription": "Do not use insecure deserializer BinaryFormatter", diff --git a/src/NetAnalyzers/RulesMissingDocumentation.md b/src/NetAnalyzers/RulesMissingDocumentation.md index 6d4e9867a0..ff2cdd9ec1 100644 --- a/src/NetAnalyzers/RulesMissingDocumentation.md +++ b/src/NetAnalyzers/RulesMissingDocumentation.md @@ -5,6 +5,6 @@ Rule ID | Missing Help Link | Title | CA1418 | | Use valid platform string | CA1839 | | Use 'Environment.ProcessPath' | CA1840 | | Use 'Environment.CurrentManagedThreadId' | -CA1841 | | Prefer Dictionary.Contains methods | CA1842 | | Do not use 'WhenAll' with a single task | CA1843 | | Do not use 'WaitAll' with a single task | +CA2250 | | Use 'string.Equals' | From 1963ab116b1e12b8dcc7b1f3b4f76650ffe6b912 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Tue, 18 May 2021 22:16:46 -0400 Subject: [PATCH 04/10] Fix build warnings --- .../Runtime/UseStringEqualsOverStringCompare.Fixer.cs | 2 ++ .../Runtime/UseStringEqualsOverStringCompare.cs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs index 51d15f4a38..05e6ae5223 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs @@ -52,5 +52,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) } public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(UseStringEqualsOverStringCompare.RuleId); + + public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; } } diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs index f932a79a23..613788e00e 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs @@ -226,7 +226,9 @@ protected static bool IsNotEqualsOperation(IOperation operation) } [DoesNotReturn] +#pragma warning disable CS8763 // A method marked [DoesNotReturn] should not return. protected static void Fail() => Debug.Fail($"'{nameof(GetReplacementExpression)}' must only be called when '{nameof(IsMatch)}' is 'true'."); +#pragma warning restore CS8763 // A method marked [DoesNotReturn] should not return. protected SyntaxNode CreateStringEqualsMemberAccessExpression(SyntaxGenerator generator) { From db69ec28f441652badc43bb5dddafbb7da0e8a84 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Tue, 18 May 2021 22:50:35 -0400 Subject: [PATCH 05/10] Fix more build warnings Remove all references to SyntaxGenerator from analyzer class --- .../UseStringEqualsOverStringCompare.Fixer.cs | 268 +++++++++++++++++- .../UseStringEqualsOverStringCompare.cs | 249 +--------------- .../UseStringEqualsOverStringCompareTests.cs | 3 - 3 files changed, 270 insertions(+), 250 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs index 05e6ae5223..a5e3ebf3df 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs @@ -1,8 +1,12 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Immutable; using System.Composition; +using System.Linq; using System.Threading.Tasks; +using Analyzer.Utilities; +using Analyzer.Utilities.Extensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.CodeFixes; @@ -10,6 +14,9 @@ using Microsoft.CodeAnalysis.Operations; using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources; +using RequiredSymbols = Microsoft.NetCore.Analyzers.Runtime.UseStringEqualsOverStringCompare.RequiredSymbols; +using System.Diagnostics.CodeAnalysis; +using System.Diagnostics; namespace Microsoft.NetCore.Analyzers.Runtime { @@ -22,7 +29,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) var token = context.CancellationToken; var semanticModel = await document.GetSemanticModelAsync(token).ConfigureAwait(false); - if (!UseStringEqualsOverStringCompare.RequiredSymbols.TryGetSymbols(semanticModel.Compilation, out var symbols)) + if (!RequiredSymbols.TryGetSymbols(semanticModel.Compilation, out var symbols)) return; var root = await document.GetSyntaxRootAsync(token).ConfigureAwait(false); @@ -30,7 +37,7 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) if (semanticModel.GetOperation(root.FindNode(context.Span, getInnermostNodeForTie: true), token) is not IBinaryOperation operation) return; - var selectors = UseStringEqualsOverStringCompare.GetSelectors(symbols); + var selectors = GetSelectors(symbols); foreach (var selector in selectors) { @@ -54,5 +61,262 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(UseStringEqualsOverStringCompare.RuleId); public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + private static ImmutableArray GetSelectors(RequiredSymbols symbols) + { + return ImmutableArray.Create( + new StringStringSelector(symbols), + new StringStringBoolSelector(symbols), + new StringStringStringComparisonSelector(symbols)); + } + + /// + /// Gets a function that selects an operation that should have a diagnostic reported for it. + /// We have to use this round-about way of implementing the analyzer because has a method + /// that takes a , and analyzers aren't allowed to reference that type in any way. Having the + /// analyzer use the s indirectly via this method avoids having duplicated logic in the analyzer and fixer. + /// + /// + /// + internal static Func GetOperationSelector(RequiredSymbols symbols) + { + var selectors = GetSelectors(symbols); + + return operation => + { + foreach (var selector in selectors) + { + if (selector.IsMatch(operation)) + return true; + } + + return false; + }; + } + + /// + /// Selects an equals or not-equals operation where one argument is a literal zero, and the other argument + /// is an eligible string.Compare invocation. + /// + private abstract class OperationSelector + { + protected OperationSelector(RequiredSymbols symbols) + { + Symbols = symbols; + } + + protected RequiredSymbols Symbols { get; } + + /// + /// Indicates whether the specified matches the current + /// + /// + /// + public abstract bool IsMatch(IOperation compareResultToLiteralZero); + + /// + /// Creates a replacement expression for the specified matching . Asserts if + /// returns for the specified . + /// + /// An that is matched by the current + /// The to use. + /// The replacement expression to be used by the code fixer. + public abstract SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator); + + /// + /// Tries to get an invocation operation that is being compared to a literal zero. + /// + /// An operation that is potentially a comparison of an invocation with a literal zero. + /// The invocation operation. + /// True if the specified operation is an equals or not-equals operation that compares a literal zero to + /// any invocation operation. + protected static bool TryGetInvocationFromComparisonWithLiteralZero(IOperation compareResultToLiteralZero, [NotNullWhen(true)] out IInvocationOperation? invocation) + { + if (compareResultToLiteralZero is IBinaryOperation binaryOperation && + binaryOperation.OperatorKind is BinaryOperatorKind.Equals or BinaryOperatorKind.NotEquals) + { + if (TryConvertOperands(binaryOperation.LeftOperand, binaryOperation.RightOperand, out invocation) || + TryConvertOperands(binaryOperation.RightOperand, binaryOperation.LeftOperand, out invocation)) + { + return true; + } + } + + invocation = default; + return false; + + // Local functions + + static bool TryConvertOperands(IOperation first, IOperation second, [NotNullWhen(true)] out IInvocationOperation? result) + { + if (first is IInvocationOperation invocation && + second is ILiteralOperation literal && + literal.ConstantValue.HasValue && + literal.ConstantValue.Value is int integer && + integer is 0) + { + result = invocation; + return true; + } + + result = default; + return false; + } + } + + protected static IInvocationOperation GetInvocationFromComparisonWithLiteralZero(IOperation compareResultToLiteralZero) + { + if (!TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation)) + Fail(); + + return invocation; + } + + protected static bool IsNotEqualsOperation(IOperation operation) + { + return operation is IBinaryOperation binaryOperation && + binaryOperation.OperatorKind is BinaryOperatorKind.NotEquals; + } + + [DoesNotReturn] +#pragma warning disable CS8763 // A method marked [DoesNotReturn] should not return. + protected static void Fail() => Debug.Fail($"'{nameof(GetReplacementExpression)}' must only be called when '{nameof(IsMatch)}' is 'true'."); +#pragma warning restore CS8763 // A method marked [DoesNotReturn] should not return. + + protected SyntaxNode CreateStringEqualsMemberAccessExpression(SyntaxGenerator generator) + { + var stringTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringType); + return generator.MemberAccessExpression(stringTypeExpression, nameof(string.Equals)); + } + } + + /// + /// Selects s that satisfy all of the following: + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is an invocation of + /// + /// + private sealed class StringStringSelector : OperationSelector + { + public StringStringSelector(RequiredSymbols symbols) + : base(symbols) + { } + + public override bool IsMatch(IOperation compareResultToLiteralZero) + { + return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && + invocation.TargetMethod.Equals(Symbols.CompareStringString, SymbolEqualityComparer.Default); + } + + public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + { + RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); + + var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); + var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); + var equalsInvocationExpression = generator.InvocationExpression( + equalsMemberAccessExpression, + invocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); + + return IsNotEqualsOperation(compareResultToLiteralZero) ? + generator.LogicalNotExpression(equalsInvocationExpression) : + equalsInvocationExpression; + } + } + + /// + /// Selects s that satisfy all of the following: + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is an invocation of + /// The argument is a literal + /// + /// + private sealed class StringStringBoolSelector : OperationSelector + { + public StringStringBoolSelector(RequiredSymbols symbols) + : base(symbols) + { } + + public override bool IsMatch(IOperation compareResultToLiteralZero) + { + // The 'ignoreCase' bool argument of string.Compare must be a literal. + return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && + invocation.TargetMethod.Equals(Symbols.CompareStringStringBool, SymbolEqualityComparer.Default) && + invocation.Arguments.GetArgumentForParameterAtIndex(2).Value is ILiteralOperation boolLiteral && + boolLiteral.ConstantValue.HasValue && + boolLiteral.ConstantValue.Value is bool; + } + + public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + { + RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); + + var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); + + // 'IsMatch' rejects operations where the 'ignoreCase' argument is not a literal. + var ignoreCaseLiteral = (ILiteralOperation)invocation.Arguments.GetArgumentForParameterAtIndex(2).Value; + + var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); + var stringComparisonTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringComparisonType); + + // Convert 'ignoreCase' boolean argument to equivalent StringComparison value. + var stringComparisonEnumMemberName = (bool)ignoreCaseLiteral.ConstantValue.Value ? + nameof(StringComparison.CurrentCultureIgnoreCase) : + nameof(StringComparison.CurrentCulture); + var stringComparisonEnumMemberAccessExpression = generator.MemberAccessExpression(stringComparisonTypeExpression, stringComparisonEnumMemberName); + + var equalsInvocationExpression = generator.InvocationExpression( + equalsMemberAccessExpression, + invocation.Arguments.GetArgumentForParameterAtIndex(0).Value.Syntax, + invocation.Arguments.GetArgumentForParameterAtIndex(1).Value.Syntax, + stringComparisonEnumMemberAccessExpression); + + return IsNotEqualsOperation(compareResultToLiteralZero) ? + generator.LogicalNotExpression(equalsInvocationExpression) : + equalsInvocationExpression; + } + } + + /// + /// Selects s that satisfy all of the following: + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is an invocation of + /// + /// + private sealed class StringStringStringComparisonSelector : OperationSelector + { + public StringStringStringComparisonSelector(RequiredSymbols symbols) + : base(symbols) + { } + + public override bool IsMatch(IOperation compareResultToLiteralZero) + { + return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && + invocation.TargetMethod.Equals(Symbols.CompareStringStringStringComparison, SymbolEqualityComparer.Default); + } + + public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + { + RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); + + var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); + + var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); + + var equalsInvocationExpression = generator.InvocationExpression( + equalsMemberAccessExpression, + invocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); + + return IsNotEqualsOperation(compareResultToLiteralZero) ? + generator.LogicalNotExpression(equalsInvocationExpression) : + equalsInvocationExpression; + } + } } } diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs index 613788e00e..265cf3abbf 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs @@ -1,16 +1,12 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; using System.Collections.Immutable; -using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Linq; using Analyzer.Utilities; using Analyzer.Utilities.Extensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; -using Microsoft.CodeAnalysis.Editing; -using Microsoft.CodeAnalysis.Operations; using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources; namespace Microsoft.NetCore.Analyzers.Runtime @@ -46,31 +42,19 @@ public override void Initialize(AnalysisContext context) if (!RequiredSymbols.TryGetSymbols(context.Compilation, out var symbols)) return; - var selectors = GetSelectors(symbols); + var selector = UseStringEqualsOverStringCompareFixer.GetOperationSelector(symbols); context.RegisterOperationAction(context => { - foreach (var selector in selectors) + if (selector(context.Operation)) { - if (selector.IsMatch(context.Operation)) - { - var diagnostic = context.Operation.CreateDiagnostic(Rule); - context.ReportDiagnostic(diagnostic); - break; - } + var diagnostic = context.Operation.CreateDiagnostic(Rule); + context.ReportDiagnostic(diagnostic); } }, OperationKind.Binary); }); } - internal static ImmutableArray GetSelectors(RequiredSymbols symbols) - { - return ImmutableArray.Create( - new StringStringSelector(symbols), - new StringStringBoolSelector(symbols), - new StringStringStringComparisonSelector(symbols)); - } - internal sealed class RequiredSymbols { // Named-constructor 'TryGetSymbols' inits all properties or doesn't construct an instance. @@ -140,230 +124,5 @@ compareStringStringBool is null || public IMethodSymbol EqualsStringString { get; init; } public IMethodSymbol EqualsStringStringStringComparison { get; init; } } - - /// - /// Selects an equals or not-equals operation where one argument is a literal zero, and the other argument - /// is an eligible string.Compare invocation. - /// - internal abstract class OperationSelector - { - protected OperationSelector(RequiredSymbols symbols) - { - Symbols = symbols; - } - - protected RequiredSymbols Symbols { get; } - - /// - /// Indicates whether the specified matches the current - /// - /// - /// - public abstract bool IsMatch(IOperation compareResultToLiteralZero); - - /// - /// Creates a replacement expression for the specified matching . Asserts if - /// returns for the specified . - /// - /// An that is matched by the current - /// The to use. - /// The replacement expression to be used by the code fixer. - public abstract SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator); - - /// - /// Tries to get an invocation operation that is being compared to a literal zero. - /// - /// An operation that is potentially a comparison of an invocation with a literal zero. - /// The invocation operation. - /// True if the specified operation is an equals or not-equals operation that compares a literal zero to - /// any invocation operation. - protected static bool TryGetInvocationFromComparisonWithLiteralZero(IOperation compareResultToLiteralZero, [NotNullWhen(true)] out IInvocationOperation? invocation) - { - if (compareResultToLiteralZero is IBinaryOperation binaryOperation && - binaryOperation.OperatorKind is BinaryOperatorKind.Equals or BinaryOperatorKind.NotEquals) - { - if (TryConvertOperands(binaryOperation.LeftOperand, binaryOperation.RightOperand, out invocation) || - TryConvertOperands(binaryOperation.RightOperand, binaryOperation.LeftOperand, out invocation)) - { - return true; - } - } - - invocation = default; - return false; - - // Local functions - - static bool TryConvertOperands(IOperation first, IOperation second, [NotNullWhen(true)] out IInvocationOperation? result) - { - if (first is IInvocationOperation invocation && - second is ILiteralOperation literal && - literal.ConstantValue.HasValue && - literal.ConstantValue.Value is int integer && - integer is 0) - { - result = invocation; - return true; - } - - result = default; - return false; - } - } - - protected static IInvocationOperation GetInvocationFromComparisonWithLiteralZero(IOperation compareResultToLiteralZero) - { - if (!TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation)) - Fail(); - - return invocation; - } - - protected static bool IsNotEqualsOperation(IOperation operation) - { - return operation is IBinaryOperation binaryOperation && - binaryOperation.OperatorKind is BinaryOperatorKind.NotEquals; - } - - [DoesNotReturn] -#pragma warning disable CS8763 // A method marked [DoesNotReturn] should not return. - protected static void Fail() => Debug.Fail($"'{nameof(GetReplacementExpression)}' must only be called when '{nameof(IsMatch)}' is 'true'."); -#pragma warning restore CS8763 // A method marked [DoesNotReturn] should not return. - - protected SyntaxNode CreateStringEqualsMemberAccessExpression(SyntaxGenerator generator) - { - var stringTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringType); - return generator.MemberAccessExpression(stringTypeExpression, nameof(string.Equals)); - } - } - - /// - /// Selects s that satisfy all of the following: - /// - /// Is an equals or not-equals operation - /// One operand is a literal zero - /// The other operand is an invocation of - /// - /// - private sealed class StringStringSelector : OperationSelector - { - public StringStringSelector(RequiredSymbols symbols) - : base(symbols) - { } - - public override bool IsMatch(IOperation compareResultToLiteralZero) - { - return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && - invocation.TargetMethod.Equals(Symbols.CompareStringString, SymbolEqualityComparer.Default); - } - - public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) - { - RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); - - var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); - var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); - var equalsInvocationExpression = generator.InvocationExpression( - equalsMemberAccessExpression, - invocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); - - return IsNotEqualsOperation(compareResultToLiteralZero) ? - generator.LogicalNotExpression(equalsInvocationExpression) : - equalsInvocationExpression; - } - } - - /// - /// Selects s that satisfy all of the following: - /// - /// Is an equals or not-equals operation - /// One operand is a literal zero - /// The other operand is an invocation of - /// The argument is a literal - /// - /// - private sealed class StringStringBoolSelector : OperationSelector - { - public StringStringBoolSelector(RequiredSymbols symbols) - : base(symbols) - { } - - public override bool IsMatch(IOperation compareResultToLiteralZero) - { - // The 'ignoreCase' bool argument of string.Compare must be a literal. - return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && - invocation.TargetMethod.Equals(Symbols.CompareStringStringBool, SymbolEqualityComparer.Default) && - invocation.Arguments.GetArgumentForParameterAtIndex(2).Value is ILiteralOperation boolLiteral && - boolLiteral.ConstantValue.HasValue && - boolLiteral.ConstantValue.Value is bool; - } - - public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) - { - RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); - - var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); - - // 'IsMatch' rejects operations where the 'ignoreCase' argument is not a literal. - var ignoreCaseLiteral = (ILiteralOperation)invocation.Arguments.GetArgumentForParameterAtIndex(2).Value; - - var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); - var stringComparisonTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringComparisonType); - - // Convert 'ignoreCase' boolean argument to equivalent StringComparison value. - var stringComparisonEnumMemberName = (bool)ignoreCaseLiteral.ConstantValue.Value ? - nameof(StringComparison.CurrentCultureIgnoreCase) : - nameof(StringComparison.CurrentCulture); - var stringComparisonEnumMemberAccessExpression = generator.MemberAccessExpression(stringComparisonTypeExpression, stringComparisonEnumMemberName); - - var equalsInvocationExpression = generator.InvocationExpression( - equalsMemberAccessExpression, - invocation.Arguments.GetArgumentForParameterAtIndex(0).Value.Syntax, - invocation.Arguments.GetArgumentForParameterAtIndex(1).Value.Syntax, - stringComparisonEnumMemberAccessExpression); - - return IsNotEqualsOperation(compareResultToLiteralZero) ? - generator.LogicalNotExpression(equalsInvocationExpression) : - equalsInvocationExpression; - } - } - - /// - /// Selects s that satisfy all of the following: - /// - /// Is an equals or not-equals operation - /// One operand is a literal zero - /// The other operand is an invocation of - /// - /// - private sealed class StringStringStringComparisonSelector : OperationSelector - { - public StringStringStringComparisonSelector(RequiredSymbols symbols) - : base(symbols) - { } - - public override bool IsMatch(IOperation compareResultToLiteralZero) - { - return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && - invocation.TargetMethod.Equals(Symbols.CompareStringStringStringComparison, SymbolEqualityComparer.Default); - } - - public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) - { - RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); - - var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); - - var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); - - var equalsInvocationExpression = generator.InvocationExpression( - equalsMemberAccessExpression, - invocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); - - return IsNotEqualsOperation(compareResultToLiteralZero) ? - generator.LogicalNotExpression(equalsInvocationExpression) : - equalsInvocationExpression; - } - } } } diff --git a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs index 9a8bdc911d..e3d1e28566 100644 --- a/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs +++ b/src/NetAnalyzers/UnitTests/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompareTests.cs @@ -1,10 +1,7 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. -using System; using System.Collections.Generic; -using System.Globalization; using System.Linq; -using System.Text; using System.Threading.Tasks; using Microsoft.CodeAnalysis; using Xunit; From 7883135d0086c36ee284fed43e85ee7ae4e9caf7 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Wed, 19 May 2021 16:41:57 -0400 Subject: [PATCH 06/10] Restructure analyzer and fixer Analyzer and fixer were intertwined in unhealthy ways that also caused CI failures. --- .../UseStringEqualsOverStringCompare.Fixer.cs | 296 ++++++------------ .../UseStringEqualsOverStringCompare.cs | 178 +++++++++-- 2 files changed, 245 insertions(+), 229 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs index a5e3ebf3df..5dd804e5f6 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs @@ -4,6 +4,7 @@ using System.Collections.Immutable; using System.Composition; using System.Linq; +using System.Threading; using System.Threading.Tasks; using Analyzer.Utilities; using Analyzer.Utilities.Extensions; @@ -15,8 +16,6 @@ using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources; using RequiredSymbols = Microsoft.NetCore.Analyzers.Runtime.UseStringEqualsOverStringCompare.RequiredSymbols; -using System.Diagnostics.CodeAnalysis; -using System.Diagnostics; namespace Microsoft.NetCore.Analyzers.Runtime { @@ -34,27 +33,31 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) var root = await document.GetSyntaxRootAsync(token).ConfigureAwait(false); - if (semanticModel.GetOperation(root.FindNode(context.Span, getInnermostNodeForTie: true), token) is not IBinaryOperation operation) + if (semanticModel.GetOperation(root.FindNode(context.Span, getInnermostNodeForTie: true), token) is not IBinaryOperation violation) return; - var selectors = GetSelectors(symbols); + // Get the replacer that applies to the reported violation. Bail out if no replacer + // matches the reported violation. + var replacer = GetOperationReplacers(symbols).FirstOrDefault(x => x.IsMatch(violation)); + if (replacer is null) + return; + + var codeAction = CodeAction.Create( + Resx.UseStringEqualsOverStringCompareCodeFixTitle, + CreateChangedDocument, + nameof(Resx.UseStringEqualsOverStringCompareCodeFixTitle)); + context.RegisterCodeFix(codeAction, context.Diagnostics); + return; - foreach (var selector in selectors) + // Local functions + + async Task CreateChangedDocument(CancellationToken token) { - if (selector.IsMatch(operation)) - { - var codeAction = CodeAction.Create( - Resx.UseStringEqualsOverStringCompareCodeFixTitle, - async token => - { - var editor = await DocumentEditor.CreateAsync(document, token).ConfigureAwait(false); - var replacementNode = selector.GetReplacementExpression(operation, editor.Generator); - editor.ReplaceNode(operation.Syntax, replacementNode); - return editor.GetChangedDocument(); - }, Resx.UseStringEqualsOverStringCompareCodeFixTitle); - context.RegisterCodeFix(codeAction, context.Diagnostics); - break; - } + var editor = await DocumentEditor.CreateAsync(document, token).ConfigureAwait(false); + var replacementNode = replacer.CreateReplacementExpression(violation, editor.Generator); + editor.ReplaceNode(violation.Syntax, replacementNode); + + return editor.GetChangedDocument(); } } @@ -62,45 +65,20 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; - private static ImmutableArray GetSelectors(RequiredSymbols symbols) - { - return ImmutableArray.Create( - new StringStringSelector(symbols), - new StringStringBoolSelector(symbols), - new StringStringStringComparisonSelector(symbols)); - } - - /// - /// Gets a function that selects an operation that should have a diagnostic reported for it. - /// We have to use this round-about way of implementing the analyzer because has a method - /// that takes a , and analyzers aren't allowed to reference that type in any way. Having the - /// analyzer use the s indirectly via this method avoids having duplicated logic in the analyzer and fixer. - /// - /// - /// - internal static Func GetOperationSelector(RequiredSymbols symbols) + private static ImmutableArray GetOperationReplacers(RequiredSymbols symbols) { - var selectors = GetSelectors(symbols); - - return operation => - { - foreach (var selector in selectors) - { - if (selector.IsMatch(operation)) - return true; - } - - return false; - }; + return ImmutableArray.Create( + new StringStringCaseReplacer(symbols), + new StringStringBoolReplacer(symbols), + new StringStringStringComparisonReplacer(symbols)); } /// - /// Selects an equals or not-equals operation where one argument is a literal zero, and the other argument - /// is an eligible string.Compare invocation. + /// Base class for an object that generate the replacement code for a reported violation. /// - private abstract class OperationSelector + private abstract class OperationReplacer { - protected OperationSelector(RequiredSymbols symbols) + protected OperationReplacer(RequiredSymbols symbols) { Symbols = symbols; } @@ -108,214 +86,132 @@ protected OperationSelector(RequiredSymbols symbols) protected RequiredSymbols Symbols { get; } /// - /// Indicates whether the specified matches the current + /// Indicates whether the current applies to the specified violation. /// - /// - /// - public abstract bool IsMatch(IOperation compareResultToLiteralZero); + /// The at the location reported by the analyzer. + /// True if the current applies to the specified violation. + public abstract bool IsMatch(IBinaryOperation violation); /// - /// Creates a replacement expression for the specified matching . Asserts if - /// returns for the specified . + /// Creates a replacement node for a violation that the current applies to. + /// Asserts if the current does not apply to the specified violation. /// - /// An that is matched by the current - /// The to use. - /// The replacement expression to be used by the code fixer. - public abstract SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator); + /// The obtained at the location reported by the analyzer. + /// must return for this operation. + /// + /// + public abstract SyntaxNode CreateReplacementExpression(IBinaryOperation violation, SyntaxGenerator generator); - /// - /// Tries to get an invocation operation that is being compared to a literal zero. - /// - /// An operation that is potentially a comparison of an invocation with a literal zero. - /// The invocation operation. - /// True if the specified operation is an equals or not-equals operation that compares a literal zero to - /// any invocation operation. - protected static bool TryGetInvocationFromComparisonWithLiteralZero(IOperation compareResultToLiteralZero, [NotNullWhen(true)] out IInvocationOperation? invocation) + protected SyntaxNode CreateEqualsMemberAccess(SyntaxGenerator generator) { - if (compareResultToLiteralZero is IBinaryOperation binaryOperation && - binaryOperation.OperatorKind is BinaryOperatorKind.Equals or BinaryOperatorKind.NotEquals) - { - if (TryConvertOperands(binaryOperation.LeftOperand, binaryOperation.RightOperand, out invocation) || - TryConvertOperands(binaryOperation.RightOperand, binaryOperation.LeftOperand, out invocation)) - { - return true; - } - } - - invocation = default; - return false; - - // Local functions - - static bool TryConvertOperands(IOperation first, IOperation second, [NotNullWhen(true)] out IInvocationOperation? result) - { - if (first is IInvocationOperation invocation && - second is ILiteralOperation literal && - literal.ConstantValue.HasValue && - literal.ConstantValue.Value is int integer && - integer is 0) - { - result = invocation; - return true; - } - - result = default; - return false; - } + var stringTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringType); + return generator.MemberAccessExpression(stringTypeExpression, nameof(string.Equals)); } - protected static IInvocationOperation GetInvocationFromComparisonWithLiteralZero(IOperation compareResultToLiteralZero) + protected static IInvocationOperation GetInvocation(IBinaryOperation violation) { - if (!TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation)) - Fail(); + var result = UseStringEqualsOverStringCompare.GetInvocationFromEqualityCheckWithLiteralZero(violation); - return invocation; - } + RoslynDebug.Assert(result is not null); - protected static bool IsNotEqualsOperation(IOperation operation) - { - return operation is IBinaryOperation binaryOperation && - binaryOperation.OperatorKind is BinaryOperatorKind.NotEquals; + return result; } - [DoesNotReturn] -#pragma warning disable CS8763 // A method marked [DoesNotReturn] should not return. - protected static void Fail() => Debug.Fail($"'{nameof(GetReplacementExpression)}' must only be called when '{nameof(IsMatch)}' is 'true'."); -#pragma warning restore CS8763 // A method marked [DoesNotReturn] should not return. - - protected SyntaxNode CreateStringEqualsMemberAccessExpression(SyntaxGenerator generator) + protected static SyntaxNode InvertIfNotEquals(SyntaxNode stringEqualsInvocationExpression, IBinaryOperation equalsOrNotEqualsOperation, SyntaxGenerator generator) { - var stringTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringType); - return generator.MemberAccessExpression(stringTypeExpression, nameof(string.Equals)); + return equalsOrNotEqualsOperation.OperatorKind is BinaryOperatorKind.NotEquals ? + generator.LogicalNotExpression(stringEqualsInvocationExpression) : + stringEqualsInvocationExpression; } } /// - /// Selects s that satisfy all of the following: - /// - /// Is an equals or not-equals operation - /// One operand is a literal zero - /// The other operand is an invocation of - /// + /// Replaces violations. /// - private sealed class StringStringSelector : OperationSelector + private sealed class StringStringCaseReplacer : OperationReplacer { - public StringStringSelector(RequiredSymbols symbols) + public StringStringCaseReplacer(RequiredSymbols symbols) : base(symbols) { } - public override bool IsMatch(IOperation compareResultToLiteralZero) - { - return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && - invocation.TargetMethod.Equals(Symbols.CompareStringString, SymbolEqualityComparer.Default); - } + public override bool IsMatch(IBinaryOperation violation) => UseStringEqualsOverStringCompare.IsStringStringCase(violation, Symbols); - public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + public override SyntaxNode CreateReplacementExpression(IBinaryOperation violation, SyntaxGenerator generator) { - RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); + RoslynDebug.Assert(IsMatch(violation)); - var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); - var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); - var equalsInvocationExpression = generator.InvocationExpression( - equalsMemberAccessExpression, - invocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); + var compareInvocation = GetInvocation(violation); + var equalsInvocationSyntax = generator.InvocationExpression( + CreateEqualsMemberAccess(generator), + compareInvocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); - return IsNotEqualsOperation(compareResultToLiteralZero) ? - generator.LogicalNotExpression(equalsInvocationExpression) : - equalsInvocationExpression; + return InvertIfNotEquals(equalsInvocationSyntax, violation, generator); } } /// - /// Selects s that satisfy all of the following: - /// - /// Is an equals or not-equals operation - /// One operand is a literal zero - /// The other operand is an invocation of - /// The argument is a literal - /// + /// Replaces violations. /// - private sealed class StringStringBoolSelector : OperationSelector + private sealed class StringStringBoolReplacer : OperationReplacer { - public StringStringBoolSelector(RequiredSymbols symbols) + public StringStringBoolReplacer(RequiredSymbols symbols) : base(symbols) { } - public override bool IsMatch(IOperation compareResultToLiteralZero) - { - // The 'ignoreCase' bool argument of string.Compare must be a literal. - return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && - invocation.TargetMethod.Equals(Symbols.CompareStringStringBool, SymbolEqualityComparer.Default) && - invocation.Arguments.GetArgumentForParameterAtIndex(2).Value is ILiteralOperation boolLiteral && - boolLiteral.ConstantValue.HasValue && - boolLiteral.ConstantValue.Value is bool; - } + public override bool IsMatch(IBinaryOperation violation) => UseStringEqualsOverStringCompare.IsStringStringBoolCase(violation, Symbols); - public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + public override SyntaxNode CreateReplacementExpression(IBinaryOperation violation, SyntaxGenerator generator) { - RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); - - var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); + RoslynDebug.Assert(IsMatch(violation)); - // 'IsMatch' rejects operations where the 'ignoreCase' argument is not a literal. - var ignoreCaseLiteral = (ILiteralOperation)invocation.Arguments.GetArgumentForParameterAtIndex(2).Value; + var compareInvocation = GetInvocation(violation); - var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); - var stringComparisonTypeExpression = generator.TypeExpressionForStaticMemberAccess(Symbols.StringComparisonType); + // We know that the 'ignoreCase' argument in 'string.Compare(string, string, bool)' is a boolean literal + // because we've asserted that 'IsMatch' returns true. + var ignoreCaseLiteral = (ILiteralOperation)compareInvocation.Arguments.GetArgumentForParameterAtIndex(2).Value; - // Convert 'ignoreCase' boolean argument to equivalent StringComparison value. + // If the violation contains a call to 'string.Compare(x, y, true)' then we + // replace it with a call to 'string.Equals(x, y, StringComparison.CurrentCultureIgnoreCase)'. + // If the violation contains a call to 'string.Compare(x, y, false)' then we + // replace it with a call to 'string.Equals(x, y, StringComparison.CurrentCulture)'. var stringComparisonEnumMemberName = (bool)ignoreCaseLiteral.ConstantValue.Value ? nameof(StringComparison.CurrentCultureIgnoreCase) : nameof(StringComparison.CurrentCulture); - var stringComparisonEnumMemberAccessExpression = generator.MemberAccessExpression(stringComparisonTypeExpression, stringComparisonEnumMemberName); + var stringComparisonMemberAccessSyntax = generator.MemberAccessExpression( + generator.TypeExpressionForStaticMemberAccess(Symbols.StringComparisonType), + stringComparisonEnumMemberName); - var equalsInvocationExpression = generator.InvocationExpression( - equalsMemberAccessExpression, - invocation.Arguments.GetArgumentForParameterAtIndex(0).Value.Syntax, - invocation.Arguments.GetArgumentForParameterAtIndex(1).Value.Syntax, - stringComparisonEnumMemberAccessExpression); + var equalsInvocationSyntax = generator.InvocationExpression( + CreateEqualsMemberAccess(generator), + compareInvocation.Arguments.GetArgumentForParameterAtIndex(0).Value.Syntax, + compareInvocation.Arguments.GetArgumentForParameterAtIndex(1).Value.Syntax, + stringComparisonMemberAccessSyntax); - return IsNotEqualsOperation(compareResultToLiteralZero) ? - generator.LogicalNotExpression(equalsInvocationExpression) : - equalsInvocationExpression; + return InvertIfNotEquals(equalsInvocationSyntax, violation, generator); } } /// - /// Selects s that satisfy all of the following: - /// - /// Is an equals or not-equals operation - /// One operand is a literal zero - /// The other operand is an invocation of - /// + /// Replaces violations. /// - private sealed class StringStringStringComparisonSelector : OperationSelector + private sealed class StringStringStringComparisonReplacer : OperationReplacer { - public StringStringStringComparisonSelector(RequiredSymbols symbols) + public StringStringStringComparisonReplacer(RequiredSymbols symbols) : base(symbols) { } - public override bool IsMatch(IOperation compareResultToLiteralZero) - { - return TryGetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero, out var invocation) && - invocation.TargetMethod.Equals(Symbols.CompareStringStringStringComparison, SymbolEqualityComparer.Default); - } + public override bool IsMatch(IBinaryOperation violation) => UseStringEqualsOverStringCompare.IsStringStringStringComparisonCase(violation, Symbols); - public override SyntaxNode GetReplacementExpression(IOperation compareResultToLiteralZero, SyntaxGenerator generator) + public override SyntaxNode CreateReplacementExpression(IBinaryOperation violation, SyntaxGenerator generator) { - RoslynDebug.Assert(IsMatch(compareResultToLiteralZero)); - - var invocation = GetInvocationFromComparisonWithLiteralZero(compareResultToLiteralZero); - - var equalsMemberAccessExpression = CreateStringEqualsMemberAccessExpression(generator); + RoslynDebug.Assert(IsMatch(violation)); - var equalsInvocationExpression = generator.InvocationExpression( - equalsMemberAccessExpression, + var invocation = GetInvocation(violation); + var equalsInvocationSyntax = generator.InvocationExpression( + CreateEqualsMemberAccess(generator), invocation.Arguments.GetArgumentsInParameterOrder().Select(x => x.Value.Syntax)); - return IsNotEqualsOperation(compareResultToLiteralZero) ? - generator.LogicalNotExpression(equalsInvocationExpression) : - equalsInvocationExpression; + return InvertIfNotEquals(equalsInvocationSyntax, violation, generator); } } } diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs index 265cf3abbf..09af19157d 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs @@ -1,5 +1,6 @@ // Copyright (c) Microsoft. All Rights Reserved. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. +using System; using System.Collections.Immutable; using System.Diagnostics.CodeAnalysis; using System.Linq; @@ -7,10 +8,21 @@ using Analyzer.Utilities.Extensions; using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; using Resx = Microsoft.NetCore.Analyzers.MicrosoftNetCoreAnalyzersResources; namespace Microsoft.NetCore.Analyzers.Runtime { + /// + /// Reports a diagnostic on any that: + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is an of an eligible + /// string.Compare overload. + /// + /// See all the Is...Case methods to see the string.Compare overloads that are supported. + /// [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] public sealed class UseStringEqualsOverStringCompare : DiagnosticAnalyzer { @@ -36,23 +48,30 @@ public override void Initialize(AnalysisContext context) { context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); context.EnableConcurrentExecution(); + context.RegisterCompilationStartAction(OnCompilationStart); + } - context.RegisterCompilationStartAction(context => - { - if (!RequiredSymbols.TryGetSymbols(context.Compilation, out var symbols)) - return; + private static void OnCompilationStart(CompilationStartAnalysisContext context) + { + if (!RequiredSymbols.TryGetSymbols(context.Compilation, out var symbols)) + return; + context.RegisterOperationAction(AnalyzeOperation, OperationKind.Binary); + return; - var selector = UseStringEqualsOverStringCompareFixer.GetOperationSelector(symbols); + // Local functions - context.RegisterOperationAction(context => + void AnalyzeOperation(OperationAnalysisContext context) + { + var operation = (IBinaryOperation)context.Operation; + foreach (var selector in CaseSelectors) { - if (selector(context.Operation)) + if (selector(operation, symbols)) { - var diagnostic = context.Operation.CreateDiagnostic(Rule); - context.ReportDiagnostic(diagnostic); + context.ReportDiagnostic(operation.CreateDiagnostic(Rule)); + return; } - }, OperationKind.Binary); - }); + } + } } internal sealed class RequiredSymbols @@ -82,25 +101,12 @@ public static bool TryGetSymbols(Compilation compilation, [NotNullWhen(true)] ou var compareStringStringBool = compareMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType, boolType); var compareStringStringStringComparison = compareMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType, stringComparisonType); - if (compareStringString is null || - compareStringStringBool is null || - compareStringStringStringComparison is null) - { - return false; - } - var equalsMethods = stringType.GetMembers(nameof(string.Equals)) .OfType() .Where(x => x.IsStatic); var equalsStringString = equalsMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType); var equalsStringStringStringComparison = equalsMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType, stringComparisonType); - if (equalsStringString is null || - equalsStringStringStringComparison is null) - { - return false; - } - symbols = new RequiredSymbols { StringType = stringType, @@ -118,11 +124,125 @@ compareStringStringBool is null || public INamedTypeSymbol StringType { get; init; } public INamedTypeSymbol BoolType { get; init; } public INamedTypeSymbol StringComparisonType { get; init; } - public IMethodSymbol CompareStringString { get; init; } - public IMethodSymbol CompareStringStringBool { get; init; } - public IMethodSymbol CompareStringStringStringComparison { get; init; } - public IMethodSymbol EqualsStringString { get; init; } - public IMethodSymbol EqualsStringStringStringComparison { get; init; } + public IMethodSymbol? CompareStringString { get; init; } + public IMethodSymbol? CompareStringStringBool { get; init; } + public IMethodSymbol? CompareStringStringStringComparison { get; init; } + public IMethodSymbol? EqualsStringString { get; init; } + public IMethodSymbol? EqualsStringStringStringComparison { get; init; } + } + + /// + /// If the specified : + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is any + /// + /// then this method returns the . + /// Otherwise, returns null. + /// + /// + /// + internal static IInvocationOperation? GetInvocationFromEqualityCheckWithLiteralZero(IBinaryOperation binaryOperation) + { + if (binaryOperation.OperatorKind is not (BinaryOperatorKind.Equals or BinaryOperatorKind.NotEquals)) + return default; + + if (IsLiteralZero(binaryOperation.LeftOperand)) + return binaryOperation.RightOperand as IInvocationOperation; + else if (IsLiteralZero(binaryOperation.RightOperand)) + return binaryOperation.LeftOperand as IInvocationOperation; + else + return default; + + // Local functions + + static bool IsLiteralZero(IOperation? operation) + { + return operation is ILiteralOperation literal && literal.ConstantValue.Value is 0; + } + } + + /// + /// Returns true if the specified : + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is any invocation of + /// + /// + /// + /// + /// + internal static bool IsStringStringCase(IBinaryOperation binaryOperation, RequiredSymbols symbols) + { + var invocation = GetInvocationFromEqualityCheckWithLiteralZero(binaryOperation); + + return invocation is not null && + // Don't report a diagnostic if either the string.Compare overload or the + // corrasponding string.Equals overload is missing. + symbols.CompareStringString is not null && + symbols.EqualsStringString is not null && + invocation.TargetMethod.Equals(symbols.CompareStringString, SymbolEqualityComparer.Default); + } + + /// + /// Returns true if the specified : + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is an invocation of + /// The ignoreCase argument is a boolean literal + /// + /// + /// + /// + /// + internal static bool IsStringStringBoolCase(IBinaryOperation binaryOperation, RequiredSymbols symbols) + { + var invocation = GetInvocationFromEqualityCheckWithLiteralZero(binaryOperation); + + return invocation is not null && + // Don't report a diagnostic if either the string.Compare overload or the + // corrasponding string.Equals overload is missing. + symbols.CompareStringStringBool is not null && + symbols.EqualsStringStringStringComparison is not null && + invocation.TargetMethod.Equals(symbols.CompareStringStringBool) && + // Only report a diagnostic if the 'ignoreCase' argument is a boolean literal. + invocation.Arguments.GetArgumentForParameterAtIndex(2).Value is ILiteralOperation literal && + literal.ConstantValue.Value is bool; } + + /// + /// Returns true if the specified : + /// + /// Is an equals or not-equals operation + /// One operand is a literal zero + /// The other operand is any invocation of + /// + /// + /// + /// + /// + internal static bool IsStringStringStringComparisonCase(IBinaryOperation binaryOperation, RequiredSymbols symbols) + { + var invocation = GetInvocationFromEqualityCheckWithLiteralZero(binaryOperation); + + return invocation is not null && + // Don't report a diagnostic if either the string.Compare overload or the + // corrasponding string.Equals overload is missing. + symbols.CompareStringStringStringComparison is not null && + symbols.EqualsStringStringStringComparison is not null && + invocation.TargetMethod.Equals(symbols.CompareStringStringStringComparison, SymbolEqualityComparer.Default); + } + + // No IOperation instances are being stored here. +#pragma warning disable RS1008 // Avoid storing per-compilation data into the fields of a diagnostic analyzer + private static readonly ImmutableArray> CaseSelectors = +#pragma warning restore RS1008 // Avoid storing per-compilation data into the fields of a diagnostic analyzer + ImmutableArray.Create>( + IsStringStringCase, + IsStringStringBoolCase, + IsStringStringStringComparisonCase); } } From 0c0d78845ce800e0f2c6b216eebf460b5bfc6d24 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Wed, 19 May 2021 17:16:15 -0400 Subject: [PATCH 07/10] Run msbuild --- src/NetAnalyzers/RulesMissingDocumentation.md | 1 - 1 file changed, 1 deletion(-) diff --git a/src/NetAnalyzers/RulesMissingDocumentation.md b/src/NetAnalyzers/RulesMissingDocumentation.md index bb87a483b2..c4389d6a65 100644 --- a/src/NetAnalyzers/RulesMissingDocumentation.md +++ b/src/NetAnalyzers/RulesMissingDocumentation.md @@ -8,5 +8,4 @@ CA1840 | | Do not use 'WhenAll' with a single task | CA1843 | | Do not use 'WaitAll' with a single task | CA1844 | | Provide memory-based overrides of async methods when subclassing 'Stream' | -CA1845 | | Use span-based 'string.Concat' | CA2250 | | Use 'string.Equals' | From 579f683e35e6c0517504e04d17922d555a9fc358 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Thu, 20 May 2021 13:27:05 -0400 Subject: [PATCH 08/10] Change diagnostic id to CA2251 --- src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md | 2 +- .../Runtime/UseStringEqualsOverStringCompare.cs | 2 +- src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md | 2 +- src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif | 6 +++--- src/NetAnalyzers/RulesMissingDocumentation.md | 2 +- src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt | 2 +- 6 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md b/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md index dcae8d294b..3ecd052626 100644 --- a/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md +++ b/src/NetAnalyzers/Core/AnalyzerReleases.Unshipped.md @@ -12,7 +12,7 @@ CA1842 | Performance | Info | DoNotUseWhenAllOrWaitAllWithSingleArgument, [Docum CA1843 | Performance | Info | DoNotUseWhenAllOrWaitAllWithSingleArgument, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1843) CA1844 | Performance | Info | ProvideStreamMemoryBasedAsyncOverrides, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1844) CA1845 | Performance | Info | UseSpanBasedStringConcat, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1845) -CA2250 | Usage | Hidden | UseStringEqualsOverStringCompare, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2250) +CA2251 | Usage | Hidden | UseStringEqualsOverStringCompare, [Documentation](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2251) ### Removed Rules diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs index 09af19157d..4ef4873c0f 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs @@ -26,7 +26,7 @@ namespace Microsoft.NetCore.Analyzers.Runtime [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)] public sealed class UseStringEqualsOverStringCompare : DiagnosticAnalyzer { - internal const string RuleId = "CA2250"; + internal const string RuleId = "CA2251"; private static readonly LocalizableString s_localizableTitle = new LocalizableResourceString(nameof(Resx.UseStringEqualsOverStringCompareTitle), Resx.ResourceManager, typeof(Resx)); private static readonly LocalizableString s_localizableMessage = new LocalizableResourceString(nameof(Resx.UseStringEqualsOverStringCompareMessage), Resx.ResourceManager, typeof(Resx)); diff --git a/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md b/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md index 2c1851c5b6..bf40946a07 100644 --- a/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md +++ b/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.md @@ -1944,7 +1944,7 @@ Calls to 'string.IndexOf' where the result is used to check for the presence/abs |CodeFix|True| --- -## [CA2250](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2250): Use 'string.Equals' +## [CA2251](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2251): Use 'string.Equals' It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero. diff --git a/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif b/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif index 43f51a604a..a4c71ac1d6 100644 --- a/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif +++ b/src/NetAnalyzers/Microsoft.CodeAnalysis.NetAnalyzers.sarif @@ -3430,12 +3430,12 @@ ] } }, - "CA2250": { - "id": "CA2250", + "CA2251": { + "id": "CA2251", "shortDescription": "Use 'string.Equals'", "fullDescription": "It is both clearer and likely faster to use 'string.Equals' instead of comparing the result of 'string.Compare' to zero.", "defaultLevel": "hidden", - "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2250", + "helpUri": "https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca2251", "properties": { "category": "Usage", "isEnabledByDefault": true, diff --git a/src/NetAnalyzers/RulesMissingDocumentation.md b/src/NetAnalyzers/RulesMissingDocumentation.md index c4389d6a65..222417efe0 100644 --- a/src/NetAnalyzers/RulesMissingDocumentation.md +++ b/src/NetAnalyzers/RulesMissingDocumentation.md @@ -8,4 +8,4 @@ CA1840 | | Do not use 'WhenAll' with a single task | CA1843 | | Do not use 'WaitAll' with a single task | CA1844 | | Provide memory-based overrides of async methods when subclassing 'Stream' | -CA2250 | | Use 'string.Equals' | +CA2251 | | Use 'string.Equals' | diff --git a/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt b/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt index 7b44225310..758fb4e7c7 100644 --- a/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt +++ b/src/Utilities/Compiler/DiagnosticCategoryAndIdRanges.txt @@ -14,7 +14,7 @@ Globalization: CA2101, CA1300-CA1310 Mobility: CA1600-CA1601 Performance: HA, CA1800-CA1845 Security: CA2100-CA2153, CA2300-CA2330, CA3000-CA3147, CA5300-CA5403 -Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2250 +Usage: CA1801, CA1806, CA1816, CA2200-CA2209, CA2211-CA2251 Naming: CA1700-CA1726 Interoperability: CA1400-CA1418 Maintainability: CA1500-CA1509 From 237184469dbb55a66347a8e23bb187d141250b0b Mon Sep 17 00:00:00 2001 From: NewellClark Date: Fri, 21 May 2021 17:54:48 -0400 Subject: [PATCH 09/10] Apply suggested changes --- .../UseStringEqualsOverStringCompare.Fixer.cs | 15 ++- .../UseStringEqualsOverStringCompare.cs | 100 +++++++++++------- 2 files changed, 70 insertions(+), 45 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs index 5dd804e5f6..3e453c2cc8 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs @@ -22,6 +22,8 @@ namespace Microsoft.NetCore.Analyzers.Runtime [ExportCodeFixProvider(LanguageNames.CSharp, LanguageNames.VisualBasic), Shared] public sealed class UseStringEqualsOverStringCompareFixer : CodeFixProvider { + public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(UseStringEqualsOverStringCompare.RuleId); + public override async Task RegisterCodeFixesAsync(CodeFixContext context) { var document = context.Document; @@ -32,15 +34,12 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) return; var root = await document.GetSyntaxRootAsync(token).ConfigureAwait(false); - - if (semanticModel.GetOperation(root.FindNode(context.Span, getInnermostNodeForTie: true), token) is not IBinaryOperation violation) + var node = root.FindNode(context.Span, getInnermostNodeForTie: true); + if (semanticModel.GetOperation(node, token) is not IBinaryOperation violation) return; - // Get the replacer that applies to the reported violation. Bail out if no replacer - // matches the reported violation. - var replacer = GetOperationReplacers(symbols).FirstOrDefault(x => x.IsMatch(violation)); - if (replacer is null) - return; + // Get the replacer that applies to the reported violation. + var replacer = GetOperationReplacers(symbols).First(x => x.IsMatch(violation)); var codeAction = CodeAction.Create( Resx.UseStringEqualsOverStringCompareCodeFixTitle, @@ -61,8 +60,6 @@ async Task CreateChangedDocument(CancellationToken token) } } - public override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(UseStringEqualsOverStringCompare.RuleId); - public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; private static ImmutableArray GetOperationReplacers(RequiredSymbols symbols) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs index 4ef4873c0f..98b59abdfa 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.cs @@ -76,10 +76,25 @@ void AnalyzeOperation(OperationAnalysisContext context) internal sealed class RequiredSymbols { - // Named-constructor 'TryGetSymbols' inits all properties or doesn't construct an instance. -#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable. - private RequiredSymbols() { } -#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable. + private RequiredSymbols( + INamedTypeSymbol stringType, + INamedTypeSymbol boolType, + INamedTypeSymbol stringComparisonType, + IMethodSymbol? compareStringString, + IMethodSymbol? compareStringStringBool, + IMethodSymbol? compareStringStringStringComparison, + IMethodSymbol? equalsStringString, + IMethodSymbol? equalsStringStringStringComparison) + { + StringType = stringType; + BoolType = boolType; + StringComparisonType = stringComparisonType; + CompareStringString = compareStringString; + CompareStringStringBool = compareStringStringBool; + CompareStringStringStringComparison = compareStringStringStringComparison; + EqualsStringString = equalsStringString; + EqualsStringStringStringComparison = equalsStringStringStringComparison; + } public static bool TryGetSymbols(Compilation compilation, [NotNullWhen(true)] out RequiredSymbols? symbols) { @@ -107,28 +122,29 @@ public static bool TryGetSymbols(Compilation compilation, [NotNullWhen(true)] ou var equalsStringString = equalsMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType); var equalsStringStringStringComparison = equalsMethods.GetFirstOrDefaultMemberWithParameterTypes(stringType, stringType, stringComparisonType); - symbols = new RequiredSymbols + // Bail if we do not have at least one complete pair of Compare-Equals methods in the compilation. + if ((compareStringString is null || equalsStringString is null) && + (compareStringStringBool is null || equalsStringStringStringComparison is null) && + (compareStringStringStringComparison is null || equalsStringStringStringComparison is null)) { - StringType = stringType, - BoolType = boolType, - StringComparisonType = stringComparisonType, - CompareStringString = compareStringString, - CompareStringStringBool = compareStringStringBool, - CompareStringStringStringComparison = compareStringStringStringComparison, - EqualsStringString = equalsStringString, - EqualsStringStringStringComparison = equalsStringStringStringComparison - }; + return false; + } + + symbols = new RequiredSymbols( + stringType, boolType, stringComparisonType, + compareStringString, compareStringStringBool, compareStringStringStringComparison, + equalsStringString, equalsStringStringStringComparison); return true; } - public INamedTypeSymbol StringType { get; init; } - public INamedTypeSymbol BoolType { get; init; } - public INamedTypeSymbol StringComparisonType { get; init; } - public IMethodSymbol? CompareStringString { get; init; } - public IMethodSymbol? CompareStringStringBool { get; init; } - public IMethodSymbol? CompareStringStringStringComparison { get; init; } - public IMethodSymbol? EqualsStringString { get; init; } - public IMethodSymbol? EqualsStringStringStringComparison { get; init; } + public INamedTypeSymbol StringType { get; } + public INamedTypeSymbol BoolType { get; } + public INamedTypeSymbol StringComparisonType { get; } + public IMethodSymbol? CompareStringString { get; } + public IMethodSymbol? CompareStringStringBool { get; } + public IMethodSymbol? CompareStringStringStringComparison { get; } + public IMethodSymbol? EqualsStringString { get; } + public IMethodSymbol? EqualsStringStringStringComparison { get; } } /// @@ -176,13 +192,17 @@ static bool IsLiteralZero(IOperation? operation) /// internal static bool IsStringStringCase(IBinaryOperation binaryOperation, RequiredSymbols symbols) { + // Don't report a diagnostic if either the string.Compare overload or the + // corrasponding string.Equals overload is missing. + if (symbols.CompareStringString is null || + symbols.EqualsStringString is null) + { + return false; + } + var invocation = GetInvocationFromEqualityCheckWithLiteralZero(binaryOperation); return invocation is not null && - // Don't report a diagnostic if either the string.Compare overload or the - // corrasponding string.Equals overload is missing. - symbols.CompareStringString is not null && - symbols.EqualsStringString is not null && invocation.TargetMethod.Equals(symbols.CompareStringString, SymbolEqualityComparer.Default); } @@ -200,15 +220,19 @@ symbols.EqualsStringString is not null && /// internal static bool IsStringStringBoolCase(IBinaryOperation binaryOperation, RequiredSymbols symbols) { + // Don't report a diagnostic if either the string.Compare overload or the + // corrasponding string.Equals overload is missing. + if (symbols.CompareStringStringBool is null || + symbols.EqualsStringStringStringComparison is null) + { + return false; + } + var invocation = GetInvocationFromEqualityCheckWithLiteralZero(binaryOperation); + // Only report a diagnostic if the 'ignoreCase' argument is a boolean literal. return invocation is not null && - // Don't report a diagnostic if either the string.Compare overload or the - // corrasponding string.Equals overload is missing. - symbols.CompareStringStringBool is not null && - symbols.EqualsStringStringStringComparison is not null && - invocation.TargetMethod.Equals(symbols.CompareStringStringBool) && - // Only report a diagnostic if the 'ignoreCase' argument is a boolean literal. + invocation.TargetMethod.Equals(symbols.CompareStringStringBool, SymbolEqualityComparer.Default) && invocation.Arguments.GetArgumentForParameterAtIndex(2).Value is ILiteralOperation literal && literal.ConstantValue.Value is bool; } @@ -226,13 +250,17 @@ symbols.EqualsStringStringStringComparison is not null && /// internal static bool IsStringStringStringComparisonCase(IBinaryOperation binaryOperation, RequiredSymbols symbols) { + // Don't report a diagnostic if either the string.Compare overload or the + // corrasponding string.Equals overload is missing. + if (symbols.CompareStringStringStringComparison is null || + symbols.EqualsStringStringStringComparison is null) + { + return false; + } + var invocation = GetInvocationFromEqualityCheckWithLiteralZero(binaryOperation); return invocation is not null && - // Don't report a diagnostic if either the string.Compare overload or the - // corrasponding string.Equals overload is missing. - symbols.CompareStringStringStringComparison is not null && - symbols.EqualsStringStringStringComparison is not null && invocation.TargetMethod.Equals(symbols.CompareStringStringStringComparison, SymbolEqualityComparer.Default); } From 6562d144c82d7c37b559d9e3bb16d938c541bd09 Mon Sep 17 00:00:00 2001 From: NewellClark Date: Fri, 21 May 2021 18:16:15 -0400 Subject: [PATCH 10/10] Assert if symbols can't be loaded in fixer --- .../Runtime/UseStringEqualsOverStringCompare.Fixer.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs index 3e453c2cc8..df579cd385 100644 --- a/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs +++ b/src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Runtime/UseStringEqualsOverStringCompare.Fixer.cs @@ -30,8 +30,8 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context) var token = context.CancellationToken; var semanticModel = await document.GetSemanticModelAsync(token).ConfigureAwait(false); - if (!RequiredSymbols.TryGetSymbols(semanticModel.Compilation, out var symbols)) - return; + _ = RequiredSymbols.TryGetSymbols(semanticModel.Compilation, out var symbols); + RoslynDebug.Assert(symbols is not null); var root = await document.GetSyntaxRootAsync(token).ConfigureAwait(false); var node = root.FindNode(context.Span, getInnermostNodeForTie: true);