[cxx-interop] Allow importing templated functions when template args do not appear in function signature #39535
Conversation
|
WIP because there seems to be some wonkiness with how this interacts with default template arguments, still investigating a fix there. |
There was a problem hiding this comment.
What a great patch! Thank you, Josh.
I really like how you commented all the code you added so it's super easy to understand and follow. Also, thanks for factoring out that lambda into a general helper function.
I have one or two small nits, but overall, this patch seems good to me :)
lib/ClangImporter/ImportType.cpp
Outdated
| bool shouldCheckResultType) -> bool { | ||
| auto paramDecl = genericParam->getClangDecl(); | ||
| auto templateTypeParam = cast<clang::TemplateTypeParmDecl>(paramDecl); | ||
| // TODO: This won't work when we support importing dependent types. we'll |
There was a problem hiding this comment.
Let's link this to the SR: XXXX.
There was a problem hiding this comment.
Oops, meant to actually give you the SR. Just update the TODO to read TODO(SR-13809):. This way we can grep for the SR and find all the places we need to update whenever we add support for dependent types.
lib/Sema/CSApply.cpp
Outdated
| @@ -109,7 +109,7 @@ Solution::computeSubstitutions(GenericSignature sig, | |||
| lookupConformanceFn); | |||
| } | |||
|
|
|||
| static ConcreteDeclRef generateDeclRefForSpecializedCXXFunctionTemplate( | |||
| static ValueDecl *generateDeclForSpecializedCXXFunctionTemplate( | |||
There was a problem hiding this comment.
I know I named this function, but it's kind of ugly, so while you're updating the signature maybe change it to generateSpecializedCXXFunctionTemplate? WDYT?
| // metatype arguments to aid in typechecking, but they shouldn't be forwarded to | ||
| // the corresponding C++ function. | ||
| static std::pair<BraceStmt *, bool> | ||
| synthesizeForwardingThunkBody(AbstractFunctionDecl *afd, void *context) { |
There was a problem hiding this comment.
Not in this patch, but at some point we should move all synthesizers to their own file. They just take up space right now and clutter the files they're scattered across.
test/Interop/Cxx/templates/Inputs/template-type-parameter-not-in-signature.h
Show resolved
Hide resolved
| templateTypeParamNotUsedInSignature(T: Int.self) | ||
| multiTemplateTypeParamNotUsedInSignature(T: Float.self, U: Int.self) | ||
| multiTemplateTypeParamNotUsedInSignatureWithUnrelatedParams(1, 1, T: Int32.self, U: Int.self) | ||
| let _: Int = templateTypeParamUsedInReturnType(10) |
There was a problem hiding this comment.
Can you add an expectEquals somewhere in both of these tests. Otherwise (very theoretically), they might just get optimized away. Also it's not a bad idea to just test that the correct thing is returned and there wasn't an argument mismatch or something.
|
@guitard0g let me know if you need help running the bots. |
guitard0g
force-pushed
the
tparam-not-used-in-fsig
branch
from
2fa8285 to
d81a5c3
Compare
guitard0g
changed the title
|
@swift-ci please test |
guitard0g
force-pushed
the
tparam-not-used-in-fsig
branch
from
d81a5c3 to
d2c68cc
Compare
|
@swift-ci please test |
|
@egorzhdan @zoecarver Thanks for the review! I believe I've addressed all the comments in the most recent revision. It changed a decent bit because I realized there were some quirks with making sure we manually invoke the generic substitutions on the signature of the synthesized thunk. Should be ready for another round of reviews. |
| static ValueDecl *generateSpecializedCXXFunctionTemplate( | ||
| ASTContext &ctx, AbstractFunctionDecl *oldDecl, SubstitutionMap subst, | ||
| clang::FunctionDecl *specialized) { | ||
| auto newFnTypeAndParams = substituteFunctionTypeAndParamList(ctx, oldDecl, subst); |
There was a problem hiding this comment.
I wish we could use auto [a, b] = 😢
| // exist in the resulting specialized FuncDecl. Note that this doesn't | ||
| // affect constructors because all template params for a constructor | ||
| // must be in the function signature by design. | ||
| continue; |
There was a problem hiding this comment.
I think various methods/static methods might take a meta type. We need to verify if this will break that.
There was a problem hiding this comment.
The tests are passing, so I guess this isn't an issue.
Now you see why I'm so excited about #36082 :P |
in the function signature by adding explicit metatype parameters to the function signature.
guitard0g
force-pushed
the
tparam-not-used-in-fsig
branch
from
d2c68cc to
f433ac2
Compare
|
@swift-ci please smoke test and merge |
Currently if we try to import a templated function when the template parameters do not appear in the function signature, the corresponding Swift function will have extra unused generic parameters, which means we can't call those functions. This patch fixes the problem by making those template params into explicit metatype function parameters to the corresponding Swift function. E.g.
becomes:
instead of:
This extra argument is only used to deduce the generic type needed to specialize the C++ function. The corresponding function call made under the hood will cut out the extra argument and forward along the proper parameters to the C++ function.