Skip to content

Add access level import diagnostic for named pattern. #77832

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
Dec 10, 2024
Merged

Add access level import diagnostic for named pattern. #77832

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
62 changes: 40 additions & 22 deletions lib/Sema/TypeCheckAccess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,33 +376,27 @@ static void highlightOffendingType(InFlightDiagnostic &diag,

/// Emit a note on \p limitImport when it restricted the access level
/// of a type.
static void noteLimitingImport(const Decl *userDecl,
ASTContext &ctx,
static void noteLimitingImport(const Decl *userDecl, ASTContext &ctx,
const ImportAccessLevel limitImport,
const TypeRepr *complainRepr) {
const Decl *complainDecl) {
if (!limitImport.has_value())
return;

assert(limitImport->accessLevel != AccessLevel::Public &&
"a public import shouldn't limit the access level of a decl");

if (auto *declRefTR = dyn_cast_or_null<DeclRefTypeRepr>(complainRepr)) {
ValueDecl *VD = declRefTR->getBoundDecl();

if (complainDecl) {
// When using an IDE in a large file the decl_import_via_here note
// may be easy to miss on the import. Duplicate the information on the
// error line as well so it can't be missed.
if (userDecl)
userDecl->diagnose(diag::decl_import_via_local,
VD,
limitImport->accessLevel,
limitImport->module.importedModule);
userDecl->diagnose(diag::decl_import_via_local, complainDecl,
limitImport->accessLevel,
limitImport->module.importedModule);

if (limitImport->importLoc.isValid())
ctx.Diags.diagnose(limitImport->importLoc,
diag::decl_import_via_here,
VD,
limitImport->accessLevel,
ctx.Diags.diagnose(limitImport->importLoc, diag::decl_import_via_here,
complainDecl, limitImport->accessLevel,
limitImport->module.importedModule);
} else if (limitImport->importLoc.isValid()) {
ctx.Diags.diagnose(limitImport->importLoc, diag::module_imported_here,
Expand All @@ -411,14 +405,21 @@ static void noteLimitingImport(const Decl *userDecl,
}
}

static void noteLimitingImport(const Decl *userDecl,
static void noteLimitingImport(const Decl *userDecl, ASTContext &ctx,
const ImportAccessLevel limitImport,
const TypeRepr *complainRepr) {
if (!limitImport.has_value())
return;
const Decl *complainDecl = nullptr;
if (auto *declRefTR = dyn_cast_or_null<DeclRefTypeRepr>(complainRepr))
complainDecl = declRefTR->getBoundDecl();

ASTContext &ctx = userDecl->getASTContext();
noteLimitingImport(userDecl, ctx, limitImport, complainRepr);
noteLimitingImport(userDecl, ctx, limitImport, complainDecl);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a small detail, but I think we still want the diagnostic to point to the TypeRepr instead of the Decl when there is a TypeRepr available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The behavior should be the same here, its just the unwrapping of the TypeRepr happens one level above in noteLimitingImport(..., const TypeRepr *complainRepr)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, if the exact source location that the diagnostic points to is not changing then this is fine. I was expecting that it would point directly to the type written in source (which would be different than the location of the decl), but I guess it doesn't already do that.

}

static void noteLimitingImport(const Decl *userDecl,
const ImportAccessLevel limitImport,
const TypeRepr *complainRepr) {
noteLimitingImport(userDecl, userDecl->getASTContext(), limitImport,
complainRepr);
}

void AccessControlCheckerBase::checkGenericParamAccess(
Expand Down Expand Up @@ -625,7 +626,8 @@ class AccessControlChecker : public AccessControlCheckerBase,
if (seenVars.count(theVar) || theVar->isInvalid())
return;

checkTypeAccess(theVar->getInterfaceType(), nullptr, theVar,
Type interfaceType = theVar->getInterfaceType();
checkTypeAccess(interfaceType, nullptr, theVar,
/*mayBeInferred*/false,
[&](AccessScope typeAccessScope,
const TypeRepr *complainRepr,
Expand All @@ -644,8 +646,24 @@ class AccessControlChecker : public AccessControlCheckerBase,
DE.diagnose(NP->getLoc(), diagID, theVar->isLet(),
isTypeContext, isExplicit, theVarAccess,
isa<FileUnit>(theVar->getDeclContext()),
typeAccess, theVar->getInterfaceType());
noteLimitingImport(theVar, importLimit, complainRepr);
typeAccess, interfaceType);

// As we pass in a null typeRepr the complainRepr will always be null.
// Extract the module import from the interface type instead.
const Decl *complainDecl = nullptr;
ImportAccessLevel complainImport = std::nullopt;
interfaceType.walk(SimpleTypeDeclFinder([&](const ValueDecl *VD) {
ImportAccessLevel import = VD->getImportAccessFrom(theVar->getDeclContext());
if (import.has_value() && import->accessLevel < VD->getFormalAccess()) {
complainDecl = VD;
complainImport = import;
return TypeWalker::Action::Stop;
}
return TypeWalker::Action::Continue;
}));

noteLimitingImport(theVar, theVar->getASTContext(), complainImport,
complainDecl);
});
}

Expand Down
9 changes: 9 additions & 0 deletions test/Sema/access-level-import-exportability.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -760,9 +760,13 @@ public struct PublicVars {

public var k = PublicImportType()
public var l = PackageImportType() // expected-error {{property cannot be declared public because its type 'PackageImportType' uses a package type}}
// expected-note @-1 {{is imported by this file as}}
public var m = InternalImportType() // expected-error {{property cannot be declared public because its type 'InternalImportType' uses an internal type}}
// expected-note @-1 {{is imported by this file as}}
public var n = FileprivateImportType() // expected-error {{property cannot be declared public because its type 'FileprivateImportType' uses a fileprivate type}}
// expected-note @-1 {{is imported by this file as}}
public var o = PrivateImportType() // expected-error {{property cannot be declared public because its type 'PrivateImportType' uses a private type}}
// expected-note @-1 {{is imported by this file as}}
}

package struct PackageVars {
Expand Down Expand Up @@ -792,8 +796,11 @@ package struct PackageVars {
package var k = PublicImportType()
package var l = PackageImportType()
package var m = InternalImportType() // expected-error {{property cannot be declared package because its type 'InternalImportType' uses an internal type}}
// expected-note @-1 {{is imported by this file as}}
package var n = FileprivateImportType() // expected-error {{property cannot be declared package because its type 'FileprivateImportType' uses a fileprivate type}}
// expected-note @-1 {{is imported by this file as}}
package var o = PrivateImportType() // expected-error {{property cannot be declared package because its type 'PrivateImportType' uses a private type}}
// expected-note @-1 {{is imported by this file as}}
}

internal struct InternalVars {
Expand Down Expand Up @@ -822,7 +829,9 @@ internal struct InternalVars {
internal var l = PackageImportType()
internal var m = InternalImportType()
internal var n = FileprivateImportType() // expected-error {{property cannot be declared internal because its type 'FileprivateImportType' uses a fileprivate type}}
// expected-note @-1 {{is imported by this file as}}
internal var o = PrivateImportType() // expected-error {{property cannot be declared internal because its type 'PrivateImportType' uses a private type}}
// expected-note @-1 {{is imported by this file as}}
}

fileprivate struct FileprivateVars {
Expand Down