Skip to content

Commit f9280d1

Browse files
smasher164smasher164
authored
[release/7.0.1xx] backport APICompat changes for GA (#28404)
This backport consists of 5 changes: 1. c3f9e6d: apicompat: add rule to check change in visibility 2. 5627049: don't emit attribute added diagnostic in strict mode 3. a917975: don't issue diagnostic when virtual is removed from sealed type 4. ac6cd39: apicompat: attribute changed on right must only be emitted in strict mode 5. 8f9568f: normalize accessibility when not including internal symbols ## Customer Impact TL;DR Rules behave correctly in strict mode and when internal members are excluded. Previously, the attribute rule was emitting strict-mode diagnostics even outside strict mode. Additionally, internal members were not being excluded from analysis even when specified. These issues caused many false positive diagnostics to be emitted. The backported changes fix this behavior by taking these modes into account. The new visibility change rule was announced for .NET 7, and also maintains parity with the legacy APICompat tool. ## Testing All ApiCompat rules tests pass. The runtime built with this backport branch no longer emits false positive diagnostics. ## Risk Low-Medium. Only affects users who are consuming the opt-in attribute and virtual modifier rules. Additionally, a new rule is added, but it shouldn't affect customers who aren't comparing in strict mode.
1 parent 39b57e7 commit f9280d1

22 files changed

+1331
-25
lines changed

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/DiagnosticIds.cs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,8 @@ public static class DiagnosticIds
2626
public const string CannotAddAttribute = "CP0016";
2727
public const string CannotChangeParameterName = "CP0017";
2828
public const string CannotAddSealedToInterfaceMember = "CP0018";
29+
public const string CannotReduceVisibility = "CP0019";
30+
public const string CannotExpandVisibility = "CP0020";
2931

3032
// Assembly loading ids
3133
public const string AssemblyNotFound = "CP1001";

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Resources.resx‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,4 +240,10 @@
240240
<data name="CannotAddSealedToInterfaceMember" xml:space="preserve">
241241
<value>Cannot add sealed keyword to default interface member '{0}'.</value>
242242
</data>
243+
<data name="CannotExpandVisibility" xml:space="preserve">
244+
<value>Visibility of '{0}' expanded from '{1}' to '{2}'.</value>
245+
</data>
246+
<data name="CannotReduceVisibility" xml:space="preserve">
247+
<value>Visibility of '{0}' reduced from '{1}' to '{2}'.</value>
248+
</data>
243249
</root>

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/AttributesMustMatch.cs‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,11 @@ private void AddDifference(IList<CompatDifference> differences, DifferenceType d
6262
return;
6363
}
6464

65+
if (!_settings.StrictMode && dt == DifferenceType.Added)
66+
{
67+
return;
68+
}
69+
6570
CompatDifference difference = dt switch
6671
{
6772
DifferenceType.Changed => new CompatDifference(
@@ -167,10 +172,17 @@ private void ReportAttributeDifferences(ISymbol containing,
167172

168173
for (int i = 0; i < rightGroup.Attributes.Count; i++)
169174
{
170-
if (!rightGroup.Seen[i])
175+
if (!rightGroup.Seen[i] && _settings.StrictMode)
171176
{
172177
// Attribute arguments exist on right but not left.
173-
// Issue "changed" diagnostic.
178+
// Left
179+
// [Foo("a")]
180+
// void F()
181+
// Right
182+
// [Foo("a")]
183+
// [Foo("b")]
184+
// void F()
185+
// Issue "changed" diagnostic when in strict mode.
174186
AddDifference(differences, DifferenceType.Changed, leftMetadata, rightMetadata, containing, itemRef, rightGroup.Attributes[i]);
175187
}
176188
}

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/CannotAddOrRemoveVirtualKeyword.cs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
using System.Collections.Generic;
55
using Microsoft.CodeAnalysis;
66
using Microsoft.DotNet.ApiCompatibility.Abstractions;
7+
using Microsoft.DotNet.ApiCompatibility.Extensions;
78

89
namespace Microsoft.DotNet.ApiCompatibility.Rules
910
{
@@ -50,6 +51,12 @@ private void RunOnMemberSymbol(ISymbol? left, ISymbol? right, ITypeSymbol leftCo
5051

5152
if (left.IsVirtual)
5253
{
54+
// Removing the virtual keyword from a member in a sealed type won't be a breaking change.
55+
if (leftContainingType.IsEffectivelySealed(_settings.IncludeInternalSymbols))
56+
{
57+
return;
58+
}
59+
5360
// If left is virtual and right is not, then emit a diagnostic
5461
// specifying that the virtual modifier cannot be removed.
5562
if (!right.IsVirtual)

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/CannotChangeVisibility.cs‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
// Copyright (c) .NET Foundation and contributors. All rights reserved.
2+
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
3+
4+
using System;
5+
using System.Collections.Generic;
6+
using System.Runtime;
7+
using Microsoft.CodeAnalysis;
8+
using Microsoft.DotNet.ApiCompatibility.Abstractions;
9+
using Microsoft.DotNet.ApiCompatibility.Extensions;
10+
11+
namespace Microsoft.DotNet.ApiCompatibility.Rules
12+
{
13+
/// <summary>
14+
/// This class implements a rule to check that the visibility of symbols is not reduced.
15+
/// In strict mode, it also checks that the visibility isn't expanded.
16+
/// </summary>
17+
public class CannotChangeVisibility : IRule
18+
{
19+
private readonly RuleSettings _settings;
20+
21+
public CannotChangeVisibility(RuleSettings settings, IRuleRegistrationContext context)
22+
{
23+
_settings = settings;
24+
context.RegisterOnMemberSymbolAction(RunOnMemberSymbol);
25+
context.RegisterOnTypeSymbolAction(RunOnTypeSymbol);
26+
}
27+
28+
private static Accessibility NormalizeInternals(Accessibility a) => a switch
29+
{
30+
Accessibility.ProtectedOrInternal => Accessibility.Protected,
31+
Accessibility.ProtectedAndInternal or Accessibility.Internal => Accessibility.Private,
32+
_ => a,
33+
};
34+
35+
private int CompareAccessibility(Accessibility a, Accessibility b)
36+
{
37+
if (!_settings.IncludeInternalSymbols)
38+
{
39+
a = NormalizeInternals(a);
40+
b = NormalizeInternals(b);
41+
}
42+
43+
if (a == b)
44+
{
45+
return 0;
46+
}
47+
48+
return (a, b) switch
49+
{
50+
(Accessibility.Public, _) => 1,
51+
(_, Accessibility.Public) => -1,
52+
(Accessibility.ProtectedOrInternal, _) => 1,
53+
(_, Accessibility.ProtectedOrInternal) => -1,
54+
(Accessibility.Protected or Accessibility.Internal, _) => 1,
55+
(_, Accessibility.Protected or Accessibility.Internal) => -1,
56+
(Accessibility.ProtectedAndInternal, _) => 1,
57+
(_, Accessibility.ProtectedAndInternal) => -1,
58+
_ => throw new NotImplementedException(),
59+
};
60+
}
61+
62+
private void RunOnSymbol(
63+
ISymbol? left,
64+
ISymbol? right,
65+
MetadataInformation leftMetadata,
66+
MetadataInformation rightMetadata,
67+
IList<CompatDifference> differences)
68+
{
69+
// The MemberMustExist rule handles missing symbols and therefore this rule only runs when left and right is not null.
70+
if (left is null || right is null)
71+
{
72+
return;
73+
}
74+
75+
Accessibility leftAccess = left.DeclaredAccessibility;
76+
Accessibility rightAccess = right.DeclaredAccessibility;
77+
int accessComparison = CompareAccessibility(leftAccess, rightAccess);
78+
79+
if (accessComparison > 0)
80+
{
81+
differences.Add(new CompatDifference(leftMetadata,
82+
rightMetadata,
83+
DiagnosticIds.CannotReduceVisibility,
84+
string.Format(Resources.CannotReduceVisibility, left, leftAccess, rightAccess),
85+
DifferenceType.Changed,
86+
left));
87+
}
88+
else if (_settings.StrictMode && accessComparison < 0)
89+
{
90+
differences.Add(new CompatDifference(leftMetadata,
91+
rightMetadata,
92+
DiagnosticIds.CannotExpandVisibility,
93+
string.Format(Resources.CannotExpandVisibility, right, leftAccess, rightAccess),
94+
DifferenceType.Changed,
95+
right));
96+
}
97+
}
98+
99+
private void RunOnTypeSymbol(
100+
ITypeSymbol? left,
101+
ITypeSymbol? right,
102+
MetadataInformation leftMetadata,
103+
MetadataInformation rightMetadata,
104+
IList<CompatDifference> differences) => RunOnSymbol(left, right, leftMetadata, rightMetadata, differences);
105+
106+
private void RunOnMemberSymbol(
107+
ISymbol? left,
108+
ISymbol? right,
109+
ITypeSymbol leftContainingType,
110+
ITypeSymbol rightContainingType,
111+
MetadataInformation leftMetadata,
112+
MetadataInformation rightMetadata,
113+
IList<CompatDifference> differences) => RunOnSymbol(left, right, leftMetadata, rightMetadata, differences);
114+
}
115+
}

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/Rules/RuleFactory.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,8 @@ public IRule[] CreateRules(RuleSettings settings, IRuleRegistrationContext conte
3939
new CannotRemoveBaseTypeOrInterface(settings, context),
4040
new CannotSealType(settings, context),
4141
new EnumsMustMatch(settings, context),
42-
new MembersMustExist(settings, context)
42+
new MembersMustExist(settings, context),
43+
new CannotChangeVisibility(settings, context)
4344
};
4445

4546
if (_enableRuleAttributesMustMatch)

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/xlf/Resources.cs.xlf‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,16 @@
8787
<target state="translated">Název parametru u člena {0} se změnil z {1} na {2}.</target>
8888
<note />
8989
</trans-unit>
90+
<trans-unit id="CannotExpandVisibility">
91+
<source>Visibility of '{0}' expanded from '{1}' to '{2}'.</source>
92+
<target state="new">Visibility of '{0}' expanded from '{1}' to '{2}'.</target>
93+
<note />
94+
</trans-unit>
95+
<trans-unit id="CannotReduceVisibility">
96+
<source>Visibility of '{0}' reduced from '{1}' to '{2}'.</source>
97+
<target state="new">Visibility of '{0}' reduced from '{1}' to '{2}'.</target>
98+
<note />
99+
</trans-unit>
90100
<trans-unit id="CannotRemoveAttribute">
91101
<source>Cannot remove attribute '{0}' from '{1}'.</source>
92102
<target state="translated">Atribut {0} nelze odebrat z {1}.</target>

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/xlf/Resources.de.xlf‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,16 @@
8787
<target state="translated">Der Parametername des Members "{0}" wurde von "{1}" in "{2}" geändert.</target>
8888
<note />
8989
</trans-unit>
90+
<trans-unit id="CannotExpandVisibility">
91+
<source>Visibility of '{0}' expanded from '{1}' to '{2}'.</source>
92+
<target state="new">Visibility of '{0}' expanded from '{1}' to '{2}'.</target>
93+
<note />
94+
</trans-unit>
95+
<trans-unit id="CannotReduceVisibility">
96+
<source>Visibility of '{0}' reduced from '{1}' to '{2}'.</source>
97+
<target state="new">Visibility of '{0}' reduced from '{1}' to '{2}'.</target>
98+
<note />
99+
</trans-unit>
90100
<trans-unit id="CannotRemoveAttribute">
91101
<source>Cannot remove attribute '{0}' from '{1}'.</source>
92102
<target state="translated">Das Attribut "{0}" kann nicht aus "{1}" entfernt werden.</target>

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/xlf/Resources.es.xlf‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,16 @@
8787
<target state="translated">El nombre de parámetro del miembro '{0}' cambió de '{1}' a '{2}'.</target>
8888
<note />
8989
</trans-unit>
90+
<trans-unit id="CannotExpandVisibility">
91+
<source>Visibility of '{0}' expanded from '{1}' to '{2}'.</source>
92+
<target state="new">Visibility of '{0}' expanded from '{1}' to '{2}'.</target>
93+
<note />
94+
</trans-unit>
95+
<trans-unit id="CannotReduceVisibility">
96+
<source>Visibility of '{0}' reduced from '{1}' to '{2}'.</source>
97+
<target state="new">Visibility of '{0}' reduced from '{1}' to '{2}'.</target>
98+
<note />
99+
</trans-unit>
90100
<trans-unit id="CannotRemoveAttribute">
91101
<source>Cannot remove attribute '{0}' from '{1}'.</source>
92102
<target state="translated">No se puede quitar el atributo '{0}' de '{1}'.</target>

‎src/ApiCompat/Microsoft.DotNet.ApiCompatibility/xlf/Resources.fr.xlf‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,16 @@
8787
<target state="translated">Le nom du paramètre sur le membre « {0} » est passé de « {1} » à « {2} ».</target>
8888
<note />
8989
</trans-unit>
90+
<trans-unit id="CannotExpandVisibility">
91+
<source>Visibility of '{0}' expanded from '{1}' to '{2}'.</source>
92+
<target state="new">Visibility of '{0}' expanded from '{1}' to '{2}'.</target>
93+
<note />
94+
</trans-unit>
95+
<trans-unit id="CannotReduceVisibility">
96+
<source>Visibility of '{0}' reduced from '{1}' to '{2}'.</source>
97+
<target state="new">Visibility of '{0}' reduced from '{1}' to '{2}'.</target>
98+
<note />
99+
</trans-unit>
90100
<trans-unit id="CannotRemoveAttribute">
91101
<source>Cannot remove attribute '{0}' from '{1}'.</source>
92102
<target state="translated">Impossible de supprimer l’attribut « {0} » de « {1} ».</target>

0 commit comments

Comments
 (0)