Sema: Don't warn on package imports providing a type extended by a package extension #73125
Conversation
The warnings on superfluously public/package imports has a hole where it could report a package import as not being used by package decls even if it provides a type extended by a package extension. This is caused by the exportability checker not being enabled for package decls, thus not triggering the usual logic registering this reference. Address this issue by adding a check specifically for package extensions. We can remove this check once we have exportability checking for package decls. rdar://126712864
|
@swift-ci Please smoke test |
xymus
changed the title
|
@swift-ci Please smoke test Windows |
| @@ -96,6 +97,9 @@ public struct NonPublicExtendedType {} | |||
| //--- UnusedImport.swift | |||
|
|
|||
| //--- UnusedPackageImport.swift | |||
There was a problem hiding this comment.
Is it intentional for UnusedPackageImport.swift to be an empty file?
There was a problem hiding this comment.
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.
| if (!extendedType) | ||
| return; | ||
|
|
||
| bool hasPackageMembers = llvm::any_of(ED->getMembers(), |
There was a problem hiding this comment.
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.
| @@ -96,6 +97,9 @@ public struct NonPublicExtendedType {} | |||
| //--- UnusedImport.swift | |||
|
|
|||
| //--- UnusedPackageImport.swift | |||
| //--- ExtendedPackageTypeImport.swift | |||
|
|
|||
| public struct ExtendedPackageType {} | |||
There was a problem hiding this comment.
Re: comment above, package struct P {} could be extended without any members like extension P {} and the warning will still be shown.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
xymus
changed the title
The warnings on superfluously public/package imports has a hole where it could report a package import as not being used by package decls even if it provides a type extended by a package extension. Since the exportability checker does not run on package decls it does not triggering the usual logic registering this reference.
Address this issue by adding a check specifically for package extensions. We can remove this check once we have exportability checking for package decls.
rdar://126712864