AST: Add a IgnoreMissingImports option to name lookup.

tshortli · tshortli · commit d002da0ef223 · 2024-07-10T22:57:15.000-07:00
Control enforcement of member import visibility requirements via a new option,
instead of piggy-backing on the existing IgnoreAccessControl option. Adopt the
option when doing fallback lookups for unviable members so that the compiler
can diagnose the reason that a member is inaccessible more reliably.
Previously, with MemberImportVisibility enabled decls with the package access
level could be mis-diagnosed as inaccessible due to their access level when
really they were inaccessible due to a missing import.

Resolves rdar://131501862.
diff --git a/include/swift/AST/LookupKinds.h b/include/swift/AST/LookupKinds.h
@@ -60,6 +60,10 @@ enum NLOptions : unsigned {
   /// This lookup should only return macro declarations.
   NL_OnlyMacros = 1 << 8,
 
+  /// Include members that would otherwise be filtered out because they come
+  /// from a module that has not been imported.
+  NL_IgnoreMissingImports = 1 << 9,
+
   /// The default set of options used for qualified name lookup.
   ///
   /// FIXME: Eventually, add NL_ProtocolMembers to this, once all of the
diff --git a/include/swift/AST/NameLookup.h b/include/swift/AST/NameLookup.h
@@ -249,7 +249,10 @@ enum class UnqualifiedLookupFlags {
   ModuleLookup           = 1 << 8,
   /// This lookup should discard 'Self' requirements in protocol extension
   /// 'where' clauses.
-  DisregardSelfBounds    = 1 << 9
+  DisregardSelfBounds    = 1 << 9,
+  /// This lookup should include members that would otherwise be filtered out
+  /// because they come from a module that has not been imported.
+  IgnoreMissingImports = 1 << 10,
 };
 
 using UnqualifiedLookupOptions = OptionSet<UnqualifiedLookupFlags>;
diff --git a/include/swift/Sema/CSFix.h b/include/swift/Sema/CSFix.h
@@ -1923,11 +1923,14 @@ class MoveOutOfOrderArgument final : public ConstraintFix {
 };
 
 class AllowInaccessibleMember final : public AllowInvalidMemberRef {
+  bool IsMissingImport;
+
   AllowInaccessibleMember(ConstraintSystem &cs, Type baseType,
                           ValueDecl *member, DeclNameRef name,
-                          ConstraintLocator *locator)
+                          ConstraintLocator *locator, bool isMissingImport)
       : AllowInvalidMemberRef(cs, FixKind::AllowInaccessibleMember, baseType,
-                              member, name, locator) {}
+                              member, name, locator),
+        IsMissingImport(isMissingImport) {}
 
 public:
   std::string getName() const override {
@@ -1942,7 +1945,8 @@ class AllowInaccessibleMember final : public AllowInvalidMemberRef {
 
   static AllowInaccessibleMember *create(ConstraintSystem &cs, Type baseType,
                                          ValueDecl *member, DeclNameRef name,
-                                         ConstraintLocator *locator);
+                                         ConstraintLocator *locator,
+                                         bool isMissingImport);
 
   static bool classof(const ConstraintFix *fix) {
     return fix->getKind() == FixKind::AllowInaccessibleMember;
diff --git a/include/swift/Sema/ConstraintSystem.h b/include/swift/Sema/ConstraintSystem.h
@@ -1975,6 +1975,10 @@ struct MemberLookupResult {
     /// The member is inaccessible (e.g. a private member in another file).
     UR_Inaccessible,
 
+    /// The member is not visible because it comes from a module that was not
+    /// imported.
+    UR_MissingImport,
+
     /// This is a `WritableKeyPath` being used to look up read-only member,
     /// used in situations involving dynamic member lookup via keypath,
     /// because it's not known upfront what access capability would the
diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp
@@ -2363,9 +2363,11 @@ static bool isAcceptableLookupResult(const DeclContext *dc,
     if (!decl->isAccessibleFrom(dc, /*forConformance*/ false,
                                 allowUsableFromInline))
       return false;
+  }
 
-    // Check that there is some import in the originating context that
-    // makes this decl visible.
+  // Check that there is some import in the originating context that makes this
+  // decl visible.
+  if (!(options & NL_IgnoreMissingImports)) {
     if (missingExplicitImportForMemberDecl(dc, decl))
       return false;
   }
@@ -3983,7 +3985,8 @@ bool IsCallAsFunctionNominalRequest::evaluate(Evaluator &evaluator,
   // that will be checked when we actually try to solve with a `callAsFunction`
   // member access.
   SmallVector<ValueDecl *, 4> results;
-  auto opts = NL_QualifiedDefault | NL_ProtocolMembers | NL_IgnoreAccessControl;
+  auto opts = NL_QualifiedDefault | NL_ProtocolMembers |
+              NL_IgnoreAccessControl | NL_IgnoreMissingImports;
   dc->lookupQualified(decl, DeclNameRef(ctx.Id_callAsFunction),
                       decl->getLoc(), opts, results);
 
@@ -4138,8 +4141,11 @@ void swift::simple_display(llvm::raw_ostream &out, NLOptions options) {
     FLAG(NL_RemoveOverridden)
     FLAG(NL_IgnoreAccessControl)
     FLAG(NL_OnlyTypes)
-    FLAG(NL_OnlyMacros)
     FLAG(NL_IncludeAttributeImplements)
+    FLAG(NL_IncludeUsableFromInline)
+    FLAG(NL_ExcludeMacroExpansions)
+    FLAG(NL_OnlyMacros)
+    FLAG(NL_IgnoreMissingImports)
 #undef FLAG
   };
 
diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp
@@ -596,6 +596,8 @@ NLOptions UnqualifiedLookupFactory::computeBaseNLOptions(
     baseNLOptions |= NL_OnlyMacros;
   if (options.contains(Flags::IgnoreAccessControl))
     baseNLOptions |= NL_IgnoreAccessControl;
+  if (options.contains(Flags::IgnoreMissingImports))
+    baseNLOptions |= NL_IgnoreMissingImports;
   return baseNLOptions;
 }
 
diff --git a/lib/ClangImporter/ClangImporter.cpp b/lib/ClangImporter/ClangImporter.cpp
@@ -6068,14 +6068,15 @@ static void lookupRelatedFuncs(AbstractFunctionDecl *func,
   else
     swiftName = func->getName();
 
+  NLOptions options = NL_IgnoreAccessControl | NL_IgnoreMissingImports;
   if (auto ty = func->getDeclContext()->getSelfNominalTypeDecl()) {
     ty->lookupQualified({ ty }, DeclNameRef(swiftName), func->getLoc(),
-                        NL_QualifiedDefault | NL_IgnoreAccessControl, results);
+                        NL_QualifiedDefault | options, results);
   }
   else {
     auto mod = func->getDeclContext()->getParentModule();
     mod->lookupQualified(mod, DeclNameRef(swiftName), func->getLoc(),
-                         NL_RemoveOverridden | NL_IgnoreAccessControl, results);
+                         NL_RemoveOverridden | options, results);
   }
 }
 
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
@@ -6187,11 +6187,12 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
   }
 
   auto loc = nameLoc.isValid() ? nameLoc.getStartLoc() : ::getLoc(anchor);
-  auto accessLevel = Member->getFormalAccessScope().accessLevelForDiagnostics();
-  if (accessLevel == AccessLevel::Public &&
-      diagnoseMissingImportForMember(Member, getDC(), loc))
+  if (IsMissingImport) {
+    diagnoseMissingImportForMember(Member, getDC(), loc);
     return true;
+  }
 
+  auto accessLevel = Member->getFormalAccessScope().accessLevelForDiagnostics();
   if (auto *CD = dyn_cast<ConstructorDecl>(Member)) {
     emitDiagnosticAt(loc, diag::init_candidate_inaccessible,
                      CD->getResultInterfaceType(), accessLevel)
diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h
@@ -1638,11 +1638,13 @@ class ClosureParamDestructuringFailure final : public FailureDiagnostic {
 /// ```
 class InaccessibleMemberFailure final : public FailureDiagnostic {
   ValueDecl *Member;
+  bool IsMissingImport;
 
 public:
   InaccessibleMemberFailure(const Solution &solution, ValueDecl *member,
-                            ConstraintLocator *locator)
-      : FailureDiagnostic(solution, locator), Member(member) {}
+                            ConstraintLocator *locator, bool isMissingImport)
+      : FailureDiagnostic(solution, locator), Member(member),
+        IsMissingImport(isMissingImport) {}
 
   bool diagnoseAsError() override;
 };
diff --git a/lib/Sema/CSFix.cpp b/lib/Sema/CSFix.cpp
@@ -1152,16 +1152,16 @@ MoveOutOfOrderArgument *MoveOutOfOrderArgument::create(
 
 bool AllowInaccessibleMember::diagnose(const Solution &solution,
                                        bool asNote) const {
-  InaccessibleMemberFailure failure(solution, getMember(), getLocator());
+  InaccessibleMemberFailure failure(solution, getMember(), getLocator(),
+                                    IsMissingImport);
   return failure.diagnose(asNote);
 }
 
-AllowInaccessibleMember *
-AllowInaccessibleMember::create(ConstraintSystem &cs, Type baseType,
-                                ValueDecl *member, DeclNameRef name,
-                                ConstraintLocator *locator) {
-  return new (cs.getAllocator())
-      AllowInaccessibleMember(cs, baseType, member, name, locator);
+AllowInaccessibleMember *AllowInaccessibleMember::create(
+    ConstraintSystem &cs, Type baseType, ValueDecl *member, DeclNameRef name,
+    ConstraintLocator *locator, bool isMissingImport) {
+  return new (cs.getAllocator()) AllowInaccessibleMember(
+      cs, baseType, member, name, locator, isMissingImport);
 }
 
 bool AllowAnyObjectKeyPathRoot::diagnose(const Solution &solution,
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -10454,30 +10454,45 @@ performMemberLookup(ConstraintKind constraintKind, DeclNameRef memberName,
   if (result.ViableCandidates.empty() && result.UnviableCandidates.empty() &&
       includeInaccessibleMembers) {
     NameLookupOptions lookupOptions = defaultMemberLookupOptions;
-    
-    // Ignore access control so we get candidates that might have been missed
-    // before.
-    lookupOptions |= NameLookupFlags::IgnoreAccessControl;
 
-    auto lookup =
-        TypeChecker::lookupMember(DC, instanceTy, memberName,
-                                  memberLoc, lookupOptions);
-    for (auto entry : lookup) {
-      auto *cand = entry.getValueDecl();
+    // Local function that looks up additional candidates using the given lookup
+    // options, recording the results as unviable candidates.
+    auto lookupUnviable =
+        [&](NameLookupOptions lookupOptions,
+            MemberLookupResult::UnviableReason reason) -> bool {
+      auto lookup = TypeChecker::lookupMember(DC, instanceTy, memberName,
+                                              memberLoc, lookupOptions);
+      for (auto entry : lookup) {
+        auto *cand = entry.getValueDecl();
+
+        // If the result is invalid, skip it.
+        if (cand->isInvalid()) {
+          result.markErrorAlreadyDiagnosed();
+          break;
+        }
+
+        if (excludedDynamicMembers.count(cand))
+          continue;
 
-      // If the result is invalid, skip it.
-      if (cand->isInvalid()) {
-        result.markErrorAlreadyDiagnosed();
-        return result;
+        result.addUnviable(getOverloadChoice(cand, /*isBridged=*/false,
+                                             /*isUnwrappedOptional=*/false),
+                           reason);
       }
 
-      if (excludedDynamicMembers.count(cand))
-        continue;
+      return !lookup.empty();
+    };
 
-      result.addUnviable(getOverloadChoice(cand, /*isBridged=*/false,
-                                           /*isUnwrappedOptional=*/false),
-                         MemberLookupResult::UR_Inaccessible);
-    }
+    // Ignore access control so we get candidates that might have been missed
+    // before.
+    if (lookupUnviable(lookupOptions | NameLookupFlags::IgnoreAccessControl,
+                       MemberLookupResult::UR_Inaccessible))
+      return result;
+
+    // Ignore missing import statements in order to find more candidates that
+    // might have been missed before.
+    if (lookupUnviable(lookupOptions | NameLookupFlags::IgnoreMissingImports,
+                       MemberLookupResult::UR_MissingImport))
+      return result;
   }
   
   return result;
@@ -10678,9 +10693,11 @@ static ConstraintFix *fixMemberRef(
     }
 
     case MemberLookupResult::UR_Inaccessible:
+    case MemberLookupResult::UR_MissingImport:
       assert(choice.isDecl());
-      return AllowInaccessibleMember::create(cs, baseTy, choice.getDecl(),
-                                             memberName, locator);
+      return AllowInaccessibleMember::create(
+          cs, baseTy, choice.getDecl(), memberName, locator,
+          *reason == MemberLookupResult::UR_MissingImport);
 
     case MemberLookupResult::UR_UnavailableInExistential: {
       return choice.isDecl()
diff --git a/lib/Sema/CodeSynthesis.cpp b/lib/Sema/CodeSynthesis.cpp
@@ -1155,7 +1155,8 @@ static void collectNonOveriddenSuperclassInits(
   superclassDecl->synthesizeSemanticMembersIfNeeded(
     DeclBaseName::createConstructor());
 
-  NLOptions subOptions = (NL_QualifiedDefault | NL_IgnoreAccessControl);
+  NLOptions subOptions =
+      (NL_QualifiedDefault | NL_IgnoreAccessControl | NL_IgnoreMissingImports);
   SmallVector<ValueDecl *, 4> lookupResults;
   subclass->lookupQualified(
       superclassDecl, DeclNameRef::createConstructor(),
diff --git a/lib/Sema/PreCheckExpr.cpp b/lib/Sema/PreCheckExpr.cpp
@@ -525,10 +525,6 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
       const ValueDecl *first = inaccessibleResults.front().getValueDecl();
       auto accessLevel =
           first->getFormalAccessScope().accessLevelForDiagnostics();
-      if (accessLevel == AccessLevel::Public &&
-          diagnoseMissingImportForMember(first, DC, Loc))
-        return errorResult();
-
       Context.Diags.diagnose(Loc, diag::candidate_inaccessible, first,
                              accessLevel);
 
@@ -544,6 +540,19 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
       return errorResult();
     }
 
+    // Try ignoring missing imports.
+    relookupOptions |= NameLookupFlags::IgnoreMissingImports;
+    auto nonImportedResults =
+        TypeChecker::lookupUnqualified(DC, LookupName, Loc, relookupOptions);
+    if (nonImportedResults) {
+      const ValueDecl *first = nonImportedResults.front().getValueDecl();
+      diagnoseMissingImportForMember(first, DC, Loc);
+
+      // Don't try to recover here; we'll get more access-related diagnostics
+      // downstream if the type of the inaccessible decl is also inaccessible.
+      return errorResult();
+    }
+
     // TODO: Name will be a compound name if it was written explicitly as
     // one, but we should also try to propagate labels into this.
     DeclNameLoc nameLoc = UDRE->getNameLoc();
diff --git a/lib/Sema/TypeCheckNameLookup.cpp b/lib/Sema/TypeCheckNameLookup.cpp
@@ -221,6 +221,8 @@ convertToUnqualifiedLookupOptions(NameLookupOptions options) {
     newOptions |= UnqualifiedLookupFlags::IncludeUsableFromInline;
   if (options.contains(NameLookupFlags::ExcludeMacroExpansions))
     newOptions |= UnqualifiedLookupFlags::ExcludeMacroExpansions;
+  if (options.contains(NameLookupFlags::IgnoreMissingImports))
+    newOptions |= UnqualifiedLookupFlags::IgnoreMissingImports;
 
   return newOptions;
 }
@@ -324,6 +326,8 @@ LookupResult TypeChecker::lookupMember(DeclContext *dc,
   NLOptions subOptions = (NL_QualifiedDefault | NL_ProtocolMembers);
   if (options.contains(NameLookupFlags::IgnoreAccessControl))
     subOptions |= NL_IgnoreAccessControl;
+  if (options.contains(NameLookupFlags::IgnoreMissingImports))
+    subOptions |= NL_IgnoreMissingImports;
 
   // We handle our own overriding/shadowing filtering.
   subOptions &= ~NL_RemoveOverridden;
@@ -420,6 +424,8 @@ LookupTypeResult TypeChecker::lookupMemberType(DeclContext *dc,
 
   if (options.contains(NameLookupFlags::IgnoreAccessControl))
     subOptions |= NL_IgnoreAccessControl;
+  if (options.contains(NameLookupFlags::IgnoreMissingImports))
+    subOptions |= NL_IgnoreMissingImports;
   if (options.contains(NameLookupFlags::IncludeUsableFromInline))
     subOptions |= NL_IncludeUsableFromInline;
 
diff --git a/lib/Sema/TypeCheckType.cpp b/lib/Sema/TypeCheckType.cpp
diff --git a/lib/Sema/TypeChecker.h b/lib/Sema/TypeChecker.h
diff --git a/test/NameLookup/members_transitive.swift b/test/NameLookup/members_transitive.swift
diff --git a/test/SPI/client_reuse_spi.swift b/test/SPI/client_reuse_spi.swift

Original file line number	Diff line number	Diff line change
`@@ -596,6 +596,8 @@ NLOptions UnqualifiedLookupFactory::computeBaseNLOptions(`
`596`	`596`	`baseNLOptions \|= NL_OnlyMacros;`
`597`	`597`	`if (options.contains(Flags::IgnoreAccessControl))`
`598`	`598`	`baseNLOptions \|= NL_IgnoreAccessControl;`
	`599`	`+ if (options.contains(Flags::IgnoreMissingImports))`
	`600`	`+ baseNLOptions \|= NL_IgnoreMissingImports;`
`599`	`601`	`return baseNLOptions;`
`600`	`602`	`}`
`601`	`603`
Original file line number	Diff line number	Diff line change
`@@ -6068,14 +6068,15 @@ static void lookupRelatedFuncs(AbstractFunctionDecl *func,`
`6068`	`6068`	`else`
`6069`	`6069`	`swiftName = func->getName();`
`6070`	`6070`
	`6071`	`+ NLOptions options = NL_IgnoreAccessControl \| NL_IgnoreMissingImports;`
`6071`	`6072`	`if (auto ty = func->getDeclContext()->getSelfNominalTypeDecl()) {`
`6072`	`6073`	`ty->lookupQualified({ ty }, DeclNameRef(swiftName), func->getLoc(),`
`6073`		`- NL_QualifiedDefault \| NL_IgnoreAccessControl, results);`
	`6074`	`+ NL_QualifiedDefault \| options, results);`
`6074`	`6075`	`}`
`6075`	`6076`	`else {`
`6076`	`6077`	`auto mod = func->getDeclContext()->getParentModule();`
`6077`	`6078`	`mod->lookupQualified(mod, DeclNameRef(swiftName), func->getLoc(),`
`6078`		`- NL_RemoveOverridden \| NL_IgnoreAccessControl, results);`
	`6079`	`+ NL_RemoveOverridden \| options, results);`
`6079`	`6080`	`}`
`6080`	`6081`	`}`
`6081`	`6082`