[cxx-interop] Fix infinite recursion of PackExpansion type. #34427
Conversation
lib/ClangImporter/ImportType.cpp
Outdated
| @@ -832,6 +832,8 @@ namespace { | |||
|
|
|||
| #define SUGAR_TYPE(KIND) \ | |||
| ImportResult Visit##KIND##Type(const clang::KIND##Type *type) { \ | |||
There was a problem hiding this comment.
When importing large C++ libraries I've noticed other types cause infinite recursion here too (not just PackExpansion). I don't remember off the top of my head which types, but I can try to find some more test cases. Anyway, I think this is a good check for all "sugar types".
There was a problem hiding this comment.
I don't fully understand this fix. Is PackExpansion sometimes a sugared type and sometimes isn't? If so, shouldn't it have a hand-written Visit method instead of returning a bogus Type() when it is in canonical form?
There was a problem hiding this comment.
Yes. Some of (all of?) these types sometimes are sugared and sometimes aren't. We should probably have custom visitors for most if not all of them. But until then, we really shouldn't create an infinite loop by visiting the same type over and over again.
The test I created is for PackExpansion but I suspect we could find test cases for all the below types that have an isSugared method. If you'd like, I can add a few more test cases.
That makes me think, I wonder if I could replace the .getTypePtr() == type with .isSugared(). I'll give that a try later today.
There was a problem hiding this comment.
I don't want to sound greedy, but ideally we would get a test case for each sugared type (I don't think I'd be able to come up with test cases for all of them TBH :) If all of them are susceptible to the infinite recursion, it's fine to have the guard where you have it currently. If it's only some of them, let's only guard those, wdyt?
There was a problem hiding this comment.
We should probably have custom visitors for most if not all of them. But until then, we really shouldn't create an infinite loop by visiting the same type over and over again.
Ah OK, so this is intended as a stop-gap fix, not as a full solution. That's fine by me. 👍🏼 (Having an inline FIXME stating that the custom visitors need to be written would be nice though.)
+1 to Martin's idea of having test cases for the different potentially-sugared types.
There was a problem hiding this comment.
Ah OK, so this is intended as a stop-gap fix, not as a full solution. That's fine by me. 👍🏼 (Having an inline FIXME stating that the custom visitors need to be written would be nice though.)
I'll add a TODO. Good suggestion.
I don't want to sound greedy, but ideally we would get a test case for each sugared type (I don't think I'd be able to come up with test cases for all of them TBH :) If all of them are susceptible to the infinite recursion, it's fine to have the guard where you have it currently. If it's only some of them, let's only guard those, wdyt?
I'll add some more test cases. I'll add as many as I can come up with but some of these, I'm not sure I can find a test case for. In that case, I still think we should bail here. Even if I can't find a test case, it's conceivable that a user will. Instead of crashing when attempting to import a huge library that contains some super weird type that I user figured out how to make, we should just fail to import that type. We never want to visit the desugared version of a non-sugared type. Even if we can't find a case where that type would exist, I still think we should keep the condition.
zoecarver
force-pushed
the
cxx/variadic-args
branch
from
78339e1 to
cb88ca0
Compare
|
Updates:
|
Fix infinite recursion issue in the SwiftTypeConverter. This patch is purely to prevent crashes.
zoecarver
force-pushed
the
cxx/variadic-args
branch
from
cb88ca0 to
937e584
Compare
|
@swift-ci please smoke test and merge. |
1 similar comment
|
@swift-ci please smoke test and merge. |
Fix infinite recursion issue in the SwiftTypeConverter. At some point, we should support variadic types properly but this patch is purely to prevent crashes.