Sema: Fix a MemberImportVisibility regression in resolveDeclRefExpr().

The changes from #84259 caused a
regression in which declarations from an outer scope could be shadowed
inappropriately by member declarations from a module that has not been
imported. This fix addresses the issue by refactoring `resolveDeclRefExpr()` so
that it performs lookups in two passes. First, it attempts to resolve the decl
ref using the complete lookup algorithm and standard name lookup options, which
will ignore members from modules that haven't been imported if
`MemberImportVisibility` is enabled. Then, if no results were found it re-runs
the full lookup algorithm again with `NameLookupFlags::IgnoreMissingImports`
included to find additional results that ought to be diagnosed as unavailable.
This insures that the unavailable results are not considered until after the
main lookup has already failed.

Resolves rdar://161078015.

Author: tshortli

lib/Sema/PreCheckTarget.cpp

@@ -518,11 +518,12 @@ diagnoseUnqualifiedInit(UnresolvedDeclRefExpr *initExpr, DeclContext *dc,
                         initExpr->getNameLoc(), /*implicit=*/true);
 }
 
-/// Bind an UnresolvedDeclRefExpr by performing name lookup and
-/// returning the resultant expression. Context is the DeclContext used
-/// for the lookup.
-Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
-                                      DeclContext *DC) {
+/// Bind an UnresolvedDeclRefExpr by performing name lookup using the given
+/// options and returning the resultant expression. `DC` is the DeclContext
+/// used for the lookup. If the lookup doesn't find any results, returns
+/// `nullptr`.
+static Expr *resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC,
+                                NameLookupOptions lookupOptions) {
   auto &Context = DC->getASTContext();
   DeclNameRef Name = UDRE->getName();
   SourceLoc Loc = UDRE->getLoc();
@@ -562,14 +563,6 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
     LookupName = DeclNameRef(lookupName);
   }
 
-  // Perform standard value name lookup.
-  NameLookupOptions lookupOptions = defaultUnqualifiedLookupOptions;
-  // TODO: Include all of the possible members to give a solver a
-  //       chance to diagnose name shadowing which requires explicit
-  //       name/module qualifier to access top-level name.
-  lookupOptions |= NameLookupFlags::IncludeOuterResults;
-  lookupOptions |= NameLookupFlags::IgnoreMissingImports;
-
   LookupResult Lookup;
 
   bool AllDeclRefs = true;
@@ -628,17 +621,8 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
   }
 
   if (!Lookup) {
-    // If we failed lookup of an operator, check to see if this is a range
-    // operator misspelling. Otherwise try to diagnose a juxtaposition
-    // e.g. (x*-4) that needs whitespace.
-    if (diagnoseRangeOperatorMisspell(Context.Diags, UDRE) ||
-        diagnoseIncDecOperator(Context.Diags, UDRE) ||
-        diagnoseOperatorJuxtaposition(UDRE, DC) ||
-        diagnoseNonexistentPowerOperator(Context.Diags, UDRE, DC)) {
-      return errorResult();
-    }
-
-    // Try ignoring access control.
+    // For the purpose of diagnosing inaccessible results, try the lookup again
+    // but ignore access control.
     NameLookupOptions relookupOptions = lookupOptions;
     relookupOptions |= NameLookupFlags::IgnoreAccessControl;
     auto inaccessibleResults =
@@ -663,117 +647,8 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
       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();
-
-    Identifier simpleName = Name.getBaseIdentifier();
-    const char *buffer = simpleName.get();
-    llvm::SmallString<64> expectedIdentifier;
-    bool isConfused = false;
-    uint32_t codepoint;
-    uint32_t firstConfusableCodepoint = 0;
-    int totalCodepoints = 0;
-    int offset = 0;
-    while ((codepoint = validateUTF8CharacterAndAdvance(buffer,
-                                                        buffer +
-                                                        strlen(buffer)))
-           != ~0U) {
-      int length = (buffer - simpleName.get()) - offset;
-      if (auto expectedCodepoint =
-          confusable::tryConvertConfusableCharacterToASCII(codepoint)) {
-        if (firstConfusableCodepoint == 0) {
-          firstConfusableCodepoint = codepoint;
-        }
-        isConfused = true;
-        expectedIdentifier += expectedCodepoint;
-      } else {
-        expectedIdentifier += (char)codepoint;
-      }
-
-      totalCodepoints++;
-
-      offset += length;
-    }
-
-    auto emitBasicError = [&] {
-      
-      if (Name.isSimpleName(Context.Id_self)) {
-        // `self` gets diagnosed with a different error when it can't be found.
-        Context.Diags
-            .diagnose(Loc, diag::cannot_find_self_in_scope)
-            .highlight(UDRE->getSourceRange());
-      } else {
-        Context.Diags
-            .diagnose(Loc, diag::cannot_find_in_scope, Name,
-                      Name.isOperator())
-            .highlight(UDRE->getSourceRange());
-      }
-
-      if (!Context.LangOpts.DisableExperimentalClangImporterDiagnostics) {
-        Context.getClangModuleLoader()->diagnoseTopLevelValue(
-            Name.getFullName());
-      }
-    };
-
-    if (!isConfused) {
-      if (Name.isSimpleName(Context.Id_Self)) {
-        if (DeclContext *typeContext = DC->getInnermostTypeContext()){
-          Type SelfType = typeContext->getSelfInterfaceType();
-
-          if (typeContext->getSelfClassDecl() &&
-              !typeContext->getSelfClassDecl()->isForeignReferenceType())
-            SelfType = DynamicSelfType::get(SelfType, Context);
-          return new (Context)
-              TypeExpr(new (Context) SelfTypeRepr(SelfType, Loc));
-        }
-      }
-
-      TypoCorrectionResults corrections(Name, nameLoc);
-
-      // FIXME: Don't perform typo correction inside macro arguments, because it
-      // will invoke synthesizing declarations in this scope, which will attempt to
-      // expand this macro which leads to circular reference errors.
-      if (!namelookup::isInMacroArgument(DC->getParentSourceFile(), UDRE->getLoc())) {
-        TypeChecker::performTypoCorrection(DC, UDRE->getRefKind(), Type(),
-                                           lookupOptions, corrections);
-      }
-
-      if (auto typo = corrections.claimUniqueCorrection()) {
-        auto diag = Context.Diags.diagnose(
-            Loc, diag::cannot_find_in_scope_corrected, Name,
-            Name.isOperator(), typo->CorrectedName.getBaseIdentifier().str());
-        diag.highlight(UDRE->getSourceRange());
-        typo->addFixits(diag);
-      } else {
-        emitBasicError();
-      }
-
-      corrections.noteAllCandidates();
-    } else {
-      emitBasicError();
-
-      if (totalCodepoints == 1) {
-        auto charNames = confusable::getConfusableAndBaseCodepointNames(
-            firstConfusableCodepoint);
-        Context.Diags
-            .diagnose(Loc, diag::single_confusable_character,
-                      UDRE->getName().isOperator(), simpleName.str(),
-                      charNames.first, expectedIdentifier, charNames.second)
-            .fixItReplace(Loc, expectedIdentifier);
-      } else {
-        Context.Diags
-            .diagnose(Loc, diag::confusable_character,
-                      UDRE->getName().isOperator(), simpleName.str(),
-                      expectedIdentifier)
-            .fixItReplace(Loc, expectedIdentifier);
-      }
-    }
-
-    // TODO: consider recovering from here.  We may want some way to suppress
-    // downstream diagnostics, though.
-
-    return errorResult();
+    // No results were found.
+    return nullptr;
   }
 
   // FIXME: Need to refactor the way we build an AST node from a lookup result!
@@ -892,8 +767,9 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
                                           UDRE->getNameLoc().getStartLoc());
     }
 
-    return buildRefExpr(ResultValues, DC, UDRE->getNameLoc(),
-                        UDRE->isImplicit(), UDRE->getFunctionRefInfo());
+    return TypeChecker::buildRefExpr(ResultValues, DC, UDRE->getNameLoc(),
+                                     UDRE->isImplicit(),
+                                     UDRE->getFunctionRefInfo());
   }
 
   ResultValues.clear();
@@ -983,6 +859,157 @@ Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
   return errorResult();
 }
 
+Expr *TypeChecker::resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE,
+                                      DeclContext *DC) {
+  auto &Context = DC->getASTContext();
+
+  // Perform standard value name lookup.
+  NameLookupOptions lookupOptions = defaultUnqualifiedLookupOptions;
+  // TODO: Include all of the possible members to give a solver a
+  //       chance to diagnose name shadowing which requires explicit
+  //       name/module qualifier to access top-level name.
+  lookupOptions |= NameLookupFlags::IncludeOuterResults;
+
+  Expr *result = ::resolveDeclRefExpr(UDRE, DC, lookupOptions);
+  if (!result && Context.LangOpts.hasFeature(Feature::MemberImportVisibility,
+                                             /*allowMigration=*/true)) {
+    // If we didn't find a result, try again but this time relax
+    // MemberImportVisibility restrictions. Note that diagnosing the missing
+    // import is already handled by resolveDeclRefExpr().
+    lookupOptions |= NameLookupFlags::IgnoreMissingImports;
+    result = ::resolveDeclRefExpr(UDRE, DC, lookupOptions);
+  }
+
+  if (result)
+    return result;
+
+  // We didn't find any results. Look for common mistakes to diagnose.
+  DeclNameRef Name = UDRE->getName();
+  SourceLoc Loc = UDRE->getLoc();
+  if (Loc.isInvalid())
+    DC = DC->getModuleScopeContext();
+
+  auto errorResult = [&]() -> Expr * {
+    return new (Context) ErrorExpr(UDRE->getSourceRange());
+  };
+
+  // If we failed lookup of an operator, check to see if this is a range
+  // operator misspelling. Otherwise try to diagnose a juxtaposition
+  // e.g. (x*-4) that needs whitespace.
+  if (diagnoseRangeOperatorMisspell(Context.Diags, UDRE) ||
+      diagnoseIncDecOperator(Context.Diags, UDRE) ||
+      diagnoseOperatorJuxtaposition(UDRE, DC) ||
+      diagnoseNonexistentPowerOperator(Context.Diags, UDRE, DC)) {
+    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();
+
+  Identifier simpleName = Name.getBaseIdentifier();
+  const char *buffer = simpleName.get();
+  llvm::SmallString<64> expectedIdentifier;
+  bool isConfused = false;
+  uint32_t codepoint;
+  uint32_t firstConfusableCodepoint = 0;
+  int totalCodepoints = 0;
+  int offset = 0;
+  while ((codepoint = validateUTF8CharacterAndAdvance(
+              buffer, buffer + strlen(buffer))) != ~0U) {
+    int length = (buffer - simpleName.get()) - offset;
+    if (auto expectedCodepoint =
+            confusable::tryConvertConfusableCharacterToASCII(codepoint)) {
+      if (firstConfusableCodepoint == 0) {
+        firstConfusableCodepoint = codepoint;
+      }
+      isConfused = true;
+      expectedIdentifier += expectedCodepoint;
+    } else {
+      expectedIdentifier += (char)codepoint;
+    }
+
+    totalCodepoints++;
+
+    offset += length;
+  }
+
+  auto emitBasicError = [&] {
+    if (Name.isSimpleName(Context.Id_self)) {
+      // `self` gets diagnosed with a different error when it can't be found.
+      Context.Diags.diagnose(Loc, diag::cannot_find_self_in_scope)
+          .highlight(UDRE->getSourceRange());
+    } else {
+      Context.Diags
+          .diagnose(Loc, diag::cannot_find_in_scope, Name, Name.isOperator())
+          .highlight(UDRE->getSourceRange());
+    }
+
+    if (!Context.LangOpts.DisableExperimentalClangImporterDiagnostics) {
+      Context.getClangModuleLoader()->diagnoseTopLevelValue(Name.getFullName());
+    }
+  };
+
+  if (!isConfused) {
+    if (Name.isSimpleName(Context.Id_Self)) {
+      if (DeclContext *typeContext = DC->getInnermostTypeContext()) {
+        Type SelfType = typeContext->getSelfInterfaceType();
+
+        if (typeContext->getSelfClassDecl() &&
+            !typeContext->getSelfClassDecl()->isForeignReferenceType())
+          SelfType = DynamicSelfType::get(SelfType, Context);
+        return new (Context)
+            TypeExpr(new (Context) SelfTypeRepr(SelfType, Loc));
+      }
+    }
+
+    TypoCorrectionResults corrections(Name, nameLoc);
+
+    // FIXME: Don't perform typo correction inside macro arguments, because it
+    // will invoke synthesizing declarations in this scope, which will attempt
+    // to expand this macro which leads to circular reference errors.
+    if (!namelookup::isInMacroArgument(DC->getParentSourceFile(),
+                                       UDRE->getLoc())) {
+      TypeChecker::performTypoCorrection(DC, UDRE->getRefKind(), Type(),
+                                         lookupOptions, corrections);
+    }
+
+    if (auto typo = corrections.claimUniqueCorrection()) {
+      auto diag = Context.Diags.diagnose(
+          Loc, diag::cannot_find_in_scope_corrected, Name, Name.isOperator(),
+          typo->CorrectedName.getBaseIdentifier().str());
+      diag.highlight(UDRE->getSourceRange());
+      typo->addFixits(diag);
+    } else {
+      emitBasicError();
+    }
+
+    corrections.noteAllCandidates();
+  } else {
+    emitBasicError();
+
+    if (totalCodepoints == 1) {
+      auto charNames = confusable::getConfusableAndBaseCodepointNames(
+          firstConfusableCodepoint);
+      Context.Diags
+          .diagnose(Loc, diag::single_confusable_character,
+                    UDRE->getName().isOperator(), simpleName.str(),
+                    charNames.first, expectedIdentifier, charNames.second)
+          .fixItReplace(Loc, expectedIdentifier);
+    } else {
+      Context.Diags
+          .diagnose(Loc, diag::confusable_character,
+                    UDRE->getName().isOperator(), simpleName.str(),
+                    expectedIdentifier)
+          .fixItReplace(Loc, expectedIdentifier);
+    }
+  }
+
+  // TODO: consider recovering from here.  We may want some way to suppress
+  // downstream diagnostics, though.
+  return errorResult();
+}
+
 /// If an expression references 'self.init' or 'super.init' in an
 /// initializer context, returns the implicit 'self' decl of the constructor.
 /// Otherwise, return nil.

test/NameLookup/Inputs/MemberImportVisibility/members_A.swift

@@ -16,6 +16,24 @@ infix operator <<<
 infix operator >>>
 infix operator <>
 
+@available(*, deprecated)
+public func shadowedByMemberOnXinA() { }
+
+@available(*, deprecated)
+public func shadowedByMemberOnXinB() { }
+
+@available(*, deprecated)
+public func shadowedByMemberOnXinC() { }
+
+@available(*, deprecated)
+public func shadowedByStaticMemberOnXinA() { }
+
+@available(*, deprecated)
+public func shadowedByStaticMemberOnXinB() { }
+
+@available(*, deprecated)
+public func shadowedByStaticMemberOnXinC() { }
+
 extension X {
   public func XinA() { }
 
@@ -28,6 +46,9 @@ extension X {
 
   public struct NestedInA {}
   public protocol ProtoNestedInA {}
+
+  public func shadowedByMemberOnXinA() { }
+  public static func shadowedByStaticMemberOnXinA() { }
 }
 
 extension Y {

test/NameLookup/Inputs/MemberImportVisibility/members_B.swift

@@ -11,6 +11,11 @@ extension X {
   public var propXinB: Bool { return true }
   package var propXinB_package: Bool { return true }
 
+  public func shadowedByMemberOnXinB() { }
+  public static func shadowedByStaticMemberOnXinB() { }
+
+  public static var max: Int { return Int.min }
+
   public static func >>>(a: Self, b: Self) -> Self { b }
 
   public struct NestedInB {}

test/NameLookup/Inputs/MemberImportVisibility/members_C.swift

@@ -10,6 +10,9 @@ extension X {
 
   public var propXinC: Bool { return true }
 
+  public func shadowedByMemberOnXinC() { }
+  public static func shadowedByStaticMemberOnXinC() { }
+
   public static func <>(a: Self, b: Self) -> Self { a }
 
   public struct NestedInC {}