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

Add basic support for frozen structs projections #2560

Merged
merged 21 commits into from
May 21, 2024

Conversation

kotlarmilos
Copy link
Member

@kotlarmilos kotlarmilos commented Apr 23, 2024

Description

This PR implements basic support for frozen structs. It refactors the tooling, including the type database and declarations, to manage recursive dependencies. It introduces Swift metadata projections and type registrar system for mapping between Swift and C#. Additionally, it updates the parser and emitter to interact with the type database and registrar, and implement basic type filtering. Tests are added for non-generic nested frozen structs with primitive properties.

This commit introduces Swift metadata types and implements basic support for frozen structs.
@kotlarmilos kotlarmilos added the area-SwiftBindings Swift bindings for .NET label Apr 23, 2024
@kotlarmilos kotlarmilos self-assigned this Apr 23, 2024
This commit refactors the tooling, including the type database and declarations, to manage recursive dependencies. It introduces a type registrar system for mapping between Swift and C#. Additionally, it updates the parser and emitter to interact with the type database and registrar, and implement basic type filtering. Tests are added for non-generic nested frozen structs with primitive properties.
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.

One thing you're going to run into in your code is that you don't have a distinction between a type declaration and a type specification. This will cause you problems later on.

Here's what the distinction is:
a type declaration is a nominal type in swift. It is anything that has a name and its public facing API. This includes top-level functions, structs, enums, classes, and actors.

A type specification is the annotation that is used to specify the type of a parameter, an associated type, a type alias target, or the return type of a method. In swift, a type specification can include nominal types (fully or partially specified), tuples, closure types, and protocol list types. In addition, parts of these types may include syntactic sugars such as option, implicitly unwrapped options, and meta information such as attributes. All of those elements need to be handled correctly.

Indeed, the representation of these types is, unto itself, a little language.

I strongly recommend that you take TypeSpecParser, TypeSpecTokenizer, TypeSpecToken, and `TypeSpec (and its subclasses). This represents a very compact recursive descent parser than can handle whatever the swift compiler can generate.

@kotlarmilos kotlarmilos changed the title [WIP] Add basic support projecting frozen structs Add basic support projecting frozen structs Apr 30, 2024
@kotlarmilos kotlarmilos marked this pull request as draft May 16, 2024 10:42
@kotlarmilos
Copy link
Member Author

I've converted this PR to draft as we plan to have additional iterations before another review. @stephen-hawley thank you for your contribution. Here are some thoughts:

  • I like your approach with the conductor class and factories, but I don't see the value of having an additional layer on top of the declarations, such as TypeEnv, MethodEnv, and ArgumentEnv. I think we should operate with a single source of truth, which should be the declarations. Do you see anything missing in the declarations?
  • I suggest moving handlers to emitters, as they actually emit code. Perhaps we can name them {Name}Emitter?
  • Can we use FieldDecl instead of ArgumentDecl?
  • It looks like the factory should replace ModuleEmitter and be invoked from Program.cs.
  • I wonder if we can simplify IFactory. The codegen might bloat with such generics usage, and we may try to simplify it. I imagine that in most cases, we would rely on string representation and not actual generic types in the IFactory implementations.

Let's integrate your changes and add comments, documentation, and more test cases.

@kotlarmilos kotlarmilos changed the title Add basic support projecting frozen structs Add basic support for frozen structs projectionsd May 16, 2024
@kotlarmilos kotlarmilos changed the title Add basic support for frozen structs projectionsd Add basic support for frozen structs projections May 16, 2024
@kotlarmilos
Copy link
Member Author

I strongly recommend that you take TypeSpecParser, TypeSpecTokenizer, TypeSpecToken, and `TypeSpec (and its subclasses). This represents a very compact recursive descent parser than can handle whatever the swift compiler can generate.

Let's add them in a follow-up PR as this one is already ~+5k loc. What is the difference between TypeDecl and TypeSpec?

This commit introduces the Conductor class, which is responsible for finding appropriate declaration handler factory. If a handler can handle a declaration, the factory creates a handler. Handlers implement the IHandler interface, which defines two methods: marshal and emit. The marshal method should collect all data required for emitting and return an environment, which is used for emitting. The emit function generates code based on the environment.
/// <param name="decl">The base declaration.</param>
public bool Handles(BaseDecl decl)
{
return decl is TypeDecl && decl is ClassDecl;

Choose a reason for hiding this comment

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

decl is TypeDecl && isn't needed. If the type is a ClassDecl it is always a TypeDecl

/// <param name="decl">The base declaration.</param>
public bool Handles(BaseDecl decl)
{
return decl is TypeDecl && decl is StructDecl;

Choose a reason for hiding this comment

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

decl is TypeDecl && isn't needed.

}
writer.WriteLine();

foreach (BaseDecl baseDecl in structDecl.Declarations)

Choose a reason for hiding this comment

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

This should be done through the conductor, otherwise we will have identical code for classes, structs, enums, and actors when it could be in one place.

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 created BaseHandler that implements HandleBaseDecl which calls into Conductor. Do you have any better ideas here?

Choose a reason for hiding this comment

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

How about putting an EmitInnerTypes method into BaseHandler which looks like:

protected EmitInnerTypes(IEnumerable<TypeDecl> types, IndentedTextWriter writer, Conductor conductor, TypeDatabase typeDatabase) {
    writer.Indent++;
    foreach (var type in types) {
        HandleBaseDecl(writer, baseDecl, conductor, typeDatabase);
    }
    writer.Indent--;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Moved the loop to the HandleBaseDecl. I didn't change the name of the method as it handles other declarations as well. We can rename it later.

@kotlarmilos kotlarmilos marked this pull request as ready for review May 20, 2024 13:57
@kotlarmilos
Copy link
Member Author

Merging this PR. Any additional feedback will be addressed in follow-up work.

@kotlarmilos kotlarmilos merged commit 9315e74 into feature/swift-bindings May 21, 2024
6 checks passed
@kotlarmilos kotlarmilos deleted the swift-bindings/frozen-structs branch May 21, 2024 07:22
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.

None yet

2 participants