Merge pull request #84659 from tshortli/member-import-visibility-resolve-decl-ref-expr-fix

Sema: Fix a MemberImportVisibility regression in `resolveDeclRefExpr()`

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 {}