[cxx-interop] Support class template specializations in namespaces.

[zoecarver]

This just changes an assertion and adds a test.

[hlopko]

This assert doesn't make much sense to me. I tried to debug into your test case and I see that the ClassTemplateSpecializationDecl->decls() returns this (to me) surprising prev decl. And it's this one that member == resultapplies to. Do you understand why is this prev decl there?

In any case, could you please update the comment there to explain why member == result is acceptable?

[zoecarver]

Let me preface this by saying I am not confident in my answer, this is only my hypothesis, someone more familiar with clang might have a more correct answer.

Because Orbiter is a redeclaration of Ship, it should get the previous declaration (Ship) as the PrevDecl (which gets added as one of the decl context's sub-declarations). However, it looks like it actually gets Orbiter as the previous declaration... not sure why that is.

Why didn't we come across this issue for record decls? Because (AFAIK) they aren't "redeclarable".

Why doesn't this issue arise for top-level declarations? Because then the "member" (really the class template specialization)'s decl context is not a nominal type decl, it's a "FileUnit" so this path wasn't ever taken.

So maybe it would be better to keep the old assertion and add:

if (member->getClangDecl() == result->getClangDecl()->getPreviousDecl())
  continue;

...with a comment.

[zoecarver]

Can you think of an example where a member would have a decl context that isn't result? Could we just say:

if (member->getDeclContext() != result) 
  continue;

[hlopko]

I'm a bit surprised to see the redeclaration as a member, but then I'm not sure I understand why we're even attempting to import the redeclaration? My first instinct would be to check if the member is in decl->redecls() and if yes skip it. I tried this quickly and it seems it doesn't break any tests. Wdyt?

Also cc @gribozavr for clang expertise.

[zoecarver]

That sounds good to me, I would also be interested to hear what Dmitri thinks, though.

I agree this is all odd. I think a similar (or the same?) problem is the genesis of the issue highlighted in #34557. In the test case in that PR, somehow we get from a fully specialized class template to the original class template (I presume via one of its redecls or something).

[zoecarver]

Great, that makes a lot of sense. I'll use that. Thanks for the information :)

The only thing that still seems odd to me: both class templates and "normal" record decls should contain injected class names, not just class templates. Any idea why this would only be failing for class templates and not all record decls?

[zoecarver]

While that does seem to work for the test case in this PR, it doesn't work for the following:

namespace Space {

template <class...> struct Ship;
template <class T, class ...Args> struct Ship<T(Args...)> {};

using Orbiter = Ship<void(bool)>;

} // namespace Space

I'd be interested to hear @gribozavr and your thoughts on why that is still asserting and on my above question (why this doesn't show up for "normal" record types).

Also, unfortunately, this doesn't fix the issue in #34557.

[zoecarver]

Oops, I see my mistake.

I decided to put the check for isInjectedClassName at the top of VisitRecordDecl (my thinking was that we should never really be visiting an injected class name, and it would have the same effect), but right above we set the decl we're visiting to decl->getDefinition() which essentially made the check a noop.

After applying your suggested fix, both test cases complied. (The issue in #34557 still did not, though.)

[hlopko]

I think we also observe injected class names with record decls, it's just that their import doesn't fail so nobody noticed? At some point of the import we ask for canonical decl and might get a cache hit for the injected class name. I'm guessing though, we could just step through that with a debugger to be sure.

[zoecarver]

I think we also observe injected class names with record decls, it's just that their import doesn't fail so nobody noticed?

The import doesn't fail for these class template specializations either; we hit an assertion. I would expect that we should hit that assertion for "normal" record decl injected class names too.

Edit: I'll set some breakpoints and see if we ever run into an injected class name for a "normal" record decl.

[hlopko]

LGTM, thanks!

[zoecarver]

@swift-ci please smoke test.

[zoecarver]

#34706 will fix the build errors.

[hlopko]

Quick comment (not sure I'll have time today to look into it deeper today). Doesn't this collide with this logic for namespaces?

swift/lib/ClangImporter/ImportDecl.cpp

Line 8893 in 5286d9c

enumDecl->addMember(member);

[zoecarver]

Hmm, you might be right. I wonder if this allows us to omit that logic entirely. (That would be nice!) I'll investigate.

[zoecarver]

So, I did some investigation, and you're right. This was the source of the assertion we're hitting. The issue is that this only needs to be here for class template specializations because they're not added in loadAllMembers. So, I removed the addMember here and moved it to a new ClassTemplateVisitor member. Let me know what you think.

[zoecarver]

Also (kind of unrelated) I'm going to put up a patch to refactor the namespace logic in loadAllMembers.

[zoecarver]

Just FYI @hlopko, ClassTemplateVisitor doesn't overlap with #33284. But you might still have merge conflicts.

[zoecarver]

@swift-ci please smoke test.

[zoecarver]

@swift-ci please smoke test OS X.

[zoecarver]

@swift-ci please test Windows.

[zoecarver]

@swift-ci please test Windows.

[zoecarver]

@swift-ci please test Windows.

[zoecarver]

@swift-ci please test Windows.

[zoecarver]

Still a no space left on disk error.

[zoecarver]

@swift-ci please test Windows.

[zoecarver]

@swift-ci please test Windows.