ApiCompat tool incorrectly handles RequiredUnreferencedCodeAttribute and possible other AOT attributes.

[m-nash]

In general adding an attribute is considered backwards compatible and removing an attribute requires judgement since it can be a breaking change if external people were relying on its presence.

For RequiredUnreferenceCodeAttribute it works in the opposite direction. Removing it is always backwards compatible and adding it is always a breaking change.

Are we able to update the tool to have special handling for these AOT attributes that we know should work in the opposite direction? @eerhardt mentioned a couple other attributes that also might fit into this bucket.

To reproduce you can add RequiredUnreferenceCodeAttribute to any api in any library that didn't already have it and run apicompat.

Expected: We should see an error saying adding this attribute is not backwards compatible.

Actual: We do not see any errors.

[dotnet-policy-service]

@dotnet/area-infrastructure-libraries a new issue has been filed in the ApiCompat area, please triage

[eerhardt]

FYI - @agocke @sbomer

[agocke]

Sorry, I'm not familiar with the ApiCompat tool. Can someone point me to it?

[eerhardt]

Sorry, I'm not familiar with the ApiCompat tool. Can someone point me to it?

https://learn.microsoft.com/dotnet/fundamentals/apicompat/package-validation/baseline-version-validator is the docs. The tool ensures that a new version of your library is "API compatible" with a previous version of your library.

https://github.com/dotnet/sdk/tree/main/src/Compatibility/ApiCompat is the code. You can see the attribute rules at:

sdk/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/AttributesMustMatch.cs

Lines 12 to 16 in baa6d97

	/// <summary>
	/// This class implements a rule to check that the attributes between public members do not change.
	/// </summary>
	public class AttributesMustMatch : IRule
	{

@ViktorHofer @carlossanlop - is it possible to have custom rules for the trimming and AOT attributes? As @m-nash points out above, adding [RequiredUnreferenceCode] should be considered a breaking change. And removing it should not be considered a breaking change.

[ViktorHofer]

The current implementation of the AttributesMustMatch rule is naive and only checks if an attributes exists or not:

sdk/src/Compatibility/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/AttributesMustMatch.cs

Lines 69 to 168 in baa6d97

	private void ReportAttributeDifferences(ISymbol containing,
	MetadataInformation leftMetadata,
	MetadataInformation rightMetadata,
	string itemRef,
	ImmutableArray<AttributeData> left,
	ImmutableArray<AttributeData> right,
	IList<CompatDifference> differences)
	{
	// Filter out attributes that should be excluded based on the settings.
	// ApiCompat intentionally considers non excluded attribute arguments. See discussion in https://github.com/dotnet/sdk/pull/27774.
	left = left.ExcludeNonVisibleOutsideOfAssembly(_settings.AttributeDataSymbolFilter, excludeWithTypeArgumentsNotVisibleOutsideOfAssembly: false);
	right = right.ExcludeNonVisibleOutsideOfAssembly(_settings.AttributeDataSymbolFilter, excludeWithTypeArgumentsNotVisibleOutsideOfAssembly: false);
	// No attributes, nothing to do. Exit early.
	if (left.Length == 0 && right.Length == 0)
	{
	return;
	}
	// Build a set of attributes for both sides, grouped by their names.
	// For example,
	// [Foo("a")]
	// [Foo("b")]
	// [Bar]
	// public void F() {}
	// would give you a set like
	// { { Foo("a"), Foo("b") }, { Bar } }
	AttributeSet leftAttributeSet = new(_settings.SymbolEqualityComparer, left);
	AttributeSet rightAttributeSet = new(_settings.SymbolEqualityComparer, right);
	foreach (AttributeGroup leftGroup in leftAttributeSet)
	{
	if (rightAttributeSet.TryGetValue(leftGroup.Representative, out AttributeGroup? rightGroup))
	{
	// If attribute exists on left and the right, compare their arguments.
	foreach (AttributeData leftAttribute in leftGroup.Attributes)
	{
	bool seen = false;
	for (int j = 0; j < rightGroup.Attributes.Count; j++)
	{
	AttributeData rightAttribute = rightGroup.Attributes[j];
	if (_settings.AttributeDataEqualityComparer.Equals(leftAttribute, rightAttribute))
	{
	rightGroup.Seen[j] = true;
	seen = true;
	break;
	}
	}
	if (!seen)
	{
	// Attribute arguments exist on left but not right.
	// Issue "changed" diagnostic.
	AddDifference(differences, DifferenceType.Changed, leftMetadata, rightMetadata, containing, itemRef, leftAttribute);
	}
	}
	for (int i = 0; i < rightGroup.Attributes.Count; i++)
	{
	if (!rightGroup.Seen[i] && _settings.StrictMode)
	{
	// Attribute arguments exist on right but not left.
	// Left
	// [Foo("a")]
	// void F()
	// Right
	// [Foo("a")]
	// [Foo("b")]
	// void F()
	// Issue "changed" diagnostic when in strict mode.
	AddDifference(differences, DifferenceType.Changed, leftMetadata, rightMetadata, containing, itemRef, rightGroup.Attributes[i]);
	}
	}
	}
	else
	{
	// Attribute exists on left but not on right.
	// Loop over left and issue "removed" diagnostic for each one.
	foreach (AttributeData leftAttribute in leftGroup.Attributes)
	{
	AddDifference(differences, DifferenceType.Removed, leftMetadata, rightMetadata, containing, itemRef, leftAttribute);
	}
	}
	}
	foreach (AttributeGroup rightGroup in rightAttributeSet)
	{
	if (leftAttributeSet.TryGetValue(rightGroup.Representative, out _))
	{
	continue;
	}
	// Attribute exists on right but not left.
	// Loop over right and issue "added" diagnostic for each one.
	foreach (AttributeData rightAttribute in rightGroup.Attributes)
	{
	AddDifference(differences, DifferenceType.Added, leftMetadata, rightMetadata, containing, itemRef, rightAttribute);
	}
	}
	}

It currently doesn't have an understanding of the different sets of attributes and their compatibility behavior. Another example where this becomes noisy is the ObsoleteAttribute. A compatibility change is reported even if just the message string changes.

#35542 is related.