Merge pull request #68639 from xymus/package-or-no-package

Sema: consolidate diagnostics about `package` modifier outside of a package

Author: xymus

include/swift/AST/DiagnosticsSema.def

@@ -1935,12 +1935,9 @@ WARNING(access_control_non_objc_open_member,none,
       "non-'@objc' %0 in extensions cannot be overridden; use 'public' instead",
       (DescriptiveDeclKind))
 ERROR(access_control_requires_package_name, none,
-      "%0 has a package access level but no -package-name was specified: %1",
-      (Identifier, StringRef))
-ERROR(access_control_requires_package_name_import, none,
-      "package import can only be used from a module with a package name; "
-      "set it with the compiler flag -package-name",
-      ())
+      "the package access level %select{|used on %1 }0"
+      "requires a package name; set it with the compiler flag -package-name",
+      (bool, const Decl*))
 ERROR(invalid_decl_attribute,none,
       "'%0' attribute cannot be applied to this declaration", (DeclAttribute))
 ERROR(attr_invalid_on_decl_kind,none,

lib/AST/Decl.cpp

@@ -4091,21 +4091,6 @@ getAccessScopeForFormalAccess(const ValueDecl *VD,
   case AccessLevel::Package: {
     auto pkg = resultDC->getPackageContext(/*lookupIfNotCurrent*/ true);
     if (!pkg) {
-      auto srcFile = resultDC->getOutermostParentSourceFile();
-      // 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;
-        auto filename = srcFile ? srcFile->getFilename() : resultDC->getParentModule()->getBaseIdentifier().str();
-        d.diagnose(VD->getLoc(),
-                   diag::access_control_requires_package_name,
-                   VD->getBaseIdentifier(),
-                   filename);
-      }
       // 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);

lib/Sema/TypeCheckAttr.cpp

@@ -996,8 +996,8 @@ bool AttributeChecker::visitAbstractAccessControlAttr(
     return true;
   }
 
+  SourceFile *File = D->getDeclContext()->getParentSourceFile();
   if (auto importDecl = dyn_cast<ImportDecl>(D)) {
-    SourceFile *File = D->getDeclContext()->getParentSourceFile();
     if (!D->getASTContext().LangOpts.hasFeature(Feature::AccessLevelOnImport) &&
         File && File->Kind != SourceFileKind::Interface) {
       diagnoseAndRemoveAttr(attr, diag::access_level_on_import_not_enabled);
@@ -1019,6 +1019,15 @@ bool AttributeChecker::visitAbstractAccessControlAttr(
     }
   }
 
+  if (attr->getAccess() == AccessLevel::Package &&
+      D->getASTContext().LangOpts.PackageName.empty() &&
+      File && File->Kind != SourceFileKind::Interface) {
+    // `package` modifier used outside of a package.
+    diagnose(attr->getLocation(), diag::access_control_requires_package_name,
+             isa<ValueDecl>(D), D);
+    return true;
+  }
+
   return false;
 }
 

lib/Sema/TypeCheckDeclPrimary.cpp

@@ -1965,14 +1965,6 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
                      target->getModuleFilename());
     }
 
-    // Report use of package import when no package name is set.
-    if (ID->getAccessLevel() == AccessLevel::Package &&
-        getASTContext().LangOpts.PackageName.empty()) {
-      auto &diags = ID->getASTContext().Diags;
-      diags.diagnose(ID->getLoc(),
-                     diag::access_control_requires_package_name_import);
-    }
-
     // Report the public import of a private module.
     if (ID->getASTContext().LangOpts.LibraryLevel == LibraryLevel::API) {
       auto importer = ID->getModuleContext();

test/Sema/package-import-no-package-name.swift

@@ -0,0 +1,27 @@
+/// Report uses of the package access-level modifier without a package name.
+
+// RUN: %empty-directory(%t)
+// RUN: split-file --leading-lines %s %t
+
+// RUN: %target-swift-frontend -emit-module %t/PackageLib.swift -o %t
+// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
+// RUN:   -enable-experimental-feature AccessLevelOnImport -verify
+// RUN: %target-swift-frontend -typecheck %t/Client.swift -I %t \
+// RUN:   -enable-experimental-feature AccessLevelOnImport \
+// RUN:   -package-name pkg
+
+//--- PackageLib.swift
+public struct PackageImportType {}
+
+//--- Client.swift
+package import PackageLib // expected-error {{the package access level requires a package name; set it with the compiler flag -package-name}}
+
+package struct PackageStruct { // expected-error {{the package access level used on 'PackageStruct' requires a package name; set it with the compiler flag -package-name}}
+  package func explicitlyPackage() {} // expected-error {{the package access level used on 'explicitlyPackage()' requires a package name; set it with the compiler flag -package-name}}
+}
+
+public struct PublicStruct {}
+
+package extension PublicStruct { // expected-error {{the package access level requires a package name; set it with the compiler flag -package-name}}
+  func implicitlyPackage() {}
+}

test/attr/package-modifier-outside-of-package.swift

@@ -3,11 +3,11 @@
 // Package name should not be empty
 // RUN: not %target-swift-frontend -typecheck %s -package-name "" 2>&1 | %FileCheck %s -check-prefix CHECK-EMPTY
 // CHECK-EMPTY: error: package-name is empty
-// CHECK-EMPTY: error: 'log' has a package access level but no -package-name was specified: {{.*}}.swift
+// CHECK-EMPTY: error: the package access level used on 'log()' requires a package name; set it with the compiler flag -package-name
 
 // If package access level is used but no package-name is passed, it should error
 // RUN: not %target-swift-frontend -typecheck %s 2>&1 | %FileCheck %s -check-prefix CHECK-MISSING
-// CHECK-MISSING: error: 'log' has a package access level but no -package-name was specified: {{.*}}.swift
+// CHECK-MISSING: error: the package access level used on 'log()' requires a package name; set it with the compiler flag -package-name
 
 // Package name can be same as the module name
 // RUN: %target-swift-frontend -module-name Logging -package-name Logging %s -emit-module -emit-module-path %t/Logging.swiftmodule