Support import of typedefs. #5787
Conversation
Unify code paths for importing classes by name and importing them indirectly when their type is referenced. Switch to using the general type import machinery to import all type declarations, which allows typedef declarations naming importable types to be used too. Fix up handling of error cases to consistently only produce an Error InstId after actually producing an error message, so that we can produce exactly one diagnostic in failure cases. Remove TODO error for unions that previously was only produced when importing them indirectly, not when importing them by name. Import of unions is exactly as complete / incomplete as import of other class types, so treating them differently doesn't seem necessary.
Generally I expect the TODO for unions represents that we're not working on making it work correctly, given there's not an interoperable type in Carbon. By contrast, with classes there's certainly a lot not working, but I think the intent there is more that we should have TODOs to catch issues and we just aren't holistically writing those TODOs (I do believe there are some, and more being added as work is done). At a very high level, we've chosen the representation that a class should translate to: a Carbon class. I believe we've said we'll translate unions to Carbon choices, but the representation of Carbon choices hasn't yet been chosen. Isn't that appropriate for a high-level TODO? |
| // CHECK:STDERR: Cpp.foo({}); | ||
| // CHECK:STDERR: ^~~~~~~ | ||
| // CHECK:STDERR: | ||
| // TODO: This should probably fail once we import members for classes and unions. |
There was a problem hiding this comment.
Note, even if you want unions to work at a high level, maybe you should add a diagnostic for cases that have members? Again, unlike classes, I don't think there are short-term plans to make this work. I would tend to expect a diagnostic to continue existing given this is tested and one was previously present.
| } | ||
|
|
||
| // Maps a C++ type to a Carbon type. | ||
| // TODO: Support more types. | ||
| static auto MapType(Context& context, SemIR::LocId loc_id, clang::QualType type) | ||
| -> TypeExpr { | ||
| if (const auto* builtin_type = dyn_cast<clang::BuiltinType>(type)) { | ||
| if (type.hasQualifiers()) { | ||
| // TODO: Support type qualifiers. |
There was a problem hiding this comment.
Could have a context.TODO?
There was a problem hiding this comment.
Could do. Generally this function is leaving the diagnosing of unsupported types up to the caller, but I don't think it makes much difference. My next patch will be fixing this anyway, so I'd prefer to not change it in this PR just to avoid the churn if that works for you?
| //@dump-sem-ir-begin | ||
| var n: Cpp.foo = 42; | ||
| var m: Cpp.bar = 76; | ||
| //@dump-sem-ir-end |
There was a problem hiding this comment.
Can we also do:
| //@dump-sem-ir-end | |
| var m: Cpp.bar = 76; | |
| // Types are equivalent. | |
| var l: Cpp.bar* = &n; |
And maybe something for class typedefs (since i32 is a special-case), such as:
// C++:
class C {};
typedef C D;
// Carbon:
fn F(m: Cpp.C, n: Cpp.D) {
// Verifies type equivalence.
var o: Cpp.C* = &m;
var p: Cpp.D* = &n;
}
There was a problem hiding this comment.
Done. (I assume you wanted the pointers to be the opposite types from the things they're pointing to, not the same?)
There was a problem hiding this comment.
Yes, that's what I intended. Thanks. :)
| // --- class_typedef.h | ||
|
|
||
| class C {}; | ||
| using D = C; |
There was a problem hiding this comment.
Shouldn't this be a typedef instead of using? I'd have thought using would look a little different in the AST. May also be good to verify using works, but it didn't seem like the focus of this support...
| using D = C; | |
| typedef C D; |
There was a problem hiding this comment.
They're really just different syntaxes for the same thing, it's a typedef either way (in the clang AST they're two different derived classes of the same TypedefNameDecl base class just to preserve the spelling, but they behave the same). The i32 test has examples using both syntaxes; so the other syntax is covered, but outside of the test for that particular syntax I'd prefer to use the more modern using syntax.
Followup of #5787.
Unify code paths for importing classes by name and importing them indirectly when their type is referenced. Switch to using the general type import machinery to import all type declarations, which allows typedef declarations naming importable types to be used too.
Fix up handling of error cases to consistently only produce an Error InstId after actually producing an error message, so that we can produce exactly one diagnostic in failure cases.
Remove TODO error for unions that previously was only produced when importing them indirectly, not when importing them by name. Import of unions is exactly as complete / incomplete as import of other class types, so treating them differently doesn't seem necessary.