Skip to content
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

Add APICompat support for checking generic constraints #40230

Merged
merged 4 commits into from
Apr 19, 2024
Merged
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
Expand Up @@ -28,6 +28,7 @@ public static class DiagnosticIds
public const string CannotAddSealedToInterfaceMember = "CP0018";
public const string CannotReduceVisibility = "CP0019";
public const string CannotExpandVisibility = "CP0020";
public const string CannotChangeGenericConstraint = "CP0021";

// Assembly loading ids
public const string AssemblyReferenceNotFound = "CP1002";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,12 @@
<data name="CannotRemoveAttribute" xml:space="preserve">
<value>Cannot remove attribute '{0}' from '{1}'.</value>
</data>
<data name="CannotAddGenericConstraint" xml:space="preserve">
<value>Cannot add constraint '{0}' on type parameter '{1}' of '{2}'.</value>
</data>
<data name="CannotRemoveGenericConstraint" xml:space="preserve">
<value>Cannot remove constraint '{0}' on type parameter '{1}' of '{2}'.</value>
</data>
<data name="CannotChangeParameterName" xml:space="preserve">
<value>Parameter name on member '{0}' changed from '{1}' to '{2}'.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,159 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Immutable;
using System.Diagnostics;
using System.Runtime;
using Microsoft.CodeAnalysis;

namespace Microsoft.DotNet.ApiCompatibility.Rules
{
/// <summary>
/// This class implements a rule to check that generic constraints on type parameters do not change.
/// TypeParameter names should not change in strict mode, or when specified
/// strict mode: no changes at all
/// sealed types and non-virtual methods: no additions, removals permitted
/// non-sealed types and virtual methods: no changes
/// </summary>
public class CannotChangeGenericConstraints : IRule
{
private readonly IRuleSettings _settings;

public CannotChangeGenericConstraints(IRuleSettings settings, IRuleRegistrationContext context)
{
_settings = settings;
context.RegisterOnMemberSymbolAction(RunOnMemberSymbol);
context.RegisterOnTypeSymbolAction(RunOnTypeSymbol);
}

private void RunOnTypeSymbol(ITypeSymbol? left,
ITypeSymbol? right,
MetadataInformation leftMetadata,
MetadataInformation rightMetadata,
IList<CompatDifference> differences)
{
if (left is not INamedTypeSymbol leftType || right is not INamedTypeSymbol rightType)
{
return;
}

var leftTypeParameters = leftType.TypeParameters;
var rightTypeParameters = rightType.TypeParameters;
ericstj marked this conversation as resolved.
Show resolved Hide resolved

// can remove constraints on sealed classes since no code should observe broader set of type parameters
bool permitConstraintRemoval = !_settings.StrictMode && leftType.IsSealed;

CompareTypeParameters(leftTypeParameters, rightTypeParameters, left, permitConstraintRemoval, leftMetadata, rightMetadata, differences);
}

private void RunOnMemberSymbol(
ISymbol? left,
ISymbol? right,
ITypeSymbol leftContainingType,
ITypeSymbol rightContainingType,
MetadataInformation leftMetadata,
MetadataInformation rightMetadata,
IList<CompatDifference> differences)
{
if (left is not IMethodSymbol leftMethod || right is not IMethodSymbol rightMethod)
{
return;
}

var leftTypeParameters = leftMethod.TypeParameters;
var rightTypeParameters = rightMethod.TypeParameters;

bool permitConstraintRemoval = !_settings.StrictMode && !leftMethod.IsVirtual;

CompareTypeParameters(leftTypeParameters, rightTypeParameters, left, permitConstraintRemoval, leftMetadata, rightMetadata, differences);
}

private void CompareTypeParameters(
ImmutableArray<ITypeParameterSymbol> leftTypeParameters,
ImmutableArray<ITypeParameterSymbol> rightTypeParameters,
ISymbol left,
bool permitConstraintRemoval,
MetadataInformation leftMetadata,
MetadataInformation rightMetadata,
IList<CompatDifference> differences)
{
Debug.Assert(leftTypeParameters.Length == rightTypeParameters.Length);
ViktorHofer marked this conversation as resolved.
Show resolved Hide resolved
for (int i = 0; i < leftTypeParameters.Length; i++)
{
ITypeParameterSymbol leftTypeParam = leftTypeParameters[i];
ITypeParameterSymbol rightTypeParam = rightTypeParameters[i];

List<string> addedConstraints = new();
List<string> removedConstraints = new();

CompareBoolConstraint(typeParam => typeParam.HasConstructorConstraint, "new()");
ericstj marked this conversation as resolved.
Show resolved Hide resolved
CompareBoolConstraint(typeParam => typeParam.HasNotNullConstraint, "notnull");
CompareBoolConstraint(typeParam => typeParam.HasReferenceTypeConstraint, "class");
CompareBoolConstraint(typeParam => typeParam.HasUnmanagedTypeConstraint, "unmanaged");
// unmanaged implies struct
CompareBoolConstraint(typeParam => typeParam.HasValueTypeConstraint & !typeParam.HasUnmanagedTypeConstraint, "struct");

HashSet<ISymbol> rightOnlyConstraints = rightTypeParam.ConstraintTypes.ToHashSet(_settings.SymbolEqualityComparer);
rightOnlyConstraints.ExceptWith(leftTypeParam.ConstraintTypes);
ViktorHofer marked this conversation as resolved.
Show resolved Hide resolved

// we could allow an addition if removals are allowed, and the addition is a less-derived base type or interface
// for example: changing a constraint from MemoryStream to Stream on a sealed type, or non-virtual member
// but we'll leave this to suppressions

addedConstraints.AddRange(rightOnlyConstraints.Select(x => x.GetDocumentationCommentId()!));

// additions
foreach(var addedConstraint in addedConstraints)
{
differences.Add(new CompatDifference(
leftMetadata,
rightMetadata,
DiagnosticIds.CannotChangeGenericConstraint,
string.Format(Resources.CannotAddGenericConstraint, addedConstraint, leftTypeParam, left),
DifferenceType.Added,
$"{left.GetDocumentationCommentId()}``{i}:{addedConstraint}"));
}

// removals
// we could allow a removal in the case of reducing to more-derived interfaces if those interfaces were previous constraints
// for example if IB : IA and a type is constrained by both IA and IB, it's safe to remove IA since it's implied by IB
// but we'll leave this to suppressions

if (!permitConstraintRemoval)
{
HashSet<ISymbol> leftOnlyConstraints = leftTypeParam.ConstraintTypes.ToHashSet(_settings.SymbolEqualityComparer);
leftOnlyConstraints.ExceptWith(rightTypeParam.ConstraintTypes);
removedConstraints.AddRange(leftOnlyConstraints.Select(x => x.GetDocumentationCommentId()!));

foreach (var removedConstraint in removedConstraints)
{
differences.Add(new CompatDifference(
leftMetadata,
rightMetadata,
DiagnosticIds.CannotChangeGenericConstraint,
string.Format(Resources.CannotRemoveGenericConstraint, removedConstraint, leftTypeParam, left),
DifferenceType.Removed,
$"{left.GetDocumentationCommentId()}``{i}:{removedConstraint}"));
}
}

void CompareBoolConstraint(Func<ITypeParameterSymbol, bool> boolConstraint, string constraintName)
{
bool leftBoolConstraint = boolConstraint(leftTypeParam);
bool rightBoolConstraint = boolConstraint(rightTypeParam);

// addition
if (!leftBoolConstraint && rightBoolConstraint)
{
addedConstraints.Add(constraintName);
}
// removal
else if (!permitConstraintRemoval && leftBoolConstraint && !rightBoolConstraint)
{
removedConstraints.Add(constraintName);
}
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ public IRule[] CreateRules(IRuleSettings settings, IRuleRegistrationContext cont
new CannotSealType(settings, context),
new EnumsMustMatch(settings, context),
new MembersMustExist(settings, context),
new CannotChangeVisibility(settings, context)
new CannotChangeVisibility(settings, context),
new CannotChangeGenericConstraints(settings, context),
};

if (enableRuleAttributesMustMatch)
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading