Skip to content

Commit 44088fa

Browse files
DougGregorDougGregor
authored
Merge pull request #17921 from DougGregor/optional-not-unwrapped-diags
[Type checker] Improve diagnostics when an optional value is not unwrapped
2 parents 07a8cf3 + 5db1901 commit 44088fa

31 files changed

+323
-116
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -902,10 +902,21 @@ NOTE(note_init_parameter,none,
902902
ERROR(missing_nullary_call,none,
903903
"function produces expected type %0; did you mean to call it with '()'?",
904904
(Type))
905-
ERROR(missing_unwrap_optional,none,
906-
"value of optional type %0 not unwrapped; did you mean to use '!' "
907-
"or '?'?",
908-
(Type))
905+
ERROR(optional_not_unwrapped,none,
906+
"value of optional type %0 must be unwrapped to a value of type %1",
907+
(Type, Type))
908+
NOTE(unwrap_with_default_value,none,
909+
"coalesce using '?" "?' to provide a default when the optional value "
910+
"contains 'nil'", ())
911+
NOTE(unwrap_with_force_value,none,
912+
"force-unwrap using '!' to abort execution if the optional value contains "
913+
"'nil'", ())
914+
ERROR(optional_base_not_unwrapped,none,
915+
"value of optional type %0 must be unwrapped to refer to member %1 of "
916+
"wrapped base type %2", (Type, DeclName, Type))
917+
NOTE(optional_base_chain,none,
918+
"chain the optional using '?' to access member %0 only for non-'nil' "
919+
"base values", (DeclName))
909920
ERROR(missing_unwrap_optional_try,none,
910921
"value of optional type %0 not unwrapped; did you mean to use 'try!' "
911922
"or chain with '?'?",

‎include/swift/AST/KnownIdentifiers.def

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@ IDENTIFIER(AssignmentPrecedence)
135135
IDENTIFIER(CastingPrecedence)
136136
IDENTIFIER(DefaultPrecedence)
137137
IDENTIFIER(FunctionArrowPrecedence)
138+
IDENTIFIER(NilCoalescingPrecedence)
138139
IDENTIFIER(TernaryPrecedence)
139140

140141
// Builtins and literals

‎lib/Sema/CSApply.cpp

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -7794,6 +7794,27 @@ static bool exprNeedsParensAfterAddingAs(TypeChecker &TC, DeclContext *DC,
77947794
return exprNeedsParensOutsideFollowingOperator(TC, DC, expr, rootExpr, asPG);
77957795
}
77967796

7797+
bool swift::exprNeedsParensBeforeAddingNilCoalescing(TypeChecker &TC,
7798+
DeclContext *DC,
7799+
Expr *expr) {
7800+
auto asPG =
7801+
TC.lookupPrecedenceGroup(DC, DC->getASTContext().Id_NilCoalescingPrecedence,
7802+
SourceLoc());
7803+
if (!asPG) return true;
7804+
return exprNeedsParensInsideFollowingOperator(TC, DC, expr, asPG);
7805+
}
7806+
7807+
bool swift::exprNeedsParensAfterAddingNilCoalescing(TypeChecker &TC,
7808+
DeclContext *DC,
7809+
Expr *expr,
7810+
Expr *rootExpr) {
7811+
auto asPG =
7812+
TC.lookupPrecedenceGroup(DC, DC->getASTContext().Id_NilCoalescingPrecedence,
7813+
SourceLoc());
7814+
if (!asPG) return true;
7815+
return exprNeedsParensOutsideFollowingOperator(TC, DC, expr, rootExpr, asPG);
7816+
}
7817+
77977818
namespace {
77987819
class ExprWalker : public ASTWalker {
77997820
ExprRewriter &Rewriter;
@@ -7955,7 +7976,7 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
79557976

79567977
switch (fix.first.getKind()) {
79577978
case FixKind::ForceOptional: {
7958-
const Expr *unwrapped = affected->getValueProvidingExpr();
7979+
Expr *unwrapped = affected->getValueProvidingExpr();
79597980
auto type = solution.simplifyType(getType(affected))
79607981
->getRValueObjectType();
79617982

@@ -7966,25 +7987,17 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
79667987
"try!");
79677988

79687989
} else {
7969-
auto diag = TC.diagnose(affected->getLoc(),
7970-
diag::missing_unwrap_optional, type);
7971-
if (affected->canAppendPostfixExpression(true)) {
7972-
diag.fixItInsertAfter(affected->getEndLoc(), "!");
7973-
} else {
7974-
diag.fixItInsert(affected->getStartLoc(), "(")
7975-
.fixItInsertAfter(affected->getEndLoc(), ")!");
7976-
}
7990+
return diagnoseUnwrap(TC, DC, unwrapped, type);
79777991
}
79787992
return true;
79797993
}
79807994

7981-
case FixKind::OptionalChaining: {
7995+
case FixKind::UnwrapOptionalBase: {
79827996
auto type = solution.simplifyType(getType(affected))
79837997
->getRValueObjectType();
7984-
auto diag = TC.diagnose(affected->getLoc(),
7985-
diag::missing_unwrap_optional, type);
7986-
diag.fixItInsertAfter(affected->getEndLoc(), "?");
7987-
return true;
7998+
DeclName memberName = fix.first.getDeclNameArgument(*this);
7999+
return diagnoseBaseUnwrapForMemberAccess(affected, type, memberName,
8000+
SourceRange());
79888001
}
79898002

79908003
case FixKind::ForceDowncast: {

‎lib/Sema/CSDiag.cpp

Lines changed: 76 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2419,15 +2419,6 @@ Expr *FailureDiagnosis::typeCheckChildIndependently(
24192419
allowFreeTypeVariables)
24202420
TCEOptions |= TypeCheckExprFlags::AllowUnresolvedTypeVariables;
24212421

2422-
// If we're not passing down contextual type information this time, but the
2423-
// original failure had type info that wasn't an optional type,
2424-
// then set the flag to prefer fixits with force unwrapping.
2425-
if (!convertType) {
2426-
auto previousType = CS.getContextualType();
2427-
if (previousType && previousType->getOptionalObjectType().isNull())
2428-
TCEOptions |= TypeCheckExprFlags::PreferForceUnwrapToOptional;
2429-
}
2430-
24312422
auto resultTy = CS.TC.typeCheckExpression(
24322423
subExpr, CS.DC, TypeLoc::withoutLoc(convertType), convertTypePurpose,
24332424
TCEOptions, listener, &CS);
@@ -8067,15 +8058,9 @@ bool FailureDiagnosis::diagnoseMemberFailures(
80678058
}
80688059

80698060
if (!optionalResult.ViableCandidates.empty()) {
8070-
// By default we assume that the LHS type is not optional.
8071-
StringRef fixIt = "!";
8072-
auto contextualType = CS.getContextualType();
8073-
if (contextualType && isa<OptionalType>(contextualType.getPointer()))
8074-
fixIt = "?";
8075-
8076-
diagnose(BaseLoc, diag::missing_unwrap_optional, baseObjTy)
8077-
.fixItInsertAfter(baseExpr->getEndLoc(), fixIt);
8078-
return true;
8061+
if (diagnoseBaseUnwrapForMemberAccess(baseExpr, baseObjTy, memberName,
8062+
memberRange))
8063+
return true;
80798064
}
80808065
}
80818066

@@ -8982,3 +8967,76 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
89828967
diagnoseFailureForExpr(expr);
89838968
return true;
89848969
}
8970+
8971+
bool swift::diagnoseUnwrap(TypeChecker &TC, DeclContext *DC,
8972+
Expr *expr, Type type) {
8973+
Type unwrappedType = type->getOptionalObjectType();
8974+
if (!unwrappedType)
8975+
return false;
8976+
8977+
TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, type,
8978+
unwrappedType);
8979+
8980+
// Suggest a default value via ?? <default value>
8981+
{
8982+
auto diag =
8983+
TC.diagnose(expr->getLoc(), diag::unwrap_with_default_value);
8984+
8985+
// Figure out what we need to parenthesize.
8986+
bool needsParensInside =
8987+
exprNeedsParensBeforeAddingNilCoalescing(TC, DC, expr);
8988+
bool needsParensOutside =
8989+
exprNeedsParensAfterAddingNilCoalescing(TC, DC, expr, expr);
8990+
8991+
llvm::SmallString<2> insertBefore;
8992+
llvm::SmallString<32> insertAfter;
8993+
if (needsParensOutside) {
8994+
insertBefore += "(";
8995+
}
8996+
if (needsParensInside) {
8997+
insertBefore += "(";
8998+
insertAfter += ")";
8999+
}
9000+
insertAfter += " ?? <" "#default value#" ">";
9001+
if (needsParensOutside)
9002+
insertAfter += ")";
9003+
9004+
if (!insertBefore.empty()) {
9005+
diag.fixItInsert(expr->getStartLoc(), insertBefore);
9006+
}
9007+
diag.fixItInsertAfter(expr->getEndLoc(), insertAfter);
9008+
}
9009+
9010+
// Suggest a force-unwrap.
9011+
{
9012+
auto diag =
9013+
TC.diagnose(expr->getLoc(), diag::unwrap_with_force_value);
9014+
if (expr->canAppendPostfixExpression(true)) {
9015+
diag.fixItInsertAfter(expr->getEndLoc(), "!");
9016+
} else {
9017+
diag.fixItInsert(expr->getStartLoc(), "(")
9018+
.fixItInsertAfter(expr->getEndLoc(), ")!");
9019+
}
9020+
}
9021+
9022+
return true;
9023+
}
9024+
9025+
bool swift::diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
9026+
DeclName memberName,
9027+
SourceRange memberRange) {
9028+
auto unwrappedBaseType = baseType->getOptionalObjectType();
9029+
if (!unwrappedBaseType)
9030+
return false;
9031+
9032+
ASTContext &ctx = baseType->getASTContext();
9033+
DiagnosticEngine &diags = ctx.Diags;
9034+
diags.diagnose(baseExpr->getLoc(), diag::optional_base_not_unwrapped,
9035+
baseType, memberName, unwrappedBaseType);
9036+
9037+
diags.diagnose(baseExpr->getLoc(), diag::optional_base_chain, memberName)
9038+
.fixItInsertAfter(baseExpr->getEndLoc(), "?");
9039+
diags.diagnose(baseExpr->getLoc(), diag::unwrap_with_force_value)
9040+
.fixItInsertAfter(baseExpr->getEndLoc(), "!");
9041+
return true;
9042+
}

‎lib/Sema/CSSimplify.cpp

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3548,18 +3548,9 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
35483548
// as such and try to recover in various ways.
35493549
if (shouldAttemptFixes() && baseObjTy->getOptionalObjectType()) {
35503550
// If the base type was an optional, look through it.
3551-
3552-
// Determine whether or not we want to provide an optional chaining fixit or
3553-
// a force unwrap fixit.
3554-
bool optionalChain;
3555-
if (!getContextualType())
3556-
optionalChain = !(Options & ConstraintSystemFlags::PreferForceUnwrapToOptional);
3557-
else
3558-
optionalChain = !getContextualType()->getOptionalObjectType().isNull();
3559-
auto fixKind = optionalChain ? FixKind::OptionalChaining : FixKind::ForceOptional;
3560-
3561-
// Note the fix.
3562-
if (recordFix(fixKind, locator))
3551+
3552+
// We're unwrapping the base to perform a member access.
3553+
if (recordFix(Fix::getUnwrapOptionalBase(*this, member), locator))
35633554
return SolutionKind::Error;
35643555

35653556
// Look through one level of optional.
@@ -4871,8 +4862,8 @@ ConstraintSystem::simplifyFixConstraint(Fix fix, Type type1, Type type2,
48714862
getDefaultDecompositionOptions(flags) | TMF_ApplyingFix;
48724863
switch (fix.getKind()) {
48734864
case FixKind::ForceOptional:
4874-
case FixKind::OptionalChaining: {
4875-
// Assume that '!' was applied to the first type.
4865+
case FixKind::UnwrapOptionalBase: {
4866+
// Assume that we've unwrapped the first type.
48764867
auto result =
48774868
matchTypes(type1->getRValueObjectType()->getOptionalObjectType(), type2,
48784869
matchKind, subflags, locator);

‎lib/Sema/CalleeCandidateInfo.cpp

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1012,10 +1012,10 @@ bool CalleeCandidateInfo::diagnoseGenericParameterErrors(Expr *badArgExpr) {
10121012
// Check for optional near miss.
10131013
if (auto argOptType = substitution->getOptionalObjectType()) {
10141014
if (isSubstitutableFor(argOptType, paramArchetype, CS.DC)) {
1015-
CS.TC.diagnose(badArgExpr->getLoc(), diag::missing_unwrap_optional,
1016-
argType);
1017-
foundFailure = true;
1018-
break;
1015+
if (diagnoseUnwrap(CS.TC, CS.DC, badArgExpr, substitution)) {
1016+
foundFailure = true;
1017+
break;
1018+
}
10191019
}
10201020
}
10211021

‎lib/Sema/Constraint.cpp

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -487,17 +487,29 @@ Fix Fix::getForcedDowncast(ConstraintSystem &cs, Type toType) {
487487
return Fix(FixKind::ForceDowncast, index);
488488
}
489489

490+
Fix Fix::getUnwrapOptionalBase(ConstraintSystem &cs, DeclName memberName) {
491+
unsigned index = cs.FixedDeclNames.size();
492+
cs.FixedDeclNames.push_back(memberName);
493+
return Fix(FixKind::UnwrapOptionalBase, index);
494+
}
495+
490496
Type Fix::getTypeArgument(ConstraintSystem &cs) const {
491497
assert(getKind() == FixKind::ForceDowncast);
492498
return cs.FixedTypes[Data];
493499
}
494500

501+
/// If this fix has a name argument, retrieve it.
502+
DeclName Fix::getDeclNameArgument(ConstraintSystem &cs) const {
503+
assert(getKind() == FixKind::UnwrapOptionalBase);
504+
return cs.FixedDeclNames[Data];
505+
}
506+
495507
StringRef Fix::getName(FixKind kind) {
496508
switch (kind) {
497509
case FixKind::ForceOptional:
498510
return "fix: force optional";
499-
case FixKind::OptionalChaining:
500-
return "fix: optional chaining";
511+
case FixKind::UnwrapOptionalBase:
512+
return "fix: unwrap optional base of member lookup";
501513
case FixKind::ForceDowncast:
502514
return "fix: force downcast";
503515
case FixKind::AddressOf:

‎lib/Sema/Constraint.h

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -233,8 +233,8 @@ enum class FixKind : uint8_t {
233233
/// Introduce a '!' to force an optional unwrap.
234234
ForceOptional,
235235

236-
/// Introduce a '?.' to begin optional chaining.
237-
OptionalChaining,
236+
/// Unwrap an optional base when we have a member access.
237+
UnwrapOptionalBase,
238238

239239
/// Append 'as! T' to force a downcast to the specified type.
240240
ForceDowncast,
@@ -265,17 +265,26 @@ class Fix {
265265
public:
266266
Fix(FixKind kind) : Kind(kind), Data(0) {
267267
assert(kind != FixKind::ForceDowncast && "Use getForceDowncast()");
268+
assert(kind != FixKind::UnwrapOptionalBase &&
269+
"Use getUnwrapOptionalBase()");
268270
}
269271

270272
/// Produce a new fix that performs a forced downcast to the given type.
271273
static Fix getForcedDowncast(ConstraintSystem &cs, Type toType);
272274

275+
/// Produce a new fix that unwraps an optional base for an access to a member
276+
/// with the given name.
277+
static Fix getUnwrapOptionalBase(ConstraintSystem &cs, DeclName memberName);
278+
273279
/// Retrieve the kind of fix.
274280
FixKind getKind() const { return Kind; }
275281

276282
/// If this fix has a type argument, retrieve it.
277283
Type getTypeArgument(ConstraintSystem &cs) const;
278284

285+
/// If this fix has a name argument, retrieve it.
286+
DeclName getDeclNameArgument(ConstraintSystem &cs) const;
287+
279288
/// Return a string representation of a fix.
280289
static llvm::StringRef getName(FixKind kind);
281290

‎lib/Sema/ConstraintSystem.h

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -787,10 +787,6 @@ enum class ConstraintSystemFlags {
787787
/// Whether we allow the solver to attempt fixes to the system.
788788
AllowFixes = 0x01,
789789

790-
/// Set if the client prefers fixits to be in the form of force unwrapping
791-
/// or optional chaining to return an optional.
792-
PreferForceUnwrapToOptional = 0x02,
793-
794790
/// If set, this is going to prevent constraint system from erasing all
795791
/// discovered solutions except the best one.
796792
ReturnAllDiscoveredSolutions = 0x04,
@@ -992,6 +988,9 @@ class ConstraintSystem {
992988
/// Types used in fixes.
993989
std::vector<Type> FixedTypes;
994990

991+
/// Declaration names used in fixes.
992+
std::vector<DeclName> FixedDeclNames;
993+
995994
/// \brief The set of remembered disjunction choices used to reach
996995
/// the current constraint system.
997996
SmallVector<std::pair<ConstraintLocator*, unsigned>, 32>
@@ -3528,6 +3527,35 @@ class InputMatcher {
35283527

35293528
size_t getNumSkippedParameters() const { return NumSkippedParameters; }
35303529
};
3530+
3531+
/// Diagnose an attempt to recover when we have a value of optional type
3532+
/// that needs to be unwrapped.
3533+
///
3534+
/// \returns true if a diagnostic was produced.
3535+
bool diagnoseUnwrap(TypeChecker &TC, DeclContext *DC, Expr *expr, Type type);
3536+
3537+
/// Diagnose an attempt to recover from a member access into a value of
3538+
/// optional type which needed to be unwrapped for the member to be found.
3539+
///
3540+
/// \returns true if a diagnostic was produced.
3541+
bool diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
3542+
DeclName memberName,
3543+
SourceRange memberRange);
3544+
3545+
// Return true if, when replacing "<expr>" with "<expr> ?? T", parentheses need
3546+
// to be added around <expr> first in order to maintain the correct precedence.
3547+
bool exprNeedsParensBeforeAddingNilCoalescing(TypeChecker &TC,
3548+
DeclContext *DC,
3549+
Expr *expr);
3550+
3551+
// Return true if, when replacing "<expr>" with "<expr> as T", parentheses need
3552+
// to be added around the new expression in order to maintain the correct
3553+
// precedence.
3554+
bool exprNeedsParensAfterAddingNilCoalescing(TypeChecker &TC,
3555+
DeclContext *DC,
3556+
Expr *expr,
3557+
Expr *rootExpr);
3558+
35313559
} // end namespace swift
35323560

35333561
#endif // LLVM_SWIFT_SEMA_CONSTRAINT_SYSTEM_H

‎lib/Sema/TypeCheckConstraints.cpp

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1953,8 +1953,6 @@ Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
19531953

19541954
// Construct a constraint system from this expression.
19551955
ConstraintSystemOptions csOptions = ConstraintSystemFlags::AllowFixes;
1956-
if (options.contains(TypeCheckExprFlags::PreferForceUnwrapToOptional))
1957-
csOptions |= ConstraintSystemFlags::PreferForceUnwrapToOptional;
19581956
ConstraintSystem cs(*this, dc, csOptions);
19591957
cs.baseCS = baseCS;
19601958
CleanupIllFormedExpressionRAII cleanup(expr);

0 commit comments

Comments
 (0)