Skip to content

Commit f810cbb

Browse files
tshortlitshortli
authored
Merge pull request #75639 from tshortli/refactor-import-access-level-diagnostics
Sema: Refactor superfluous public import tracking
2 parents 226099a + a077469 commit f810cbb

File tree

5 files changed

+138
-142
lines changed

5 files changed

+138
-142
lines changed

‎include/swift/AST/SourceFile.h

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -440,9 +440,11 @@ class SourceFile final : public FileUnit {
440440
AccessLevel
441441
getMaxAccessLevelUsingImport(const ModuleDecl *import) const;
442442

443-
/// Register the use of \p import from an API with \p accessLevel.
444-
void registerAccessLevelUsingImport(AttributedImport<ImportedModule> import,
445-
AccessLevel accessLevel);
443+
/// Register the requirement that \p mod be imported with an access level
444+
/// that is at least as permissive as \p accessLevel in order to satisfy
445+
/// access or exportability checking constraints.
446+
void registerRequiredAccessLevelForModule(ModuleDecl *mod,
447+
AccessLevel accessLevel);
446448

447449
enum ImportQueryKind {
448450
/// Return the results for testable or private imports.

‎lib/AST/Module.cpp

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2728,10 +2728,8 @@ SourceFile::getMaxAccessLevelUsingImport(
27282728
return known->second;
27292729
}
27302730

2731-
void SourceFile::registerAccessLevelUsingImport(
2732-
AttributedImport<ImportedModule> import,
2733-
AccessLevel accessLevel) {
2734-
auto mod = import.module.importedModule;
2731+
void SourceFile::registerRequiredAccessLevelForModule(ModuleDecl *mod,
2732+
AccessLevel accessLevel) {
27352733
auto known = ImportsUseAccessLevel.find(mod);
27362734
if (known == ImportsUseAccessLevel.end())
27372735
ImportsUseAccessLevel[mod] = accessLevel;
@@ -2754,7 +2752,7 @@ void SourceFile::registerAccessLevelUsingImport(
27542752
auto otherImportModName = otherImportMod->getName();
27552753
if (otherImportMod == declaringMod ||
27562754
llvm::find(bystanders, otherImportModName) != bystanders.end()) {
2757-
registerAccessLevelUsingImport(otherImport, accessLevel);
2755+
registerRequiredAccessLevelForModule(otherImportMod, accessLevel);
27582756
}
27592757
}
27602758
}

‎lib/Sema/ResilienceDiagnostics.cpp

Lines changed: 51 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -75,22 +75,16 @@ bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
7575
}
7676
}
7777

78-
ImportAccessLevel problematicImport = D->getImportAccessFrom(DC);
79-
if (problematicImport.has_value()) {
80-
auto SF = DC->getParentSourceFile();
81-
if (SF)
82-
SF->registerAccessLevelUsingImport(problematicImport.value(),
83-
AccessLevel::Public);
84-
85-
if (Context.LangOpts.EnableModuleApiImportRemarks) {
86-
ModuleDecl *importedVia = problematicImport->module.importedModule,
87-
*sourceModule = D->getModuleContext();
88-
Context.Diags.diagnose(loc, diag::module_api_import,
89-
D, importedVia, sourceModule,
90-
importedVia == sourceModule,
91-
/*isImplicit*/false);
92-
}
93-
}
78+
// Remember that the module defining the decl must be imported publicly.
79+
recordRequiredImportAccessLevelForDecl(
80+
D, DC, AccessLevel::Public,
81+
[&](AttributedImport<ImportedModule> attributedImport) {
82+
ModuleDecl *importedVia = attributedImport.module.importedModule,
83+
*sourceModule = D->getModuleContext();
84+
Context.Diags.diagnose(loc, diag::module_api_import, D, importedVia,
85+
sourceModule, importedVia == sourceModule,
86+
/*isImplicit*/ false);
87+
});
9488

9589
// General check on access-level of the decl.
9690
auto declAccessScope =
@@ -141,6 +135,7 @@ bool TypeChecker::diagnoseInlinableDeclRefAccess(SourceLoc loc,
141135

142136
Context.Diags.diagnose(D, diag::resilience_decl_declared_here, D);
143137

138+
ImportAccessLevel problematicImport = D->getImportAccessFrom(DC);
144139
if (problematicImport.has_value() &&
145140
problematicImport->accessLevel < D->getFormalAccess()) {
146141
Context.Diags.diagnose(problematicImport->importLoc,
@@ -161,25 +156,20 @@ static bool diagnoseTypeAliasDeclRefExportability(SourceLoc loc,
161156
if (!D)
162157
return false;
163158

164-
auto exportingModule = where.getDeclContext()->getParentModule();
159+
const DeclContext *DC = where.getDeclContext();
160+
auto exportingModule = DC->getParentModule();
165161
ASTContext &ctx = exportingModule->getASTContext();
166162

167-
ImportAccessLevel problematicImport = D->getImportAccessFrom(
168-
where.getDeclContext());
169-
if (problematicImport.has_value()) {
170-
auto SF = where.getDeclContext()->getParentSourceFile();
171-
if (SF)
172-
SF->registerAccessLevelUsingImport(problematicImport.value(),
173-
AccessLevel::Public);
174-
175-
if (ctx.LangOpts.EnableModuleApiImportRemarks) {
176-
ModuleDecl *importedVia = problematicImport->module.importedModule,
177-
*sourceModule = D->getModuleContext();
178-
ctx.Diags.diagnose(loc, diag::module_api_import_aliases,
179-
D, importedVia, sourceModule,
180-
importedVia == sourceModule);
181-
}
182-
}
163+
// Remember that the module defining the underlying type must be imported
164+
// publicly.
165+
recordRequiredImportAccessLevelForDecl(
166+
D, DC, AccessLevel::Public,
167+
[&](AttributedImport<ImportedModule> attributedImport) {
168+
ModuleDecl *importedVia = attributedImport.module.importedModule,
169+
*sourceModule = D->getModuleContext();
170+
ctx.Diags.diagnose(loc, diag::module_api_import_aliases, D, importedVia,
171+
sourceModule, importedVia == sourceModule);
172+
});
183173

184174
auto ignoredDowngradeToWarning = DowngradeToWarning::No;
185175
auto originKind =
@@ -224,7 +214,6 @@ static bool diagnoseTypeAliasDeclRefExportability(SourceLoc loc,
224214

225215
// If limited by an import, note which one.
226216
if (originKind == DisallowedOriginKind::NonPublicImport) {
227-
const DeclContext *DC = where.getDeclContext();
228217
ImportAccessLevel limitImport = D->getImportAccessFrom(DC);
229218
assert(limitImport.has_value() &&
230219
limitImport->accessLevel < AccessLevel::Public &&
@@ -250,14 +239,20 @@ static bool diagnoseValueDeclRefExportability(SourceLoc loc, const ValueDecl *D,
250239
ASTContext &ctx = DC->getASTContext();
251240
auto originKind = getDisallowedOriginKind(D, where, downgradeToWarning);
252241

253-
// If we got here it was used in API, we can record the use of the import.
254-
ImportAccessLevel import = D->getImportAccessFrom(DC);
255-
if (import.has_value() && reason.has_value()) {
256-
auto SF = DC->getParentSourceFile();
257-
if (SF)
258-
SF->registerAccessLevelUsingImport(import.value(),
259-
AccessLevel::Public);
260-
}
242+
// Remember that the module defining the decl must be imported publicly.
243+
recordRequiredImportAccessLevelForDecl(
244+
D, DC, AccessLevel::Public,
245+
[&](AttributedImport<ImportedModule> attributedImport) {
246+
if (where.isExported() && reason != ExportabilityReason::General &&
247+
originKind != DisallowedOriginKind::NonPublicImport) {
248+
// These may be reported twice, for the Type and for the TypeRepr.
249+
ModuleDecl *importedVia = attributedImport.module.importedModule,
250+
*sourceModule = D->getModuleContext();
251+
ctx.Diags.diagnose(loc, diag::module_api_import, D, importedVia,
252+
sourceModule, importedVia == sourceModule,
253+
/*isImplicit*/ false);
254+
}
255+
});
261256

262257
// Access levels from imports are reported with the others access levels.
263258
// Except for extensions and protocol conformances, we report them here.
@@ -277,19 +272,6 @@ static bool diagnoseValueDeclRefExportability(SourceLoc loc, const ValueDecl *D,
277272
return false;
278273
}
279274

280-
if (ctx.LangOpts.EnableModuleApiImportRemarks &&
281-
import.has_value() && where.isExported() &&
282-
reason != ExportabilityReason::General &&
283-
originKind != DisallowedOriginKind::NonPublicImport) {
284-
// These may be reported twice, for the Type and for the TypeRepr.
285-
ModuleDecl *importedVia = import->module.importedModule,
286-
*sourceModule = D->getModuleContext();
287-
ctx.Diags.diagnose(loc, diag::module_api_import,
288-
D, importedVia, sourceModule,
289-
importedVia == sourceModule,
290-
/*isImplicit*/false);
291-
}
292-
293275
if (originKind == DisallowedOriginKind::None)
294276
return false;
295277

@@ -336,6 +318,7 @@ static bool diagnoseValueDeclRefExportability(SourceLoc loc, const ValueDecl *D,
336318
}
337319

338320
// If limited by an import, note which one.
321+
ImportAccessLevel import = D->getImportAccessFrom(DC);
339322
if (originKind == DisallowedOriginKind::NonPublicImport) {
340323
assert(import.has_value() &&
341324
import->accessLevel < AccessLevel::Public &&
@@ -379,25 +362,22 @@ TypeChecker::diagnoseConformanceExportability(SourceLoc loc,
379362
if (ext->getParentModule()->isBuiltinModule())
380363
return false;
381364

365+
const DeclContext *DC = where.getDeclContext();
382366
ModuleDecl *M = ext->getParentModule();
383367
ASTContext &ctx = M->getASTContext();
384368

385-
ImportAccessLevel problematicImport = ext->getImportAccessFrom(where.getDeclContext());
386-
if (problematicImport.has_value()) {
387-
auto SF = where.getDeclContext()->getParentSourceFile();
388-
if (SF)
389-
SF->registerAccessLevelUsingImport(problematicImport.value(),
390-
AccessLevel::Public);
391-
392-
if (ctx.LangOpts.EnableModuleApiImportRemarks) {
393-
ModuleDecl *importedVia = problematicImport->module.importedModule,
394-
*sourceModule = ext->getModuleContext();
395-
ctx.Diags.diagnose(loc, diag::module_api_import_conformance,
396-
rootConf->getType(), rootConf->getProtocol(),
397-
importedVia, sourceModule,
398-
importedVia == sourceModule);
399-
}
400-
}
369+
// Remember that the module defining the conformance must be imported
370+
// publicly.
371+
recordRequiredImportAccessLevelForDecl(
372+
ext, DC, AccessLevel::Public,
373+
[&](AttributedImport<ImportedModule> attributedImport) {
374+
ModuleDecl *importedVia = attributedImport.module.importedModule,
375+
*sourceModule = ext->getModuleContext();
376+
ctx.Diags.diagnose(loc, diag::module_api_import_conformance,
377+
rootConf->getType(), rootConf->getProtocol(),
378+
importedVia, sourceModule,
379+
importedVia == sourceModule);
380+
});
401381

402382
auto originKind = getDisallowedOriginKind(ext, where);
403383
if (originKind == DisallowedOriginKind::None)
@@ -425,7 +405,6 @@ TypeChecker::diagnoseConformanceExportability(SourceLoc loc,
425405

426406
// If limited by an import, note which one.
427407
if (originKind == DisallowedOriginKind::NonPublicImport) {
428-
const DeclContext *DC = where.getDeclContext();
429408
ImportAccessLevel limitImport = ext->getImportAccessFrom(DC);
430409
assert(limitImport.has_value() &&
431410
limitImport->accessLevel < AccessLevel::Public &&

‎lib/Sema/TypeCheckAccess.cpp

Lines changed: 69 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -207,29 +207,21 @@ void AccessControlCheckerBase::checkTypeAccessImpl(
207207
return;
208208

209209
// Report where it was imported from.
210-
if (contextAccessScope.isPublic() ||
211-
contextAccessScope.isPackage()) {
212-
auto SF = useDC->getParentSourceFile();
210+
if (contextAccessScope.isPublicOrPackage()) {
213211
auto report = [&](const DeclRefTypeRepr *typeRepr, const ValueDecl *VD) {
214-
ImportAccessLevel import = VD->getImportAccessFrom(useDC);
215-
if (import.has_value()) {
216-
if (SF) {
217-
auto useLevel = contextAccessScope.isPublic() ? AccessLevel::Public
218-
: AccessLevel::Package;
219-
SF->registerAccessLevelUsingImport(import.value(), useLevel);
220-
}
221-
222-
if (Context.LangOpts.EnableModuleApiImportRemarks) {
223-
SourceLoc diagLoc = typeRepr? typeRepr->getLoc()
224-
: extractNearestSourceLoc(useDC);
225-
ModuleDecl *importedVia = import->module.importedModule,
226-
*sourceModule = VD->getModuleContext();
227-
Context.Diags.diagnose(diagLoc, diag::module_api_import,
228-
VD, importedVia, sourceModule,
229-
importedVia == sourceModule,
230-
/*isImplicit*/!typeRepr);
231-
}
232-
}
212+
// Remember that the module defining the decl must be imported publicly.
213+
recordRequiredImportAccessLevelForDecl(
214+
VD, useDC, contextAccessScope.accessLevelForDiagnostics(),
215+
[&](AttributedImport<ImportedModule> attributedImport) {
216+
SourceLoc diagLoc =
217+
typeRepr ? typeRepr->getLoc() : extractNearestSourceLoc(useDC);
218+
ModuleDecl *importedVia = attributedImport.module.importedModule,
219+
*sourceModule = VD->getModuleContext();
220+
Context.Diags.diagnose(diagLoc, diag::module_api_import, VD,
221+
importedVia, sourceModule,
222+
importedVia == sourceModule,
223+
/*isImplicit*/ !typeRepr);
224+
});
233225
};
234226

235227
if (typeRepr) {
@@ -2390,30 +2382,24 @@ class DeclAvailabilityChecker : public DeclVisitor<DeclAvailabilityChecker> {
23902382
!ED->getInherited().empty());
23912383
checkConstrainedExtensionRequirements(ED, hasExportedMembers);
23922384

2393-
if (!hasExportedMembers &&
2394-
!ED->getInherited().empty()) {
2395-
// If we haven't already visited the extended nominal visit it here.
2396-
// This logic is too wide but prevents false reports of an unused public
2397-
// import. We should instead check for public generic requirements
2398-
// similarly to ShouldPrintForModuleInterface::shouldPrint.
2385+
// If we haven't already visited the extended nominal visit it here.
2386+
// This logic is too wide but prevents false reports of an unused public
2387+
// import. We should instead check for public generic requirements
2388+
// similarly to ShouldPrintForModuleInterface::shouldPrint.
2389+
if (!hasExportedMembers && !ED->getInherited().empty()) {
23992390
auto DC = Where.getDeclContext();
2400-
ImportAccessLevel import = extendedType->getImportAccessFrom(DC);
2401-
if (import.has_value()) {
2402-
auto SF = DC->getParentSourceFile();
2403-
if (SF)
2404-
SF->registerAccessLevelUsingImport(import.value(),
2405-
AccessLevel::Public);
2406-
2407-
auto &ctx = DC->getASTContext();
2408-
if (ctx.LangOpts.EnableModuleApiImportRemarks) {
2409-
ModuleDecl *importedVia = import->module.importedModule,
2410-
*sourceModule = ED->getModuleContext();
2411-
ED->diagnose(diag::module_api_import,
2412-
ED, importedVia, sourceModule,
2413-
importedVia == sourceModule,
2414-
/*isImplicit*/false);
2415-
}
2416-
}
2391+
2392+
// Remember that the module defining the extended type must be imported
2393+
// publicly.
2394+
recordRequiredImportAccessLevelForDecl(
2395+
extendedType, DC, AccessLevel::Public,
2396+
[&](AttributedImport<ImportedModule> attributedImport) {
2397+
ModuleDecl *importedVia = attributedImport.module.importedModule,
2398+
*sourceModule = ED->getModuleContext();
2399+
ED->diagnose(diag::module_api_import, ED, importedVia, sourceModule,
2400+
importedVia == sourceModule,
2401+
/*isImplicit*/ false);
2402+
});
24172403
}
24182404
}
24192405

@@ -2510,6 +2496,32 @@ DisallowedOriginKind swift::getDisallowedOriginKind(const Decl *decl,
25102496
return getDisallowedOriginKind(decl, where, downgradeToWarning);
25112497
}
25122498

2499+
void swift::recordRequiredImportAccessLevelForDecl(
2500+
const Decl *decl, const DeclContext *dc, AccessLevel accessLevel,
2501+
RequiredImportAccessLevelCallback remark) {
2502+
auto sf = dc->getParentSourceFile();
2503+
if (!sf)
2504+
return;
2505+
2506+
auto definingModule = decl->getModuleContext();
2507+
if (definingModule == dc->getParentModule())
2508+
return;
2509+
2510+
sf->registerRequiredAccessLevelForModule(definingModule, accessLevel);
2511+
2512+
if (auto attributedImport = sf->getImportAccessLevel(definingModule)) {
2513+
auto importedModule = attributedImport->module.importedModule;
2514+
2515+
// If the defining module is transitively imported, mark the responsible
2516+
// module as requiring the minimum access level too.
2517+
if (importedModule != definingModule)
2518+
sf->registerRequiredAccessLevelForModule(importedModule, accessLevel);
2519+
2520+
if (dc->getASTContext().LangOpts.EnableModuleApiImportRemarks)
2521+
remark(*attributedImport);
2522+
}
2523+
}
2524+
25132525
void swift::diagnoseUnnecessaryPublicImports(SourceFile &SF) {
25142526
ASTContext &ctx = SF.getASTContext();
25152527
if (ctx.TypeCheckerOpts.SkipFunctionBodies != FunctionBodySkipping::None)
@@ -2577,23 +2589,18 @@ void registerPackageAccessForPackageExtendedType(ExtensionDecl *ED) {
25772589
return;
25782590

25792591
DeclContext *DC = ED->getDeclContext();
2580-
ImportAccessLevel import = extendedType->getImportAccessFrom(DC);
2581-
if (import.has_value()) {
2582-
auto SF = DC->getParentSourceFile();
2583-
if (SF)
2584-
SF->registerAccessLevelUsingImport(import.value(),
2585-
AccessLevel::Package);
2586-
2587-
auto &ctx = DC->getASTContext();
2588-
if (ctx.LangOpts.EnableModuleApiImportRemarks) {
2589-
ModuleDecl *importedVia = import->module.importedModule,
2590-
*sourceModule = ED->getModuleContext();
2591-
ED->diagnose(diag::module_api_import,
2592-
ED, importedVia, sourceModule,
2593-
importedVia == sourceModule,
2594-
/*isImplicit*/false);
2595-
}
2596-
}
2592+
2593+
// Remember that the module defining the decl must be imported with at least
2594+
// package visibility.
2595+
recordRequiredImportAccessLevelForDecl(
2596+
extendedType, DC, AccessLevel::Package,
2597+
[&](AttributedImport<ImportedModule> attributedImport) {
2598+
ModuleDecl *importedVia = attributedImport.module.importedModule,
2599+
*sourceModule = ED->getModuleContext();
2600+
ED->diagnose(diag::module_api_import, ED, importedVia, sourceModule,
2601+
importedVia == sourceModule,
2602+
/*isImplicit*/ false);
2603+
});
25972604
}
25982605

25992606
void swift::checkAccessControl(Decl *D) {

0 commit comments

Comments
 (0)