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

Support implicit blittability for structs declared in and not exposed outside of the current compilation. #1126

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
59 changes: 51 additions & 8 deletions DllImportGenerator/DllImportGenerator.UnitTests/CodeSnippets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -511,14 +511,6 @@ struct MyStruct
private int i;
private short s;
}";
public static string GenericBlittableStructParametersAndModifiers = BasicParametersAndModifiers("MyStruct<int>") + @"
#pragma warning disable CS0169
[BlittableType]
struct MyStruct<T>
{
private T t;
private short s;
}";

public static string ArrayParametersAndModifiers(string elementType) => $@"
using System.Runtime.InteropServices;
Expand Down Expand Up @@ -1040,5 +1032,56 @@ public static partial int Method2(
#endif
public static int Foo() => throw null;
}}";

public static string MaybeBlittableGenericTypeParametersAndModifiers(string typeArgument) => BasicParametersAndModifiers($"Generic<{typeArgument}>") + @"
[BlittableType]
struct Generic<T>
{
#pragma warning disable CS0649
public T field;
}
";

public static string MaybeBlittableGenericTypeParametersAndModifiers<T>() =>
MaybeBlittableGenericTypeParametersAndModifiers(typeof(T).ToString());

public static string ImplicitlyBlittableStructParametersAndModifiers(string visibility = "") => BasicParametersAndModifiers("ImplicitBlittable") + $@"
#pragma warning disable CS0649
#pragma warning disable CS0169
{visibility} struct ImplicitBlittable
{{
int i;
}}";

public static string ImplicitlyBlittableGenericTypeParametersAndModifiers(string typeArgument, string visibility = "") => BasicParametersAndModifiers($"Generic<{typeArgument}>") + $@"
{visibility} struct Generic<T>
{{
#pragma warning disable CS0649
#pragma warning disable CS0169
public T field;
}}
";

public static string ImplicitlyBlittableGenericTypeParametersAndModifiers<T>(string visibility = "") =>
ImplicitlyBlittableGenericTypeParametersAndModifiers(typeof(T).ToString(), visibility);

public static string RecursiveImplicitlyBlittableStruct => BasicParametersAndModifiers("RecursiveStruct") + @"
struct RecursiveStruct
{
RecursiveStruct s;
int i;
}";
public static string MutuallyRecursiveImplicitlyBlittableStruct => BasicParametersAndModifiers("RecursiveStruct1") + @"
struct RecursiveStruct1
{
RecursiveStruct2 s;
int i;
}

struct RecursiveStruct2
{
RecursiveStruct1 s;
int i;
}";
}
}
32 changes: 32 additions & 0 deletions DllImportGenerator/DllImportGenerator.UnitTests/CompileFails.cs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@ public static IEnumerable<object[]> CodeSnippetsToCompile()

// Abstract SafeHandle type by reference
yield return new object[] { CodeSnippets.BasicParameterWithByRefModifier("ref", "System.Runtime.InteropServices.SafeHandle"), 1, 0 };

// Non-blittable instantiation of generic type
yield return new object[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<bool>(), 5, 0 };

// No marshalling annotations

yield return new object[] { CodeSnippets.ImplicitlyBlittableStructParametersAndModifiers("public"), 5, 0 };
yield return new object[] { CodeSnippets.ImplicitlyBlittableGenericTypeParametersAndModifiers<bool>(), 5, 0 };
yield return new object[] { CodeSnippets.ImplicitlyBlittableGenericTypeParametersAndModifiers<int>("public"), 5, 0 };
}

[Theory]
Expand All @@ -113,5 +122,28 @@ public async Task ValidateSnippets(string source, int expectedGeneratorErrors, i
int compilerErrors = newComp.GetDiagnostics().Count(d => d.Severity == DiagnosticSeverity.Error);
Assert.Equal(expectedCompilerErrors, compilerErrors);
}

public static IEnumerable<object[]> CodeSnippetsToCompile_InvalidCode()
{
yield return new object[] { CodeSnippets.RecursiveImplicitlyBlittableStruct, 5, 1 };
yield return new object[] { CodeSnippets.MutuallyRecursiveImplicitlyBlittableStruct, 5, 2 };
}

[Theory]
[MemberData(nameof(CodeSnippetsToCompile_InvalidCode))]
public async Task ValidateSnippets_InvalidCodeGracefulFailure(string source, int expectedGeneratorErrors, int expectedCompilerErrors)
{
// Do not validate that the compilation has no errors that the generator will not fix.
Compilation comp = await TestUtils.CreateCompilation(source);

var newComp = TestUtils.RunGenerators(comp, out var generatorDiags, new Microsoft.Interop.DllImportGenerator());

// Verify the compilation failed with errors.
int generatorErrors = generatorDiags.Count(d => d.Severity == DiagnosticSeverity.Error);
Assert.Equal(expectedGeneratorErrors, generatorErrors);

int compilerErrors = newComp.GetDiagnostics().Count(d => d.Severity == DiagnosticSeverity.Error);
Assert.Equal(expectedCompilerErrors, compilerErrors);
}
}
}
21 changes: 20 additions & 1 deletion DllImportGenerator/DllImportGenerator.UnitTests/Compiles.cs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@ public static IEnumerable<object[]> CodeSnippetsToCompile()

// Structs
yield return new[] { CodeSnippets.BlittableStructParametersAndModifiers };
yield return new[] { CodeSnippets.GenericBlittableStructParametersAndModifiers };

// SafeHandle
yield return new[] { CodeSnippets.BasicParametersAndModifiers("Microsoft.Win32.SafeHandles.SafeFileHandle") };
Expand Down Expand Up @@ -184,6 +183,26 @@ public static IEnumerable<object[]> CodeSnippetsToCompile()
yield return new[] { CodeSnippets.ByValueParameterWithName("Method", "@event") };
yield return new[] { CodeSnippets.ByValueParameterWithName("Method", "@var") };
yield return new[] { CodeSnippets.ByValueParameterWithName("@params", "i") };

// Generics
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<byte>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<sbyte>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<short>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<ushort>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<int>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<uint>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<long>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<ulong>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<float>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<double>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<IntPtr>() };
yield return new[] { CodeSnippets.MaybeBlittableGenericTypeParametersAndModifiers<UIntPtr>() };

// Implicit blittable types
yield return new[] { CodeSnippets.ImplicitlyBlittableStructParametersAndModifiers() };
yield return new[] { CodeSnippets.ImplicitlyBlittableStructParametersAndModifiers("internal") };
yield return new[] { CodeSnippets.ImplicitlyBlittableGenericTypeParametersAndModifiers<int>() };
yield return new[] { CodeSnippets.ImplicitlyBlittableGenericTypeParametersAndModifiers<int>("internal") };
}

[Theory]
Expand Down
17 changes: 16 additions & 1 deletion DllImportGenerator/DllImportGenerator/TypePositionInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,12 @@ private static MarshallingInfo GetMarshallingInfo(ITypeSymbol type, IEnumerable<

if (SymbolEqualityComparer.Default.Equals(compilation.GetTypeByMetadataName(TypeNames.BlittableTypeAttribute), attributeClass))
{
return new BlittableTypeAttributeInfo();
// If type is generic, then we need to re-evaluate that it is blittable at usage time.
if (type is INamedTypeSymbol { IsGenericType: false } || type.HasOnlyBlittableFields())
{
return new BlittableTypeAttributeInfo();
}
break;
}
else if (SymbolEqualityComparer.Default.Equals(compilation.GetTypeByMetadataName(TypeNames.NativeMarshallingAttribute), attributeClass))
{
Expand Down Expand Up @@ -335,6 +340,16 @@ static bool TryCreateTypeBasedMarshallingInfo(ITypeSymbol type, DefaultMarshalli
marshallingInfo = new ArrayMarshallingInfo(GetMarshallingInfo(elementType, Array.Empty<AttributeData>(), defaultInfo, compilation, diagnostics, scopeSymbol));
return true;
}

if (type is INamedTypeSymbol { IsValueType: true } valueType
&& !valueType.IsExposedOutsideOfCurrentCompilation()
&& valueType.IsConsideredBlittable())
{
// Allow implicit [BlittableType] on internal value types.
marshallingInfo = new BlittableTypeAttributeInfo();
return true;
}

marshallingInfo = NoMarshallingInfo.Instance;
return false;
}
Expand Down
92 changes: 61 additions & 31 deletions DllImportGenerator/DllImportGenerator/TypeSymbolExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,34 +13,44 @@ namespace Microsoft.Interop
{
static class TypeSymbolExtensions
{
public static bool HasOnlyBlittableFields(this ITypeSymbol type)
public static bool HasOnlyBlittableFields(this ITypeSymbol type) => HasOnlyBlittableFields(type, new HashSet<ITypeSymbol>(SymbolEqualityComparer.Default));

private static bool HasOnlyBlittableFields(this ITypeSymbol type, HashSet<ITypeSymbol> seenTypes)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do me a favor here and pass a recursion counter - when it reaches 0 we throw. Alternatively this could be converted to use a Stack<T> and turned into an iterative solution.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same protection for IsConsideredBlittable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed offline, I think we should leave Stack-overflow protection due to too many nested structs to the Roslyn compiler infrastructure.

{
if (seenTypes.Contains(type))
{
// A recursive struct type isn't blittable.
// It's also illegal in C#, but I believe that source generators run
// before that is detected, so we check here to avoid a stack overflow.
return false;
}
seenTypes.Add(type);
foreach (var field in type.GetMembers().OfType<IFieldSymbol>())
{
bool? fieldBlittable = field switch
if (!field.IsStatic)
{
{ IsStatic : true } => null,
{ Type : { IsReferenceType : true }} => false,
{ Type : IPointerTypeSymbol ptr } => IsConsideredBlittable(ptr.PointedAtType),
{ Type : IFunctionPointerTypeSymbol } => true,
not { Type : { SpecialType : SpecialType.None }} => IsSpecialTypeBlittable(field.Type.SpecialType),
// Assume that type parameters that can be blittable are blittable.
// We'll re-evaluate blittability for generic fields of generic types at instantation time.
{ Type : ITypeParameterSymbol } => true,
{ Type : { IsValueType : false }} => false,
// A recursive struct type isn't blittable.
// It's also illegal in C#, but I believe that source generators run
// before that is detected, so we check here to avoid a stack overflow.
// [TODO]: Handle mutual recursion.
_ => !SymbolEqualityComparer.Default.Equals(field.Type, type) && IsConsideredBlittable(field.Type)
};

if (fieldBlittable is false)
{
return false;
bool fieldBlittable = field switch
{
{ Type: { IsReferenceType: true } } => false,
{ Type: IPointerTypeSymbol ptr } => IsConsideredBlittable(ptr.PointedAtType),
{ Type: IFunctionPointerTypeSymbol } => true,
not { Type: { SpecialType: SpecialType.None } } => IsSpecialTypeBlittable(field.Type.SpecialType),
// Assume that type parameters that can be blittable are blittable.
// We'll re-evaluate blittability for generic fields of generic types at instantation time.
{ Type: ITypeParameterSymbol } => true,
{ Type: { IsValueType: false } } => false,
_ => IsConsideredBlittable(field.Type, seenTypes)
};

if (!fieldBlittable)
{
seenTypes.Remove(type);
return false;
}
}
}

seenTypes.Remove(type);
return true;
}

Expand All @@ -62,14 +72,16 @@ or SpecialType.System_IntPtr
_ => false
};

public static bool IsConsideredBlittable(this ITypeSymbol type)
public static bool IsConsideredBlittable(this ITypeSymbol type) => IsConsideredBlittable(type, new HashSet<ITypeSymbol>(SymbolEqualityComparer.Default));

private static bool IsConsideredBlittable(this ITypeSymbol type, HashSet<ITypeSymbol> seenTypes)
{
if (type.SpecialType != SpecialType.None)
{
return IsSpecialTypeBlittable(type.SpecialType);
}

if (type.TypeKind == TypeKind.FunctionPointer)
if (type.TypeKind is TypeKind.FunctionPointer or TypeKind.Pointer)
{
return true;
}
Expand All @@ -79,14 +91,9 @@ public static bool IsConsideredBlittable(this ITypeSymbol type)
return false;
}

if (type is IPointerTypeSymbol { PointedAtType: ITypeSymbol pointedAtType })
{
return pointedAtType.IsConsideredBlittable();
}

if (type is INamedTypeSymbol { TypeKind: TypeKind.Enum, EnumUnderlyingType: ITypeSymbol underlyingType })
{
return underlyingType!.IsConsideredBlittable();
return underlyingType.IsConsideredBlittable(seenTypes);
}

bool hasNativeMarshallingAttribute = false;
Expand All @@ -107,7 +114,7 @@ public static bool IsConsideredBlittable(this ITypeSymbol type)
// since we are guaranteed that if a type has generic fields,
// they will be present in the contract assembly to ensure
// that recursive structs can be identified at build time.
return generic.HasOnlyBlittableFields();
return generic.HasOnlyBlittableFields(seenTypes);
}
return true;
}
Expand All @@ -126,7 +133,16 @@ public static bool IsConsideredBlittable(this ITypeSymbol type)
// The struct type has generated marshalling via a source generator.
// We can't guarantee that we can see the results of the struct source generator,
// so we re-calculate if the type is blittable here.
return type.HasOnlyBlittableFields();
return type.HasOnlyBlittableFields(seenTypes);
}

if (type is INamedTypeSymbol namedType
&& namedType.DeclaringSyntaxReferences.Length != 0
&& !namedType.IsExposedOutsideOfCurrentCompilation())
{
// If a type is declared in the current compilation and not exposed outside of it,
// we will allow it to be considered blittable if its fields are considered blittable.
return type.HasOnlyBlittableFields(seenTypes);
}
return false;
}
Expand Down Expand Up @@ -165,5 +181,19 @@ or SpecialType.System_IntPtr
_ => false
};
}

public static bool IsExposedOutsideOfCurrentCompilation(this INamedTypeSymbol type)
{
for (; type is not null; type = type.ContainingType)
{
Accessibility accessibility = type.DeclaredAccessibility;

if (accessibility is Accessibility.Internal or Accessibility.ProtectedAndInternal or Accessibility.Private or Accessibility.Friend or Accessibility.ProtectedAndFriend)
{
return false;
}
}
return true;
}
}
}
2 changes: 1 addition & 1 deletion eng/Versions.props
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
<!-- Roslyn dependencies -->
<CompilerPlatformVersion>3.10.0-3.21229.26</CompilerPlatformVersion>
<CompilerPlatformTestingVersion>1.0.1-beta1.20478.1</CompilerPlatformTestingVersion>
<MicrosoftCodeAnalysisAnalyzersVersion>3.3.2</MicrosoftCodeAnalysisAnalyzersVersion>
<MicrosoftCodeAnalysisAnalyzersVersion>3.3.3-beta1.21268.3</MicrosoftCodeAnalysisAnalyzersVersion>
<!-- Package dependencies -->
<SystemCommandLineVersion>2.0.0-beta1.21118.1</SystemCommandLineVersion>
<IcedVersion>1.8.0</IcedVersion>
Expand Down