Sema: Pick the most relevant import for diagnostics about the source of a decl #76358
Conversation
xymus
requested review from
hborla,
slavapestov,
xedin,
zoecarver,
hyp,
egorzhdan,
beccadax and
ian-twilightcoder
as code owners
|
@swift-ci Please smoke test |
Change how we pick the one import to point to in diagnostics about a referenced decl. This mostly affects the warning about superfluously public imports. This warning encourages the developer to delete imports, let's make sure we push them towards deleting the right ones. The order was previously not well defined, except that we always picked one of the most public imports. We now prioritize imports in this order: 1. The most public import. (Preserving the current behavior in type-checking of access-level on imports) 2. The import of the public version of the module defining the decl, determined via export_as or -public-module-name. 3. The import of the module defining the decl. 4. The first import in the sources bringing the decl via reexports. 5. Any other import, usually via an @_exported import in a different file. rdar://135357155
This service on ModuleDecl wasn't actually used before this PR. The main client in ASTPrinter calls direclty the underlying logic in FileUnit. Let's update it for our needs.
xymus
force-pushed
the
authoritative-import
branch
from
328096a to
007b260
Compare
|
@swift-ci Please smoke test |
We may want to add a similar logic from getImportAccessLevel where we determine the defining module in `diagnoseAndFixMissingImportForMember`. It should recommend to add the authoritative import, without considering those already present in the file. Then we can delete the `registerRequiredAccessLevelForModule(definingModule, accessLevel);` in `recordRequiredImportAccessLevelForDecl` and point to more superfluous public imports.
xymus
force-pushed
the
authoritative-import
branch
from
007b260 to
9ce34ab
Compare
|
Removing a commit that disabled marking the defining module as in use. It broke the diagnostics guiding adding missing imports tested in We may want to add a similar logic or consolidate with how we determine the defining module in This isn't a big problem at this time, having less than more of the superfluously public import warnings isn't harmful. |
|
@swift-ci Please smoke test |
| @@ -2552,7 +2552,7 @@ void swift::diagnoseUnnecessaryPublicImports(SourceFile &SF) { | |||
| diag::remove_package_import; | |||
| auto inFlight = ctx.Diags.diagnose(import.importLoc, | |||
| diagId, | |||
| importedModule); | |||
| importedModule->getName()); | |||
There was a problem hiding this comment.
Do we not need to use the public name here?
There was a problem hiding this comment.
We want the real module name here, the one that matches the import statement. This would be the diagnostics on a module with an -public-module-name otherwise:
public import SwiftPublicNameCore // expected-warning {{public import of 'SwiftPublicName' was not used in public declarations or inlinable code}}
public import SwiftPublicName
This is not covered anymore since I removed a commit. If I have to fix more things on this PR I'll look to add this back in the test.
Improve how we pick the import declaration used in diagnostics as a source of a referenced decl. In some cases more than one import gives access to a decl via reexports. This change mostly affects the warning about superfluously public imports where the compiler warns on imports not used in API. This warning encourages the developer to remove import statements from the sources, it should push them to remove only bad ones.
The order was previously limited to picking one of the most public imports. It also had a tendency to pick the implicit import of the underlying module. This lead to diagnostics that are not useful and may encourage deleting imports in the sources to instead rely on transitive import. We should instead at least accept dependencies declared locally even if they could be removed.
We now prioritize imports in this order:
export_asor-public-module-name.@_exportedfrom a different file.Since the rule 1. doesn't change, this doesn't affect the results of access-level on imports type-checking. Future work could review that priority and allow the access-level on more precise imports to override the one from more general imports.
rdar://135357155