From 1a4a47fee3e3adf2bd8c983a7e7b502316ae3899 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexis=20Laferri=C3=A8re?= Date: Tue, 19 Sep 2023 14:53:47 -0700 Subject: [PATCH 1/2] [Sema] Merge diagnostics about `package` modifier outside of a package Show the same error for both decls and imports when using the package access level and no package name is set. Also brings up this check to run once on decls and avoid repeated diagnostics. --- include/swift/AST/DiagnosticsSema.def | 5 +--- lib/AST/Decl.cpp | 15 ----------- lib/Sema/TypeCheckAttr.cpp | 10 ++++++- lib/Sema/TypeCheckDeclPrimary.cpp | 8 ------ .../Sema/package-import-no-package-name.swift | 17 ------------ .../package-modifier-outside-of-package.swift | 27 +++++++++++++++++++ .../package-name-diagnostics.swift | 4 +-- 7 files changed, 39 insertions(+), 47 deletions(-) delete mode 100644 test/Sema/package-import-no-package-name.swift create mode 100644 test/attr/package-modifier-outside-of-package.swift diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 492c428d69be9..16c38f9ed0040 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -1935,10 +1935,7 @@ 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; " + "the package access level requires a package name; " "set it with the compiler flag -package-name", ()) ERROR(invalid_decl_attribute,none, diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp index 0397439d0a9c5..2003a4b55b707 100644 --- a/lib/AST/Decl.cpp +++ b/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); diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp index 9751f50e12de4..698dc6b5bcd49 100644 --- a/lib/Sema/TypeCheckAttr.cpp +++ b/lib/Sema/TypeCheckAttr.cpp @@ -996,8 +996,8 @@ bool AttributeChecker::visitAbstractAccessControlAttr( return true; } + SourceFile *File = D->getDeclContext()->getParentSourceFile(); if (auto importDecl = dyn_cast(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,14 @@ 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); + return true; + } + return false; } diff --git a/lib/Sema/TypeCheckDeclPrimary.cpp b/lib/Sema/TypeCheckDeclPrimary.cpp index a893ef03308ba..ae389d5ef65a2 100644 --- a/lib/Sema/TypeCheckDeclPrimary.cpp +++ b/lib/Sema/TypeCheckDeclPrimary.cpp @@ -1965,14 +1965,6 @@ class DeclChecker : public DeclVisitor { 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(); diff --git a/test/Sema/package-import-no-package-name.swift b/test/Sema/package-import-no-package-name.swift deleted file mode 100644 index 34395d7243eba..0000000000000 --- a/test/Sema/package-import-no-package-name.swift +++ /dev/null @@ -1,17 +0,0 @@ -/// Report uses of package import without a package. - -// 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 {{package import can only be used from a module with a package name; set it with the compiler flag -package-name}} diff --git a/test/attr/package-modifier-outside-of-package.swift b/test/attr/package-modifier-outside-of-package.swift new file mode 100644 index 0000000000000..31eddcc72d84c --- /dev/null +++ b/test/attr/package-modifier-outside-of-package.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 requires a package name; set it with the compiler flag -package-name}} + package func explicitlyPackage() {} // expected-error {{the package access level 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() {} +} diff --git a/test/diagnostics/package-name-diagnostics.swift b/test/diagnostics/package-name-diagnostics.swift index fc0f4e963033a..309a188d572e2 100644 --- a/test/diagnostics/package-name-diagnostics.swift +++ b/test/diagnostics/package-name-diagnostics.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 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 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 From 4cd10483f4a0699f5444b152e9c3ccd4e0a5f15f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alexis=20Laferri=C3=A8re?= Date: Tue, 19 Sep 2023 17:13:19 -0700 Subject: [PATCH 2/2] [Sema] Name the decl in the error on package level without a package --- include/swift/AST/DiagnosticsSema.def | 6 +++--- lib/Sema/TypeCheckAttr.cpp | 3 ++- test/attr/package-modifier-outside-of-package.swift | 4 ++-- test/diagnostics/package-name-diagnostics.swift | 4 ++-- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def index 16c38f9ed0040..c9e00afdeeb19 100644 --- a/include/swift/AST/DiagnosticsSema.def +++ b/include/swift/AST/DiagnosticsSema.def @@ -1935,9 +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, - "the package access level requires 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, diff --git a/lib/Sema/TypeCheckAttr.cpp b/lib/Sema/TypeCheckAttr.cpp index 698dc6b5bcd49..16be45340b58a 100644 --- a/lib/Sema/TypeCheckAttr.cpp +++ b/lib/Sema/TypeCheckAttr.cpp @@ -1023,7 +1023,8 @@ bool AttributeChecker::visitAbstractAccessControlAttr( 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); + diagnose(attr->getLocation(), diag::access_control_requires_package_name, + isa(D), D); return true; } diff --git a/test/attr/package-modifier-outside-of-package.swift b/test/attr/package-modifier-outside-of-package.swift index 31eddcc72d84c..1dff753bff212 100644 --- a/test/attr/package-modifier-outside-of-package.swift +++ b/test/attr/package-modifier-outside-of-package.swift @@ -16,8 +16,8 @@ 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 requires a package name; set it with the compiler flag -package-name}} - package func explicitlyPackage() {} // 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 {} diff --git a/test/diagnostics/package-name-diagnostics.swift b/test/diagnostics/package-name-diagnostics.swift index 309a188d572e2..0d917cffa874e 100644 --- a/test/diagnostics/package-name-diagnostics.swift +++ b/test/diagnostics/package-name-diagnostics.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: the package access level requires a package name; set it with the compiler flag -package-name +// 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: the package access level requires a package name; set it with the compiler flag -package-name +// 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