Skip to content

[Diagnostics] SR-7349 Improve accessibility diagnostics for inheritance from generic classes #16223

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 3 commits into from
Apr 28, 2018
Merged

[Diagnostics] SR-7349 Improve accessibility diagnostics for inheritance from generic classes #16223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ef12fe
[Diagnostics] Resolves SR-7349
AnthonyLatsis Apr 27, 2018
4c6ad08
updated 'Compatibility' tests (warnings)
AnthonyLatsis Apr 27, 2018
78d0d0a
updated the diagnostic's error version
AnthonyLatsis Apr 28, 2018
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
25 changes: 14 additions & 11 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3192,18 +3192,21 @@ ERROR(bool_intrinsics_not_found,none,
ERROR(class_super_access,none,
"class %select{must be declared %select{"
"%select{private|fileprivate|internal|%error|%error}2|private or fileprivate}3"
"|cannot be declared "
"%select{in this context|fileprivate|internal|public|open}1}0 "
"because its superclass is "
"%select{private|fileprivate|internal|%error|%error}2",
(bool, AccessLevel, AccessLevel, bool))
"|cannot be declared %select{in this context|fileprivate|internal|public|open}1}0 "
"because its superclass "
"%select{is %select{private|fileprivate|internal|%error|%error}2"
"|uses %select{a private|a fileprivate|an internal|%error|%error}2 "
"type as a generic parameter}4",
(bool, AccessLevel, AccessLevel, bool, bool))
WARNING(class_super_access_warn,none,
"class %select{should be declared "
"%select{private|fileprivate|internal|%error|%error}2"
"|should not be declared %select{in this context|fileprivate|internal|public|open}1}0 "
"because its superclass is "
"%select{private|fileprivate|internal|%error|%error}2",
(bool, AccessLevel, AccessLevel, bool))
"class %select{should be declared "
"%select{private|fileprivate|internal|%error|%error}2"
"|should not be declared %select{in this context|fileprivate|internal|public|open}1}0 "
"because its superclass "
"%select{is %select{private|fileprivate|internal|%error|%error}2"
"|uses %select{a private|a fileprivate|an internal|%error|%error}2 "
"type as a generic parameter}4",
(bool, AccessLevel, AccessLevel, bool, bool))
ERROR(dot_protocol_on_non_existential,none,
"cannot use 'Protocol' with non-protocol type %0", (Type))
ERROR(tuple_single_element,none,
Expand Down
59 changes: 25 additions & 34 deletions lib/Sema/TypeCheckDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1785,51 +1785,41 @@ static void checkGenericParamAccess(TypeChecker &TC,
return;

// This must stay in sync with diag::generic_param_access.
enum {
ACEK_Parameter = 0,
ACEK_Requirement
enum ACEK {
Parameter = 0,
Requirement
Copy link
Contributor

Choose a reason for hiding this comment

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

This pollutes the surrounding namespace unless the enum is declared with enum class.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, true. It is OK to commit the change here?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can open separate PR for to correct this, it's not a problem!

} accessControlErrorKind;
auto minAccessScope = AccessScope::getPublic();
const TypeRepr *complainRepr = nullptr;
auto downgradeToWarning = DowngradeToWarning::Yes;

auto callbackACEK = ACEK::Parameter;

auto callback = [&](AccessScope typeAccessScope,
const TypeRepr *thisComplainRepr,
DowngradeToWarning thisDowngrade) {
if (typeAccessScope.isChildOf(minAccessScope) ||
(thisDowngrade == DowngradeToWarning::No &&
downgradeToWarning == DowngradeToWarning::Yes) ||
(!complainRepr &&
typeAccessScope.hasEqualDeclContextWith(minAccessScope))) {
minAccessScope = typeAccessScope;
complainRepr = thisComplainRepr;
accessControlErrorKind = callbackACEK;
downgradeToWarning = thisDowngrade;
}
};
for (auto param : *params) {
if (param->getInherited().empty())
continue;
assert(param->getInherited().size() == 1);
checkTypeAccessImpl(TC, param->getInherited().front(), accessScope,
owner->getDeclContext(),
[&](AccessScope typeAccessScope,
const TypeRepr *thisComplainRepr,
DowngradeToWarning thisDowngrade) {
if (typeAccessScope.isChildOf(minAccessScope) ||
(thisDowngrade == DowngradeToWarning::No &&
downgradeToWarning == DowngradeToWarning::Yes) ||
(!complainRepr &&
typeAccessScope.hasEqualDeclContextWith(minAccessScope))) {
minAccessScope = typeAccessScope;
complainRepr = thisComplainRepr;
accessControlErrorKind = ACEK_Parameter;
downgradeToWarning = thisDowngrade;
}
});
callback);
}
callbackACEK = ACEK::Requirement;

for (auto &requirement : params->getRequirements()) {
auto callback = [&](AccessScope typeAccessScope,
const TypeRepr *thisComplainRepr,
DowngradeToWarning thisDowngrade) {
if (typeAccessScope.isChildOf(minAccessScope) ||
(thisDowngrade == DowngradeToWarning::No &&
downgradeToWarning == DowngradeToWarning::Yes) ||
(!complainRepr &&
typeAccessScope.hasEqualDeclContextWith(minAccessScope))) {
minAccessScope = typeAccessScope;
complainRepr = thisComplainRepr;
accessControlErrorKind = ACEK_Requirement;
downgradeToWarning = thisDowngrade;
}
};
switch (requirement.getKind()) {
case RequirementReprKind::TypeConstraint:
checkTypeAccessImpl(TC, requirement.getSubjectLoc(),
Expand Down Expand Up @@ -2168,12 +2158,13 @@ static void checkAccessControl(TypeChecker &TC, const Decl *D) {
bool isExplicit = CD->getAttrs().hasAttribute<AccessControlAttr>();
auto diagID = diag::class_super_access;
if (downgradeToWarning == DowngradeToWarning::Yes ||
outerDowngradeToWarning == DowngradeToWarning::Yes) {
outerDowngradeToWarning == DowngradeToWarning::Yes)
diagID = diag::class_super_access_warn;
}

auto diag = TC.diagnose(CD, diagID, isExplicit, CD->getFormalAccess(),
typeAccess,
isa<FileUnit>(CD->getDeclContext()));
isa<FileUnit>(CD->getDeclContext()),
superclassLocIter->getTypeRepr() != complainRepr);
highlightOffendingType(TC, diag, complainRepr);
});
}
Expand Down
6 changes: 5 additions & 1 deletion test/Compatibility/accessibility.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ class DefaultSubclassPublic : PublicClass {}
class DefaultSubclassInternal : InternalClass {}
class DefaultSubclassPrivate : PrivateClass {} // expected-error {{class must be declared private or fileprivate because its superclass is private}}

// expected-note@+1 * {{superclass is declared here}}
public class PublicGenericClass<T> {}
// expected-note@+2 * {{type declared here}}
// expected-note@+1 * {{superclass is declared here}}
Expand All @@ -441,7 +442,10 @@ open class OpenConcreteSubclassInternal : InternalGenericClass<Int> {} // expect
public class PublicConcreteSubclassPublic : PublicGenericClass<Int> {}
public class PublicConcreteSubclassInternal : InternalGenericClass<Int> {} // expected-warning {{class should not be declared public because its superclass is internal}}
public class PublicConcreteSubclassPrivate : PrivateGenericClass<Int> {} // expected-warning {{class should not be declared public because its superclass is private}}
public class PublicConcreteSubclassPublicPrivateArg : PublicGenericClass<PrivateStruct> {} // expected-warning {{class should not be declared public because its superclass is private}}
public class PublicConcreteSubclassPublicPrivateArg : PublicGenericClass<PrivateStruct> {} // expected-warning {{class should not be declared public because its superclass uses a private type as a generic parameter}}
public class PublicConcreteSubclassPublicInternalArg : PublicGenericClass<InternalStruct> {} // expected-warning {{class should not be declared public because its superclass uses an internal type as a generic parameter}}
open class OpenConcreteSubclassPublicFilePrivateArg : PublicGenericClass<FilePrivateStruct> {} // expected-warning {{class should not be declared open because its superclass uses a fileprivate type as a generic parameter}} expected-error {{superclass 'PublicGenericClass<FilePrivateStruct>' of open class must be open}}
internal class InternalConcreteSubclassPublicFilePrivateArg : InternalGenericClass<PrivateStruct> {} // expected-warning {{class should not be declared internal because its superclass uses a private type as a generic parameter}}

open class OpenGenericSubclassInternal<T> : InternalGenericClass<T> {} // expected-warning {{class should not be declared open because its superclass is internal}} expected-error {{superclass 'InternalGenericClass<T>' of open class must be open}}
public class PublicGenericSubclassPublic<T> : PublicGenericClass<T> {}
Expand Down
6 changes: 5 additions & 1 deletion test/Sema/accessibility.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ class DefaultSubclassPublic : PublicClass {}
class DefaultSubclassInternal : InternalClass {}
class DefaultSubclassPrivate : PrivateClass {} // expected-error {{class must be declared private or fileprivate because its superclass is private}}

// expected-note@+1 * {{superclass is declared here}}
public class PublicGenericClass<T> {}
// expected-note@+2 * {{type declared here}}
// expected-note@+1 * {{superclass is declared here}}
Expand All @@ -441,7 +442,10 @@ open class OpenConcreteSubclassInternal : InternalGenericClass<Int> {} // expect
public class PublicConcreteSubclassPublic : PublicGenericClass<Int> {}
public class PublicConcreteSubclassInternal : InternalGenericClass<Int> {} // expected-error {{class cannot be declared public because its superclass is internal}}
public class PublicConcreteSubclassPrivate : PrivateGenericClass<Int> {} // expected-error {{class cannot be declared public because its superclass is private}}
public class PublicConcreteSubclassPublicPrivateArg : PublicGenericClass<PrivateStruct> {} // expected-error {{class cannot be declared public because its superclass is private}}
public class PublicConcreteSubclassPublicPrivateArg : PublicGenericClass<PrivateStruct> {} // expected-error {{class cannot be declared public because its superclass uses a private type as a generic parameter}}
public class PublicConcreteSubclassPublicInternalArg : PublicGenericClass<InternalStruct> {} // expected-error {{class cannot be declared public because its superclass uses an internal type as a generic parameter}}
open class OpenConcreteSubclassPublicFilePrivateArg : PublicGenericClass<FilePrivateStruct> {} // expected-error {{class cannot be declared open because its superclass uses a fileprivate type as a generic parameter}} expected-error {{superclass 'PublicGenericClass<FilePrivateStruct>' of open class must be open}}
internal class InternalConcreteSubclassPublicFilePrivateArg : InternalGenericClass<PrivateStruct> {} // expected-error {{class cannot be declared internal because its superclass uses a private type as a generic parameter}}

open class OpenGenericSubclassInternal<T> : InternalGenericClass<T> {} // expected-error {{class cannot be declared open because its superclass is internal}} expected-error {{superclass 'InternalGenericClass<T>' of open class must be open}}
public class PublicGenericSubclassPublic<T> : PublicGenericClass<T> {}
Expand Down