Skip to content

Commit 5235ee8

Browse files
xedinxedin
authored
Merge pull request swiftlang#23179 from xedin/rdar-47776586
[ConstraintSystem] Detect and diagnose missing optional unwrap in arguments
2 parents f1ba7ef + f950360 commit 5235ee8

File tree

4 files changed

+140
-61
lines changed

4 files changed

+140
-61
lines changed

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 109 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -549,16 +549,67 @@ bool MemberAccessOnOptionalBaseFailure::diagnoseAsError() {
549549
resultIsOptional, SourceRange());
550550
}
551551

552-
// Suggest a default value via ?? <default value>
553-
static void offerDefaultValueUnwrapFixit(TypeChecker &TC, DeclContext *DC, Expr *expr, Expr *rootExpr) {
554-
auto diag =
555-
TC.diagnose(expr->getLoc(), diag::unwrap_with_default_value);
552+
Optional<AnyFunctionType::Param>
553+
MissingOptionalUnwrapFailure::getOperatorParameterFor(Expr *expr) const {
554+
auto *parentExpr = findParentExpr(expr);
555+
if (!parentExpr)
556+
return None;
557+
558+
auto getArgIdx = [](TupleExpr *tuple, Expr *argExpr) -> unsigned {
559+
for (unsigned i = 0, n = tuple->getNumElements(); i != n; ++i) {
560+
if (tuple->getElement(i) == argExpr)
561+
return i;
562+
}
563+
llvm_unreachable("argument is not in enclosing tuple?!");
564+
};
565+
566+
auto *tupleExpr = dyn_cast<TupleExpr>(parentExpr);
567+
if (!(tupleExpr && tupleExpr->isImplicit()))
568+
return None;
569+
570+
parentExpr = findParentExpr(tupleExpr);
571+
if (!(parentExpr && isa<ApplyExpr>(parentExpr)))
572+
return None;
573+
574+
auto &cs = getConstraintSystem();
575+
auto *fnExpr = cast<ApplyExpr>(parentExpr)->getFn();
576+
if (auto overload =
577+
getOverloadChoiceIfAvailable(cs.getConstraintLocator(fnExpr))) {
578+
if (auto *decl = overload->choice.getDecl()) {
579+
if (!decl->isOperator())
580+
return None;
581+
582+
auto *fnType = overload->openedType->castTo<FunctionType>();
583+
return fnType->getParams()[getArgIdx(tupleExpr, expr)];
584+
}
585+
}
586+
587+
return None;
588+
}
589+
590+
void MissingOptionalUnwrapFailure::offerDefaultValueUnwrapFixIt(
591+
DeclContext *DC, Expr *expr) const {
592+
auto *anchor = getAnchor();
593+
594+
// If anchor is an explicit address-of, or expression which produces
595+
// an l-value (e.g. first argument of `+=` operator), let's not
596+
// suggest default value here because that would produce r-value type.
597+
if (isa<InOutExpr>(anchor))
598+
return;
556599

600+
if (auto param = getOperatorParameterFor(anchor)) {
601+
if (param->isInOut())
602+
return;
603+
}
604+
605+
auto diag = emitDiagnostic(expr->getLoc(), diag::unwrap_with_default_value);
606+
607+
auto &TC = getTypeChecker();
557608
// Figure out what we need to parenthesize.
558609
bool needsParensInside =
559-
exprNeedsParensBeforeAddingNilCoalescing(TC, DC, expr);
610+
exprNeedsParensBeforeAddingNilCoalescing(TC, DC, expr);
560611
bool needsParensOutside =
561-
exprNeedsParensAfterAddingNilCoalescing(TC, DC, expr, rootExpr);
612+
exprNeedsParensAfterAddingNilCoalescing(TC, DC, expr, getParentExpr());
562613

563614
llvm::SmallString<2> insertBefore;
564615
llvm::SmallString<32> insertAfter;
@@ -580,8 +631,8 @@ static void offerDefaultValueUnwrapFixit(TypeChecker &TC, DeclContext *DC, Expr
580631
}
581632

582633
// Suggest a force-unwrap.
583-
static void offerForceUnwrapFixit(ConstraintSystem &CS, Expr *expr) {
584-
auto diag = CS.TC.diagnose(expr->getLoc(), diag::unwrap_with_force_value);
634+
void MissingOptionalUnwrapFailure::offerForceUnwrapFixIt(Expr *expr) const {
635+
auto diag = emitDiagnostic(expr->getLoc(), diag::unwrap_with_force_value);
585636

586637
// If expr is optional as the result of an optional chain and this last
587638
// dot isn't a member returning optional, then offer to force the last
@@ -590,7 +641,7 @@ static void offerForceUnwrapFixit(ConstraintSystem &CS, Expr *expr) {
590641
if (auto dotExpr =
591642
dyn_cast<UnresolvedDotExpr>(optionalChain->getSubExpr())) {
592643
auto bind = dyn_cast<BindOptionalExpr>(dotExpr->getBase());
593-
if (bind && !CS.getType(dotExpr)->getOptionalObjectType()) {
644+
if (bind && !getType(dotExpr)->getOptionalObjectType()) {
594645
diag.fixItReplace(SourceRange(bind->getLoc()), "!");
595646
return;
596647
}
@@ -601,7 +652,7 @@ static void offerForceUnwrapFixit(ConstraintSystem &CS, Expr *expr) {
601652
diag.fixItInsertAfter(expr->getEndLoc(), "!");
602653
} else {
603654
diag.fixItInsert(expr->getStartLoc(), "(")
604-
.fixItInsertAfter(expr->getEndLoc(), ")!");
655+
.fixItInsertAfter(expr->getEndLoc(), ")!");
605656
}
606657
}
607658

@@ -622,8 +673,38 @@ class VarDeclMultipleReferencesChecker : public ASTWalker {
622673
int referencesCount() { return count; }
623674
};
624675

625-
static bool diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Expr *rootExpr, Type baseType,
626-
Type unwrappedType) {
676+
bool MissingOptionalUnwrapFailure::diagnoseAsError() {
677+
if (hasComplexLocator())
678+
return false;
679+
680+
auto *anchor = getAnchor();
681+
682+
if (auto assignExpr = dyn_cast<AssignExpr>(anchor))
683+
anchor = assignExpr->getSrc();
684+
685+
auto *unwrappedExpr = anchor->getValueProvidingExpr();
686+
687+
if (auto *tryExpr = dyn_cast<OptionalTryExpr>(unwrappedExpr)) {
688+
bool isSwift5OrGreater = getASTContext().isSwiftVersionAtLeast(5);
689+
auto subExprType = getType(tryExpr->getSubExpr());
690+
bool subExpressionIsOptional = (bool)subExprType->getOptionalObjectType();
691+
692+
if (isSwift5OrGreater && subExpressionIsOptional) {
693+
// Using 'try!' won't change the type for a 'try?' with an optional
694+
// sub-expr under Swift 5+, so just report that a missing unwrap can't be
695+
// handled here.
696+
return false;
697+
}
698+
699+
emitDiagnostic(tryExpr->getTryLoc(), diag::missing_unwrap_optional_try,
700+
getType(anchor)->getRValueType())
701+
.fixItReplace({tryExpr->getTryLoc(), tryExpr->getQuestionLoc()},
702+
"try!");
703+
return true;
704+
}
705+
706+
auto baseType = getBaseType();
707+
auto unwrappedType = getUnwrappedType();
627708

628709
assert(!baseType->hasTypeVariable() &&
629710
"Base type must not be a type variable");
@@ -633,15 +714,14 @@ static bool diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Expr *rootExpr, Typ
633714
if (!baseType->getOptionalObjectType())
634715
return false;
635716

636-
CS.TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, baseType,
637-
unwrappedType);
717+
emitDiagnostic(unwrappedExpr->getLoc(), diag::optional_not_unwrapped,
718+
baseType, unwrappedType);
638719

639720
// If the expression we're unwrapping is the only reference to a
640721
// local variable whose type isn't explicit in the source, then
641722
// offer unwrapping fixits on the initializer as well.
642-
if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
723+
if (auto declRef = dyn_cast<DeclRefExpr>(unwrappedExpr)) {
643724
if (auto varDecl = dyn_cast<VarDecl>(declRef->getDecl())) {
644-
645725
bool singleUse = false;
646726
AbstractFunctionDecl *AFD = nullptr;
647727
if (auto contextDecl = varDecl->getDeclContext()->getAsDecl()) {
@@ -658,69 +738,37 @@ static bool diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Expr *rootExpr, Typ
658738

659739
Expr *initializer = varDecl->getParentInitializer();
660740
if (auto declRefExpr = dyn_cast<DeclRefExpr>(initializer)) {
661-
if (declRefExpr->getDecl()->getAttrs().hasAttribute<ImplicitlyUnwrappedOptionalAttr>()) {
662-
CS.TC.diagnose(declRefExpr->getLoc(), diag::unwrap_iuo_initializer,
741+
if (declRefExpr->getDecl()
742+
->getAttrs()
743+
.hasAttribute<ImplicitlyUnwrappedOptionalAttr>()) {
744+
emitDiagnostic(declRefExpr->getLoc(), diag::unwrap_iuo_initializer,
663745
baseType);
664746
}
665747
}
666748

667749
auto fnTy = AFD->getInterfaceType()->castTo<AnyFunctionType>();
668-
bool voidReturn = fnTy->getResult()->isEqual(TupleType::getEmpty(CS.DC->getASTContext()));
750+
bool voidReturn =
751+
fnTy->getResult()->isEqual(TupleType::getEmpty(getASTContext()));
669752

670-
auto diag = CS.TC.diagnose(varDecl->getLoc(), diag::unwrap_with_guard);
753+
auto diag = emitDiagnostic(varDecl->getLoc(), diag::unwrap_with_guard);
671754
diag.fixItInsert(binding->getStartLoc(), "guard ");
672755
if (voidReturn) {
673756
diag.fixItInsertAfter(binding->getEndLoc(), " else { return }");
674757
} else {
675758
diag.fixItInsertAfter(binding->getEndLoc(), " else { return <"
676-
"#default value#" "> }");
759+
"#default value#"
760+
"> }");
677761
}
678762
diag.flush();
679763

680-
offerDefaultValueUnwrapFixit(CS.TC, varDecl->getDeclContext(),
681-
initializer, rootExpr);
682-
offerForceUnwrapFixit(CS, initializer);
764+
offerDefaultValueUnwrapFixIt(varDecl->getDeclContext(), initializer);
765+
offerForceUnwrapFixIt(initializer);
683766
}
684767
}
685768
}
686769

687-
offerDefaultValueUnwrapFixit(CS.TC, CS.DC, expr, rootExpr);
688-
offerForceUnwrapFixit(CS, expr);
689-
return true;
690-
}
691-
692-
bool MissingOptionalUnwrapFailure::diagnoseAsError() {
693-
if (hasComplexLocator())
694-
return false;
695-
696-
auto *anchor = getAnchor();
697-
auto *rootExpr = getParentExpr();
698-
699-
if (auto assignExpr = dyn_cast<AssignExpr>(anchor))
700-
anchor = assignExpr->getSrc();
701-
702-
auto *unwrapped = anchor->getValueProvidingExpr();
703-
auto type = getType(anchor)->getRValueType();
704-
705-
auto *tryExpr = dyn_cast<OptionalTryExpr>(unwrapped);
706-
if (!tryExpr) {
707-
return diagnoseUnwrap(getConstraintSystem(), unwrapped, rootExpr, getBaseType(),
708-
getUnwrappedType());
709-
}
710-
711-
bool isSwift5OrGreater = getTypeChecker().getLangOpts().isSwiftVersionAtLeast(5);
712-
auto subExprType = getType(tryExpr->getSubExpr());
713-
bool subExpressionIsOptional = (bool)subExprType->getOptionalObjectType();
714-
715-
716-
if (isSwift5OrGreater && subExpressionIsOptional) {
717-
// Using 'try!' won't change the type for a 'try?' with an optional sub-expr
718-
// under Swift 5+, so just report that a missing unwrap can't be handled here.
719-
return false;
720-
}
721-
722-
emitDiagnostic(tryExpr->getTryLoc(), diag::missing_unwrap_optional_try, type)
723-
.fixItReplace({tryExpr->getTryLoc(), tryExpr->getQuestionLoc()}, "try!");
770+
offerDefaultValueUnwrapFixIt(getDC(), unwrappedExpr);
771+
offerForceUnwrapFixIt(unwrappedExpr);
724772
return true;
725773
}
726774

‎lib/Sema/CSDiagnostics.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -565,6 +565,15 @@ class MissingOptionalUnwrapFailure final : public FailureDiagnostic {
565565
Type getUnwrappedType() const {
566566
return resolveType(UnwrappedType, /*reconstituteSugar=*/true);
567567
}
568+
569+
/// Suggest a default value via `?? <default value>`
570+
void offerDefaultValueUnwrapFixIt(DeclContext *DC, Expr *expr) const;
571+
/// Suggest a force optional unwrap via `!`
572+
void offerForceUnwrapFixIt(Expr *expr) const;
573+
574+
/// Determine whether given expression is an argument used in the
575+
/// operator invocation, and if so return corresponding parameter.
576+
Optional<AnyFunctionType::Param> getOperatorParameterFor(Expr *expr) const;
568577
};
569578

570579
/// Diagnose errors associated with rvalues in positions

‎lib/Sema/CSSimplify.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1925,6 +1925,16 @@ repairFailures(ConstraintSystem &cs, Type lhs, Type rhs,
19251925

19261926
auto &elt = path.back();
19271927
switch (elt.getKind()) {
1928+
case ConstraintLocator::LValueConversion:
1929+
case ConstraintLocator::ApplyArgToParam: {
1930+
if (lhs->getOptionalObjectType() && !rhs->getOptionalObjectType()) {
1931+
conversionsOrFixes.push_back(
1932+
ForceOptional::create(cs, lhs, lhs->getOptionalObjectType(),
1933+
cs.getConstraintLocator(locator)));
1934+
}
1935+
break;
1936+
}
1937+
19281938
case ConstraintLocator::FunctionArgument: {
19291939
auto *argLoc = cs.getConstraintLocator(
19301940
locator.withPathElement(LocatorPathElt::getSynthesizedArgument(0)));

‎test/Constraints/optional.swift

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -333,3 +333,15 @@ func sr_9893_2(cString: UnsafePointer<CChar>) {
333333
}
334334
}
335335
}
336+
337+
// rdar://problem/47776586 - Diagnostic refers to '&' which is not present in the source code
338+
func rdar47776586() {
339+
func foo(_: inout Int) {}
340+
var x: Int? = 0
341+
foo(&x) // expected-error {{value of optional type 'Int?' must be unwrapped to a value of type 'Int'}}
342+
// expected-note@-1 {{force-unwrap using '!' to abort execution if the optional value contains 'nil'}} {{7-7=(}} {{9-9=)!}}
343+
344+
var dict = [1: 2]
345+
dict[1] += 1 // expected-error {{value of optional type 'Int?' must be unwrapped to a value of type 'Int'}}
346+
// expected-note@-1 {{force-unwrap using '!' to abort execution if the optional value contains 'nil'}} {{10-10=!}}
347+
}

0 commit comments

Comments
 (0)