Merge pull request #73094 from tshortli/rename-extension-import-visibility-feature-6.0

[6.0] Rename ExtensionImportVisibility to MemberImportVisibility and fix bugs

Author: tshortli

include/swift/AST/Decl.h

@@ -2788,7 +2788,7 @@ class ValueDecl : public Decl {
 public:
   /// Find the import that makes the given declaration available.
   std::optional<AttributedImport<ImportedModule>>
-  findImport(const DeclContext *fromDC);
+  findImport(const DeclContext *fromDC) const;
 
   /// Return true if this protocol member is a protocol requirement.
   ///

include/swift/AST/DiagnosticsSema.def

@@ -167,9 +167,9 @@ ERROR(init_candidate_inaccessible,none,
 
 ERROR(candidate_from_missing_import,none,
       "%0 %1 is not available due to missing import of defining module %2",
-      (DescriptiveDeclKind, DeclName, DeclName))
+      (DescriptiveDeclKind, DeclName, ModuleDecl *))
 NOTE(candidate_add_import,none,
-     "add import of module %0", (DeclName))
+     "add import of module %0", (ModuleDecl *))
 
 ERROR(cannot_pass_rvalue_mutating_subelement,none,
       "cannot use mutating member on immutable value: %0",

include/swift/AST/Module.h

@@ -1269,6 +1269,11 @@ void collectParsedExportedImports(const ModuleDecl *M,
                                   llvm::SmallDenseMap<ModuleDecl *, SmallPtrSet<Decl *, 4>, 4> &QualifiedImports,
                                   llvm::function_ref<bool(AttributedImport<ImportedModule>)> includeImport = nullptr);
 
+/// If the import that would make the given declaration visibile is absent,
+/// emit a diagnostic and a fix-it suggesting adding the missing import.
+bool diagnoseMissingImportForMember(const ValueDecl *decl,
+                                    const DeclContext *dc, SourceLoc loc);
+
 } // end namespace swift
 
 #endif

include/swift/AST/SourceFile.h

@@ -457,6 +457,9 @@ class SourceFile final : public FileUnit {
   /// If not, we can fast-path module checks.
   bool hasImportsWithFlag(ImportFlags flag) const;
 
+  /// Returns the import flags that are active on imports of \p module.
+  ImportFlags getImportFlags(const ModuleDecl *module) const;
+
   /// Get the most permissive restriction applied to the imports of \p module.
   RestrictedImportKind getRestrictedImportKind(const ModuleDecl *module) const;
 

include/swift/Basic/Features.def

@@ -364,8 +364,8 @@ EXPERIMENTAL_FEATURE(ClosureIsolation, true)
 // Enable isolated(any) attribute on function types.
 CONDITIONALLY_SUPPRESSIBLE_EXPERIMENTAL_FEATURE(IsolatedAny, true)
 
-// Whether members of extensions respect the enclosing file's imports.
-EXPERIMENTAL_FEATURE_EXCLUDED_FROM_MODULE_INTERFACE(ExtensionImportVisibility, true)
+// Whether lookup of members respects the enclosing file's imports.
+EXPERIMENTAL_FEATURE_EXCLUDED_FROM_MODULE_INTERFACE(MemberImportVisibility, true)
 
 // Alias for IsolatedAny
 EXPERIMENTAL_FEATURE(IsolatedAny2, true)

lib/AST/Decl.cpp

@@ -3856,7 +3856,7 @@ ValueDecl::getSatisfiedProtocolRequirements(bool Sorted) const {
 }
 
 std::optional<AttributedImport<ImportedModule>>
-ValueDecl::findImport(const DeclContext *fromDC) {
+ValueDecl::findImport(const DeclContext *fromDC) const {
   // If the type is from the current module, there's no import.
   auto module = getModuleContext();
   if (module == fromDC->getParentModule())

lib/AST/FeatureSet.cpp

@@ -680,7 +680,7 @@ static bool usesFeatureIsolatedAny(Decl *decl) {
   });
 }
 
-UNINTERESTING_FEATURE(ExtensionImportVisibility)
+UNINTERESTING_FEATURE(MemberImportVisibility)
 UNINTERESTING_FEATURE(IsolatedAny2)
 
 UNINTERESTING_FEATURE(ObjCImplementation)

lib/AST/Module.cpp

@@ -2702,6 +2702,15 @@ bool SourceFile::hasImportsWithFlag(ImportFlags flag) const {
       ctx.evaluator, HasImportsMatchingFlagRequest{mutableThis, flag}, false);
 }
 
+ImportFlags SourceFile::getImportFlags(const ModuleDecl *module) const {
+  unsigned flags = 0x0;
+  for (auto import : *Imports) {
+    if (import.module.importedModule == module)
+      flags |= import.options.toRaw();
+  }
+  return ImportFlags(flags);
+}
+
 bool SourceFile::hasTestableOrPrivateImport(
     AccessLevel accessLevel, const swift::ValueDecl *ofDecl,
     SourceFile::ImportQueryKind queryKind) const {
@@ -4001,3 +4010,62 @@ version::Version ModuleDecl::getLanguageVersionBuiltWith() const {
 
   return version::Version();
 }
+
+bool swift::diagnoseMissingImportForMember(const ValueDecl *decl,
+                                           const DeclContext *dc,
+                                           SourceLoc loc) {
+  if (decl->findImport(dc))
+    return false;
+
+  auto &ctx = dc->getASTContext();
+  auto definingModule = decl->getModuleContext();
+  ctx.Diags.diagnose(loc, diag::candidate_from_missing_import,
+                     decl->getDescriptiveKind(), decl->getName(),
+                     definingModule);
+
+  SourceLoc bestLoc =
+      ctx.Diags.getBestAddImportFixItLoc(decl, dc->getParentSourceFile());
+  if (!bestLoc.isValid())
+    return false;
+
+  llvm::SmallString<64> importText;
+
+  // Check other source files for import flags that should be applied to the
+  // fix-it for consistency with the rest of the imports in the module.
+  auto parentModule = dc->getParentModule();
+  OptionSet<ImportFlags> flags;
+  for (auto file : parentModule->getFiles()) {
+    if (auto sf = dyn_cast<SourceFile>(file))
+      flags |= sf->getImportFlags(definingModule);
+  }
+
+  if (flags.contains(ImportFlags::Exported) ||
+      parentModule->isClangOverlayOf(definingModule))
+    importText += "@_exported ";
+  if (flags.contains(ImportFlags::ImplementationOnly))
+    importText += "@_implementationOnly ";
+  if (flags.contains(ImportFlags::WeakLinked))
+    importText += "@_weakLinked ";
+  if (flags.contains(ImportFlags::SPIOnly))
+    importText += "@_spiOnly ";
+
+  // FIXME: Access level should be considered, too.
+
+  // @_spi imports.
+  if (decl->isSPI()) {
+    auto spiGroups = decl->getSPIGroups();
+    if (!spiGroups.empty()) {
+      importText += "@_spi(";
+      importText += spiGroups[0].str();
+      importText += ") ";
+    }
+  }
+
+  importText += "import ";
+  importText += definingModule->getName().str();
+  importText += "\n";
+  ctx.Diags.diagnose(bestLoc, diag::candidate_add_import, definingModule)
+      .fixItInsert(bestLoc, importText);
+
+  return true;
+}

lib/AST/NameLookup.cpp

@@ -2293,7 +2293,7 @@ static bool isAcceptableLookupResult(const DeclContext *dc,
     // makes this decl visible.
     if (decl->getDeclContext()->getParentModule() != dc->getParentModule() &&
         dc->getASTContext().LangOpts.hasFeature(
-            Feature::ExtensionImportVisibility) &&
+            Feature::MemberImportVisibility) &&
         !decl->findImport(dc))
       return false;
   }

lib/Sema/CSDiagnostics.cpp

@@ -6187,40 +6187,11 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
 
   auto loc = nameLoc.isValid() ? nameLoc.getStartLoc() : ::getLoc(anchor);
   auto accessLevel = Member->getFormalAccessScope().accessLevelForDiagnostics();
-  bool suppressDeclHereNote = false;
   if (accessLevel == AccessLevel::Public &&
-      !Member->findImport(getDC())) {
-    auto definingModule = Member->getDeclContext()->getParentModule();
-    emitDiagnosticAt(loc, diag::candidate_from_missing_import,
-                     Member->getDescriptiveKind(), Member->getName(),
-                     definingModule->getName());
-
-    auto enclosingSF = getDC()->getParentSourceFile();
-    SourceLoc bestLoc =
-        getBestAddImportFixItLocation(Member, getDC()->getParentSourceFile());
-    if (bestLoc.isValid()) {
-      llvm::SmallString<64> importText;
-
-      // @_spi imports.
-      if (Member->isSPI()) {
-        auto spiGroups = Member->getSPIGroups();
-        if (!spiGroups.empty()) {
-          importText += "@_spi(";
-          importText += spiGroups[0].str();
-          importText += ") ";
-        }
-      }
-
-      importText += "import ";
-      importText += definingModule->getName().str();
-      importText += "\n";
-      emitDiagnosticAt(bestLoc, diag::candidate_add_import,
-                       definingModule->getName())
-        .fixItInsert(bestLoc, importText);
-    }
+      diagnoseMissingImportForMember(Member, getDC(), loc))
+    return true;
 
-    suppressDeclHereNote = true;
-  } else if (auto *CD = dyn_cast<ConstructorDecl>(Member)) {
+  if (auto *CD = dyn_cast<ConstructorDecl>(Member)) {
     emitDiagnosticAt(loc, diag::init_candidate_inaccessible,
                      CD->getResultInterfaceType(), accessLevel)
         .highlight(nameLoc.getSourceRange());
@@ -6230,8 +6201,7 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
         .highlight(nameLoc.getSourceRange());
   }
 
-  if (!suppressDeclHereNote)
-    emitDiagnosticAt(Member, diag::decl_declared_here, Member);
+  emitDiagnosticAt(Member, diag::decl_declared_here, Member);
   return true;
 }
 

lib/Sema/PreCheckExpr.cpp

@@ -521,9 +521,14 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
     if (inaccessibleResults) {
       // FIXME: What if the unviable candidates have different levels of access?
       const ValueDecl *first = inaccessibleResults.front().getValueDecl();
-      Context.Diags.diagnose(
-          Loc, diag::candidate_inaccessible, first,
-          first->getFormalAccessScope().accessLevelForDiagnostics());
+      auto accessLevel =
+          first->getFormalAccessScope().accessLevelForDiagnostics();
+      if (accessLevel == AccessLevel::Public &&
+          diagnoseMissingImportForMember(first, DC, Loc))
+        return errorResult();
+
+      Context.Diags.diagnose(Loc, diag::candidate_inaccessible, first,
+                             accessLevel);
 
       // FIXME: If any of the candidates (usually just one) are in the same
       // module we could offer a fix-it.

lib/Sema/TypeCheckType.cpp

@@ -30,6 +30,7 @@
 #include "swift/AST/ExistentialLayout.h"
 #include "swift/AST/ForeignErrorConvention.h"
 #include "swift/AST/GenericEnvironment.h"
+#include "swift/AST/Module.h"
 #include "swift/AST/NameLookup.h"
 #include "swift/AST/PackExpansionMatcher.h"
 #include "swift/AST/ParameterList.h"
@@ -1364,8 +1365,14 @@ static Type diagnoseUnknownType(TypeResolution resolution,
     if (!inaccessibleResults.empty()) {
       // FIXME: What if the unviable candidates have different levels of access?
       auto first = cast<TypeDecl>(inaccessibleResults.front().getValueDecl());
-      diags.diagnose(repr->getNameLoc(), diag::candidate_inaccessible,
-                     first, first->getFormalAccess());
+      auto formalAccess = first->getFormalAccess();
+      auto nameLoc = repr->getNameLoc();
+      if (formalAccess >= AccessLevel::Public &&
+          diagnoseMissingImportForMember(first, dc, nameLoc.getStartLoc()))
+        return ErrorType::get(ctx);
+
+      diags.diagnose(nameLoc, diag::candidate_inaccessible, first,
+                     formalAccess);
 
       // FIXME: If any of the candidates (usually just one) are in the same
       // module we could offer a fix-it.
@@ -1454,8 +1461,13 @@ static Type diagnoseUnknownType(TypeResolution resolution,
   if (inaccessibleMembers) {
     // FIXME: What if the unviable candidates have different levels of access?
     const TypeDecl *first = inaccessibleMembers.front().Member;
-    diags.diagnose(repr->getNameLoc(), diag::candidate_inaccessible,
-                   first, first->getFormalAccess());
+    auto formalAccess = first->getFormalAccess();
+    auto nameLoc = repr->getNameLoc();
+    if (formalAccess >= AccessLevel::Public &&
+        diagnoseMissingImportForMember(first, dc, nameLoc.getStartLoc()))
+      return ErrorType::get(ctx);
+
+    diags.diagnose(nameLoc, diag::candidate_inaccessible, first, formalAccess);
 
     // FIXME: If any of the candidates (usually just one) are in the same module
     // we could offer a fix-it.

test/NameLookup/Inputs/MemberImportVisibility/Bridging.h

@@ -0,0 +1,9 @@
+@import Categories_A;
+
+@interface X (BridgingHeader)
+- (void)fromBridgingHeader;
+@end
+
+struct StructInBridgingHeader {
+  int member;
+};

test/NameLookup/Inputs/Categories/Categories_A.h renamed to test/NameLookup/Inputs/MemberImportVisibility/Categories_A.h

@@ -0,0 +1,4 @@
+
+struct UnderlyingStruct {
+  int a;
+};

test/NameLookup/Inputs/Categories/Categories_A.swift renamed to test/NameLookup/Inputs/MemberImportVisibility/Categories_A.swift

@@ -15,11 +15,19 @@ infix operator <>
 extension X {
   public func XinA() { }
 
+  public var propXinA: Bool { return true }
+
   public static func <<<(a: Self, b: Self) -> Self { a }
+
+  public struct NestedInA {}
 }
 
 extension Y {
   public func YinA() { }
 
   public static func <<<(a: Self, b: Self) -> Self { a }
 }
+
+public enum EnumInA {
+  case caseInA
+}

test/NameLookup/Inputs/Categories/Categories_B.h renamed to test/NameLookup/Inputs/MemberImportVisibility/Categories_B.h

@@ -1,10 +1,14 @@
-import extensions_A
+import members_A
 
 
 extension X {
   public func XinB() { }
 
+  public var propXinB: Bool { return true }
+
   public static func >>>(a: Self, b: Self) -> Self { b }
+
+  public struct NestedInB {}
 }
 
 extension Y {
@@ -13,3 +17,6 @@ extension Y {
   public static func >>>(a: Self, b: Self) -> Self { b }
 }
 
+public enum EnumInB {
+  case caseInB
+}

test/NameLookup/Inputs/Categories/Categories_B.swift renamed to test/NameLookup/Inputs/MemberImportVisibility/Categories_B.swift

@@ -1,15 +1,23 @@
-@_exported import extensions_A
-import extensions_B
+@_exported import members_A
+import members_B
 
 
 extension X {
   public func XinC() { }
 
+  public var propXinC: Bool { return true }
+
   public static func <>(a: Self, b: Self) -> Self { a }
+
+  public struct NestedInC {}
 }
 
 extension Y {
   public func YinC() { }
 
   public static func <>(a: Self, b: Self) -> Self { a }
 }
+
+public enum EnumInC {
+  case caseInC
+}

test/NameLookup/Inputs/Categories/Categories_C.h renamed to test/NameLookup/Inputs/MemberImportVisibility/Categories_C.h

@@ -23,3 +23,7 @@ module Categories_D {
   }
 }
 
+module Underlying {
+  header "Underlying.h"
+  export *
+}