Merge pull request #66183 from apple/es-rel-crash

[5.9] Fix a crash during lookup of an operator with package acl
Description: Type-checking an operator requires look up of all of its decls regardless of which access modifier is used before filtering. If one of them is a package decl in an imported module that was built with package-name, but the use site of the operator decl is in a module that is not built with package-name, it currently crashes as it tries to access package context of the use site, which is null. The change in this PR checks if both decl site and use site have package contexts before accessing the package name property, otherwise return false to be filtered out. This PR also includes a check of source file kind before showing diagnostics to ensure that errors are not shown if source is interface as interface does not include package symbols.
Risk: Low. It adds a null check on package context before accessing package name.
Original PR: Fix a crash during lookup of an operator with package acl #66155
Reviewed By: @tshortli @xedin @xymus
Testing: Added a test that ensures coverage of the affected flow
Resolves: rdar://108961906

Author: elsh

lib/AST/Decl.cpp

@@ -3973,11 +3973,19 @@ getAccessScopeForFormalAccess(const ValueDecl *VD,
   case AccessLevel::Package: {
     auto pkg = resultDC->getPackageContext(/*lookupIfNotCurrent*/ true);
     if (!pkg) {
-      // No package context was found; show diagnostics
-      auto &d = VD->getASTContext().Diags;
-      d.diagnose(VD->getLoc(), diag::access_control_requires_package_name);
-      // Instead of reporting and failing early, return the scope of
-      // resultDC to allow continuation (should still non-zero exit later)
+      auto srcFile = resultDC->getParentSourceFile();
+      // Check if the file containing package decls is an interface file; if an
+      // interface file contains package decls, they must be usableFromInline or
+      // inlinable and are accessed within the defining module, so package-name
+      // is not needed; do not show diagnostics in such case.
+      auto shouldSkipDiag = srcFile && srcFile->Kind == SourceFileKind::Interface;
+      if (!shouldSkipDiag) {
+        // No package context was found; show diagnostics
+        auto &d = VD->getASTContext().Diags;
+        d.diagnose(VD->getLoc(), diag::access_control_requires_package_name);
+      }
+      // Instead of reporting and failing early, return the scope of resultDC to
+      // allow continuation (should still non-zero exit later if in script mode)
       return AccessScope(resultDC);
     } else {
       return AccessScope(pkg);
@@ -4169,9 +4177,17 @@ static bool checkAccess(const DeclContext *useDC, const ValueDecl *VD,
     return useSF && useSF->hasTestableOrPrivateImport(access, sourceModule);
   }
   case AccessLevel::Package: {
+    auto srcFile = sourceDC->getParentSourceFile();
+
+    // srcFile could be null if VD decl is from an imported .swiftmodule
+    if (srcFile && srcFile->Kind == SourceFileKind::Interface) {
+      // If source file is interface, package decls must be usableFromInline or
+      // inlinable, and are accessed only within the defining module so return true
+      return true;
+    }
     auto srcPkg = sourceDC->getPackageContext(/*lookupIfNotCurrent*/ true);
     auto usePkg = useDC->getPackageContext(/*lookupIfNotCurrent*/ true);
-    return usePkg->isSamePackageAs(srcPkg);
+    return srcPkg && usePkg && usePkg->isSamePackageAs(srcPkg);
   }
   case AccessLevel::Public:
   case AccessLevel::Open:

test/Sema/accessibility_package_in_extension.swift

@@ -0,0 +1,35 @@
+// RUN: %empty-directory(%t)
+// RUN: split-file %s %t
+
+// RUN: %target-swift-frontend -verify -module-name Lib %t/Lib.swift -emit-module -emit-module-path %t/Lib.swiftmodule -package-name myPkg
+
+/// Type-checking an operator (L30 below) causes look up of all of its decls regardless of which access modifier is
+/// used (decl in both Client and Lib in this case) before filtering. The decl in Lib is a package decl and has
+/// package-name associated with it, but the use site (in Client) does not have package-name. The package decl
+/// should be filtered out and the public == decl should be picked.
+// RUN: %target-swift-frontend -typecheck -verify %t/Client.swift -I %t
+
+
+//--- Lib.swift
+public class Decl {
+  package static func == (lhs: Decl, rhs: Decl) -> Bool {
+    return true
+  }
+}
+
+//--- Client.swift
+import Lib
+
+extension Decl: Equatable {
+  public static func == (lhs: Decl, rhs: Decl) -> Bool {
+    return false
+  }
+}
+
+public protocol Proto {}
+extension Proto {
+  func foo(first: Decl, second: Decl) -> Bool {
+    // Type-checking below causes a look up of == in both Client and Lib
+    return first == second
+  }
+}