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

[SwiftBindings] Added post processing step to compute struct frozenness #2912

Merged
merged 9 commits into from
Jan 10, 2025

Conversation

jkurdek
Copy link
Member

@jkurdek jkurdek commented Jan 8, 2025

Implements #2886

Initialises a post ABI parsing step which calculates information about types and populates type database.

Follow-ups:

  • Add tooling sorting modules in topological order for processing
  • Remove type database usage in ABI parsing
  • Make type database immutable after post-processing step

@jkurdek jkurdek added the area-SwiftBindings Swift bindings for .NET label Jan 8, 2025
@jkurdek jkurdek self-assigned this Jan 8, 2025
@jkurdek jkurdek changed the title [SwiftBindings] [SwiftBindings] Added post processing step to compute struct frozenness Jan 8, 2025
Copy link

@stephen-hawley stephen-hawley left a comment

Choose a reason for hiding this comment

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

Love to see the tests in place. This is a good start, but there are some things that need to be addressed. If it's not practical to resolve them, open issues and keep going.

ProcessStructFields(structDecl);

// TODO: Remove loading dylib
IntPtr metadataPtr = DynamicLibraryLoader.invoke(_dylibPath, $"{structDecl.MangledName}Ma");

Choose a reason for hiding this comment

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

As we discussed, we shouldn't do this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. This will be the next step. Here is a tracking issue #2914. Should also cover the comments about blittability and size, stride, alignment.

/// <returns>True if blittable, false otherwise.</returns>
private unsafe bool EvaluateBlittability(SwiftTypeInfo swiftTypeInfo)
{
// TODO: Replace with a manual approach.

Choose a reason for hiding this comment

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

👍 Is there an issue for this?

{
_typeDatabase.Registrar.RegisterType(namedTypeSpec.Module,
namedTypeSpec.NameWithoutModule,
new TypeRecord

Choose a reason for hiding this comment

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

This is a separate issue, but we will need the size, stride, and alignment as well.

/// <param name="structDecl">The struct declaration.</param>
private void ProcessStruct(NamedTypeSpec namedTypeSpec, StructDecl structDecl)
{
// Ensure all fields are processed or known in the database.

Choose a reason for hiding this comment

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

All nominal types can have inner types. Because fields in the structure may depend on them, we should process any inner types before we process fields.

Copy link
Member Author

Choose a reason for hiding this comment

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

They are processed during ABI parsing so we have full information. Added test for that and patched some logic.

namedTypeSpec.NameWithoutModule,
new TypeRecord
{
Namespace = namedTypeSpec.Module,

Choose a reason for hiding this comment

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

I'd like to see a separate type for holding a type name. Something like:

public class /* or record or struct */ TypeName {
     public string Namespace { get; init; }
     public string Name { get; init; }
     public string FullName { get; } => Namespace + "." + Name;
     // if not a record, make ToString, Equals, etc
}

This will make it easier to set up import statements in Swift and using statements in C#.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a change ready, however it will affect multiple areas so Ill bring it in another PR

@@ -146,7 +146,7 @@ public ModuleDecl GetModuleDecl()

moduleDecl.Fields = decls.OfType<FieldDecl>().ToList();
moduleDecl.Methods = decls.OfType<MethodDecl>().ToList();
moduleDecl.Declarations = decls.Where(d => !(d is MethodDecl) && !(d is FieldDecl)).ToList();
moduleDecl.Types = decls.OfType<TypeDecl>().ToDictionary(d => new NamedTypeSpec(d.FullyQualifiedName), d => d);

Choose a reason for hiding this comment

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

One thing that we will run into right here is that the *Decl types are already C# and that type may not be the same as the Swift from which it came. For example, if we get a Swift type that is unpronounceable in C# (eg, contains emoji) then we've just created an incorrect NamedTypesSpec.

typeRecord.IsProcessed = true;

if (node.Children != null && decl != null)
if (decl != null)

Choose a reason for hiding this comment

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

favor is not null over != null to avoid possible issues that arise from operator implementations.

@jkurdek
Copy link
Member Author

jkurdek commented Jan 9, 2025

CI failures unrelated - #2896 broke compilation. should be fixed once #2909 gets in

@kotlarmilos kotlarmilos self-requested a review January 10, 2025 11:07
@jkurdek jkurdek merged commit da7345b into feature/swift-bindings Jan 10, 2025
6 checks passed
@jkurdek jkurdek deleted the swift/improve-frozen-struct-detection branch January 10, 2025 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-SwiftBindings Swift bindings for .NET
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants