Skip to content

[Diagnostics] Adding assignments directly to CS and diagnosing from there. #18950

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
Aug 26, 2018
Merged

[Diagnostics] Adding assignments directly to CS and diagnosing from there. #18950

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
821f63f
Make assignments and assignment failure diagnoses directly in the CS.
gregomni Aug 24, 2018
31ca8db
Move all shouldRecordFix logic back into CS itself keying off of FixK…
gregomni Aug 25, 2018
6d5a69c
Remove excess namespacing
gregomni Aug 25, 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
11 changes: 2 additions & 9 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3832,18 +3832,11 @@ namespace {
}

Expr *visitAssignExpr(AssignExpr *expr) {
// Compute the type to which the source must be converted to allow
// assignment to the destination.
//
// FIXME: This is also computed when the constraint system is set up.
auto destTy = cs.computeAssignDestType(expr->getDest(), expr->getLoc());
if (!destTy)
return nullptr;

// Convert the source to the simplified destination type.
auto destTy = simplifyType(cs.getType(expr->getDest()));
auto locator =
ConstraintLocatorBuilder(cs.getConstraintLocator(expr->getSrc()));
Expr *src = coerceToType(expr->getSrc(), destTy, locator);
Expr *src = coerceToType(expr->getSrc(), destTy->getRValueType(), locator);
if (!src)
return nullptr;

Expand Down
82 changes: 47 additions & 35 deletions lib/Sema/CSDiag.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -689,6 +689,17 @@ resolveImmutableBase(Expr *expr, ConstraintSystem &CS) {
return { expr, member };
}

if (auto tupleExpr = dyn_cast<TupleExpr>(SE->getIndex())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there no better way to check this, @jckarter ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Have SubscriptDecls been sufficiently parameter-list-ified that we can look at the argument labels and parameter list instead? cc @slavapestov

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but here we're looking at the arguments to the call, not the subscript declaration.

if (tupleExpr->getNumElements() == 1 && tupleExpr->getElementName(0).str() == "keyPath") {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would at least compare Identifiers and not strings, so tupleExpr->getElementName(0) == ctx.getIdentifier("keyPath").

However I imagine this check ("is this a subscript that's really a keypath application") must appear elsewhere in the compiler. Can we extract it into a common method?

auto indexType = CS.simplifyType(CS.getType(tupleExpr->getElement(0)));
if (auto bgt = indexType->getAs<BoundGenericType>()) {
auto decl = bgt->getDecl();
if (decl == CS.getASTContext().getKeyPathDecl())
return resolveImmutableBase(tupleExpr->getElement(0), CS);
}
}
}

// If it is settable, then the base must be the problem, recurse.
return resolveImmutableBase(SE->getBase(), CS);
}
Expand Down Expand Up @@ -744,6 +755,9 @@ resolveImmutableBase(Expr *expr, ConstraintSystem &CS) {
if (!isa<LoadExpr>(ICE->getSubExpr()))
return resolveImmutableBase(ICE->getSubExpr(), CS);

if (auto *SAE = dyn_cast<SelfApplyExpr>(expr))
return resolveImmutableBase(SAE->getFn(), CS);

return { expr, nullptr };
}

Expand Down Expand Up @@ -840,7 +854,11 @@ void swift::diagnoseSubElementFailure(Expr *destExpr,
message += VD->getName().str().str();
message += "'";

if (VD->isCaptureList())
auto type = CS.simplifyType(CS.getType(immInfo.first));
auto bgt = type ? type->getAs<BoundGenericType>() : nullptr;
if (bgt && bgt->getDecl() == CS.getASTContext().getKeyPathDecl())
message += " is a read-only key path";
else if (VD->isCaptureList())
message += " is an immutable capture";
else if (VD->isImplicit())
message += " is immutable";
Expand Down Expand Up @@ -898,6 +916,14 @@ void swift::diagnoseSubElementFailure(Expr *destExpr,
return;
}

// If a keypath was the problem but wasn't resolved into a vardecl
// it is ambiguous or unable to be used for setting.
if (auto *KPE = dyn_cast_or_null<KeyPathExpr>(immInfo.first)) {
TC.diagnose(loc, diagID, "immutable key path")
.highlight(KPE->getSourceRange());
return;
}

// If the expression is the result of a call, it is an rvalue, not a mutable
// lvalue.
if (auto *AE = dyn_cast<ApplyExpr>(immInfo.first)) {
Expand Down Expand Up @@ -3401,44 +3427,30 @@ bool FailureDiagnosis::diagnoseContextualConversionError(

/// When an assignment to an expression is detected and the destination is
/// invalid, emit a detailed error about the condition.
void ConstraintSystem::diagnoseAssignmentFailure(Expr *dest, Type destTy,
bool ConstraintSystem::diagnoseAssignmentFailure(Expr *dest, Type destTy,
SourceLoc equalLoc) {
auto &TC = getTypeChecker();

// Diagnose obvious assignments to literals.
if (isa<LiteralExpr>(dest->getValueProvidingExpr())) {
TC.diagnose(equalLoc, diag::cannot_assign_to_literal);
return;
}

// Diagnose assignments to let-properties in delegating initializers.
if (auto *member = dyn_cast<UnresolvedDotExpr>(dest)) {
if (auto *ctor = dyn_cast<ConstructorDecl>(DC)) {
if (auto *baseRef = dyn_cast<DeclRefExpr>(member->getBase())) {
if (baseRef->getDecl() == ctor->getImplicitSelfDecl() &&
ctor->getDelegatingOrChainedInitKind(nullptr) ==
ConstructorDecl::BodyInitKind::Delegating) {
auto resolved = resolveImmutableBase(member, *this);
assert(resolved.first == member);
TC.diagnose(equalLoc, diag::assignment_let_property_delegating_init,
member->getName());
if (resolved.second) {
TC.diagnose(resolved.second, diag::decl_declared_here,
member->getName());
}
return;
}
}
// Assignments to let-properties in delegating initializers will be caught
// elsewhere now, so if we see them here, it isn't an assignment problem.
if (auto *ctor = dyn_cast<ConstructorDecl>(DC)) {
DeclRefExpr *baseRef = nullptr;
if (auto *member = dyn_cast<UnresolvedDotExpr>(dest)) {
baseRef = dyn_cast<DeclRefExpr>(member->getBase());
} else if (auto *member = dyn_cast<MemberRefExpr>(dest)) {
if (auto *load = dyn_cast<LoadExpr>(member->getBase()))
baseRef = dyn_cast<DeclRefExpr>(load->getSubExpr());
}
if (baseRef && baseRef->getDecl() == ctor->getImplicitSelfDecl() &&
ctor->getDelegatingOrChainedInitKind(nullptr) ==
ConstructorDecl::BodyInitKind::Delegating) {
return false;
}
}

Diag<StringRef> diagID;
if (isa<ApplyExpr>(dest))
if (isa<ApplyExpr>(dest) || isa<SelfApplyExpr>(dest))
diagID = diag::assignment_lhs_is_apply_expression;
else if (isa<DeclRefExpr>(dest))
diagID = diag::assignment_lhs_is_immutable_variable;
else if (isa<ForceValueExpr>(dest))
diagID = diag::assignment_bang_has_immutable_subcomponent;
else if (isa<UnresolvedDotExpr>(dest) || isa<MemberRefExpr>(dest))
diagID = diag::assignment_lhs_is_immutable_property;
else if (auto sub = dyn_cast<SubscriptExpr>(dest)) {
Expand All @@ -3451,12 +3463,12 @@ void ConstraintSystem::diagnoseAssignmentFailure(Expr *dest, Type destTy,
sub->getArgumentLabels().size() == 1 &&
sub->getArgumentLabels().front() == TC.Context.Id_dynamicMember)
diagID = diag::assignment_dynamic_property_has_immutable_base;
} else {
} else
diagID = diag::assignment_lhs_is_immutable_variable;
}

diagnoseSubElementFailure(dest, equalLoc, *this, diagID,
diag::assignment_lhs_not_lvalue);
return true;
}

//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -6545,8 +6557,8 @@ bool FailureDiagnosis::visitAssignExpr(AssignExpr *assignExpr) {
if (diagnoseSubscriptErrors(subscriptExpr, /* inAssignmentDestination = */ true))
return true;
}
CS.diagnoseAssignmentFailure(destExpr, destType, assignExpr->getLoc());
return true;
if (CS.diagnoseAssignmentFailure(destExpr, destType, assignExpr->getLoc()))
return true;
}

auto *srcExpr = assignExpr->getSrc();
Expand Down
62 changes: 54 additions & 8 deletions lib/Sema/CSDiagnostics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,10 @@ bool MissingForcedDowncastFailure::diagnoseAsError() {

auto &TC = getTypeChecker();

auto *coerceExpr = dyn_cast<CoerceExpr>(getAnchor());
auto *expr = getAnchor();
if (auto *assignExpr = dyn_cast<AssignExpr>(expr))
expr = assignExpr->getSrc();
auto *coerceExpr = dyn_cast<CoerceExpr>(expr);
if (!coerceExpr)
return false;

Expand Down Expand Up @@ -331,6 +334,8 @@ bool MissingExplicitConversionFailure::diagnoseAsError() {
auto &TC = getTypeChecker();

auto *anchor = getAnchor();
if (auto *assign = dyn_cast<AssignExpr>(anchor))
anchor = assign->getSrc();
if (auto *paren = dyn_cast<ParenExpr>(anchor))
anchor = paren->getSubExpr();

Expand Down Expand Up @@ -538,6 +543,10 @@ bool MissingOptionalUnwrapFailure::diagnoseAsError() {
return false;

auto *anchor = getAnchor();

if (auto assignExpr = dyn_cast<AssignExpr>(anchor))
anchor = assignExpr->getSrc();

auto *unwrapped = anchor->getValueProvidingExpr();
auto type = getType(anchor)->getRValueType();

Expand All @@ -552,9 +561,13 @@ bool MissingOptionalUnwrapFailure::diagnoseAsError() {

bool RValueTreatedAsLValueFailure::diagnoseAsError() {
Diag<StringRef> subElementDiagID;
Diag<Type> rvalueDiagID;
Diag<Type> rvalueDiagID = diag::assignment_lhs_not_lvalue;
Expr *diagExpr = getLocator()->getAnchor();
SourceLoc loc;
SourceLoc loc = diagExpr->getLoc();

if (auto assignExpr = dyn_cast<AssignExpr>(diagExpr)) {
diagExpr = assignExpr->getDest();
}

if (auto callExpr = dyn_cast<ApplyExpr>(diagExpr)) {
Expr *argExpr = callExpr->getArg();
Expand All @@ -569,7 +582,7 @@ bool RValueTreatedAsLValueFailure::diagnoseAsError() {
rvalueDiagID = diag::cannot_apply_lvalue_binop_to_rvalue;
auto argTuple = dyn_cast<TupleExpr>(argExpr);
diagExpr = argTuple->getElement(0);
} else {
} else if (getLocator()->getPath().size() > 0) {
auto lastPathElement = getLocator()->getPath().back();
assert(lastPathElement.getKind() ==
ConstraintLocator::PathElementKind::ApplyArgToParam);
Expand All @@ -580,10 +593,11 @@ bool RValueTreatedAsLValueFailure::diagnoseAsError() {
diagExpr = argTuple->getElement(lastPathElement.getValue());
else if (auto parens = dyn_cast<ParenExpr>(argExpr))
diagExpr = parens->getSubExpr();
} else {
subElementDiagID = diag::assignment_lhs_is_apply_expression;
}
} else if (auto inoutExpr = dyn_cast<InOutExpr>(diagExpr)) {
Type type = getConstraintSystem().getType(inoutExpr);
if (auto restriction = restrictionForType(type)) {
if (auto restriction = getRestrictionForType(getType(inoutExpr))) {
PointerTypeKind pointerKind;
if (restriction->second == ConversionRestrictionKind::ArrayToPointer &&
restriction->first->getAnyPointerElementType(pointerKind) &&
Expand All @@ -601,10 +615,42 @@ bool RValueTreatedAsLValueFailure::diagnoseAsError() {

subElementDiagID = diag::cannot_pass_rvalue_inout_subelement;
rvalueDiagID = diag::cannot_pass_rvalue_inout;
loc = diagExpr->getLoc();
diagExpr = inoutExpr->getSubExpr();
} else if (isa<DeclRefExpr>(diagExpr)) {
subElementDiagID = diag::assignment_lhs_is_immutable_variable;
} else if (isa<ForceValueExpr>(diagExpr)) {
subElementDiagID = diag::assignment_bang_has_immutable_subcomponent;
} else if (isa<MemberRefExpr>(diagExpr)) {
subElementDiagID = diag::assignment_lhs_is_immutable_property;
} else if (auto member = dyn_cast<UnresolvedDotExpr>(diagExpr)) {
subElementDiagID = diag::assignment_lhs_is_immutable_property;

if (auto *ctor = dyn_cast<ConstructorDecl>(getDC())) {
if (auto *baseRef = dyn_cast<DeclRefExpr>(member->getBase())) {
if (baseRef->getDecl() == ctor->getImplicitSelfDecl() &&
ctor->getDelegatingOrChainedInitKind(nullptr) ==
ConstructorDecl::BodyInitKind::Delegating) {
emitDiagnostic(loc, diag::assignment_let_property_delegating_init,
member->getName());
if (auto *ref = getResolvedMemberRef(member)) {
emitDiagnostic(ref, diag::decl_declared_here, member->getName());
}
return true;
}
}
}
} else if (auto sub = dyn_cast<SubscriptExpr>(diagExpr)) {
subElementDiagID = diag::assignment_subscript_has_immutable_base;

// If the destination is a subscript with a 'dynamicLookup:' label and if
// the tuple is implicit, then this was actually a @dynamicMemberLookup
// access. Emit a more specific diagnostic.
if (sub->getIndex()->isImplicit() &&
sub->getArgumentLabels().size() == 1 &&
sub->getArgumentLabels().front() == getTypeChecker().Context.Id_dynamicMember)
subElementDiagID = diag::assignment_dynamic_property_has_immutable_base;
} else {
return false;
subElementDiagID = diag::assignment_lhs_is_immutable_variable;
}

diagnoseSubElementFailure(diagExpr, loc, getConstraintSystem(),
Expand Down
7 changes: 6 additions & 1 deletion lib/Sema/CSDiagnostics.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ class FailureDiagnostic {
DeclContext *getDC() const { return CS.DC; }

Optional<std::pair<Type, ConversionRestrictionKind>>
restrictionForType(Type type) const {
getRestrictionForType(Type type) const {
for (auto &restriction : CS.ConstraintRestrictions) {
if (std::get<0>(restriction)->isEqual(type))
return std::pair<Type, ConversionRestrictionKind>(
Expand All @@ -104,6 +104,11 @@ class FailureDiagnostic {
return None;
}

ValueDecl *getResolvedMemberRef(UnresolvedDotExpr *member) {
auto locator = CS.getConstraintLocator(member, ConstraintLocator::Member);
return CS.findResolvedMemberRef(locator);
}

Optional<SelectedOverload>
getOverloadChoiceIfAvailable(ConstraintLocator *locator) const {
if (auto *overload = getResolvedOverload(locator))
Expand Down
36 changes: 20 additions & 16 deletions lib/Sema/CSGen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2745,26 +2745,30 @@ namespace {
auto typeVar = CS.createTypeVariable(locator);
return LValueType::get(typeVar);
}


static Type genAssignDestType(Expr *expr, ConstraintSystem &CS) {
if (auto *TE = dyn_cast<TupleExpr>(expr)) {
SmallVector<TupleTypeElt, 4> destTupleTypes;
for (unsigned i = 0; i != TE->getNumElements(); ++i) {
Type subType = genAssignDestType(TE->getElement(i), CS);
destTupleTypes.push_back(TupleTypeElt(subType, TE->getElementName(i)));
}
return TupleType::get(destTupleTypes, CS.getASTContext());
} else {
Type destTy = CS.createTypeVariable(CS.getConstraintLocator(expr));
CS.addConstraint(ConstraintKind::Bind, CS.getType(expr), LValueType::get(destTy),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - I think this should be reversed - LValueType::get(destType) bind CS.getType(expr) because we'd rather always bind to something we are going to return (which also happens to be a type variable always).

CS.getConstraintLocator(expr));
return destTy;
}
}

Type visitAssignExpr(AssignExpr *expr) {
// Handle invalid code.
if (!expr->getDest() || !expr->getSrc())
return Type();

// Compute the type to which the source must be converted to allow
// assignment to the destination.
auto destTy = CS.computeAssignDestType(expr->getDest(), expr->getLoc());
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to computeAssignDestType()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After I kill the other caller in CSApply, which shouldn't need it any more because any conversion restrictions should already be in the CS, then it gets deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sweet

if (!destTy)
return Type();
if (destTy->is<UnresolvedType>()) {
return CS.createTypeVariable(CS.getConstraintLocator(expr));
}

// The source must be convertible to the destination.
CS.addConstraint(ConstraintKind::Conversion,
CS.getType(expr->getSrc()), destTy,
CS.getConstraintLocator(expr->getSrc()));

Type destTy = genAssignDestType(expr->getDest(), CS);
CS.addConstraint(ConstraintKind::Conversion, CS.getType(expr->getSrc()), destTy,
CS.getConstraintLocator(expr));
return TupleType::getEmpty(CS.getASTContext());
}

Expand Down
Loading