From f2b02029f45cea37ed4e0b4889e5c8b02d9f75e7 Mon Sep 17 00:00:00 2001 From: Wes Haggard Date: Wed, 10 Jan 2018 16:12:45 -0800 Subject: [PATCH 1/6] Update CSharpWriter to handle private fields in structs For compile-time compat, the following rules should work for producing a reference assembly. We drop all private fields, but add back certain synthesized private fields for a value type (struct) as follows: - If there are any private fields that are or contain any value type members, add a single private field of type int - If there are any private fields that are or contain any reference type members, add a single private field of type object. - If the type is generic, then for every type parameter of the type, if there are any private fields that are or contain any members whose type is that type parameter, we add a direct private field of that type. For more details see issue https://github.com/dotnet/corefx/issues/6185 this blog is helpful as well http://blog.paranoidcoding.com/2016/02/15/are-private-members-api-surface.html --- .../Extensions/CSharp/CSharpCciExtensions.cs | 35 +++++++- .../Traversers/SimpleTypeMemberTraverser.cs | 7 +- .../CSharp/CSDeclarationWriter.Fields.cs | 87 ++++++++++++++++++- .../Writers/CSharp/CSharpWriter.cs | 67 +++++++++++++- 4 files changed, 191 insertions(+), 5 deletions(-) diff --git a/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs b/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs index 68cc3da80f..40339f2aa7 100644 --- a/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs +++ b/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs @@ -111,6 +111,39 @@ public static ITypeReference GetEnumType(this ITypeDefinition type) throw new InvalidOperationException("All enums should have a value__ field!"); } + public static bool IsOrContainsReferenceType(this ITypeReference type) + { + Queue typesToCheck = new Queue(); + HashSet visited = new HashSet(); + + typesToCheck.Enqueue(type); + + while (typesToCheck.Count != 0) + { + var typeToCheck = typesToCheck.Dequeue(); + visited.Add(typeToCheck); + + if (!typeToCheck.IsValueType) + return true; + + var resolvedType = typeToCheck.ResolvedType; + + // If it is dummy we cannot really check so assume it does because that is will be the most conservative + if (resolvedType is Dummy) + return true; + + foreach (var field in resolvedType.Fields) + { + if (!visited.Contains(field.Type)) + { + typesToCheck.Enqueue(field.Type); + } + } + } + + return false; + } + public static bool IsConversionOperator(this IMethodDefinition method) { return (method.IsSpecialName && @@ -420,7 +453,7 @@ public static IMethodDefinition GetHiddenBaseMethod(this IMethodDefinition metho if (IsAssembly(baseMethod) && !InSameUnit(baseMethod, method)) continue; - if (filter != null && !filter.Include(baseMethod)) + if (filter != null && !filter.Include(baseMethod.UnWrapMember())) continue; // NOTE: Do not check method.IsHiddenBySignature here. C# is *always* hide-by-signature regardless of the metadata flag. diff --git a/src/Microsoft.Cci.Extensions/Traversers/SimpleTypeMemberTraverser.cs b/src/Microsoft.Cci.Extensions/Traversers/SimpleTypeMemberTraverser.cs index 543444ef3e..161ae31b89 100644 --- a/src/Microsoft.Cci.Extensions/Traversers/SimpleTypeMemberTraverser.cs +++ b/src/Microsoft.Cci.Extensions/Traversers/SimpleTypeMemberTraverser.cs @@ -69,7 +69,7 @@ public virtual string GetTypeKey(ITypeDefinition type) public virtual void Visit(ITypeDefinition type) { - Visit(type.Fields); + Visit(type, type.Fields); Visit(type.Methods.Where(m => m.IsConstructor)); Visit(type.Properties); Visit(type.Events); @@ -86,6 +86,11 @@ public virtual void Visit(IEnumerable members) Visit(member); } + public virtual void Visit(ITypeDefinition parentType, IEnumerable fields) + { + this.Visit((IEnumerable)fields); + } + public virtual string GetMemberKey(ITypeDefinitionMember member) { return member.UniqueId(); diff --git a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Fields.cs b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Fields.cs index 79b22f2d5f..6a03e90512 100644 --- a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Fields.cs +++ b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Fields.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System.Collections.Generic; using Microsoft.Cci.Extensions.CSharp; namespace Microsoft.Cci.Writers.CSharp @@ -14,7 +15,7 @@ private void WriteFieldDefinition(IFieldDefinition field) return; WriteAttributes(field.Attributes); - if (!field.IsStatic && field.ContainingTypeDefinition.Layout == LayoutKind.Explicit) + if (!field.IsStatic && field.ContainingTypeDefinition.Layout == LayoutKind.Explicit && !(field is DummyPrivateField)) { WriteFakeAttribute("System.Runtime.InteropServices.FieldOffsetAttribute", field.Offset.ToString()); } @@ -102,4 +103,88 @@ private void WriteFieldDefinitionValue(IFieldDefinition field) { WriteMetadataConstant(field.Constant); } } + + public class DummyPrivateField : IFieldDefinition + { + private ITypeDefinition _parentType; + private ITypeReference _type; + private IName _name; + + public DummyPrivateField(ITypeDefinition parentType, ITypeReference type) + { + _parentType = parentType; + _type = type; + _name = new NameTable().GetNameFor("_dummy"); + } + + public uint BitLength => 0; + + public IMetadataConstant CompileTimeValue => null; + + public ISectionBlock FieldMapping => null; + + public bool IsBitField => false; + + public bool IsCompileTimeConstant => false; + + public bool IsMapped => throw new System.NotImplementedException(); + + public bool IsMarshalledExplicitly => throw new System.NotImplementedException(); + + public bool IsNotSerialized => false; + + public bool IsReadOnly => _parentType.Attributes.HasIsReadOnlyAttribute(); + + public bool IsRuntimeSpecial => false; + + public bool IsSpecialName => false; + + public IMarshallingInformation MarshallingInformation => throw new System.NotImplementedException(); + + public uint Offset => 0; + + public int SequenceNumber => throw new System.NotImplementedException(); + + public ITypeDefinition ContainingTypeDefinition => _parentType; + + public TypeMemberVisibility Visibility => TypeMemberVisibility.Private; + + public ITypeDefinition Container => throw new System.NotImplementedException(); + + public IName Name => _name; + + public IScope ContainingScope => throw new System.NotImplementedException(); + + public IEnumerable CustomModifiers => System.Linq.Enumerable.Empty(); + + public uint InternedKey => throw new System.NotImplementedException(); + + public bool IsModified => throw new System.NotImplementedException(); + + public bool IsStatic => false; + + public ITypeReference Type => _type; + + public IFieldDefinition ResolvedField => throw new System.NotImplementedException(); + + public ITypeReference ContainingType => _parentType; + + public ITypeDefinitionMember ResolvedTypeDefinitionMember => throw new System.NotImplementedException(); + + public IEnumerable Attributes => System.Linq.Enumerable.Empty(); + + public IEnumerable Locations => throw new System.NotImplementedException(); + + public IMetadataConstant Constant => null; + + public void Dispatch(IMetadataVisitor visitor) + { + throw new System.NotImplementedException(); + } + + public void DispatchAsReference(IMetadataVisitor visitor) + { + throw new System.NotImplementedException(); + } + } } diff --git a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs index 8f530ee8f8..60b6ec3524 100644 --- a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs +++ b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs @@ -20,6 +20,9 @@ public class CSharpWriter : SimpleTypeMemberTraverser, ICciWriter private readonly IStyleSyntaxWriter _styleWriter; private readonly CSDeclarationWriter _declarationWriter; private readonly bool _writeAssemblyAttributes; + private readonly bool _apiOnly; + private readonly ICciFilter _cciFilter; + private bool _firstMemberGroup; public CSharpWriter(ISyntaxWriter writer, ICciFilter filter, bool apiOnly, bool writeAssemblyAttributes = false) @@ -27,6 +30,8 @@ public CSharpWriter(ISyntaxWriter writer, ICciFilter filter, bool apiOnly, bool { _syntaxWriter = writer; _styleWriter = writer as IStyleSyntaxWriter; + _apiOnly = apiOnly; + _cciFilter = filter; _declarationWriter = new CSDeclarationWriter(_syntaxWriter, filter, !apiOnly); _writeAssemblyAttributes = writeAssemblyAttributes; } @@ -59,8 +64,8 @@ public string PlatformNotSupportedExceptionMessage public void WriteAssemblies(IEnumerable assemblies) { - foreach (var assembly in assemblies) - Visit(assembly); + foreach (var assembly in assemblies) + Visit(assembly); } public override void Visit(IAssembly assembly) @@ -115,6 +120,7 @@ public override void Visit(ITypeDefinition type) _declarationWriter.WriteDeclaration(CSDeclarationWriter.GetDummyConstructor(type)); _syntaxWriter.WriteLine(); } + _firstMemberGroup = true; base.Visit(type); } @@ -128,6 +134,63 @@ public override void Visit(IEnumerable members) base.Visit(members); } + public override void Visit(ITypeDefinition parentType, IEnumerable fields) + { + if (parentType.IsStruct && !_apiOnly) + { + // For compile-time compat, the following rules should work for producing a reference assembly. We drop all private fields, but add back certain synthesized private fields for a value type (struct) as follows: + // - If there are any private fields that are or contain any value type members, add a single private field of type int + // - If there are any private fields that are or contain any reference type members, add a single private field of type object. + // - If the type is generic, then for every type parameter of the type, if there are any private fields that are or contain any members whose type is that type parameter, we add a direct private field of that type. + + // For more details see issue https://github.com/dotnet/corefx/issues/6185 + // this blog is helpful as well http://blog.paranoidcoding.com/2016/02/15/are-private-members-api-surface.html + + List newFields = new List(); + var includedVisibleFields = fields.Where(f => f.IsVisibleOutsideAssembly()).Where(_cciFilter.Include); + includedVisibleFields = includedVisibleFields.OrderBy(GetMemberKey, StringComparer.OrdinalIgnoreCase); + + var excludedFields = fields.Except(includedVisibleFields).Where(f => !f.IsStatic); + + if (excludedFields.Any()) + { + var genericTypedFields = excludedFields.Where(f => f.Type.UnWrap().IsGenericParameter()); + + // Compiler needs to see any fields, even private, that have generic arugments to be able + // to validate there aren't any struct layout cycles + foreach (var genericField in genericTypedFields) + newFields.Add(genericField); + + if (!genericTypedFields.Any()) + { + // For definiteassignment checks the compiler needs to know there is a private field + // that has not be initialized so if there are any we need to add a dummy private + // field to help the compiler do its job and error about uninitialized structs + bool hasRefPrivateField = excludedFields.Any(f => f.Type.IsOrContainsReferenceType()); + ITypeReference fieldType = parentType.PlatformType.SystemInt32; + + // If at least one the private fields contains a reference type then we need to + // set this field type to object or reference field to inform the compiler to block + // taking pointers to this struct because the GC will not track updating those references + if (hasRefPrivateField) + fieldType = parentType.PlatformType.SystemObject; + + newFields.Add(new DummyPrivateField(parentType, fieldType)); + } + } + + foreach (var visibleField in includedVisibleFields) + newFields.Add(visibleField); + + foreach (var field in newFields) + Visit(field); + } + else + { + base.Visit(parentType, fields); + } + } + public override void Visit(ITypeDefinitionMember member) { IDisposable style = null; From 04124b36399be8aaebdb9f58072f920a50d8d144 Mon Sep 17 00:00:00 2001 From: Wes Haggard Date: Thu, 11 Jan 2018 10:13:30 -0800 Subject: [PATCH 2/6] Update IsOrContainsReferenceType function IsReferenceType is not always the inverse of IsValueType. Things like unmanaged pointer references aren't value types nor are they a managed reference type. Changed to use the logic already in CCI to detect a reference type but it requires a resolved type definition instead of a type reference. --- .../Extensions/CSharp/CSharpCciExtensions.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs b/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs index 40339f2aa7..91122c6cec 100644 --- a/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs +++ b/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs @@ -123,16 +123,16 @@ public static bool IsOrContainsReferenceType(this ITypeReference type) var typeToCheck = typesToCheck.Dequeue(); visited.Add(typeToCheck); - if (!typeToCheck.IsValueType) - return true; - var resolvedType = typeToCheck.ResolvedType; // If it is dummy we cannot really check so assume it does because that is will be the most conservative if (resolvedType is Dummy) return true; - foreach (var field in resolvedType.Fields) + if (resolvedType.IsReferenceType) + return true; + + foreach (var field in resolvedType.Fields.Where(f => !f.IsStatic)) { if (!visited.Contains(field.Type)) { From f34ee45b4184e48638b3d62f0f7d016c581896e7 Mon Sep 17 00:00:00 2001 From: Wes Haggard Date: Thu, 11 Jan 2018 16:07:52 -0800 Subject: [PATCH 3/6] Escape chars from compiler generated backing fields --- .../Writers/CSharp/CSDeclarationWriter.Fields.cs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Fields.cs b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Fields.cs index 6a03e90512..7e17b701e6 100644 --- a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Fields.cs +++ b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSDeclarationWriter.Fields.cs @@ -52,7 +52,13 @@ private void WriteFieldDefinition(IFieldDefinition field) WriteKeyword("new"); WriteTypeName(field.Type); - WriteIdentifier(field.Name); + + string name = field.Name.Value; + if (name.Contains("<") || name.Contains(">")) + { + name = name.Replace("<", "_").Replace(">", "_"); + } + WriteIdentifier(name, true); if (field.Constant != null && field.IsCompileTimeConstant) { From 7f72f683d195ad17fa002434ff86722cfa9b91a6 Mon Sep 17 00:00:00 2001 From: Wes Haggard Date: Thu, 11 Jan 2018 16:08:34 -0800 Subject: [PATCH 4/6] Use primitive field type for private fields of primitives --- .../Writers/CSharp/CSharpWriter.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs index 60b6ec3524..4934f8f1bc 100644 --- a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs +++ b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs @@ -175,6 +175,17 @@ public override void Visit(ITypeDefinition parentType, IEnumerable Date: Thu, 11 Jan 2018 16:32:34 -0800 Subject: [PATCH 5/6] Treat ByReference as a reference type The runtime treats ByReference special as a ref T, so we need the tools to also treat it as a reference. --- .../Extensions/CSharp/CSharpCciExtensions.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs b/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs index 91122c6cec..5233d33610 100644 --- a/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs +++ b/src/Microsoft.Cci.Extensions/Extensions/CSharp/CSharpCciExtensions.cs @@ -132,6 +132,10 @@ public static bool IsOrContainsReferenceType(this ITypeReference type) if (resolvedType.IsReferenceType) return true; + // ByReference is a special type understood by runtime to hold a ref T. + if (resolvedType.AreEquivalent("System.ByReference")) + return true; + foreach (var field in resolvedType.Fields.Where(f => !f.IsStatic)) { if (!visited.Contains(field.Type)) From 2518fcaa34eafc8ea871d332b82fe4ef8a491aad Mon Sep 17 00:00:00 2001 From: Wes Haggard Date: Thu, 11 Jan 2018 17:26:22 -0800 Subject: [PATCH 6/6] Fix typos in comments --- src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs index 4934f8f1bc..dd3aa9c368 100644 --- a/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs +++ b/src/Microsoft.Cci.Extensions/Writers/CSharp/CSharpWriter.cs @@ -169,13 +169,13 @@ public override void Visit(ITypeDefinition parentType, IEnumerable f.Type.IsOrContainsReferenceType()); ITypeReference fieldType = parentType.PlatformType.SystemInt32; - // If at least one the private fields contains a reference type then we need to + // If at least one of the private fields contains a reference type then we need to // set this field type to object or reference field to inform the compiler to block // taking pointers to this struct because the GC will not track updating those references if (hasRefPrivateField) fieldType = parentType.PlatformType.SystemObject; - // For primiative types that have a field of their type set the dummy field to that type + // For primitive types that have a field of their type set the dummy field to that type if (excludedFields.Count() == 1) { var onlyField = excludedFields.First();