Skip to content

[6.0] Sema: Improve warnings and notes related to access-level on imports #73203

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions include/swift/AST/DiagnosticsSema.def
Original file line number Diff line number Diff line change
Expand Up @@ -1164,6 +1164,9 @@ WARNING(implementation_only_requires_library_evolution,none,
"using '@_implementationOnly' without enabling library evolution "
"for %0 may lead to instability during execution",
(Identifier))
WARNING(implementation_only_deprecated,none,
"'@_implementationOnly' is deprecated, use 'internal import' instead",
())

ERROR(module_allowable_client_violation,none,
"module %0 doesn't allow importation from module %1",
Expand Down Expand Up @@ -2501,6 +2504,11 @@ NOTE(decl_import_via_here,none,
"'%select{private|fileprivate|internal|package|%ERROR|%ERROR}1' "
"from %2 here",
(const Decl *, AccessLevel, const ModuleDecl*))
NOTE(decl_import_via_local,none,
"%kind0 is imported by this file as "
"'%select{private|fileprivate|internal|package|%ERROR|%ERROR}1' "
"from %2",
(const Decl *, AccessLevel, const ModuleDecl*))

// Opaque return types
ERROR(opaque_type_invalid_constraint,none,
Expand Down Expand Up @@ -3678,6 +3686,10 @@ ERROR(inconsistent_implicit_access_level_on_import,none,
"ambiguous implicit access level for import of %0; it is imported as "
"'%select{private|fileprivate|internal|package|public|%error}1' elsewhere",
(Identifier, AccessLevel))
NOTE(inconsistent_implicit_access_level_on_import_silence,none,
"silence these warnings by adopting the upcoming feature "
"'InternalImportsByDefault'",
())
NOTE(inconsistent_implicit_access_level_on_import_here,none,
"imported "
"'%select{private|fileprivate|internal|package|public|%error}0' here",
Expand Down
15 changes: 11 additions & 4 deletions include/swift/AST/Import.h
Original file line number Diff line number Diff line change
Expand Up @@ -592,17 +592,22 @@ struct AttributedImport {
/// if the access level was implicit or explicit.
SourceRange accessLevelRange;

/// Location of the `@_implementationOnly` attribute if set.
SourceRange implementationOnlyRange;

AttributedImport(ModuleInfo module, SourceLoc importLoc = SourceLoc(),
ImportOptions options = ImportOptions(),
StringRef filename = {}, ArrayRef<Identifier> spiGroups = {},
SourceRange preconcurrencyRange = {},
std::optional<AccessLevel> docVisibility = std::nullopt,
AccessLevel accessLevel = AccessLevel::Public,
SourceRange accessLevelRange = SourceRange())
SourceRange accessLevelRange = SourceRange(),
SourceRange implementationOnlyRange = SourceRange())
: module(module), importLoc(importLoc), options(options),
sourceFileArg(filename), spiGroups(spiGroups),
preconcurrencyRange(preconcurrencyRange), docVisibility(docVisibility),
accessLevel(accessLevel), accessLevelRange(accessLevelRange) {
accessLevel(accessLevel), accessLevelRange(accessLevelRange),
implementationOnlyRange(implementationOnlyRange) {
assert(!(options.contains(ImportFlags::Exported) &&
options.contains(ImportFlags::ImplementationOnly)) ||
options.contains(ImportFlags::Reserved));
Expand All @@ -613,7 +618,8 @@ struct AttributedImport {
: AttributedImport(module, other.importLoc, other.options,
other.sourceFileArg, other.spiGroups,
other.preconcurrencyRange, other.docVisibility,
other.accessLevel, other.accessLevelRange) { }
other.accessLevel, other.accessLevelRange,
other.implementationOnlyRange) { }

friend bool operator==(const AttributedImport<ModuleInfo> &lhs,
const AttributedImport<ModuleInfo> &rhs) {
Expand All @@ -623,7 +629,8 @@ struct AttributedImport {
lhs.spiGroups == rhs.spiGroups &&
lhs.docVisibility == rhs.docVisibility &&
lhs.accessLevel == rhs.accessLevel &&
lhs.accessLevelRange == rhs.accessLevelRange;
lhs.accessLevelRange == rhs.accessLevelRange &&
lhs.implementationOnlyRange == rhs.implementationOnlyRange;
}

AttributedImport<ImportedModule> getLoaded(ModuleDecl *loadedModule) const {
Expand Down
30 changes: 23 additions & 7 deletions lib/Sema/ImportResolution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -577,8 +577,10 @@ UnboundImport::UnboundImport(ImportDecl *ID)
if (ID->getAttrs().hasAttribute<TestableAttr>())
import.options |= ImportFlags::Testable;

if (ID->getAttrs().hasAttribute<ImplementationOnlyAttr>())
if (auto attr = ID->getAttrs().getAttribute<ImplementationOnlyAttr>()) {
import.options |= ImportFlags::ImplementationOnly;
import.implementationOnlyRange = attr->Range;
}

import.accessLevel = ID->getAccessLevel();
if (auto attr = ID->getAttrs().getAttribute<AccessControlAttr>()) {
Expand Down Expand Up @@ -802,7 +804,14 @@ void UnboundImport::validateResilience(NullablePtr<ModuleDecl> topLevelModule,
// We exempt some imports using @_implementationOnly in a safe way from
// packages that cannot be resilient.
if (import.options.contains(ImportFlags::ImplementationOnly) &&
!SF.getParentModule()->isResilient() && topLevelModule &&
import.implementationOnlyRange.isValid()) {
if (SF.getParentModule()->isResilient()) {
// Encourage replacing `@_implementationOnly` with `internal import`.
auto inFlight =
ctx.Diags.diagnose(import.importLoc,
diag::implementation_only_deprecated);
inFlight.fixItReplace(import.implementationOnlyRange, "internal");
} else if ( // Non-resilient
!(((targetName.str() == "CCryptoBoringSSL" ||
targetName.str() == "CCryptoBoringSSLShims") &&
(importerName.str() == "Crypto" ||
Expand All @@ -811,11 +820,13 @@ void UnboundImport::validateResilience(NullablePtr<ModuleDecl> topLevelModule,
((targetName.str() == "CNIOBoringSSL" ||
targetName.str() == "CNIOBoringSSLShims") &&
importerName.str() == "NIOSSL"))) {
ctx.Diags.diagnose(import.importLoc,
diag::implementation_only_requires_library_evolution,
importerName);
ctx.Diags.diagnose(import.importLoc,
diag::implementation_only_requires_library_evolution,
importerName);
}
}

// Report public imports of non-resilient modules from a resilient module.
if (import.options.contains(ImportFlags::ImplementationOnly) ||
import.accessLevel < AccessLevel::Public)
return;
Expand Down Expand Up @@ -1066,11 +1077,16 @@ CheckInconsistentAccessLevelOnImport::evaluate(
auto &diags = mod->getDiags();
{
InFlightDiagnostic error =
diags.diagnose(implicitImport, diag::inconsistent_implicit_access_level_on_import,
implicitImport->getModule()->getName(), otherAccessLevel);
diags.diagnose(implicitImport,
diag::inconsistent_implicit_access_level_on_import,
implicitImport->getModule()->getName(),
otherAccessLevel);
error.fixItInsert(implicitImport->getStartLoc(),
diag::inconsistent_implicit_access_level_on_import_fixit,
otherAccessLevel);
error.flush();
diags.diagnose(implicitImport,
diag::inconsistent_implicit_access_level_on_import_silence);
}

SourceLoc accessLevelLoc = otherImport->getStartLoc();
Expand Down
Loading