Skip to content

Check access control for the generic requirements of subscripts and typealiases #23726

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 2 commits into from
Apr 2, 2019
Merged

Check access control for the generic requirements of subscripts and typealiases #23726

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b4e476e
Check access control for the generic requirements of subscripts
jrose-apple Apr 2, 2019
883fc5b
Check access control for the generic requirements of typealiases.
jrose-apple Apr 2, 2019
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
50 changes: 34 additions & 16 deletions lib/Sema/TypeCheckAccess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,29 +162,30 @@ void AccessControlCheckerBase::checkTypeAccessImpl(
llvm::function_ref<CheckTypeAccessCallback> diagnose) {
if (TC.Context.isAccessControlDisabled())
return;
if (!type)
return;
// Don't spend time checking local declarations; this is always valid by the
// time we get to this point.
if (!contextAccessScope.isPublic() &&
contextAccessScope.getDeclContext()->isLocalContext())
return;

Optional<AccessScope> typeAccessScope =
TypeAccessScopeChecker::getAccessScope(type, useDC,
checkUsableFromInline);
auto downgradeToWarning = DowngradeToWarning::No;
AccessScope problematicAccessScope = AccessScope::getPublic();
if (type) {
Optional<AccessScope> typeAccessScope =
TypeAccessScopeChecker::getAccessScope(type, useDC,
checkUsableFromInline);

// Note: This means that the type itself is invalid for this particular
// context, because it references declarations from two incompatible scopes.
// In this case we should have diagnosed the bad reference already.
if (!typeAccessScope.hasValue())
return;
// Note: This means that the type itself is invalid for this particular
// context, because it references declarations from two incompatible scopes.
// In this case we should have diagnosed the bad reference already.
if (!typeAccessScope.hasValue())
return;
problematicAccessScope = *typeAccessScope;
}

AccessScope problematicAccessScope = *typeAccessScope;
auto downgradeToWarning = DowngradeToWarning::No;

if (contextAccessScope.hasEqualDeclContextWith(*typeAccessScope) ||
contextAccessScope.isChildOf(*typeAccessScope)) {
if (contextAccessScope.hasEqualDeclContextWith(problematicAccessScope) ||
contextAccessScope.isChildOf(problematicAccessScope)) {

// /Also/ check the TypeRepr, if present. This can be important when we're
// unable to preserve typealias sugar that's present in the TypeRepr.
Expand All @@ -194,7 +195,9 @@ void AccessControlCheckerBase::checkTypeAccessImpl(
Optional<AccessScope> typeReprAccessScope =
TypeReprAccessScopeChecker::getAccessScope(typeRepr, useDC,
checkUsableFromInline);
assert(typeReprAccessScope && "valid Type but not valid TypeRepr?");
if (!typeReprAccessScope.hasValue())
return;

if (contextAccessScope.hasEqualDeclContextWith(*typeReprAccessScope) ||
contextAccessScope.isChildOf(*typeReprAccessScope)) {
// Only if both the Type and the TypeRepr follow the access rules can
Expand Down Expand Up @@ -365,9 +368,16 @@ void AccessControlCheckerBase::checkGenericParamAccess(
if (minAccessScope.isPublic())
return;

// FIXME: Promote these to an error in the next -swift-version break.
if (isa<SubscriptDecl>(owner) || isa<TypeAliasDecl>(owner))
downgradeToWarning = DowngradeToWarning::Yes;

if (checkUsableFromInline) {
auto diagID = diag::generic_param_usable_from_inline;
if (!TC.Context.isSwiftVersionAtLeast(5))
downgradeToWarning = DowngradeToWarning::Yes;

auto diagID = diag::generic_param_usable_from_inline;
if (downgradeToWarning == DowngradeToWarning::Yes)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't do it in this PR even if you hadn't merged it already, but it might make sense to build something in to the DiagnosticEngine that downgrades errors to warnings if the language version is not high enough.

TC.diagnoseLanguageChange(/*after:*/5, owner, diag::generic_param_usable_from_inline ...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's come up before. The main barrier in this case is that the text for these is slightly different (one is phrased as "oh, you shouldn't do this" and the other is "this doesn't work"), but maybe that could be abstracted out too.

diagID = diag::generic_param_usable_from_inline_warn;
auto diag = TC.diagnose(owner,
diagID,
Expand Down Expand Up @@ -535,6 +545,8 @@ class AccessControlChecker : public AccessControlCheckerBase,
}

void visitTypeAliasDecl(TypeAliasDecl *TAD) {
checkGenericParamAccess(TAD->getGenericParams(), TAD);

checkTypeAccess(TAD->getUnderlyingTypeLoc(), TAD, /*mayBeInferred*/false,
[&](AccessScope typeAccessScope,
const TypeRepr *complainRepr,
Expand Down Expand Up @@ -802,6 +814,8 @@ class AccessControlChecker : public AccessControlCheckerBase,
}

void visitSubscriptDecl(SubscriptDecl *SD) {
checkGenericParamAccess(SD->getGenericParams(), SD);

auto minAccessScope = AccessScope::getPublic();
const TypeRepr *complainRepr = nullptr;
auto downgradeToWarning = DowngradeToWarning::No;
Expand Down Expand Up @@ -1119,6 +1133,8 @@ class UsableFromInlineChecker : public AccessControlCheckerBase,
}

void visitTypeAliasDecl(TypeAliasDecl *TAD) {
checkGenericParamAccess(TAD->getGenericParams(), TAD);

checkTypeAccess(TAD->getUnderlyingTypeLoc(), TAD, /*mayBeInferred*/false,
[&](AccessScope typeAccessScope,
const TypeRepr *complainRepr,
Expand Down Expand Up @@ -1294,6 +1310,8 @@ class UsableFromInlineChecker : public AccessControlCheckerBase,
}

void visitSubscriptDecl(SubscriptDecl *SD) {
checkGenericParamAccess(SD->getGenericParams(), SD);

for (auto &P : *SD->getIndices()) {
checkTypeAccess(P->getTypeLoc(), SD, /*mayBeInferred*/false,
[&](AccessScope typeAccessScope,
Expand Down
4 changes: 2 additions & 2 deletions test/IDE/print_ast_tc_decls.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1375,5 +1375,5 @@ public typealias MyPairI<B> = MyPair<Int, B>
// PASS_PRINT_AST: public typealias MyPairI<B> = MyPair<Int, B>
public typealias MyPairAlias<T, U> = MyPair<T, U>
// PASS_PRINT_AST: public typealias MyPairAlias<T, U> = MyPair<T, U>
public typealias MyPairAlias2<T: FooProtocol, U> = MyPair<T, U> where U: BarProtocol
// PASS_PRINT_AST: public typealias MyPairAlias2<T, U> = MyPair<T, U> where T : FooProtocol, U : BarProtocol
typealias MyPairAlias2<T: FooProtocol, U> = MyPair<T, U> where U: BarProtocol
// PASS_PRINT_AST: typealias MyPairAlias2<T, U> = MyPair<T, U> where T : FooProtocol, U : BarProtocol
10 changes: 10 additions & 0 deletions test/Sema/accessibility.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -869,3 +869,13 @@ public extension ObjCSub {
set {}
}
}

public struct TestGenericSubscripts {
// We'd like these to be errors in a future version of Swift, but they weren't
// in Swift 5.0.
public subscript<T: PrivateProto>(_: T) -> Int { return 0 } // expected-warning {{subscript should not be declared public because its generic parameter uses a private type}} {{none}}
public subscript<T>(where _: T) -> Int where T: PrivateProto { return 0 } // expected-warning {{subscript should not be declared public because its generic requirement uses a private type}} {{none}}
}

public typealias TestGenericAlias<T: PrivateProto> = T // expected-warning {{type alias should not be declared public because its generic parameter uses a private type}}
public typealias TestGenericAliasWhereClause<T> = T where T: PrivateProto // expected-warning {{type alias should not be declared public because its generic requirement uses a private type}}
10 changes: 9 additions & 1 deletion test/attr/attr_usableFromInline.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ internal struct InternalStruct {}
// expected-error@-1 {{type referenced from the underlying type of a '@usableFromInline' type alias must be '@usableFromInline' or public}}

protocol InternalProtocol {
// expected-note@-1 4{{type declared here}}
// expected-note@-1 * {{type declared here}}
associatedtype T
}

Expand Down Expand Up @@ -147,3 +147,11 @@ enum BadEnum {
@usableFromInline
class BadClass : InternalClass {}
// expected-error@-1 {{type referenced from the superclass of a '@usableFromInline' class must be '@usableFromInline' or public}}

public struct TestGenericSubscripts {
@usableFromInline subscript<T: InternalProtocol>(_: T) -> Int { return 0 } // expected-warning {{type referenced from a generic parameter of a '@usableFromInline' subscript should be '@usableFromInline' or public}}
@usableFromInline subscript<T>(where _: T) -> Int where T: InternalProtocol { return 0 } // expected-warning {{type referenced from a generic requirement of a '@usableFromInline' subscript should be '@usableFromInline' or public}}
}

@usableFromInline typealias TestGenericAlias<T: InternalProtocol> = T // expected-warning {{type referenced from a generic parameter of a '@usableFromInline' type alias should be '@usableFromInline' or public}}
@usableFromInline typealias TestGenericAliasWhereClause<T> = T where T: InternalProtocol // expected-warning {{type referenced from a generic requirement of a '@usableFromInline' type alias should be '@usableFromInline' or public}}