Skip to content

Add Message to suppressions and simplify ISuppressibleLog #45951

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Globalization;
using Microsoft.DotNet.ApiCompatibility;
using Microsoft.DotNet.ApiCompatibility.Logging;
using Microsoft.DotNet.ApiCompatibility.Rules;
Expand All @@ -21,6 +22,7 @@ public static int Run(Func<ISuppressionEngine, ISuppressibleLog> logFactory,
bool enableRuleAttributesMustMatch,
string[]? excludeAttributesFiles,
bool enableRuleCannotChangeParameterName,
string suppressionCulture,
string[] leftAssemblies,
string[] rightAssemblies,
bool enableStrictMode,
Expand All @@ -30,6 +32,16 @@ public static int Run(Func<ISuppressionEngine, ISuppressibleLog> logFactory,
(string CaptureGroupPattern, string ReplacementString)[]? leftAssembliesTransformationPatterns,
(string CaptureGroupPattern, string ReplacementString)[]? rightAssembliesTransformationPatterns)
{
// When generating the suppression file and baselining all errors, change the resource language
// to neutral. This guarantees that suppression files aren't language specific.
if (generateSuppressionFile)
{
CultureInfo suppressionCultureInfo = new(suppressionCulture);
Resources.Culture = suppressionCultureInfo;
CommonResources.Culture = suppressionCultureInfo;
ApiCompatibility.ResourceSingleton.ChangeCulture(suppressionCultureInfo);
}

// Initialize the service provider
ApiCompatServiceProvider serviceProvider = new(logFactory,
() => SuppressionFileHelper.CreateSuppressionEngine(suppressionFiles, noWarn, generateSuppressionFile),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Globalization;
using Microsoft.DotNet.ApiCompatibility.Logging;
using Microsoft.DotNet.ApiCompatibility.Rules;
using Microsoft.DotNet.PackageValidation;
Expand All @@ -23,6 +24,7 @@ public static int Run(Func<ISuppressionEngine, ISuppressibleLog> logFactory,
bool enableRuleAttributesMustMatch,
string[]? excludeAttributesFiles,
bool enableRuleCannotChangeParameterName,
string suppressionCulture,
string? packagePath,
bool runApiCompat,
bool enableStrictModeForCompatibleTfms,
Expand All @@ -34,6 +36,22 @@ public static int Run(Func<ISuppressionEngine, ISuppressibleLog> logFactory,
IReadOnlyDictionary<NuGetFramework, IEnumerable<string>>? baselinePackageAssemblyReferences,
string[]? baselinePackageFrameworksToIgnore)
{
// When generating the suppression file and baselining all errors, change the resource language
// to neutral. This guarantees that suppression files aren't language specific.
if (generateSuppressionFile)
{
CultureInfo suppressionCultureInfo = new(suppressionCulture);
Resources.Culture = suppressionCultureInfo;
CommonResources.Culture = suppressionCultureInfo;
PackageValidation.ResourceSingleton.ChangeCulture(suppressionCultureInfo);
}

// If a runtime graph is provided, parse and use it for asset selection during the in-memory package construction.
if (runtimeGraph != null)
{
Package.InitializeRuntimeGraph(runtimeGraph);
}

// Initialize the service provider
ApiCompatServiceProvider serviceProvider = new(logFactory,
() => SuppressionFileHelper.CreateSuppressionEngine(suppressionFiles, noWarn, generateSuppressionFile),
Expand All @@ -43,12 +61,6 @@ public static int Run(Func<ISuppressionEngine, ISuppressibleLog> logFactory,
respectInternals,
excludeAttributesFiles);

// If a runtime graph is provided, parse and use it for asset selection during the in-memory package construction.
if (runtimeGraph != null)
{
Package.InitializeRuntimeGraph(runtimeGraph);
}

// Create the in-memory representation of the passed in package path
Package package = Package.Create(packagePath, packageAssemblyReferences);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,24 @@ internal sealed class SuppressibleMSBuildLog(NET.Build.Tasks.Logger log,
public bool HasLoggedErrorSuppressions { get; private set; }

/// <inheritdoc />
public bool LogError(Suppression suppression, string code, string message)
public bool LogError(Suppression suppression)
{
if (suppressionEngine.IsErrorSuppressed(suppression))
return false;

HasLoggedErrorSuppressions = true;
LogError(code, message);
LogError(suppression.DiagnosticId, suppression.Message);

return true;
}

/// <inheritdoc />
public bool LogWarning(Suppression suppression, string code, string message)
public bool LogWarning(Suppression suppression)
{
if (suppressionEngine.IsErrorSuppressed(suppression))
return false;

LogWarning(code, message);
LogWarning(suppression.DiagnosticId, suppression.Message);

return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@ public class ValidateAssembliesTask : TaskBase
/// </summary>
public bool EnableRuleCannotChangeParameterName { get; set; }

/// <summary>
/// The culture to use in suppressions when <see cref="GenerateSuppressionFile"/> is true. Defaults to en-US.
/// </summary>
public string SuppressionCulture { get; set; } = "en-US";

/// <summary>
/// Performs api comparison checks in strict mode.
/// </summary>
Expand Down Expand Up @@ -144,6 +149,7 @@ protected override void ExecuteCore()
EnableRuleAttributesMustMatch,
ExcludeAttributesFiles,
EnableRuleCannotChangeParameterName,
SuppressionCulture,
LeftAssemblies!,
RightAssemblies!,
EnableStrictMode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,11 @@ public class ValidatePackageTask : TaskBase
/// </summary>
public string? NoWarn { get; set; }

/// <summary>
/// The culture to use in suppressions when <see cref="GenerateSuppressionFile"/> is true. Defaults to en-US.
/// </summary>
public string SuppressionCulture { get; set; } = "en-US";

/// <summary>
/// Assembly references grouped by target framework, for the assets inside the package.
/// </summary>
Expand Down Expand Up @@ -152,6 +157,7 @@ protected override void ExecuteCore()
EnableRuleAttributesMustMatch,
ExcludeAttributesFiles,
EnableRuleCannotChangeParameterName,
SuppressionCulture,
PackageTargetPath!,
RunApiCompat,
EnableStrictModeForCompatibleTfms,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
EnableRuleAttributesMustMatch="$(ApiCompatEnableRuleAttributesMustMatch)"
ExcludeAttributesFiles="@(ApiCompatExcludeAttributesFile)"
EnableRuleCannotChangeParameterName="$(ApiCompatEnableRuleCannotChangeParameterName)"
SuppressionCulture="$(ApiCompatSuppressionCulture)"
EnableStrictMode="$(ApiCompatStrictMode)"
LeftAssembliesReferences="@(ApiCompatLeftAssembliesReferences)"
RightAssembliesReferences="@(ApiCompatRightAssembliesReferences)"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,12 @@ static int Main(string[] args)
Description = "If true, enables rule to check that the parameter names between public methods do not change.",
Recursive = true
};
CliOption<string> suppressionCultureOption = new("--suppression-culture")
{
Description = "The culture to use in suppressions when the '--generate-suppression-file' argument is supplied. Defaults to en-US.",
Recursive = true,
DefaultValueFactory = _ => "en-US"
};

// Root command
CliOption<string[]> leftAssembliesOption = new("--left-assembly", "--left", "-l")
Expand Down Expand Up @@ -158,6 +164,7 @@ static int Main(string[] args)
rootCommand.Options.Add(enableRuleAttributesMustMatchOption);
rootCommand.Options.Add(excludeAttributesFilesOption);
rootCommand.Options.Add(enableRuleCannotChangeParameterNameOption);
rootCommand.Options.Add(suppressionCultureOption);

rootCommand.Options.Add(leftAssembliesOption);
rootCommand.Options.Add(rightAssembliesOption);
Expand Down Expand Up @@ -186,6 +193,7 @@ static int Main(string[] args)
bool enableRuleAttributesMustMatch = parseResult.GetValue(enableRuleAttributesMustMatchOption);
string[]? excludeAttributesFiles = parseResult.GetValue(excludeAttributesFilesOption);
bool enableRuleCannotChangeParameterName = parseResult.GetValue(enableRuleCannotChangeParameterNameOption);
string suppressionCulture = parseResult.GetValue(suppressionCultureOption)!;

string[] leftAssemblies = parseResult.GetValue(leftAssembliesOption)!;
string[] rightAssemblies = parseResult.GetValue(rightAssembliesOption)!;
Expand All @@ -208,6 +216,7 @@ static int Main(string[] args)
enableRuleAttributesMustMatch,
excludeAttributesFiles,
enableRuleCannotChangeParameterName,
suppressionCulture,
leftAssemblies,
rightAssemblies,
strictMode,
Expand Down Expand Up @@ -308,6 +317,7 @@ static int Main(string[] args)
bool enableRuleAttributesMustMatch = parseResult.GetValue(enableRuleAttributesMustMatchOption);
string[]? excludeAttributesFiles = parseResult.GetValue(excludeAttributesFilesOption);
bool enableRuleCannotChangeParameterName = parseResult.GetValue(enableRuleCannotChangeParameterNameOption);
string suppressionCulture = parseResult.GetValue(suppressionCultureOption)!;

string? package = parseResult.GetValue(packageArgument);
bool runApiCompat = parseResult.GetValue(runApiCompatOption);
Expand All @@ -332,6 +342,7 @@ static int Main(string[] args)
enableRuleAttributesMustMatch,
excludeAttributesFiles,
enableRuleCannotChangeParameterName,
suppressionCulture,
package,
runApiCompat,
enableStrictModeForCompatibleTfms,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,24 @@ internal sealed class SuppressibleConsoleLog(ISuppressionEngine suppressionEngin
public bool HasLoggedErrorSuppressions { get; private set; }

/// <inheritdoc />
public bool LogError(Suppression suppression, string code, string message)
public bool LogError(Suppression suppression)
{
if (suppressionEngine.IsErrorSuppressed(suppression))
return false;

HasLoggedErrorSuppressions = true;
LogError(code, message);
LogError(suppression.DiagnosticId, suppression.Message);

return true;
}

/// <inheritdoc />
public bool LogWarning(Suppression suppression, string code, string message)
public bool LogWarning(Suppression suppression)
{
if (suppressionEngine.IsErrorSuppressed(suppression))
return false;

LogWarning(code, message);
LogWarning(suppression.DiagnosticId, suppression.Message);

return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,17 @@ public interface ISuppressibleLog : ILog
bool HasLoggedErrorSuppressions { get; }

/// <summary>
/// Log an error based on a passed in suppression, code and message.
/// Log an error based on a passed in suppression.
/// </summary>
/// <param name="suppression">The suppression object which contains the rule information.</param>
/// <param name="code">The suppression code</param>
/// <param name="message">The message</param>
/// <returns>Returns true if the error is logged and not suppressed.</returns>
bool LogError(Suppression suppression, string code, string message);
bool LogError(Suppression suppression);

/// <summary>
/// Log a warning based on the passed in suppression, code and message.
/// Log a warning based on the passed in suppression.
/// </summary>
/// <param name="suppression">The suppression object which contains the rule information.</param>
/// <param name="code">The suppression code</param>
/// <param name="message">The message</param>
/// <returns>Returns true if the warning is logged and not suppressed.</returns>
bool LogWarning(Suppression suppression, string code, string message);
bool LogWarning(Suppression suppression);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ public class Suppression : IEquatable<Suppression>
/// </summary>
public string DiagnosticId { get; set; }

/// <summary>
/// The diagnostic message describing the suppression.
/// </summary>
public string Message { get; set; }

/// <summary>
/// The target of where to suppress the <see cref="DiagnosticId"/>
/// </summary>
Expand All @@ -37,15 +42,18 @@ public class Suppression : IEquatable<Suppression>
private Suppression()
{
DiagnosticId = string.Empty;
Message = string.Empty;
}

public Suppression(string diagnosticId,
string message,
string? target = null,
string? left = null,
string? right = null,
bool isBaselineSuppression = false)
{
DiagnosticId = diagnosticId;
Message = message;
Target = target;
Left = left;
Right = right;
Expand Down Expand Up @@ -74,6 +82,7 @@ public Suppression(string diagnosticId,
/// <inheritdoc/>
public bool Equals(Suppression? other)
{
// Message is intentionally not considered part of the unique object ID as it could contain language specific text.
return other != null &&
AreEqual(DiagnosticId, other.DiagnosticId) &&
AreEqual(Target, other.Target) &&
Expand Down Expand Up @@ -106,6 +115,7 @@ public override string ToString()
}

stringBuilder.Append(DiagnosticId);
stringBuilder.Append(": " + Message);
stringBuilder.Append(" (");

bool requiresDelimiter = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,13 @@ public bool IsErrorSuppressed(Suppression error)
if (error.DiagnosticId.StartsWith("cp", StringComparison.InvariantCultureIgnoreCase))
{
// - DiagnosticId, Target, IsBaselineSuppression
Suppression globalTargetSuppression = new(error.DiagnosticId, error.Target, isBaselineSuppression: error.IsBaselineSuppression);
Suppression globalTargetSuppression = new(error.DiagnosticId, error.Message, error.Target, isBaselineSuppression: error.IsBaselineSuppression);

// - Left, Right, IsBaselineSuppression
Suppression globalLeftRightSuppression = new(string.Empty, left: error.Left, right: error.Right, isBaselineSuppression: error.IsBaselineSuppression);
Suppression globalLeftRightSuppression = new(string.Empty, string.Empty, left: error.Left, right: error.Right, isBaselineSuppression: error.IsBaselineSuppression);

// - DiagnosticId, Left, Right, IsBaselineSuppression
Suppression globalDiagnosticIdLeftRightSuppression = new(error.DiagnosticId, left: error.Left, right: error.Right, isBaselineSuppression: error.IsBaselineSuppression);
Suppression globalDiagnosticIdLeftRightSuppression = new(error.DiagnosticId, error.Message, left: error.Left, right: error.Right, isBaselineSuppression: error.IsBaselineSuppression);

if (_suppressions.Contains(globalTargetSuppression) ||
_suppressions.Contains(globalLeftRightSuppression) ||
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Globalization;

namespace Microsoft.DotNet.ApiCompatibility
{
public static class ResourceSingleton
{
/// <summary>
/// Change the embedded resources culture.
/// </summary>
public static void ChangeCulture(CultureInfo culture) => Resources.Culture = culture;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,12 @@ public void ExecuteWorkItems()

foreach (CompatDifference difference in differenceGroup)
{
Suppression suppression = new(difference.DiagnosticId)
{
Target = difference.ReferenceId,
Left = difference.Left.AssemblyId,
Right = difference.Right.AssemblyId,
IsBaselineSuppression = workItem.Options.IsBaselineComparison
};
Suppression suppression = new(difference.DiagnosticId,
difference.Message,
difference.ReferenceId,
difference.Left.AssemblyId,
difference.Right.AssemblyId,
workItem.Options.IsBaselineComparison);

// If the error is suppressed, don't log anything.
if (suppressionEngine.IsErrorSuppressed(suppression))
Expand All @@ -87,9 +86,7 @@ public void ExecuteWorkItems()
workItem.Options.IsBaselineComparison ? difference.Right.FullPath : "right"));
}

log.LogError(suppression,
difference.DiagnosticId,
difference.Message);
log.LogError(suppression);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,11 @@ private static MetadataInformation GetMetadataInformation(ISuppressibleLog log,
// See if the package's assembly reference entries have the same target framework.
if (!package.AssemblyReferences.TryGetValue(nuGetFramework, out assemblyReferences))
{
log.LogWarning(new Suppression(DiagnosticIds.SearchDirectoriesNotFoundForTfm) { Target = displayString },
DiagnosticIds.SearchDirectoriesNotFoundForTfm,
log.LogWarning(new Suppression(DiagnosticIds.SearchDirectoriesNotFoundForTfm,
string.Format(Resources.MissingSearchDirectory,
nuGetFramework.GetShortFolderName(),
displayString));
displayString),
displayString));
}
}
}
Expand Down
Loading
Loading