Skip to content

Commit d6dd7a0

Browse files
authored
Harden GenAPI and ApiCompat (#4585)
- Do not emit fake `[StructLayout]` attributes matching defaults - see #3144 discussions (not an exact fix for that request) - `LayoutKind.Auto` is _not_ the default for value types - nit: sort `using`s - Improve discovery of accessors - #2066 - handle cases where accessors are overridden but related events or properties are not - use method prefixes rather than focus on events and properties defined in current type - Add GenAPI `--respect-internals` option - add supporting `InternalsAndPublicCciFilter` - see discussions in #4488 - nits: - reorder `GetFilter(...)` conditions for clarity and to reflect precedence - use new-ish named parameter syntax - sort `using`s - add a few more curly braces and newlines - Avoid GenAPI NRE when naming value types - #4488 problem (1) - not all tuples have named parameters - nit: remove and sort `using`s - Base `ExcludeAttributesFilter` on `IncludeAllFilter` - avoid unintentionally limiting inclusions to `public` members - Correct generation of faked `[MethodImpl]` attributes - avoid C# compilation errors - from `[....MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining, NoOptimization)]` - to `[....MethodImpl(System.Runtime.CompilerServices.MethodImplOptions.NoInlining|System.Runtime.CompilerServices.MethodImplOptions.NoOptimization)]` - Add GenAPI `--exclude-compiler-generated` option - #4488 problem (2) - add supporting `ExcludeCompilerGeneratedCciFilter` - Write correct C# for `static` constructors - #4488 problem (3) - correct method name - remove illegal return type and access modifier - Write correct C# for `unsafe` methods - #4488 problem (4) - avoid C# compilation errors - generate `unsafe` keyword when base constructor call is unsafe - generate `unsafe` keyword for unsafe methods in interfaces - Respect `ICciFilter` when generating fields in value types - #2031 aka #2033 aka #4488 problem (6) - remove `IsVisibleOutsideAssembly()` use; was at best redundant - Take `ExcludeAttributesFilter` breaking change - !fixup! Don't (uselessly) check for attributes on a namespace - Split and extend `TypeExtensions` - split member-related methods into `MemberExtensions` - add `MemberExtensions.IsVisibleToFriendAssemblies(...)` - add `TypeExtensions.IsConstructorVisibleToFriendAssemblies(...)` - Use new extension methods in `InternalAndPublicCciFilter` - Remove extra checks in `ICciFilter` implementations - remove parameter `null` checks - remove `ContainingTypeDefinition` visibility checks from `Include(ITypeDefinitionMember)` - Change `ExcludeAttributesFilter` to implement `ICciFilter` directly - Use a `const` for `[InternalsVisibleTo]` type name - Add ApiCompat `--respect-internals` option - nit: remove and sort `using`s - Create `private` constructors in `WritePrivateConstructor(...)` - Add `$(AdditionalApiCompatOptions)` extension point for ApiCompat - correct `$(MatchingRefApiCompatBaseline)` setting; read and write same file - nits: - correct spelling of `$(ImplementationAssemblyAsContract)` property - remove incorrect "must be last option" comments - Do not compare everything in `ICustomModifier` implementations - lack of comparer led to "X != X" messages due to varying `InternedKey` and similar properties - Smarten visibility choice for constructor `WritePrivateConstructor(...) generates - choose `private` if internal constructors are likely to be generated by default - flatten `IntersectionFilter.Filters` list to make `WritePrivateConstructor(...)` use reasonable - also improves overall efficiency - Add `--exclude-compiler-generated` option to ApiCompat - aligns with generation exclusions in GenAPI (if enabled) - Correct ApiCompat `--contract-depends` command-line argument in the `RunMatchingRefApiCompat` target - Filter attributes in both directions - otherwise, `ValidateApiCompatForSrc` target succeeds while `RunMatchingRefApiCompat` fails in some cases - or visa versa - Simplify a visibility check - remove redundant bits - Remove `$(IntermediateOutputPath)` use in ApiCompat commands
1 parent f492130 commit d6dd7a0

18 files changed

+782
-303
lines changed

src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,6 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33
// See the LICENSE file in the project root for more information.
44

5-
using Microsoft.Cci.Filters;
6-
using Microsoft.Cci.Writers.CSharp;
7-
using Microsoft.Cci.Writers.Syntax;
85
using System;
96
using System.Collections.Generic;
107
using System.Collections.Immutable;
@@ -13,9 +10,10 @@
1310
using System.Linq;
1411
using System.Reflection.Metadata;
1512
using System.Reflection.Metadata.Ecma335;
16-
using System.Reflection.PortableExecutable;
1713
using System.Text.RegularExpressions;
18-
using System.Xml.Linq;
14+
using Microsoft.Cci.Filters;
15+
using Microsoft.Cci.Writers.CSharp;
16+
using Microsoft.Cci.Writers.Syntax;
1917
using SRMetadataReader = System.Reflection.Metadata.MetadataReader;
2018

2119
namespace Microsoft.Cci.Extensions.CSharp
@@ -782,7 +780,7 @@ public static bool HasIsReadOnlyAttribute(this IEnumerable<ICustomAttribute> att
782780
public static string[] GetValueTupleNames(this IEnumerable<ICustomAttribute> attributes)
783781
{
784782
string[] names = null;
785-
var attribute = attributes.GetAttributeOfType("System.Runtime.CompilerServices.TupleElementNamesAttribute");
783+
var attribute = attributes?.GetAttributeOfType("System.Runtime.CompilerServices.TupleElementNamesAttribute");
786784
if (attribute != null && attribute.Arguments.Single() is IMetadataCreateArray createArray)
787785
{
788786
names = new string[createArray.Sizes.Single()];
Lines changed: 257 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,257 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System;
6+
using System.ComponentModel;
7+
using System.Linq;
8+
using Microsoft.Cci.Extensions.CSharp;
9+
10+
namespace Microsoft.Cci.Extensions
11+
{
12+
public static class MemberExtensions
13+
{
14+
public static bool IsVisibleOutsideAssembly(this ITypeDefinitionMember member)
15+
{
16+
return MemberHelper.IsVisibleOutsideAssembly(member);
17+
}
18+
19+
public static bool IsVisibleToFriendAssemblies(this ITypeDefinitionMember member)
20+
{
21+
// MemberHelper in CCI doesn't have a method like IsVisibleOutsideAssembly(...) to use here. This method
22+
// has similar behavior except that it returns true for all internal and internal protected members as well
23+
// as non-sealed private protected members.
24+
switch (member.Visibility)
25+
{
26+
case TypeMemberVisibility.Assembly:
27+
case TypeMemberVisibility.FamilyOrAssembly:
28+
case TypeMemberVisibility.Public:
29+
return true;
30+
31+
case TypeMemberVisibility.Family:
32+
case TypeMemberVisibility.FamilyAndAssembly:
33+
return !member.ContainingTypeDefinition.IsSealed;
34+
}
35+
36+
var containingType = member.ContainingTypeDefinition;
37+
return member switch
38+
{
39+
IMethodDefinition methodDefinition =>
40+
IsExplicitImplementationVisible(methodDefinition, containingType),
41+
IPropertyDefinition propertyDefinition =>
42+
IsExplicitImplementationVisible(propertyDefinition.Getter, containingType) ||
43+
IsExplicitImplementationVisible(propertyDefinition.Setter, containingType),
44+
IEventDefinition eventDefinition =>
45+
IsExplicitImplementationVisible(eventDefinition.Adder, containingType) ||
46+
IsExplicitImplementationVisible(eventDefinition.Remover, containingType),
47+
_ => false,
48+
};
49+
}
50+
51+
public static bool IsGenericInstance(this IMethodReference method)
52+
{
53+
return method is IGenericMethodInstanceReference;
54+
}
55+
56+
public static bool IsWindowsRuntimeMember(this ITypeMemberReference member)
57+
{
58+
var assemblyRef = member.GetAssemblyReference();
59+
60+
return assemblyRef.IsWindowsRuntimeAssembly();
61+
}
62+
63+
public static string FullName(this ICustomAttribute attribute)
64+
{
65+
if (attribute is FakeCustomAttribute fca)
66+
{
67+
return fca.FullTypeName;
68+
}
69+
70+
return attribute.Type.FullName();
71+
}
72+
73+
public static string GetMethodSignature(this IMethodDefinition method)
74+
{
75+
return MemberHelper.GetMethodSignature(method, NameFormattingOptions.Signature);
76+
}
77+
78+
public static T UnWrapMember<T>(this T member)
79+
where T : ITypeMemberReference
80+
{
81+
return member switch
82+
{
83+
IGenericMethodInstanceReference genericMethod => (T)genericMethod.GenericMethod.UnWrapMember(),
84+
ISpecializedNestedTypeReference type => (T)type.UnspecializedVersion.UnWrapMember(),
85+
ISpecializedMethodReference method => (T)method.UnspecializedVersion.UnWrapMember(),
86+
ISpecializedFieldReference field => (T)field.UnspecializedVersion.UnWrapMember(),
87+
ISpecializedPropertyDefinition property => (T)property.UnspecializedVersion.UnWrapMember(),
88+
ISpecializedEventDefinition evnt => (T)evnt.UnspecializedVersion.UnWrapMember(),
89+
_ => member
90+
};
91+
}
92+
93+
public static bool IsPropertyOrEventAccessor(this IMethodDefinition method)
94+
{
95+
return method.GetAccessorType() != AccessorType.None;
96+
}
97+
98+
public static AccessorType GetAccessorType(this IMethodDefinition methodDefinition)
99+
{
100+
if (!methodDefinition.IsSpecialName)
101+
{
102+
return AccessorType.None;
103+
}
104+
105+
// Cannot use MemberHelper.IsAdder(...) and similar due to their TypeMemberVisibility.Public restriction.
106+
var name = methodDefinition.GetNameWithoutExplicitType();
107+
if (name.StartsWith("get_"))
108+
{
109+
return AccessorType.EventAdder;
110+
}
111+
else if (name.StartsWith("set_"))
112+
{
113+
return AccessorType.PropertyGetter;
114+
}
115+
else if (name.StartsWith("add_"))
116+
{
117+
return AccessorType.EventRemover;
118+
}
119+
else if (name.StartsWith("remove_"))
120+
{
121+
return AccessorType.PropertySetter;
122+
}
123+
124+
return AccessorType.None;
125+
}
126+
127+
public static bool IsEditorBrowseableStateNever(this ICustomAttribute attribute)
128+
{
129+
if (attribute.Type.FullName() != typeof(EditorBrowsableAttribute).FullName)
130+
{
131+
return false;
132+
}
133+
134+
if (attribute.Arguments == null || attribute.Arguments.Count() != 1)
135+
{
136+
return false;
137+
}
138+
139+
var singleArgument = attribute.Arguments.Single() as IMetadataConstant;
140+
if (singleArgument == null || !(singleArgument.Value is int))
141+
{
142+
return false;
143+
}
144+
145+
if (EditorBrowsableState.Never != (EditorBrowsableState)singleArgument.Value)
146+
{
147+
return false;
148+
}
149+
150+
return true;
151+
}
152+
153+
public static bool IsObsoleteWithUsageTreatedAsCompilationError(this ICustomAttribute attribute)
154+
{
155+
if (attribute.Type.FullName() != typeof(ObsoleteAttribute).FullName)
156+
{
157+
return false;
158+
}
159+
160+
if (attribute.Arguments == null || attribute.Arguments.Count() != 2)
161+
{
162+
return false;
163+
}
164+
165+
var messageArgument = attribute.Arguments.ElementAt(0) as IMetadataConstant;
166+
var errorArgument = attribute.Arguments.ElementAt(1) as IMetadataConstant;
167+
if (messageArgument == null || errorArgument == null)
168+
{
169+
return false;
170+
}
171+
172+
if (!(messageArgument.Value is string && errorArgument.Value is bool))
173+
{
174+
return false;
175+
}
176+
177+
return (bool)errorArgument.Value;
178+
}
179+
180+
public static ApiKind GetApiKind(this ITypeDefinitionMember member)
181+
{
182+
if (member.ContainingTypeDefinition.IsDelegate)
183+
return ApiKind.DelegateMember;
184+
185+
switch (member)
186+
{
187+
case IFieldDefinition field:
188+
if (member.ContainingTypeDefinition.IsEnum && field.IsSpecialName)
189+
{
190+
return ApiKind.EnumField;
191+
}
192+
193+
return ApiKind.Field;
194+
195+
case IPropertyDefinition _:
196+
return ApiKind.Property;
197+
198+
case IEventDefinition _:
199+
return ApiKind.Event;
200+
}
201+
202+
203+
var method = (IMethodDefinition)member;
204+
if (method.IsConstructor || method.IsStaticConstructor)
205+
{
206+
return ApiKind.Constructor;
207+
}
208+
209+
var accessorType = method.GetAccessorType();
210+
switch (accessorType)
211+
{
212+
case AccessorType.PropertyGetter:
213+
case AccessorType.PropertySetter:
214+
return ApiKind.PropertyAccessor;
215+
216+
case AccessorType.EventAdder:
217+
case AccessorType.EventRemover:
218+
return ApiKind.EventAccessor;
219+
220+
default:
221+
return ApiKind.Method;
222+
}
223+
}
224+
225+
// A rewrite of MemberHelper.IsExplicitImplementationVisible(...) with looser visibility checks.
226+
private static bool IsExplicitImplementationVisible(IMethodReference method, ITypeDefinition containingType)
227+
{
228+
if (method == null)
229+
{
230+
return false;
231+
}
232+
233+
using var enumerator = containingType.ExplicitImplementationOverrides.GetEnumerator();
234+
while (enumerator.MoveNext())
235+
{
236+
var current = enumerator.Current;
237+
if (current.ImplementingMethod.InternedKey == method.InternedKey)
238+
{
239+
var resolvedMethod = current.ImplementedMethod.ResolvedMethod;
240+
if (resolvedMethod is Dummy)
241+
{
242+
return true;
243+
}
244+
245+
// Reviewers: Recursive call that mimics MemberHelper methods. But, why is this safe? (I'm undoing
246+
// my InternalsAndPublicCciFilter.SimpleInclude(...) failsafe but double-checking.)
247+
if (IsVisibleToFriendAssemblies(resolvedMethod))
248+
{
249+
return true;
250+
}
251+
}
252+
}
253+
254+
return false;
255+
}
256+
}
257+
}

0 commit comments

Comments
 (0)