Remove usages of `void *` in CASTBridging.h

[ahoppen]

We are using void * for all parameters that pass AST nodes around. This gives us no type safety and makes it really easy to e.g. pass an Expr to a parameter that takes a Decl.

We have two options to fix this:

• Use C++ interop to pass around the actual AST node types
• For every C++ AST node, define a C struct that wraps the void *, e.g. BridgedExpr { void *expr } and then mark the parameter as taking a BridgedExpr instead of void *

[saehejkang]

@ahoppen I would like to work on this issue first because it is dependent on #68350 . Seems like a smaller piece that will help me wrap my head around the codebase.

[ahoppen]

Oh, this is quite a big issue. We will need to decide whether we want to use C++ interop (and getting that building might be very non-trivial, especially if you’r unfamiliar with the codebase) or define the C structs as I mentioned in the description.

[saehejkang]

i see it is not labeled as a good first issue, but wanted to take a stab at it 😅 . I am more familar with C as a programming language, so defining structs makes more sense. But, is there a little more direction you can give me for this issue before I tackle it myself?

[ahoppen]

I really appreciate your enthusiasm here. The question is whether we want to define structs at all or whether we want to expose the C++ types using C++ interop. I’m leaning towards the latter but until we have made that decision, we can’t really resolve this issue. And introducing C++ interop might be more about figuring out what the magic CMake incantations are to making it work, rather than actually writing code. So, I just want to set reasonable expectations that even among the issues not marked as “good first issue”, this is one of the harder ones that requires knowledge of the entire compiler build as a whole.

[saehejkang]

I really appreciate your enthusiasm here. The question is whether we want to define structs at all or whether we want to expose the C++ types using C++ interop. I’m leaning towards the latter but until we have made that decision, we can’t really resolve this issue. And introducing C++ interop might be more about figuring out what the magic CMake incantations are to making it work, rather than actually writing code. So, I just want to set reasonable expectations that even among the issues not marked as “good first issue”, this is one of the harder ones that requires knowledge of the entire compiler build as a whole.

is it still appropriate to work on the other issue that is dependent on this one? The learning curve seems flatter. I can work on all the steps except the one that relies on this issue.

[AnthonyLatsis]

is it still appropriate to work on the other issue that is dependent on this one?

Absolutely! Item 3 of #68350 is the only one dependent on this, and we can easily extract it into a separate issue once the other items are ticked off.

[saehejkang]

I understand that this issue is not really something we want to focus on at the moment, but my mind keeps coming back to it after working on #68350. I just wanted some clarification about pieces, for if I ever get a chance to work on it.

Looking at the CastBridging.h file, I searched for any instances of void *.

On line 571, I took this snippet

void *AttributedTypeRepr_create(BridgedASTContext cContext, void *base,
                                BridgedTypeAttributes cAttributes);

The parameter base is of type void* and I looked up anywhere it is being called. Are all the functions like these the ones we want updated?

Furthermore, in Types.swift, I found this piece of code

type = .type(AttributedTypeRepr_create(self.ctx, type.rawValue, typeAttributes))

type is set based on the visit functions and it returns an ASTNode. So I see we want to update the params of AttributedTypeRepr_create , but how do we know which type (expr, decl, stmt) should be explicitly passed per function? Finally, I then see how we update the function declaration with the correct explicit type we really want (the C struct way).

Does my thought process make sense here? I wanted to understand what was going on for practice, even if y'all are unsure how to tackle this (C++ interop or C structs).

[ahoppen]

but how do we know which type (expr, decl, stmt) should be explicitly passed per function?

I’m not entirely sure I understand the question. Just two thoughts that might answer it though:

• The visit methods should no longer have the return type ASTNode but e.g. visit(TypeSyntax) should return a Type C++ AST node.
• AttributedTypeRepr_create should no longer return a void * but an AttributeTypeRepr type that represents the C++ AST node.

But overall, I think your thoughts make sense.

[saehejkang]

void *AttributedTypeRepr_create(BridgedASTContext cContext, void *base,
                                BridgedTypeAttributes cAttributes);

I’m not entirely sure I understand the question.

We want to remove the return type of void* and the parameter void* base correct?

type = .type(AttributedTypeRepr_create(self.ctx, type.rawValue, typeAttributes))

An UnsafeMutableRawPointer is being passed currently AND I see that the value is being set in ASTGen.swift. The whole point is to not pass any type and instead the EXPLICIT type. For void *base, do we want to pass the ASTNode.Type instead?

should return a Type C++ AST node

Is this what you mean by returning node Type?

  func generate(_ node: TypeSyntax) -> ASTNode.Type {
    return generate(Syntax(node))
  }

[ahoppen]

Is this what you mean by returning node Type?

Not really. If we choose to use C++ interop, it should be returning Type where Type is the C++ class defined here: https://github.com/apple/swift/blob/0c1a2c943d19ba8e5d03372ce2ad52594435f681/include/swift/AST/Type.h#L245