Skip to content

Commit

Permalink
Add an analyzer for Debug.Assert (#7416)
Browse files Browse the repository at this point in the history
* Add an analyzer for Debug.Assert

As @jaredpar found in dotnet/roslyn#75163, interpolated strings in `Debug.Assert` can consume a surprising amount of memory. On modern .NET, this is fine; `Debug.Assert` has an interpolated string handler that will avoid the allocations if the assert isn't triggered. However, on our framework tests, this can be very bad and OOM our tests. So, this analyzer looks for cases where interpolated strings are passed to `Debug.Assert`, and recommends moving over to `RoslynDebug.Assert` instead, which is an interpolated string handler on all platforms. Note that I only did C# support, as there's no equivalent handler API for VB.

* Make sure RoslynDebug is available in one test.

* Trailing whitespace

* Run pack.

* Remove VB

* Pack again

* Use batch fixall provider

* Avoid passing empty string as command line argument

See microsoft/dotnet#1062

* Don't warn for constant strings.

* Apply suggestions from code review

Co-authored-by: Jared Parsons <[email protected]>

---------

Co-authored-by: Sam Harwell <[email protected]>
Co-authored-by: Jared Parsons <[email protected]>
  • Loading branch information
3 people authored Sep 26, 2024
1 parent 8d4f432 commit 1d3c176
Show file tree
Hide file tree
Showing 25 changed files with 603 additions and 4 deletions.
8 changes: 6 additions & 2 deletions eng/GenerateAnalyzerNuspec.targets
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
<GenerateAnalyzerRulesMissingDocumentationFile Condition="'$(Sign)' == 'true'">false</GenerateAnalyzerRulesMissingDocumentationFile>
</PropertyGroup>

<PropertyGroup Condition="'$(ReleaseTrackingOptOut)' == ''">
<ReleaseTrackingOptOut>false</ReleaseTrackingOptOut>
</PropertyGroup>

<PropertyGroup Condition="'$(GeneratePackagePropsFile)' == 'true'">
<PackagePropsFileDir>$(IntermediateOutputPath)Build</PackagePropsFileDir>
<PackagePropsFileName>$(NuspecPackageId).props</PackagePropsFileName>
Expand Down Expand Up @@ -108,9 +112,9 @@

<!-- Only run validate only in CI builds. Running them in local builds will prevent refreshing auto-generated files. -->
<Exec Condition="'$(ContinuousIntegrationBuild)' == 'true'"
Command='"$(DotNetExecutable)" --roll-forward major "$(_GenerateDocumentationAndConfigFilesPath)" "-validateOnly:true" "$(_GeneratedRulesetsDir)" "$(_GeneratedEditorconfigsDir)" "$(_GeneratedGlobalAnalyzerConfigsDir)" "$(ArtifactsBinDir)$(EscapeDirectorySuffix)" "$(Configuration)" "%(AnalyzerRulesetAssembly.TargetFramework)" "@(AnalyzerRulesetAssembly)" "$(PackagePropsFileDir)" "$(PackagePropsFileName)" "$(PackageTargetsFileDir)" "$(PackageTargetsFileName)" "$(DisableNETAnalyzersPackagePropsFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerDocumentationFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerSarifFileName)" "$(VersionPrefix)" $(NuspecPackageId) $(ContainsPortedFxCopRules) $(GenerateAnalyzerRulesMissingDocumentationFile) "$(ReleaseTrackingOptOut)" $(_ValidateOffline)' />
Command='"$(DotNetExecutable)" --roll-forward major "$(_GenerateDocumentationAndConfigFilesPath)" "-validateOnly:true" "$(_GeneratedRulesetsDir)" "$(_GeneratedEditorconfigsDir)" "$(_GeneratedGlobalAnalyzerConfigsDir)" "$(ArtifactsBinDir)$(EscapeDirectorySuffix)" "$(Configuration)" "%(AnalyzerRulesetAssembly.TargetFramework)" "@(AnalyzerRulesetAssembly)" "$(PackagePropsFileDir)" "$(PackagePropsFileName)" "$(PackageTargetsFileDir)" "$(PackageTargetsFileName)" "$(DisableNETAnalyzersPackagePropsFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerDocumentationFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerSarifFileName)" "$(VersionPrefix)" $(NuspecPackageId) $(ContainsPortedFxCopRules) $(GenerateAnalyzerRulesMissingDocumentationFile) $(ReleaseTrackingOptOut) $(_ValidateOffline)' />

<Exec Command='"$(DotNetExecutable)" --roll-forward major "$(_GenerateDocumentationAndConfigFilesPath)" "-validateOnly:false" "$(_GeneratedRulesetsDir)" "$(_GeneratedEditorconfigsDir)" "$(_GeneratedGlobalAnalyzerConfigsDir)" "$(ArtifactsBinDir)$(EscapeDirectorySuffix)" "$(Configuration)" "%(AnalyzerRulesetAssembly.TargetFramework)" "@(AnalyzerRulesetAssembly)" "$(PackagePropsFileDir)" "$(PackagePropsFileName)" "$(PackageTargetsFileDir)" "$(PackageTargetsFileName)" "$(DisableNETAnalyzersPackagePropsFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerDocumentationFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerSarifFileName)" "$(VersionPrefix)" $(NuspecPackageId) $(ContainsPortedFxCopRules) $(GenerateAnalyzerRulesMissingDocumentationFile) "$(ReleaseTrackingOptOut)" $(_ValidateOffline)' />
<Exec Command='"$(DotNetExecutable)" --roll-forward major "$(_GenerateDocumentationAndConfigFilesPath)" "-validateOnly:false" "$(_GeneratedRulesetsDir)" "$(_GeneratedEditorconfigsDir)" "$(_GeneratedGlobalAnalyzerConfigsDir)" "$(ArtifactsBinDir)$(EscapeDirectorySuffix)" "$(Configuration)" "%(AnalyzerRulesetAssembly.TargetFramework)" "@(AnalyzerRulesetAssembly)" "$(PackagePropsFileDir)" "$(PackagePropsFileName)" "$(PackageTargetsFileDir)" "$(PackageTargetsFileName)" "$(DisableNETAnalyzersPackagePropsFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerDocumentationFileName)" "$(AnalyzerSarifFileDir)" "$(AnalyzerSarifFileName)" "$(VersionPrefix)" $(NuspecPackageId) $(ContainsPortedFxCopRules) $(GenerateAnalyzerRulesMissingDocumentationFile) $(ReleaseTrackingOptOut) $(_ValidateOffline)' />

<ItemGroup Condition="Exists('$(PackageTargetsFileDir)\$(PackageTargetsFileName)')">
<AnalyzerNupkgFile Include="$(PackageTargetsFileDir)\$(PackageTargetsFileName)"/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
RS0062 | RoslynDiagnosticsMaintainability | Disabled | DoNotCapturePrimaryConstructorParametersAnalyzer
RS0063 | RoslynDiagnosticsPerformance | Disabled | CSharpDoNotUseDebugAssertForInterpolatedStrings
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System.Collections.Immutable;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Operations;
using Roslyn.Diagnostics.Analyzers;

namespace Roslyn.Diagnostics.CSharp.Analyzers
{
using static RoslynDiagnosticsAnalyzersResources;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class CSharpDoNotUseDebugAssertForInterpolatedStrings : DiagnosticAnalyzer
{
internal static readonly DiagnosticDescriptor Rule = new(
RoslynDiagnosticIds.DoNotUseInterpolatedStringsWithDebugAssertRuleId,
CreateLocalizableResourceString(nameof(DoNotUseInterpolatedStringsWithDebugAssertTitle)),
CreateLocalizableResourceString(nameof(DoNotUseInterpolatedStringsWithDebugAssertMessage)),
DiagnosticCategory.RoslynDiagnosticsPerformance,
DiagnosticSeverity.Warning,
isEnabledByDefault: false,
description: CreateLocalizableResourceString(nameof(DoNotUseInterpolatedStringsWithDebugAssertDescription)),
helpLinkUri: null,
customTags: WellKnownDiagnosticTagsExtensions.Telemetry);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);

public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.EnableConcurrentExecution();

context.RegisterCompilationStartAction(context =>
{
var debugType = context.Compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemDiagnosticsDebug);

if (debugType is null)
{
return;
}

IMethodSymbol? assertMethod = null;

foreach (var member in debugType.GetMembers("Assert"))
{
if (member is IMethodSymbol { Parameters: [{ Type.SpecialType: SpecialType.System_Boolean }, { Type.SpecialType: SpecialType.System_String }] } method)
{
assertMethod = method;
break;
}
}

if (assertMethod is null)
{
return;
}

context.RegisterOperationAction(context =>
{
var invocation = (IInvocationOperation)context.Operation;

if (invocation.TargetMethod.Equals(assertMethod) &&
invocation.Arguments is [_, IArgumentOperation { Value: IInterpolatedStringOperation { ConstantValue.HasValue: false } }])
{
context.ReportDiagnostic(invocation.CreateDiagnostic(Rule));
}
}, OperationKind.Invocation);
});
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information.

using System.Collections.Immutable;
using System.Composition;
using System.Threading;
using System.Threading.Tasks;
using Analyzer.Utilities;
using Analyzer.Utilities.Extensions;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Editing;
using Roslyn.Diagnostics.Analyzers;

namespace Roslyn.Diagnostics.CSharp.Analyzers
{
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(CSharpDoNotUseDebugAssertForInterpolatedStringsFixer))]
[Shared]
public sealed class CSharpDoNotUseDebugAssertForInterpolatedStringsFixer : CodeFixProvider
{
public override ImmutableArray<string> FixableDiagnosticIds { get; } = ImmutableArray.Create(RoslynDiagnosticIds.DoNotUseInterpolatedStringsWithDebugAssertRuleId);

public override async Task RegisterCodeFixesAsync(CodeFixContext context)
{
var compilation = await context.Document.Project.GetCompilationAsync(context.CancellationToken);

if (compilation is null)
{
return;
}

var roslynDebugSymbol = compilation.GetOrCreateTypeByMetadataName(WellKnownTypeNames.RoslynDebug);

if (roslynDebugSymbol is null)
{
return;
}

foreach (var diagnostic in context.Diagnostics)
{
context.RegisterCodeFix(
CodeAction.Create(
RoslynDiagnosticsAnalyzersResources.DoNotUseInterpolatedStringsWithDebugAssertCodeFix,
ct => ReplaceWithDebugAssertAsync(context.Document, diagnostic.Location, roslynDebugSymbol, ct),
equivalenceKey: nameof(CSharpDoNotUseDebugAssertForInterpolatedStringsFixer)),
diagnostic);
}
}

private static async Task<Document> ReplaceWithDebugAssertAsync(Document document, Location location, INamedTypeSymbol roslynDebugSymbol, CancellationToken cancellationToken)
{
var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
var syntax = root.FindNode(location.SourceSpan, getInnermostNodeForTie: true);
var generator = SyntaxGenerator.GetGenerator(document);

if (syntax is not InvocationExpressionSyntax
{
Expression: MemberAccessExpressionSyntax
{
Expression: IdentifierNameSyntax { Identifier.ValueText: "Debug" } debugIdentifierNode,
Name.Identifier.ValueText: "Assert"
},
})
{
return document;
}

var roslynDebugNode = generator.TypeExpression(roslynDebugSymbol)
.WithAddImportsAnnotation()
.WithLeadingTrivia(debugIdentifierNode.GetLeadingTrivia())
.WithTrailingTrivia(debugIdentifierNode.GetTrailingTrivia());

var newRoot = root.ReplaceNode(debugIdentifierNode, roslynDebugNode);
return document.WithSyntaxRoot(newRoot);
}

public override FixAllProvider? GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ internal static class RoslynDiagnosticIds
public const string OverloadWithOptionalParametersShouldHaveMostParametersInternal = "RS0060";
public const string ExposedNoninstantiableTypeRuleIdInternal = "RS0061";
public const string DoNotCapturePrimaryConstructorParametersRuleId = "RS0062";
public const string DoNotUseInterpolatedStringsWithDebugAssertRuleId = "RS0063";

//public const string WrapStatementsRuleId = "RS0100"; // Now ported to dotnet/roslyn https://github.com/dotnet/roslyn/pull/50358
//public const string BlankLinesRuleId = "RS0101"; // Now ported to dotnet/roslyn https://github.com/dotnet/roslyn/pull/50358
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -403,4 +403,16 @@
<data name="DoNotCapturePrimaryConstructorParametersDescription" xml:space="preserve">
<value>Primary constructor parameters should not be implicitly captured. Manually assign them to fields at the start of the type.</value>
</data>
<data name="DoNotUseInterpolatedStringsWithDebugAssertTitle" xml:space="preserve">
<value>Do not use interpolated strings with 'Debug.Assert'</value>
</data>
<data name="DoNotUseInterpolatedStringsWithDebugAssertMessage" xml:space="preserve">
<value>Do not use interpolated strings with 'Debug.Assert'. Use 'RoslynDebug.Assert' instead.</value>
</data>
<data name="DoNotUseInterpolatedStringsWithDebugAssertDescription" xml:space="preserve">
<value>'Debug.Assert' on .NET Framework eagerly creates the string value. This can cause OOMs in tests, particularly for strings that involve syntax nodes. Use 'RoslynDebug.Assert' instead, which will only create the string if required.</value>
</data>
<data name="DoNotUseInterpolatedStringsWithDebugAssertCodeFix" xml:space="preserve">
<value>Use 'RoslynDebug.Assert'.</value>
</data>
</root>
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,26 @@
<target state="translated">Nepodporované použití nekopírovatelného typu {0} v operaci {1}</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertCodeFix">
<source>Use 'RoslynDebug.Assert'.</source>
<target state="new">Use 'RoslynDebug.Assert'.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertDescription">
<source>'Debug.Assert' on .NET Framework eagerly creates the string value. This can cause OOMs in tests, particularly for strings that involve syntax nodes. Use 'RoslynDebug.Assert' instead, which will only create the string if required.</source>
<target state="new">'Debug.Assert' on .NET Framework eagerly creates the string value. This can cause OOMs in tests, particularly for strings that involve syntax nodes. Use 'RoslynDebug.Assert' instead, which will only create the string if required.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertMessage">
<source>Do not use interpolated strings with 'Debug.Assert'. Use 'RoslynDebug.Assert' instead.</source>
<target state="new">Do not use interpolated strings with 'Debug.Assert'. Use 'RoslynDebug.Assert' instead.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertTitle">
<source>Do not use interpolated strings with 'Debug.Assert'</source>
<target state="new">Do not use interpolated strings with 'Debug.Assert'</target>
<note />
</trans-unit>
<trans-unit id="ExportedPartsShouldHaveImportingConstructorCodeFix_ImplicitConstructor">
<source>Explicitly define the importing constructor</source>
<target state="translated">Explicitně definujte importující konstruktor.</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,26 @@
<target state="translated">Nicht unterstützte Verwendung des nicht kopierbaren Typs "{0}" im Vorgang "{1}".</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertCodeFix">
<source>Use 'RoslynDebug.Assert'.</source>
<target state="new">Use 'RoslynDebug.Assert'.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertDescription">
<source>'Debug.Assert' on .NET Framework eagerly creates the string value. This can cause OOMs in tests, particularly for strings that involve syntax nodes. Use 'RoslynDebug.Assert' instead, which will only create the string if required.</source>
<target state="new">'Debug.Assert' on .NET Framework eagerly creates the string value. This can cause OOMs in tests, particularly for strings that involve syntax nodes. Use 'RoslynDebug.Assert' instead, which will only create the string if required.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertMessage">
<source>Do not use interpolated strings with 'Debug.Assert'. Use 'RoslynDebug.Assert' instead.</source>
<target state="new">Do not use interpolated strings with 'Debug.Assert'. Use 'RoslynDebug.Assert' instead.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertTitle">
<source>Do not use interpolated strings with 'Debug.Assert'</source>
<target state="new">Do not use interpolated strings with 'Debug.Assert'</target>
<note />
</trans-unit>
<trans-unit id="ExportedPartsShouldHaveImportingConstructorCodeFix_ImplicitConstructor">
<source>Explicitly define the importing constructor</source>
<target state="translated">Importierenden Konstruktor explizit definieren</target>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,26 @@
<target state="translated">Uso no admitido del tipo "{0}" que no se puede copiar en la operación "{1}"</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertCodeFix">
<source>Use 'RoslynDebug.Assert'.</source>
<target state="new">Use 'RoslynDebug.Assert'.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertDescription">
<source>'Debug.Assert' on .NET Framework eagerly creates the string value. This can cause OOMs in tests, particularly for strings that involve syntax nodes. Use 'RoslynDebug.Assert' instead, which will only create the string if required.</source>
<target state="new">'Debug.Assert' on .NET Framework eagerly creates the string value. This can cause OOMs in tests, particularly for strings that involve syntax nodes. Use 'RoslynDebug.Assert' instead, which will only create the string if required.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertMessage">
<source>Do not use interpolated strings with 'Debug.Assert'. Use 'RoslynDebug.Assert' instead.</source>
<target state="new">Do not use interpolated strings with 'Debug.Assert'. Use 'RoslynDebug.Assert' instead.</target>
<note />
</trans-unit>
<trans-unit id="DoNotUseInterpolatedStringsWithDebugAssertTitle">
<source>Do not use interpolated strings with 'Debug.Assert'</source>
<target state="new">Do not use interpolated strings with 'Debug.Assert'</target>
<note />
</trans-unit>
<trans-unit id="ExportedPartsShouldHaveImportingConstructorCodeFix_ImplicitConstructor">
<source>Explicitly define the importing constructor</source>
<target state="translated">Definir explícitamente el constructor de importación</target>
Expand Down
Loading

0 comments on commit 1d3c176

Please sign in to comment.