Skip to content

Sema: Don't warn on package imports providing a type extended by a package extension #73125

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.

Sign up for GitHub

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 1 commit into from
Apr 22, 2024
Merged

Sema: Don't warn on package imports providing a type extended by a package extension #73125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 44 additions & 0 deletions lib/Sema/TypeCheckAccess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2538,6 +2538,49 @@ void swift::diagnoseUnnecessaryPublicImports(SourceFile &SF) {
}
}

/// Register the type extended by \p ED as being used in a package decl if
/// any member is a package decl. This patches a hole in the warnings on
/// superfluously public imports which usually relies on exportability checking
/// that is not currently executed for package decls.
void registerPackageAccessForPackageExtendedType(ExtensionDecl *ED) {
auto extendedType = ED->getExtendedNominal();
if (!extendedType)
return;

bool hasPackageMembers = llvm::any_of(ED->getMembers(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be another check to see if this ED doesn't have members but the nominal type it's extending has a package access level; otherwise the warning will still be thrown.

[](const Decl *member) -> bool {
auto *VD = dyn_cast<ValueDecl>(member);
if (!VD)
return false;

AccessScope accessScope =
VD->getFormalAccessScope(nullptr,
/*treatUsableFromInlineAsPublic*/true);
return accessScope.isPackage();
});
if (!hasPackageMembers)
return;

DeclContext *DC = ED->getDeclContext();
ImportAccessLevel import = extendedType->getImportAccessFrom(DC);
if (import.has_value()) {
auto SF = DC->getParentSourceFile();
if (SF)
SF->registerAccessLevelUsingImport(import.value(),
AccessLevel::Package);

auto &ctx = DC->getASTContext();
if (ctx.LangOpts.EnableModuleApiImportRemarks) {
ModuleDecl *importedVia = import->module.importedModule,
*sourceModule = ED->getModuleContext();
ED->diagnose(diag::module_api_import,
ED, importedVia, sourceModule,
importedVia == sourceModule,
/*isImplicit*/false);
}
}
}

void swift::checkAccessControl(Decl *D) {
if (isa<ValueDecl>(D) || isa<PatternBindingDecl>(D)) {
bool allowInlineable =
Expand All @@ -2546,6 +2589,7 @@ void swift::checkAccessControl(Decl *D) {
UsableFromInlineChecker().visit(D);
} else if (auto *ED = dyn_cast<ExtensionDecl>(D)) {
checkExtensionGenericParamAccess(ED);
registerPackageAccessForPackageExtendedType(ED);
}

if (isa<AccessorDecl>(D))
Expand Down
9 changes: 9 additions & 0 deletions test/Sema/superfluously-public-imports.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// RUN: %target-swift-frontend -emit-module %t/ExtendedDefinitionNonPublic.swift -o %t -I %t
// RUN: %target-swift-frontend -emit-module %t/UnusedImport.swift -o %t -I %t
// RUN: %target-swift-frontend -emit-module %t/UnusedPackageImport.swift -o %t -I %t
// RUN: %target-swift-frontend -emit-module %t/ExtendedPackageTypeImport.swift -o %t -I %t
// RUN: %target-swift-frontend -emit-module %t/ImportNotUseFromAPI.swift -o %t -I %t
// RUN: %target-swift-frontend -emit-module %t/ImportUsedInPackage.swift -o %t -I %t
// RUN: %target-swift-frontend -emit-module %t/ExportedUnused.swift -o %t -I %t
Expand Down Expand Up @@ -96,6 +97,9 @@ public struct NonPublicExtendedType {}
//--- UnusedImport.swift

//--- UnusedPackageImport.swift
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intentional for UnusedPackageImport.swift to be an empty file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's imported but nothing it would contain would be used, but the import does show the diagnostic that we're testing. I could have had a single Empty.swift file and compile it into many different modules, but I find it useful to have a file with the module name.

//--- ExtendedPackageTypeImport.swift

public struct ExtendedPackageType {}
Copy link
Contributor

@elsh elsh Apr 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: comment above, package struct P {} could be extended without any members like extension P {} and the warning will still be shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that warning would be appropriate then as an empty extension would be internal or below.

package import DefinesP // warning: package import of 'DefinesP' was not used in package declarations

extension P {}

It would fall in the same scenario as extending the type with internal decls where there's no requirement to make the import package or public as these services are not visible to other modules. Does that make sense or am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. For some reason I thought the default access level of the extension is whatever access level its nominal type has, but it looks to be internal when not explicitly marked. FYI package exportability PR #73161

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect #73161 may be more challenging to land as it adds errors. So I'll merge this one but please feel free to revert it as part of #73161 if you see it interfering with the diagnostics.


//--- ImportNotUseFromAPI.swift
public struct NotAnAPIType {}
Expand Down Expand Up @@ -143,6 +147,7 @@ package import UnusedImport // expected-warning {{package import of 'UnusedImpor
// expected-warning @-1 {{module 'UnusedImport' is imported as 'public' from the same file; this 'package' access level will be ignored}}

package import UnusedPackageImport // expected-warning {{package import of 'UnusedPackageImport' was not used in package declarations}} {{1-9=}}
package import ExtendedPackageTypeImport
public import ImportNotUseFromAPI // expected-warning {{public import of 'ImportNotUseFromAPI' was not used in public declarations or inlinable code}} {{1-8=}}
public import ImportUsedInPackage // expected-warning {{public import of 'ImportUsedInPackage' was not used in public declarations or inlinable code}} {{1-7=package}}

Expand Down Expand Up @@ -241,6 +246,10 @@ public protocol Countable {
extension Extended: Countable { // expected-remark {{struct 'Extended' is imported via 'RetroactiveConformance'}}
}

extension ExtendedPackageType { // expected-remark {{struct 'ExtendedPackageType' is imported via 'ExtendedPackageTypeImport'}}
package func useExtendedPackageType() { }
}

/// Tests for imports of clang modules.
//--- module.modulemap
module ClangSimpleUnused {
Expand Down