Skip to content

Commit aaf545a

Browse files
AnnaZaksAnnaZaks
authored
Merge pull request #17955 from DougGregor/optional-not-unwrapped-diags-4.2
[4.2] [Type checker] Improve diagnostics when an optional value is not unwrapped
2 parents 9ba0b68 + f027bd5 commit aaf545a

32 files changed

+327
-117
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -885,10 +885,21 @@ NOTE(note_init_parameter,none,
885885
ERROR(missing_nullary_call,none,
886886
"function produces expected type %0; did you mean to call it with '()'?",
887887
(Type))
888-
ERROR(missing_unwrap_optional,none,
889-
"value of optional type %0 not unwrapped; did you mean to use '!' "
890-
"or '?'?",
891-
(Type))
888+
ERROR(optional_not_unwrapped,none,
889+
"value of optional type %0 must be unwrapped to a value of type %1",
890+
(Type, Type))
891+
NOTE(unwrap_with_default_value,none,
892+
"coalesce using '?" "?' to provide a default when the optional value "
893+
"contains 'nil'", ())
894+
NOTE(unwrap_with_force_value,none,
895+
"force-unwrap using '!' to abort execution if the optional value contains "
896+
"'nil'", ())
897+
ERROR(optional_base_not_unwrapped,none,
898+
"value of optional type %0 must be unwrapped to refer to member %1 of "
899+
"wrapped base type %2", (Type, DeclName, Type))
900+
NOTE(optional_base_chain,none,
901+
"chain the optional using '?' to access member %0 only for non-'nil' "
902+
"base values", (DeclName))
892903
ERROR(missing_unwrap_optional_try,none,
893904
"value of optional type %0 not unwrapped; did you mean to use 'try!' "
894905
"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
@@ -7906,6 +7906,27 @@ static bool exprNeedsParensAfterAddingAs(TypeChecker &TC, DeclContext *DC,
79067906
return exprNeedsParensOutsideFollowingOperator(TC, DC, expr, rootExpr, asPG);
79077907
}
79087908

7909+
bool swift::exprNeedsParensBeforeAddingNilCoalescing(TypeChecker &TC,
7910+
DeclContext *DC,
7911+
Expr *expr) {
7912+
auto asPG =
7913+
TC.lookupPrecedenceGroup(DC, DC->getASTContext().Id_NilCoalescingPrecedence,
7914+
SourceLoc());
7915+
if (!asPG) return true;
7916+
return exprNeedsParensInsideFollowingOperator(TC, DC, expr, asPG);
7917+
}
7918+
7919+
bool swift::exprNeedsParensAfterAddingNilCoalescing(TypeChecker &TC,
7920+
DeclContext *DC,
7921+
Expr *expr,
7922+
Expr *rootExpr) {
7923+
auto asPG =
7924+
TC.lookupPrecedenceGroup(DC, DC->getASTContext().Id_NilCoalescingPrecedence,
7925+
SourceLoc());
7926+
if (!asPG) return true;
7927+
return exprNeedsParensOutsideFollowingOperator(TC, DC, expr, rootExpr, asPG);
7928+
}
7929+
79097930
namespace {
79107931
class ExprWalker : public ASTWalker {
79117932
ExprRewriter &Rewriter;
@@ -8067,7 +8088,7 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
80678088

80688089
switch (fix.first.getKind()) {
80698090
case FixKind::ForceOptional: {
8070-
const Expr *unwrapped = affected->getValueProvidingExpr();
8091+
Expr *unwrapped = affected->getValueProvidingExpr();
80718092
auto type = solution.simplifyType(getType(affected))
80728093
->getRValueObjectType();
80738094

@@ -8078,25 +8099,17 @@ bool ConstraintSystem::applySolutionFix(Expr *expr,
80788099
"try!");
80798100

80808101
} else {
8081-
auto diag = TC.diagnose(affected->getLoc(),
8082-
diag::missing_unwrap_optional, type);
8083-
if (affected->canAppendPostfixExpression(true)) {
8084-
diag.fixItInsertAfter(affected->getEndLoc(), "!");
8085-
} else {
8086-
diag.fixItInsert(affected->getStartLoc(), "(")
8087-
.fixItInsertAfter(affected->getEndLoc(), ")!");
8088-
}
8102+
return diagnoseUnwrap(TC, DC, unwrapped, type);
80898103
}
80908104
return true;
80918105
}
80928106

8093-
case FixKind::OptionalChaining: {
8107+
case FixKind::UnwrapOptionalBase: {
80948108
auto type = solution.simplifyType(getType(affected))
80958109
->getRValueObjectType();
8096-
auto diag = TC.diagnose(affected->getLoc(),
8097-
diag::missing_unwrap_optional, type);
8098-
diag.fixItInsertAfter(affected->getEndLoc(), "?");
8099-
return true;
8110+
DeclName memberName = fix.first.getDeclNameArgument(*this);
8111+
return diagnoseBaseUnwrapForMemberAccess(affected, type, memberName,
8112+
SourceRange());
81008113
}
81018114

81028115
case FixKind::ForceDowncast: {

‎lib/Sema/CSDiag.cpp

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

2258-
// If we're not passing down contextual type information this time, but the
2259-
// original failure had type info that wasn't an optional type,
2260-
// then set the flag to prefer fixits with force unwrapping.
2261-
if (!convertType) {
2262-
auto previousType = CS.getContextualType();
2263-
if (previousType && previousType->getOptionalObjectType().isNull())
2264-
TCEOptions |= TypeCheckExprFlags::PreferForceUnwrapToOptional;
2265-
}
2266-
22672258
auto resultTy = CS.TC.typeCheckExpression(
22682259
subExpr, CS.DC, TypeLoc::withoutLoc(convertType), convertTypePurpose,
22692260
TCEOptions, listener, &CS);
@@ -7745,15 +7736,9 @@ bool FailureDiagnosis::diagnoseMemberFailures(
77457736
}
77467737

77477738
if (!optionalResult.ViableCandidates.empty()) {
7748-
// By default we assume that the LHS type is not optional.
7749-
StringRef fixIt = "!";
7750-
auto contextualType = CS.getContextualType();
7751-
if (contextualType && isa<OptionalType>(contextualType.getPointer()))
7752-
fixIt = "?";
7753-
7754-
diagnose(BaseLoc, diag::missing_unwrap_optional, baseObjTy)
7755-
.fixItInsertAfter(baseExpr->getEndLoc(), fixIt);
7756-
return true;
7739+
if (diagnoseBaseUnwrapForMemberAccess(baseExpr, baseObjTy, memberName,
7740+
memberRange))
7741+
return true;
77577742
}
77587743
}
77597744

@@ -8660,3 +8645,76 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
86608645
diagnoseFailureForExpr(expr);
86618646
return true;
86628647
}
8648+
8649+
bool swift::diagnoseUnwrap(TypeChecker &TC, DeclContext *DC,
8650+
Expr *expr, Type type) {
8651+
Type unwrappedType = type->getOptionalObjectType();
8652+
if (!unwrappedType)
8653+
return false;
8654+
8655+
TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, type,
8656+
unwrappedType);
8657+
8658+
// Suggest a default value via ?? <default value>
8659+
{
8660+
auto diag =
8661+
TC.diagnose(expr->getLoc(), diag::unwrap_with_default_value);
8662+
8663+
// Figure out what we need to parenthesize.
8664+
bool needsParensInside =
8665+
exprNeedsParensBeforeAddingNilCoalescing(TC, DC, expr);
8666+
bool needsParensOutside =
8667+
exprNeedsParensAfterAddingNilCoalescing(TC, DC, expr, expr);
8668+
8669+
llvm::SmallString<2> insertBefore;
8670+
llvm::SmallString<32> insertAfter;
8671+
if (needsParensOutside) {
8672+
insertBefore += "(";
8673+
}
8674+
if (needsParensInside) {
8675+
insertBefore += "(";
8676+
insertAfter += ")";
8677+
}
8678+
insertAfter += " ?? <" "#default value#" ">";
8679+
if (needsParensOutside)
8680+
insertAfter += ")";
8681+
8682+
if (!insertBefore.empty()) {
8683+
diag.fixItInsert(expr->getStartLoc(), insertBefore);
8684+
}
8685+
diag.fixItInsertAfter(expr->getEndLoc(), insertAfter);
8686+
}
8687+
8688+
// Suggest a force-unwrap.
8689+
{
8690+
auto diag =
8691+
TC.diagnose(expr->getLoc(), diag::unwrap_with_force_value);
8692+
if (expr->canAppendPostfixExpression(true)) {
8693+
diag.fixItInsertAfter(expr->getEndLoc(), "!");
8694+
} else {
8695+
diag.fixItInsert(expr->getStartLoc(), "(")
8696+
.fixItInsertAfter(expr->getEndLoc(), ")!");
8697+
}
8698+
}
8699+
8700+
return true;
8701+
}
8702+
8703+
bool swift::diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
8704+
DeclName memberName,
8705+
SourceRange memberRange) {
8706+
auto unwrappedBaseType = baseType->getOptionalObjectType();
8707+
if (!unwrappedBaseType)
8708+
return false;
8709+
8710+
ASTContext &ctx = baseType->getASTContext();
8711+
DiagnosticEngine &diags = ctx.Diags;
8712+
diags.diagnose(baseExpr->getLoc(), diag::optional_base_not_unwrapped,
8713+
baseType, memberName, unwrappedBaseType);
8714+
8715+
diags.diagnose(baseExpr->getLoc(), diag::optional_base_chain, memberName)
8716+
.fixItInsertAfter(baseExpr->getEndLoc(), "?");
8717+
diags.diagnose(baseExpr->getLoc(), diag::unwrap_with_force_value)
8718+
.fixItInsertAfter(baseExpr->getEndLoc(), "!");
8719+
return true;
8720+
}

‎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
// Value member lookup has some hacks too.
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
@@ -992,10 +992,10 @@ bool CalleeCandidateInfo::diagnoseGenericParameterErrors(Expr *badArgExpr) {
992992
// Check for optional near miss.
993993
if (auto argOptType = substitution->getOptionalObjectType()) {
994994
if (isSubstitutableFor(argOptType, paramArchetype, CS.DC)) {
995-
CS.TC.diagnose(badArgExpr->getLoc(), diag::missing_unwrap_optional,
996-
argType);
997-
foundFailure = true;
998-
break;
995+
if (diagnoseUnwrap(CS.TC, CS.DC, badArgExpr, substitution)) {
996+
foundFailure = true;
997+
break;
998+
}
999999
}
10001000
}
10011001

‎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: 33 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -796,10 +796,6 @@ enum class ConstraintSystemFlags {
796796
/// Whether we allow the solver to attempt fixes to the system.
797797
AllowFixes = 0x01,
798798

799-
/// Set if the client prefers fixits to be in the form of force unwrapping
800-
/// or optional chaining to return an optional.
801-
PreferForceUnwrapToOptional = 0x02,
802-
803799
/// If set, this is going to prevent constraint system from erasing all
804800
/// discovered solutions except the best one.
805801
ReturnAllDiscoveredSolutions = 0x04,
@@ -1001,6 +997,9 @@ class ConstraintSystem {
1001997
/// Types used in fixes.
1002998
std::vector<Type> FixedTypes;
1003999

1000+
/// Declaration names used in fixes.
1001+
std::vector<DeclName> FixedDeclNames;
1002+
10041003
/// \brief The set of remembered disjunction choices used to reach
10051004
/// the current constraint system.
10061005
SmallVector<std::pair<ConstraintLocator*, unsigned>, 32>
@@ -3480,6 +3479,36 @@ class OverloadSetCounter : public ASTWalker {
34803479
return { true, expr };
34813480
}
34823481
};
3482+
3483+
3484+
/// Diagnose an attempt to recover when we have a value of optional type
3485+
/// that needs to be unwrapped.
3486+
///
3487+
/// \returns true if a diagnostic was produced.
3488+
bool diagnoseUnwrap(TypeChecker &TC, DeclContext *DC, Expr *expr, Type type);
3489+
3490+
/// Diagnose an attempt to recover from a member access into a value of
3491+
/// optional type which needed to be unwrapped for the member to be found.
3492+
///
3493+
/// \returns true if a diagnostic was produced.
3494+
bool diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
3495+
DeclName memberName,
3496+
SourceRange memberRange);
3497+
3498+
// Return true if, when replacing "<expr>" with "<expr> ?? T", parentheses need
3499+
// to be added around <expr> first in order to maintain the correct precedence.
3500+
bool exprNeedsParensBeforeAddingNilCoalescing(TypeChecker &TC,
3501+
DeclContext *DC,
3502+
Expr *expr);
3503+
3504+
// Return true if, when replacing "<expr>" with "<expr> as T", parentheses need
3505+
// to be added around the new expression in order to maintain the correct
3506+
// precedence.
3507+
bool exprNeedsParensAfterAddingNilCoalescing(TypeChecker &TC,
3508+
DeclContext *DC,
3509+
Expr *expr,
3510+
Expr *rootExpr);
3511+
34833512
} // end namespace swift
34843513

34853514
#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
@@ -1856,8 +1856,6 @@ Type TypeChecker::typeCheckExpression(Expr *&expr, DeclContext *dc,
18561856

18571857
// Construct a constraint system from this expression.
18581858
ConstraintSystemOptions csOptions = ConstraintSystemFlags::AllowFixes;
1859-
if (options.contains(TypeCheckExprFlags::PreferForceUnwrapToOptional))
1860-
csOptions |= ConstraintSystemFlags::PreferForceUnwrapToOptional;
18611859
ConstraintSystem cs(*this, dc, csOptions);
18621860
cs.baseCS = baseCS;
18631861
CleanupIllFormedExpressionRAII cleanup(Context, expr);

0 commit comments

Comments
 (0)