Skip to content

Check protocol witnesses using access scopes. #4561

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
Aug 31, 2016
Merged

Check protocol witnesses using access scopes. #4561

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
32 changes: 27 additions & 5 deletions lib/Sema/MiscDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3598,6 +3598,20 @@ void swift::performStmtDiagnostics(TypeChecker &TC, const Stmt *S) {
// Utility functions
//===----------------------------------------------------------------------===//


Accessibility
swift::accessibilityFromScopeForDiagnostics(const DeclContext *accessScope) {
if (!accessScope)
return Accessibility::Public;
if (isa<ModuleDecl>(accessScope))
return Accessibility::Internal;
if (accessScope->isModuleScopeContext() &&
accessScope->getASTContext().LangOpts.EnableSwift3Private) {
return Accessibility::FilePrivate;
}
return Accessibility::Private;
}

void swift::fixItAccessibility(InFlightDiagnostic &diag, ValueDecl *VD,
Accessibility desiredAccess, bool isForSetter) {
StringRef fixItString;
Expand All @@ -3610,7 +3624,7 @@ void swift::fixItAccessibility(InFlightDiagnostic &diag, ValueDecl *VD,
}

DeclAttributes &attrs = VD->getAttrs();
DeclAttribute *attr;
AbstractAccessibilityAttr *attr;
if (isForSetter) {
attr = attrs.getAttribute<SetterAccessibilityAttr>();
cast<AbstractStorageDecl>(VD)->overwriteSetterAccessibility(desiredAccess);
Expand Down Expand Up @@ -3638,10 +3652,18 @@ void swift::fixItAccessibility(InFlightDiagnostic &diag, ValueDecl *VD,
diag.fixItRemove(attr->Range);

} else if (attr) {
// This uses getLocation() instead of getRange() because we don't want to
// replace the "(set)" part of a setter attribute.
diag.fixItReplace(attr->getLocation(), fixItString.drop_back());
attr->setInvalid();
// If the formal access already matches the desired access, the problem
// must be in a parent scope. Don't emit a fix-it.
// FIXME: It's also possible for access to already be /broader/ than what's
// desired, in which case the problem is also in a parent scope. However,
// this function is sometimes called to make access narrower, so assuming
// that a broader scope is acceptable breaks some diagnostics.
if (attr->getAccess() != desiredAccess) {
// This uses getLocation() instead of getRange() because we don't want to
// replace the "(set)" part of a setter attribute.
diag.fixItReplace(attr->getLocation(), fixItString.drop_back());
attr->setInvalid();
}

} else if (auto var = dyn_cast<VarDecl>(VD)) {
if (auto PBD = var->getParentPatternBinding())
Expand Down
7 changes: 7 additions & 0 deletions lib/Sema/MiscDiagnostics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,13 @@ namespace swift {
class TypeChecker;
class ValueDecl;

/// Returns the access level associated with \p accessScope, for diagnostic
/// purposes.
///
/// \sa ValueDecl::getFormalAccessScope
Accessibility
accessibilityFromScopeForDiagnostics(const DeclContext *accessScope);

/// \brief Emit diagnostics for syntactic restrictions on a given expression.
void performSyntacticExprDiagnostics(TypeChecker &TC, const Expr *E,
const DeclContext *DC,
Expand Down
17 changes: 0 additions & 17 deletions lib/Sema/TypeCheckDecl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1539,23 +1539,6 @@ static void highlightOffendingType(TypeChecker &TC, InFlightDiagnostic &diag,
}
}

/// Returns the access level associated with \p accessScope, for diagnostic
/// purposes.
///
/// \sa ValueDecl::getFormalAccessScope
static Accessibility
accessibilityFromScopeForDiagnostics(const DeclContext *accessScope) {
if (!accessScope)
return Accessibility::Public;
if (isa<ModuleDecl>(accessScope))
return Accessibility::Internal;
if (accessScope->isModuleScopeContext() &&
accessScope->getASTContext().LangOpts.EnableSwift3Private) {
return Accessibility::FilePrivate;
}
return Accessibility::Private;
}

static void checkGenericParamAccessibility(TypeChecker &TC,
const GenericParamList *params,
const Decl *owner,
Expand Down
101 changes: 51 additions & 50 deletions lib/Sema/TypeCheckProtocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ namespace {
unsigned &bestIdx,
bool &doNotDiagnoseMatches);

bool checkWitnessAccessibility(Accessibility *requiredAccess,
bool checkWitnessAccessibility(const DeclContext *&requiredAccessScope,
ValueDecl *requirement,
ValueDecl *witness,
bool *isSetter);
Expand All @@ -77,7 +77,7 @@ namespace {
ValueDecl *witness,
AvailabilityContext *requirementInfo);

RequirementCheck checkWitness(Accessibility requiredAccess,
RequirementCheck checkWitness(const DeclContext *requiredAccessScope,
ValueDecl *requirement,
RequirementMatch match);
};
Expand Down Expand Up @@ -392,24 +392,24 @@ namespace {
struct RequirementCheck {
CheckKind Kind;

/// The required accessibility, if the check failed due to the
/// The required access scope, if the check failed due to the
/// witness being less accessible than the requirement.
Accessibility RequiredAccess;
const DeclContext *RequiredAccessScope;

/// The required availability, if the check failed due to the
/// witness being less available than the requirement.
AvailabilityContext RequiredAvailability;

RequirementCheck(CheckKind kind)
: Kind(kind), RequiredAccess(Accessibility::Public),
: Kind(kind), RequiredAccessScope(nullptr),
RequiredAvailability(AvailabilityContext::alwaysAvailable()) { }

RequirementCheck(CheckKind kind, Accessibility requiredAccess)
: Kind(kind), RequiredAccess(requiredAccess),
RequirementCheck(CheckKind kind, const DeclContext *requiredAccessScope)
: Kind(kind), RequiredAccessScope(requiredAccessScope),
RequiredAvailability(AvailabilityContext::alwaysAvailable()) { }

RequirementCheck(CheckKind kind, AvailabilityContext requiredAvailability)
: Kind(kind), RequiredAccess(Accessibility::Public),
: Kind(kind), RequiredAccessScope(nullptr),
RequiredAvailability(requiredAvailability) { }
};
}
Expand Down Expand Up @@ -1214,48 +1214,35 @@ bool WitnessChecker::findBestWitness(ValueDecl *requirement,
}

bool WitnessChecker::
checkWitnessAccessibility(Accessibility *requiredAccess,
checkWitnessAccessibility(const DeclContext *&requiredAccessScope,
ValueDecl *requirement,
ValueDecl *witness,
bool *isSetter) {
*isSetter = false;

*requiredAccess = std::min(Proto->getFormalAccess(), *requiredAccess);
if (TC.getLangOpts().EnableSwift3Private)
*requiredAccess = std::max(*requiredAccess, Accessibility::FilePrivate);
const DeclContext *protoAccessScope = Proto->getFormalAccessScope(DC);

Accessibility witnessAccess = witness->getFormalAccess(DC);

// Leave a hole for old-style top-level operators to be declared 'private' for
// a fileprivate conformance.
if (witnessAccess == Accessibility::Private &&
witness->getDeclContext()->isModuleScopeContext()) {
witnessAccess = Accessibility::FilePrivate;
// FIXME: This is the same operation as TypeCheckDecl.cpp's
// TypeAccessScopeChecker::intersectAccess.
if (!requiredAccessScope) {
requiredAccessScope = protoAccessScope;
} else if (protoAccessScope) {
if (protoAccessScope->isChildContextOf(requiredAccessScope)) {
requiredAccessScope = protoAccessScope;
} else {
assert(requiredAccessScope == protoAccessScope ||
requiredAccessScope->isChildContextOf(protoAccessScope));
}
}

if (witnessAccess < *requiredAccess)
if (!witness->isAccessibleFrom(requiredAccessScope))
return true;

if (requirement->isSettable(DC)) {
*isSetter = true;

auto ASD = cast<AbstractStorageDecl>(witness);
const DeclContext *accessDC;
switch (*requiredAccess) {
case Accessibility::Open:
case Accessibility::Public:
accessDC = nullptr;
break;
case Accessibility::Internal:
accessDC = DC->getParentModule();
break;
case Accessibility::FilePrivate:
case Accessibility::Private:
accessDC = DC->getModuleScopeContext();
break;
}

if (!ASD->isSetterAccessibleFrom(accessDC))
if (!ASD->isSetterAccessibleFrom(requiredAccessScope))
return true;
}

Expand All @@ -1272,19 +1259,19 @@ checkWitnessAvailability(ValueDecl *requirement,
}

RequirementCheck WitnessChecker::
checkWitness(Accessibility requiredAccess,
checkWitness(const DeclContext *requiredAccessScope,
ValueDecl *requirement,
RequirementMatch match) {
if (!match.OptionalAdjustments.empty())
return CheckKind::OptionalityConflict;

bool isSetter = false;
if (checkWitnessAccessibility(&requiredAccess, requirement,
if (checkWitnessAccessibility(requiredAccessScope, requirement,
match.Witness, &isSetter)) {
CheckKind kind = (isSetter
? CheckKind::AccessibilityOfSetter
: CheckKind::Accessibility);
return RequirementCheck(kind, requiredAccess);
return RequirementCheck(kind, requiredAccessScope);
}

auto requiredAvailability = AvailabilityContext::alwaysAvailable();
Expand Down Expand Up @@ -1910,17 +1897,23 @@ void ConformanceChecker::recordTypeWitness(AssociatedTypeDecl *assocType,

if (typeDecl) {
// Check access.
Accessibility requiredAccess = Adoptee->getAnyNominal()->getFormalAccess();
const DeclContext *requiredAccessScope =
Adoptee->getAnyNominal()->getFormalAccessScope(DC);
bool isSetter = false;
if (checkWitnessAccessibility(&requiredAccess, assocType, typeDecl,
if (checkWitnessAccessibility(requiredAccessScope, assocType, typeDecl,
&isSetter)) {
assert(!isSetter);

// Avoid relying on the lifetime of 'this'.
const DeclContext *DC = this->DC;
diagnoseOrDefer(assocType, false,
[typeDecl, requiredAccess, assocType](
[DC, typeDecl, requiredAccessScope, assocType](
TypeChecker &tc, NormalProtocolConformance *conformance) {
Accessibility requiredAccess =
accessibilityFromScopeForDiagnostics(requiredAccessScope);
auto proto = conformance->getProtocol();
bool protoForcesAccess = (requiredAccess == proto->getFormalAccess());
bool protoForcesAccess =
(requiredAccessScope == proto->getFormalAccessScope(DC));
auto diagKind = protoForcesAccess
? diag::type_witness_not_accessible_proto
: diag::type_witness_not_accessible_type;
Expand Down Expand Up @@ -2010,6 +2003,8 @@ void ConformanceChecker::recordTypeWitness(AssociatedTypeDecl *assocType,
static void diagnoseNoWitness(ValueDecl *Requirement, Type RequirementType,
NormalProtocolConformance *Conformance,
TypeChecker &TC) {
// FIXME: Try an ignore-access lookup?

SourceLoc FixitLocation;
SourceLoc TypeLoc;
if (auto Extension = dyn_cast<ExtensionDecl>(Conformance->getDeclContext())) {
Expand Down Expand Up @@ -2158,21 +2153,27 @@ ConformanceChecker::resolveWitnessViaLookup(ValueDecl *requirement) {
});
}

Accessibility requiredAccess = Adoptee->getAnyNominal()->getFormalAccess();
auto check = checkWitness(requiredAccess, requirement, best);
const DeclContext *nominalAccessScope =
Adoptee->getAnyNominal()->getFormalAccessScope(DC);
auto check = checkWitness(nominalAccessScope, requirement, best);

switch (check.Kind) {
case CheckKind::Success:
break;

case CheckKind::Accessibility:
case CheckKind::AccessibilityOfSetter: {
// Avoid relying on the lifetime of 'this'.
const DeclContext *DC = this->DC;
diagnoseOrDefer(requirement, false,
[witness, check, requirement](
[DC, witness, check, requirement](
TypeChecker &tc, NormalProtocolConformance *conformance) {
Accessibility requiredAccess =
accessibilityFromScopeForDiagnostics(check.RequiredAccessScope);

auto proto = conformance->getProtocol();
bool protoForcesAccess =
(check.RequiredAccess == proto->getFormalAccess());
(check.RequiredAccessScope == proto->getFormalAccessScope(DC));
auto diagKind = protoForcesAccess
? diag::witness_not_accessible_proto
: diag::witness_not_accessible_type;
Expand All @@ -2182,9 +2183,9 @@ ConformanceChecker::resolveWitnessViaLookup(ValueDecl *requirement) {
getRequirementKind(requirement),
witness->getFullName(),
isSetter,
check.RequiredAccess,
requiredAccess,
proto->getName());
fixItAccessibility(diag, witness, check.RequiredAccess, isSetter);
fixItAccessibility(diag, witness, requiredAccess, isSetter);
});
break;
}
Expand Down Expand Up @@ -4978,7 +4979,7 @@ DefaultWitnessChecker::resolveWitnessViaLookup(ValueDecl *requirement) {

// Perform the same checks as conformance witness matching, but silently
// ignore the candidate instead of diagnosing anything.
auto check = checkWitness(Accessibility::Public, requirement, best);
auto check = checkWitness(/*access: public*/nullptr, requirement, best);
if (check.Kind != CheckKind::Success)
return ResolveWitnessResult::ExplicitFailed;

Expand Down
6 changes: 4 additions & 2 deletions test/NameBinding/accessibility.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,9 +96,10 @@ protocol MethodProto {
}

extension OriginallyEmpty : MethodProto {}
extension HiddenMethod : MethodProto {} // expected-error {{type 'HiddenMethod' does not conform to protocol 'MethodProto'}}
// TESTABLE-NOT: :[[@LINE-1]]:{{[^:]+}}:
#if !ACCESS_DISABLED
extension HiddenMethod : MethodProto {} // expected-error {{type 'HiddenMethod' does not conform to protocol 'MethodProto'}}

extension Foo : MethodProto {} // expected-error {{type 'Foo' does not conform to protocol 'MethodProto'}}
#endif

Expand All @@ -108,9 +109,10 @@ protocol TypeProto {
}

extension OriginallyEmpty {}
#if !ACCESS_DISABLED
extension HiddenType : TypeProto {} // expected-error {{type 'HiddenType' does not conform to protocol 'TypeProto'}}
// TESTABLE-NOT: :[[@LINE-1]]:{{[^:]+}}:
#if !ACCESS_DISABLED

extension Foo : TypeProto {} // expected-error {{type 'Foo' does not conform to protocol 'TypeProto'}}
#endif

Expand Down
Loading