Skip to content

[ConstraintSystem] Detect and fix use of static members in a key path in the solver #24052

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.

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 16, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 2 additions & 4 deletions lib/Sema/CSApply.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4572,10 +4572,8 @@ namespace {
}

// Key paths don't currently support static members.
if (varDecl->isStatic()) {
cs.TC.diagnose(componentLoc, diag::expr_keypath_static_member,
property->getFullName());
}
// There is a fix which diagnoses such situation already.
assert(!varDecl->isStatic());
}

cs.TC.requestMemberLayout(property);
Expand Down
57 changes: 44 additions & 13 deletions lib/Sema/CSDiagnostics.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1748,12 +1748,16 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {

Expr *expr = getParentExpr();
SourceRange baseRange = expr ? expr->getSourceRange() : SourceRange();
auto resolvedOverloadChoice = getResolvedOverload(locator)->Choice;

auto overload = getOverloadChoiceIfAvailable(locator);
if (!overload)
return false;

ValueDecl *decl = nullptr;

if (!resolvedOverloadChoice.isDecl()) {
if (auto MT = resolvedOverloadChoice.getBaseType()->getAs<MetatypeType>()) {
if (!overload->choice.isDecl()) {
auto baseTy = overload->choice.getBaseType();
if (auto MT = baseTy->getAs<MetatypeType>()) {
if (auto VD = dyn_cast<ValueDecl>(
MT->getMetatypeInstanceType()->getAnyNominal()->getAsDecl())) {
decl = VD;
Expand All @@ -1763,7 +1767,7 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {
}
}

auto member = decl ? decl : resolvedOverloadChoice.getDecl();
auto member = decl ? decl : overload->choice.getDecl();

// If the base is an implicit self type reference, and we're in a
// an initializer, then the user wrote something like:
Expand Down Expand Up @@ -1933,16 +1937,24 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {

return true;
}

if (isa<EnumElementDecl>(member)) {
Diag.emplace(emitDiagnostic(loc, diag::could_not_use_enum_element_on_instance,
Name));

// If this is a reference to a static member by one of the key path
// components, let's provide a tailored diagnostic and return because
// that is unsupported so there is no fix-it.
if (locator->isForKeyPathComponent()) {
InvalidStaticMemberRefInKeyPath failure(expr, getConstraintSystem(),
member, locator);
return failure.diagnoseAsError();
}
else {
Diag.emplace(emitDiagnostic(loc, diag::could_not_use_type_member_on_instance,
baseObjTy, Name));

if (isa<EnumElementDecl>(member)) {
Diag.emplace(emitDiagnostic(
loc, diag::could_not_use_enum_element_on_instance, Name));
} else {
Diag.emplace(emitDiagnostic(
loc, diag::could_not_use_type_member_on_instance, baseObjTy, Name));
}

Diag->highlight(getAnchor()->getSourceRange());

if (Name.isSimpleName(DeclBaseName::createConstructor()) &&
Expand Down Expand Up @@ -2002,7 +2014,7 @@ bool AllowTypeOrInstanceMemberFailure::diagnoseAsError() {
}
}
}

// Fall back to a fix-it with a full type qualifier
auto nominal = member->getDeclContext()->getSelfNominalTypeDecl();
SmallString<32> typeName;
Expand Down Expand Up @@ -2436,3 +2448,22 @@ bool KeyPathSubscriptIndexHashableFailure::diagnoseAsError() {
resolveType(NonConformingType));
return true;
}

bool InvalidStaticMemberRefInKeyPath::diagnoseAsError() {
auto *anchor = getRawAnchor();
auto loc = anchor->getLoc();

if (auto *KPE = dyn_cast<KeyPathExpr>(anchor)) {
auto *locator = getLocator();
auto component =
llvm::find_if(locator->getPath(), [](const LocatorPathElt &elt) {
return elt.isKeyPathComponent();
});

assert(component != locator->getPath().end());
loc = KPE->getComponents()[component->getValue()].getLoc();
}

emitDiagnostic(loc, diag::expr_keypath_static_member, Member->getBaseName());
return true;
}
24 changes: 24 additions & 0 deletions lib/Sema/CSDiagnostics.h
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,30 @@ class KeyPathSubscriptIndexHashableFailure final : public FailureDiagnostic {
bool diagnoseAsError() override;
};

/// Diagnose an attempt to reference a static member as a key path component
/// e.g.
///
/// ```swift
/// struct S {
/// static var foo: Int = 42
/// }
///
/// _ = \S.Type.foo
/// ```
class InvalidStaticMemberRefInKeyPath final : public FailureDiagnostic {
ValueDecl *Member;

public:
InvalidStaticMemberRefInKeyPath(Expr *root, ConstraintSystem &cs,
ValueDecl *member, ConstraintLocator *locator)
: FailureDiagnostic(root, cs, locator), Member(member) {
assert(member->hasName());
assert(locator->isForKeyPathComponent());
}

bool diagnoseAsError() override;
};

} // end namespace constraints
} // end namespace swift

Expand Down
13 changes: 13 additions & 0 deletions lib/Sema/CSFix.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -422,3 +422,16 @@ TreatKeyPathSubscriptIndexAsHashable::create(ConstraintSystem &cs, Type type,
return new (cs.getAllocator())
TreatKeyPathSubscriptIndexAsHashable(cs, type, locator);
}

bool AllowStaticMemberRefInKeyPath::diagnose(Expr *root, bool asNote) const {
InvalidStaticMemberRefInKeyPath failure(root, getConstraintSystem(), Member,
getLocator());
return failure.diagnose(asNote);
}

AllowStaticMemberRefInKeyPath *
AllowStaticMemberRefInKeyPath::create(ConstraintSystem &cs, ValueDecl *member,
ConstraintLocator *locator) {
return new (cs.getAllocator())
AllowStaticMemberRefInKeyPath(cs, member, locator);
}
24 changes: 23 additions & 1 deletion lib/Sema/CSFix.h
Original file line number Diff line number Diff line change
Expand Up @@ -139,10 +139,13 @@ enum class FixKind : uint8_t {

/// Allow KeyPaths to use AnyObject as root type
AllowAnyObjectKeyPathRoot,

/// Using subscript references in the keypath requires that each
/// of the index arguments to be Hashable.
TreatKeyPathSubscriptIndexAsHashable,

/// Allow a reference to a static member as a key path component.
AllowStaticMemberRefInKeyPath,
};

class ConstraintFix {
Expand Down Expand Up @@ -780,6 +783,25 @@ class TreatKeyPathSubscriptIndexAsHashable final : public ConstraintFix {
create(ConstraintSystem &cs, Type type, ConstraintLocator *locator);
};

class AllowStaticMemberRefInKeyPath final : public ConstraintFix {
ValueDecl *Member;

AllowStaticMemberRefInKeyPath(ConstraintSystem &cs, ValueDecl *member,
ConstraintLocator *locator)
: ConstraintFix(cs, FixKind::AllowStaticMemberRefInKeyPath, locator),
Member(member) {}

public:
std::string getName() const override {
return "allow reference to a static member as a key path component";
}

bool diagnose(Expr *root, bool asNote = false) const override;

static AllowStaticMemberRefInKeyPath *
create(ConstraintSystem &cs, ValueDecl *member, ConstraintLocator *locator);
};

} // end namespace constraints
} // end namespace swift

Expand Down
16 changes: 16 additions & 0 deletions lib/Sema/CSSimplify.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5015,11 +5015,26 @@ ConstraintSystem::simplifyKeyPathConstraint(Type keyPathTy,
if (!choices[i].isDecl()) {
return SolutionKind::Error;
}

auto storage = dyn_cast<AbstractStorageDecl>(choices[i].getDecl());
if (!storage) {
return SolutionKind::Error;
}

// Referencing static members in key path is not currently allowed.
if (storage->isStatic()) {
if (!shouldAttemptFixes())
return SolutionKind::Error;

auto componentLoc =
locator.withPathElement(LocatorPathElt::getKeyPathComponent(i));
auto *fix = AllowStaticMemberRefInKeyPath::create(
*this, choices[i].getDecl(), getConstraintLocator(componentLoc));

if (recordFix(fix))
return SolutionKind::Error;
}

if (isReadOnlyKeyPathComponent(storage)) {
capability = ReadOnly;
continue;
Expand Down Expand Up @@ -6314,6 +6329,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyFixConstraint(
case FixKind::AllowInaccessibleMember:
case FixKind::AllowAnyObjectKeyPathRoot:
case FixKind::TreatKeyPathSubscriptIndexAsHashable:
case FixKind::AllowStaticMemberRefInKeyPath:
llvm_unreachable("handled elsewhere");
}

Expand Down
6 changes: 6 additions & 0 deletions lib/Sema/ConstraintLocator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,12 @@ bool ConstraintLocator::isKeyPathSubscriptComponent() const {
});
}

bool ConstraintLocator::isForKeyPathComponent() const {
return llvm::any_of(getPath(), [&](const LocatorPathElt &elt) {
return elt.isKeyPathComponent();
});
}

void ConstraintLocator::dump(SourceManager *sm) {
dump(sm, llvm::errs());
llvm::errs() << "\n";
Expand Down
6 changes: 5 additions & 1 deletion lib/Sema/ConstraintLocator.h
Original file line number Diff line number Diff line change
Expand Up @@ -540,10 +540,14 @@ class ConstraintLocator : public llvm::FoldingSetNode {
/// as result of the key path dynamic member lookup operation.
bool isResultOfKeyPathDynamicMemberLookup() const;

/// Determine whether given locator points to a subscript component
/// Determine whether this locator points to a subscript component
/// of the key path at some index.
bool isKeyPathSubscriptComponent() const;

/// Determine whether this locator points to one of the key path
/// components.
bool isForKeyPathComponent() const;

/// Produce a profile of this locator, for use in a folding set.
static void Profile(llvm::FoldingSetNodeID &id, Expr *anchor,
ArrayRef<PathElement> path);
Expand Down
2 changes: 1 addition & 1 deletion test/SILOptimizer/access_wmo_diagnose.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ public class C {
}

public func testGlobalProp() {
let a: AnyKeyPath = \C.globalProp // expected-error{{static member 'globalProp' cannot be used on instance of type 'C'}}
let a: AnyKeyPath = \C.globalProp // expected-error{{key path cannot refer to static member 'globalProp'}}
}
44 changes: 42 additions & 2 deletions test/expr/unary/keypath/keypath.swift
Original file line number Diff line number Diff line change
Expand Up @@ -514,9 +514,9 @@ class X {
}

func testStaticKeyPathComponent() {
_ = \X.a // expected-error{{}}
_ = \X.a // expected-error{{cannot refer to static member}}
_ = \X.Type.a // expected-error{{cannot refer to static member}}
_ = \X.b // expected-error{{}}
_ = \X.b // expected-error{{cannot refer to static member}}
_ = \X.Type.b // expected-error{{cannot refer to static member}}
}

Expand Down Expand Up @@ -708,6 +708,46 @@ var identity11: AnyKeyPath = \Container.self

var interleavedIdentityComponents = \Container.self.base.self?.self.i.self

protocol P_With_Static_Members {
static var x: Int { get }
static var arr: [Int] { get }
}

func test_keypath_with_static_members(_ p: P_With_Static_Members) {
let _ = p[keyPath: \.x]
// expected-error@-1 {{key path cannot refer to static member 'x'}}
let _: KeyPath<P_With_Static_Members, Int> = \.x
// expected-error@-1 {{key path cannot refer to static member 'x'}}
let _ = \P_With_Static_Members.arr.count
// expected-error@-1 {{key path cannot refer to static member 'arr'}}
let _ = p[keyPath: \.arr.count]
// expected-error@-1 {{key path cannot refer to static member 'arr'}}

struct S {
static var foo: String = "Hello"
var bar: Bar
}

struct Bar {
static var baz: Int = 42
}

func foo(_ s: S) {
let _ = \S.Type.foo
// expected-error@-1 {{key path cannot refer to static member 'foo'}}
let _ = s[keyPath: \.foo]
// expected-error@-1 {{key path cannot refer to static member 'foo'}}
let _: KeyPath<S, String> = \.foo
// expected-error@-1 {{key path cannot refer to static member 'foo'}}
let _ = \S.foo
// expected-error@-1 {{key path cannot refer to static member 'foo'}}
let _ = \S.bar.baz
// expected-error@-1 {{key path cannot refer to static member 'baz'}}
let _ = s[keyPath: \.bar.baz]
// expected-error@-1 {{key path cannot refer to static member 'baz'}}
}
}

func testSyntaxErrors() { // expected-note{{}}
_ = \. ; // expected-error{{expected member name following '.'}}
_ = \.a ;
Expand Down