Fix effective context construction for NS_OPTIONS in linkage spec

[NuriAmari]

This patch is motivated by more errors produced when finalizing the SwiftLookupTable with cxx-interop enabled. In particular, SwiftLookupTable::translateContextDecl only knows how to convert a small set of decls into a StoredContext. When cxx-interop is turned on, we get more LinkageSpecDecl nodes that ordinary.

If we ever created an EffectiveClangContext which holds reference to a LinkageSpecDecl we'll get mapping errors such those shown in the added test.

In this patch, we have modified the constructor of EffectiveClangContext to skip over any LinkageSpecDecls. We have noticed that determineEffectiveContext includes some of the same behavior. In particular, it dispatches to getRedeclContext which will skip so called "transparent" contexts, LinkageSpecDecls included:

DeclContext *DeclContext::getRedeclContext() {
  DeclContext *Ctx = this;

  // In C, a record type is the redeclaration context for its fields only. If
  // we arrive at a record context after skipping anything else, we should skip
  // the record as well. Currently, this means skipping enumerations because
  // they're the only transparent context that can exist within a struct or
  // union.
  bool SkipRecords = getDeclKind() == Decl::Kind::Enum &&
                     !getParentASTContext().getLangOpts().CPlusPlus;

  // Skip through contexts to get to the redeclaration context. Transparent
  // contexts are always skipped.
  while ((SkipRecords && Ctx->isRecord()) || Ctx->isTransparentContext())
    Ctx = Ctx->getParent();
  return Ctx;
}

This particular error, can also be fixed by calling determineEffectiveContext on produced contexts like so:

diff --git a/lib/ClangImporter/ImportName.cpp b/lib/ClangImporter/ImportName.cpp
--- a/lib/ClangImporter/ImportName.cpp
+++ b/lib/ClangImporter/ImportName.cpp
@@ -1773,7 +1773,8 @@
         // It also means they may not have intended this API to be imported like this.
         if (importer::isUnavailableInSwift(typedefType->getDecl(), nullptr, true)) {
           result.setDeclName(swiftCtx.getIdentifier(typedefType->getDecl()->getName()));
-          result.setEffectiveContext(D->getDeclContext());
+          result.setEffectiveContext(
+              determineEffectiveContext(D, D->getDeclContext(), version));
           return result;
         }
       }

We could use some input on if this unwrapping should occur at all times, and the best place for it (determineEffectiveContext vs EffectiveClangContext::EffectiveClangContext) as the logic seems split between the two at the moment.

[NuriAmari]

@swift-ci please test

[NuriAmari]

@swift-ci please test

[NuriAmari]

@swift-ci please test

[NuriAmari]

@swift-ci please test

[plotfi]

Very nice work @NuriAmari. I think I overlooked the possibility of nesting LinkageSpecDecls when I previously did the fix for EffectiveClangContext. I suspect that this would be the case in UIKit because the UIKIT_EXTERN macro is used in code as well when the [extern_C] specifier is also set in the module.modulemap.

[NuriAmari]

@swift-ci please test source compatibility

[zoecarver]

Can you add a test with multiple extern "C" blocks? (That seems to be why you added the while loop, right?)

[zoecarver]

This LGTM though

[NuriAmari]

Can you add a test with multiple extern "C" blocks? (That seems to be why you added the while loop, right?)

The existing test already has two nested, one explicit and one introduced by UIKIT_EXTERN.

[plotfi]

Can you add a test with multiple extern "C" blocks? (That seems to be why you added the while loop, right?)

I think in practice the double extern C happens implicitly by having an extern C both in code and in the module specifier in the modulemap. I think for testing there should probably be a doubly nested extern C in code case and an extern C from module map plus an extern C from code case.

[NuriAmari]

I think in practice the double extern C happens implicitly by having an extern C both in code and in the module specifier in the modulemap. I think for testing there should probably be a doubly nested extern C in code case and an extern C from module map plus an extern C from code case.

Alright, I'll add a test to cover the "extern C from module map" case.

[NuriAmari]

@swift-ci please test