[cxx-interop] Forbid C++ function template instantiation with Swift types

[j-hui]

Instantiating C++ function templates with Swift types is not currently supported, but the Swift compiler attempts to do so. Sometimes this leads to a compiler crash; other times it leads to a runtime crash. This patch turns those crashes into compiler errors.

At the call site of a C++ function template, the Swift compiler must convert Swift types deduced by the Swift type checker back into Clang types, which are then used to instantiate the C++ function template. Prior to this patch, this conversion was performed by ClangTypeConverter::convert(), which converts unsupported Swift types. This patch factors out some reusable parts of convert() and uses them in a new ClangTypeConverter::convertTemplateArgument() method that only converts supported template argument types. In the future, this method can be elaborated to support instantiating C++ templates with more types.

rdar://112692940

[j-hui]

@swift-ci please test

[j-hui]

@swift-ci please test

[j-hui]

@swift-ci Please test

[j-hui]

This patch does not attempt to fix an existing issue, where the Swift compiler crashes when trying to instantiate a C++ function template with an optional pointer type. Notably, doing this still causes the compiler to crash:

// C++
template <typename T> void T takes(T t) { }    // Some C++ template function

int *ptr;                                      // Some C++ ptr

// Swift
takes(ptr)    // crashes, because the type of ptr is UnsafeMutablePointer<Int32>?

A workaround is to remove the optionality from the pointer:

let sptr = ptr!
takes(sptr)   // OK

[j-hui]

C function pointers are also not supported as a result of this patch. I cannot seem reliably obtain the original Clang type for imported function types.

[Xazax-hun]

Could you open some tickets for these issues that are not covered by this PR?

[Xazax-hun]

I wonder if it would be possible to split this up to 2 PRs. One that is purely a refactoring, and one that actually introduces the functional changes. It would make the review much easier. But in case that is too much work it is fine as is.

[j-hui]

@swift-ci please test

[j-hui]

@swift-ci please test

[j-hui]

To track the issues with instantiating C++ function templates with pointer types, I've opened rdar://142850017

[Xazax-hun]

Looks good to me, some minor nits/questions inline.

[Xazax-hun]

I think it is possible to replace the double lookup in Cache case of a cache miss using try_emplace. Something like:

auto [it, inserted] = Cache.try_emplace(type, clang::QualType{});
if (!inserted)
  it->second = lookup();
return it->second;

Also, if lookup() returns null, do we want to repeat it the next call? Is there any reason to expect a different result next time?

[Xazax-hun]

I see there are other parts of this file that are doing double lookups in case of cache misses, up to you if you want to fix those as well.

[j-hui]

Also, if lookup() returns null, do we want to repeat it the next call? Is there any reason to expect a different result next time?

Probably not, but I'm not fully confident of that. Do you reckon it's worth inserting an #ifndef NDEBUG to catch such circumstances? I think I'll do that in a subsequent patch, if at all. But it could be a good invariant to check for during debug builds.

[j-hui]

I see there are other parts of this file that are doing double lookups in case of cache misses, up to you if you want to fix those as well.

I think it might be better to fix those in a follow-up patch, since this one is starting to get large. I'll change my use of Cache.find() though.

[j-hui]

Er, sanity check: you meant if (inserted), right?

auto [it, inserted] = Cache.try_emplace(type, clang::QualType{});
if (inserted)
  it->second = lookup();
return it->second;

inserted == true would imply there was previously no entry, which is when we would need to perform a lookup(). If inserted == false then there was already a cached entry.

[Xazax-hun]

Sorry, my bad! You are right!

[j-hui]

As it turns out, DenseMap::try_emplace() does not work because the returned iterator may get invalidated by insertions during nested conversions, i.e.:

auto [it, inserted] = Cache.try_emplace(type, clang::QualType{});
if (inserted)
  it->second = lookup(); // lookup() may cause insertions that invalidate it
return it->second;

So I'm going to revert that change.

(Also I realize that lookup() is not the best choice of variable name; I've renamed it to conversion().)

[j-hui]

Also, following up on this:

Also, if lookup() returns null, do we want to repeat it the next call? Is there any reason to expect a different result next time?

It turns out that it is necessary, even though the original convert() function did not appear to do this. I don't remember why I even added that check, or how the old convert() function was working before... I'll leave it as is for now but investigate that in a follow-up patch.

[Xazax-hun]

I am a bit on the edge whether we want to use a TypeVisitor here, but maybe it is OK as is for now.

[j-hui]

I considered doing that but I figured we don't visit more types than we visit, and setting up the TypeVisitor pattern also involves a lot of additional boilerplate. My thinking was that we can refactor this in the future, according to our needs then.

[Xazax-hun]

In case there is a concise way to describe what needs to be true for a Swift type to be supported as a C++ template argument, that would be a useful comment to add. In case it would just repeat the implementation details, feel free to ignore this.

[j-hui]

I had written something describing what you are asking in a comment, which I moved to the declaration in the header file in afe1e61.

[j-hui]

@swift-ci Please test

[j-hui]

@swift-ci Please test

[j-hui]

Hm. I'm not sure why I'm now encountering errors while building this in CI.... I'm going to try this one more time.

[j-hui]

@swift-ci please test

[j-hui]

@swift-ci Please test