Merge pull request swiftlang#76336 from tshortli/member-import-visibility-ranking

tshortli · web-flow · commit 3d47e1cdd849 · 2024-09-10T13:28:30.000-07:00
ConstraintSystem: Use scoring to implement MemberImportVisibility
diff --git a/include/swift/AST/DeclContext.h b/include/swift/AST/DeclContext.h
@@ -680,6 +680,9 @@ class alignas(1 << DeclContextAlignInBits) DeclContext
   /// Looks up a precedence group with a given \p name.
   PrecedenceGroupLookupResult lookupPrecedenceGroup(Identifier name) const;
 
+  /// Returns true if the parent module of \p decl is imported by this context.
+  bool isDeclImported(const Decl *decl) const;
+
   /// Return the ASTContext for a specified DeclContext by
   /// walking up to the enclosing module and returning its ASTContext.
   LLVM_READONLY
diff --git a/include/swift/AST/SourceFile.h b/include/swift/AST/SourceFile.h
@@ -507,11 +507,6 @@ class SourceFile final : public FileUnit {
     MissingImportForMemberDiagnostics[decl].push_back(loc);
   }
 
-  /// Returns true if there is a pending missing import diagnostic for \p decl.
-  bool hasDelayedMissingImportForMemberDiagnostic(const ValueDecl *decl) const {
-    return MissingImportForMemberDiagnostics.contains(decl);
-  }
-
   DelayedMissingImportForMemberDiags
   takeDelayedMissingImportForMemberDiagnostics() {
     DelayedMissingImportForMemberDiags diags;
diff --git a/include/swift/Sema/CSFix.h b/include/swift/Sema/CSFix.h
@@ -1926,14 +1926,11 @@ class MoveOutOfOrderArgument final : public ConstraintFix {
 };
 
 class AllowInaccessibleMember final : public AllowInvalidMemberRef {
-  bool IsMissingImport;
-
   AllowInaccessibleMember(ConstraintSystem &cs, Type baseType,
                           ValueDecl *member, DeclNameRef name,
-                          ConstraintLocator *locator, bool isMissingImport)
+                          ConstraintLocator *locator)
       : AllowInvalidMemberRef(cs, FixKind::AllowInaccessibleMember, baseType,
-                              member, name, locator),
-        IsMissingImport(isMissingImport) {}
+                              member, name, locator) {}
 
 public:
   std::string getName() const override {
@@ -1948,8 +1945,7 @@ class AllowInaccessibleMember final : public AllowInvalidMemberRef {
 
   static AllowInaccessibleMember *create(ConstraintSystem &cs, Type baseType,
                                          ValueDecl *member, DeclNameRef name,
-                                         ConstraintLocator *locator,
-                                         bool isMissingImport);
+                                         ConstraintLocator *locator);
 
   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
@@ -960,6 +960,8 @@ enum ScoreKind: unsigned int {
   SK_Hole,
   /// A reference to an @unavailable declaration.
   SK_Unavailable,
+  /// A reference to a declaration from a module that has not been imported.
+  SK_MissingImport,
   /// A reference to an async function in a synchronous context.
   ///
   /// \note Any score kind after this is considered a conversion that doesn't
@@ -1124,6 +1126,9 @@ struct Score {
     case SK_Unavailable:
       return "use of an unavailable declaration";
 
+    case SK_MissingImport:
+      return "use of a declaration that has not been imported";
+
     case SK_AsyncInSyncMismatch:
       return "async-in-synchronous mismatch";
 
@@ -1975,10 +1980,6 @@ 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
@@ -2325,8 +2325,7 @@ static bool missingImportForMemberDecl(const DeclContext *dc, ValueDecl *decl) {
   if (!ctx.LangOpts.hasFeature(Feature::MemberImportVisibility))
     return false;
 
-  auto declModule = decl->getDeclContext()->getParentModule();
-  return !ctx.getImportCache().isImportedBy(declModule, dc);
+  return !dc->isDeclImported(decl);
 }
 
 /// Determine whether the given declaration is an acceptable lookup
@@ -2504,6 +2503,11 @@ bool DeclContext::lookupQualified(Type type,
                          loc, options, decls);
 }
 
+bool DeclContext::isDeclImported(const Decl *decl) const {
+  auto declModule = decl->getDeclContext()->getParentModule();
+  return getASTContext().getImportCache().isImportedBy(declModule, this);
+}
+
 static void installPropertyWrapperMembersIfNeeded(NominalTypeDecl *target,
                                                   DeclNameRef member) {
   auto &Context = target->getASTContext();
diff --git a/lib/Sema/CSDiagnostics.cpp b/lib/Sema/CSDiagnostics.cpp
@@ -6250,11 +6250,6 @@ bool InaccessibleMemberFailure::diagnoseAsError() {
   }
 
   auto loc = nameLoc.isValid() ? nameLoc.getStartLoc() : ::getLoc(anchor);
-  if (IsMissingImport) {
-    maybeDiagnoseMissingImportForMember(Member, getDC(), loc);
-    return true;
-  }
-
   auto accessLevel = Member->getFormalAccessScope().accessLevelForDiagnostics();
   if (auto *CD = dyn_cast<ConstructorDecl>(Member)) {
     emitDiagnosticAt(loc, diag::init_candidate_inaccessible,
diff --git a/lib/Sema/CSDiagnostics.h b/lib/Sema/CSDiagnostics.h
@@ -1640,13 +1640,11 @@ class ClosureParamDestructuringFailure final : public FailureDiagnostic {
 /// ```
 class InaccessibleMemberFailure final : public FailureDiagnostic {
   ValueDecl *Member;
-  bool IsMissingImport;
 
 public:
   InaccessibleMemberFailure(const Solution &solution, ValueDecl *member,
-                            ConstraintLocator *locator, bool isMissingImport)
-      : FailureDiagnostic(solution, locator), Member(member),
-        IsMissingImport(isMissingImport) {}
+                            ConstraintLocator *locator)
+      : FailureDiagnostic(solution, locator), Member(member) {}
 
   bool diagnoseAsError() override;
 };
diff --git a/lib/Sema/CSFix.cpp b/lib/Sema/CSFix.cpp
@@ -1153,16 +1153,16 @@ MoveOutOfOrderArgument *MoveOutOfOrderArgument::create(
 
 bool AllowInaccessibleMember::diagnose(const Solution &solution,
                                        bool asNote) const {
-  InaccessibleMemberFailure failure(solution, getMember(), getLocator(),
-                                    IsMissingImport);
+  InaccessibleMemberFailure failure(solution, getMember(), getLocator());
   return failure.diagnose(asNote);
 }
 
-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);
+AllowInaccessibleMember *
+AllowInaccessibleMember::create(ConstraintSystem &cs, Type baseType,
+                                ValueDecl *member, DeclNameRef name,
+                                ConstraintLocator *locator) {
+  return new (cs.getAllocator())
+      AllowInaccessibleMember(cs, baseType, member, name, locator);
 }
 
 bool AllowAnyObjectKeyPathRoot::diagnose(const Solution &solution,
diff --git a/lib/Sema/CSRanking.cpp b/lib/Sema/CSRanking.cpp
@@ -963,7 +963,7 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
              ? SolutionCompareResult::Better
              : SolutionCompareResult::Worse;
   }
-  
+
   // Compute relative score.
   unsigned score1 = 0;
   unsigned score2 = 0;
diff --git a/lib/Sema/CSSimplify.cpp b/lib/Sema/CSSimplify.cpp
@@ -10346,7 +10346,8 @@ performMemberLookup(ConstraintKind constraintKind, DeclNameRef memberName,
   // include them in the unviable candidates list.
   if (result.ViableCandidates.empty() && result.UnviableCandidates.empty() &&
       includeInaccessibleMembers) {
-    NameLookupOptions lookupOptions = defaultMemberLookupOptions;
+    NameLookupOptions lookupOptions =
+        defaultConstraintSolverMemberLookupOptions;
 
     // Local function that looks up additional candidates using the given lookup
     // options, recording the results as unviable candidates.
@@ -10380,12 +10381,6 @@ performMemberLookup(ConstraintKind constraintKind, DeclNameRef memberName,
     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;
@@ -10586,11 +10581,9 @@ static ConstraintFix *fixMemberRef(
     }
 
     case MemberLookupResult::UR_Inaccessible:
-    case MemberLookupResult::UR_MissingImport:
       assert(choice.isDecl());
-      return AllowInaccessibleMember::create(
-          cs, baseTy, choice.getDecl(), memberName, locator,
-          *reason == MemberLookupResult::UR_MissingImport);
+      return AllowInaccessibleMember::create(cs, baseTy, choice.getDecl(),
+                                             memberName, locator);
 
     case MemberLookupResult::UR_UnavailableInExistential: {
       return choice.isDecl()
diff --git a/lib/Sema/ConstraintSystem.cpp b/lib/Sema/ConstraintSystem.cpp
@@ -292,8 +292,8 @@ LookupResult &ConstraintSystem::lookupMember(Type base, DeclNameRef name,
   if (result) return *result;
 
   // Lookup the member.
-  result = TypeChecker::lookupMember(DC, base, name, loc,
-                                     defaultMemberLookupOptions);
+  result = TypeChecker::lookupMember(
+      DC, base, name, loc, defaultConstraintSolverMemberLookupOptions);
 
   // If we are in an @_unsafeInheritExecutor context, swap out
   // declarations for their _unsafeInheritExecutor_ counterparts if they
@@ -3966,6 +3966,12 @@ void ConstraintSystem::resolveOverload(ConstraintLocator *locator,
     if (isDeclUnavailable(decl, locator))
       increaseScore(SK_Unavailable, locator);
 
+    // If the declaration is from a module that hasn't been imported, note that.
+    if (getASTContext().LangOpts.hasFeature(Feature::MemberImportVisibility)) {
+      if (!useDC->isDeclImported(decl))
+        increaseScore(SK_MissingImport, locator);
+    }
+
     // If this overload is disfavored, note that.
     if (decl->getAttrs().hasAttribute<DisfavoredOverloadAttr>())
       increaseScore(SK_DisfavoredOverload, locator);
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -6163,6 +6163,40 @@ diagnoseDictionaryLiteralDuplicateKeyEntries(const Expr *E,
   const_cast<Expr *>(E)->walk(Walker);
 }
 
+// Verifies that references to members are valid under the restrictions of the
+// MemberImportVisibility feature.
+static void diagnoseMissingMemberImports(const Expr *E, const DeclContext *DC) {
+  class DiagnoseWalker : public BaseDiagnosticWalker {
+    const DeclContext *dc;
+
+  public:
+    DiagnoseWalker(const DeclContext *dc) : dc(dc) {}
+
+    PreWalkResult<Expr *> walkToExprPre(Expr *E) override {
+      if (auto declRef = E->getReferencedDecl())
+        checkDecl(declRef.getDecl(), E->getLoc());
+
+      return Action::Continue(E);
+    }
+
+    void checkDecl(const ValueDecl *decl, SourceLoc loc) {
+      // Only diagnose uses of members.
+      if (!decl->getDeclContext()->isTypeContext())
+        return;
+
+      if (!dc->isDeclImported(decl))
+        maybeDiagnoseMissingImportForMember(decl, dc, loc);
+    }
+  };
+
+  auto &ctx = DC->getASTContext();
+  if (!ctx.LangOpts.hasFeature(Feature::MemberImportVisibility))
+    return;
+
+  DiagnoseWalker walker(DC);
+  const_cast<Expr *>(E)->walk(walker);
+}
+
 //===----------------------------------------------------------------------===//
 // High-level entry points.
 //===----------------------------------------------------------------------===//
@@ -6189,6 +6223,7 @@ void swift::performSyntacticExprDiagnostics(const Expr *E,
   diagnoseConstantArgumentRequirement(E, DC);
   diagUnqualifiedAccessToMethodNamedSelf(E, DC);
   diagnoseDictionaryLiteralDuplicateKeyEntries(E, DC);
+  diagnoseMissingMemberImports(E, DC);
 }
 
 void swift::performStmtDiagnostics(const Stmt *S, DeclContext *DC) {
diff --git a/lib/Sema/ResilienceDiagnostics.cpp b/lib/Sema/ResilienceDiagnostics.cpp
@@ -279,8 +279,7 @@ static bool diagnoseValueDeclRefExportability(SourceLoc loc, const ValueDecl *D,
   // Some diagnostics emitted with the `MemberImportVisibility` feature enabled
   // subsume these diagnostics.
   if (originKind == DisallowedOriginKind::MissingImport &&
-      ctx.LangOpts.hasFeature(Feature::MemberImportVisibility) && SF &&
-      SF->hasDelayedMissingImportForMemberDiagnostic(D))
+      ctx.LangOpts.hasFeature(Feature::MemberImportVisibility) && SF)
     return false;
 
   if (auto accessor = dyn_cast<AccessorDecl>(D)) {
diff --git a/lib/Sema/TypeChecker.h b/lib/Sema/TypeChecker.h
@@ -174,6 +174,14 @@ inline NameLookupOptions operator|(NameLookupFlags flag1,
 /// Default options for member name lookup.
 const NameLookupOptions defaultMemberLookupOptions;
 
+/// Default options for member name lookup in the constraint solver.
+/// Overloads which come from modules that have not been imported should be
+/// deprioritized by ranking and diagnosed by MiscDiagnostics, so we allow
+/// them to be found in constraint solver lookups to improve diagnostics
+/// overall.
+const NameLookupOptions defaultConstraintSolverMemberLookupOptions(
+    NameLookupFlags::IgnoreMissingImports);
+
 /// Default options for member type lookup.
 const NameLookupOptions defaultMemberTypeLookupOptions;
 
diff --git a/test/NameLookup/Inputs/MemberImportVisibility/members_A.swift b/test/NameLookup/Inputs/MemberImportVisibility/members_A.swift
@@ -1,4 +1,6 @@
-public struct X { }
+public struct X {
+  public init() {}
+}
 
 public protocol P { }
 
diff --git a/test/NameLookup/Inputs/MemberImportVisibility/members_B.swift b/test/NameLookup/Inputs/MemberImportVisibility/members_B.swift
@@ -14,6 +14,13 @@ extension X {
   package struct NestedInB_package {}
 }
 
+// Members with the same names are also declared in C.
+extension X {
+  public init(_ x: Int) { self.init() }
+  public func ambiguous() -> Int { return 1 }
+  public func ambiguousDisfavored() -> Int { return 1 }
+}
+
 extension Y {
   public func YinB() { }
 
diff --git a/test/NameLookup/Inputs/MemberImportVisibility/members_C.swift b/test/NameLookup/Inputs/MemberImportVisibility/members_C.swift
@@ -12,6 +12,13 @@ extension X {
   public struct NestedInC {}
 }
 
+// Members with the same names are also declared in B.
+extension X {
+  public init(_ x: Bool) { self.init() }
+  public func ambiguous() -> Bool { return false }
+  @_disfavoredOverload public func ambiguousDisfavored() -> Bool { return false }
+}
+
 extension Y {
   public func YinC() { }
 
diff --git a/test/NameLookup/members_transitive.swift b/test/NameLookup/members_transitive.swift
@@ -2,12 +2,12 @@
 // RUN: %target-swift-frontend -emit-module -o %t %S/Inputs/MemberImportVisibility/members_A.swift
 // RUN: %target-swift-frontend -emit-module -I %t -o %t -package-name TestPackage %S/Inputs/MemberImportVisibility/members_B.swift
 // RUN: %target-swift-frontend -emit-module -I %t -o %t %S/Inputs/MemberImportVisibility/members_C.swift
-// RUN: %target-swift-frontend -typecheck %s -I %t -verify -swift-version 5 -package-name TestPackage
-// RUN: %target-swift-frontend -typecheck %s -I %t -verify -swift-version 6 -package-name TestPackage
+// RUN: %target-swift-frontend -typecheck %s -I %t -verify -swift-version 5 -package-name TestPackage -verify-additional-prefix ambiguity-
+// RUN: %target-swift-frontend -typecheck %s -I %t -verify -swift-version 6 -package-name TestPackage -verify-additional-prefix ambiguity-
 // RUN: %target-swift-frontend -typecheck %s -I %t -verify -swift-version 5 -package-name TestPackage -enable-experimental-feature MemberImportVisibility -verify-additional-prefix member-visibility-
 
 import members_C
-// expected-member-visibility-note 12{{add import of module 'members_B'}}{{1-1=internal import members_B\n}}
+// expected-member-visibility-note 15{{add import of module 'members_B'}}{{1-1=internal import members_B\n}}
 
 
 func testExtensionMembers(x: X, y: Y<Z>) {
@@ -20,6 +20,24 @@ func testExtensionMembers(x: X, y: Y<Z>) {
 
   x.XinC()
   y.YinC()
+
+  _ = X(true)
+  _ = X(1) // expected-member-visibility-error{{initializer 'init(_:)' is not available due to missing import of defining module 'members_B'}}
+
+  _ = x.ambiguous() // expected-ambiguity-error{{ambiguous use of 'ambiguous()'}}
+  let _: Bool = x.ambiguous()
+  let _: Int = x.ambiguous() // expected-member-visibility-error{{instance method 'ambiguous()' is not available due to missing import of defining module 'members_B'}}
+
+  // ambiguousDisfavored() has two overloads:
+  // - members_B: public func ambiguousDisfavored() -> Int
+  // - members_C: @_disfavoredOverload public func ambiguousDisfavored() -> Bool
+  // With MemberImportVisibility, the overload from members_C should be picked.
+  // Otherwise, the overload from members_B should be picked.
+  let disfavoredResult = x.ambiguousDisfavored()
+  let _: Bool = disfavoredResult // expected-ambiguity-error{{type 'Int' cannot be used as a boolean; test for '!= 0' instead}}
+  let _: Int = disfavoredResult // expected-member-visibility-error{{cannot convert value of type 'Bool' to specified type 'Int'}}
+  let _: Bool = x.ambiguousDisfavored()
+  let _: Int = x.ambiguousDisfavored() // expected-member-visibility-error{{instance method 'ambiguousDisfavored()' is not available due to missing import of defining module 'members_B'}}
 }
 
 func testOperatorMembers(x: X, y: Y<Z>) {
diff --git a/test/NameLookup/members_transitive_multifile_access_level.swift b/test/NameLookup/members_transitive_multifile_access_level.swift
@@ -35,19 +35,19 @@
 
 //--- function_bodies.swift
 
-// FIXME: The access level of some of these fix-its should be public/package instead of internal.
+// FIXME: The access level on the fix-it for PackageUsesOnly is wrong.
 import Swift // Just here to anchor the fix-its
 // expected-note      {{add import of module 'InternalUsesOnly'}}{{1-1=internal import InternalUsesOnly\n}}
 // expected-note@-1   {{add import of module 'InternalUsesOnlyDefaultedImport'}}{{1-1=import InternalUsesOnlyDefaultedImport\n}}
-// expected-note@-2   {{add import of module 'PackageUsesOnly'}}{{1-1=internal import PackageUsesOnly\n}}
-// expected-note@-3   {{add import of module 'PublicUsesOnly'}}{{1-1=internal import PublicUsesOnly\n}}
+// expected-note@-2   {{add import of module 'PackageUsesOnly'}}{{1-1=public import PackageUsesOnly\n}}
+// expected-note@-3   {{add import of module 'PublicUsesOnly'}}{{1-1=public import PublicUsesOnly\n}}
 // expected-note@-4   {{add import of module 'PublicUsesOnlyDefaultedImport'}}{{1-1=import PublicUsesOnlyDefaultedImport\n}}
-// expected-note@-5 3 {{add import of module 'MixedUses'}}{{1-1=internal import MixedUses\n}}
+// expected-note@-5 3 {{add import of module 'MixedUses'}}{{1-1=public import MixedUses\n}}
 // expected-note@-6   {{add import of module 'InternalUsesOnlyTransitivelyImported'}}{{1-1=internal import InternalUsesOnlyTransitivelyImported\n}}
 // expected-note@-7   {{add import of module 'InternalUsesOnlySPIOnly'}}{{1-1=internal import InternalUsesOnlySPIOnly\n}}
 // expected-public-by-default-note@-8     {{add import of module 'InternalUsesOnlyDefaultedImportSPIOnly'}}{{1-1=@_spiOnly import InternalUsesOnlyDefaultedImportSPIOnly\n}}
 // expected-internal-by-default-note@-9   {{add import of module 'InternalUsesOnlyDefaultedImportSPIOnly'}}{{1-1=import InternalUsesOnlyDefaultedImportSPIOnly\n}}
-// expected-note@-10  {{add import of module 'PublicUsesOnlySPIOnly'}}{{1-1=internal import PublicUsesOnlySPIOnly\n}}
+// expected-note@-10  {{add import of module 'PublicUsesOnlySPIOnly'}}{{1-1=@_spiOnly public import PublicUsesOnlySPIOnly\n}}
 
 
 func internalFunc(_ x: Int) {
diff --git a/test/NameLookup/members_transitive_objc.swift b/test/NameLookup/members_transitive_objc.swift

Original file line number	Diff line number	Diff line change
`@@ -963,7 +963,7 @@ SolutionCompareResult ConstraintSystem::compareSolutions(`
`963`	`963`	`? SolutionCompareResult::Better`
`964`	`964`	`: SolutionCompareResult::Worse;`
`965`	`965`	`}`
`966`		`-`
	`966`	`+`
`967`	`967`	`// Compute relative score.`
`968`	`968`	`unsigned score1 = 0;`
`969`	`969`	`unsigned score2 = 0;`