Skip to content

Commit 34f6a1f

Browse files
gregomnigregomni
authored
Merge pull request swiftlang#18699 from gregomni/opty3
[ConstraintSystem] Further unwrap cleanup
2 parents 6f91a4e + df34cdc commit 34f6a1f

File tree

6 files changed

+154
-151
lines changed

6 files changed

+154
-151
lines changed

‎lib/Sema/CSDiag.cpp

Lines changed: 2 additions & 134 deletions
Original file line numberDiff line numberDiff line change
@@ -9108,139 +9108,6 @@ bool ConstraintSystem::salvage(SmallVectorImpl<Solution> &viable, Expr *expr) {
91089108
return true;
91099109
}
91109110

9111-
// Suggest a default value via ?? <default value>
9112-
static void offerDefaultValueUnwrapFixit(TypeChecker &TC, DeclContext *DC, Expr *expr) {
9113-
auto diag =
9114-
TC.diagnose(expr->getLoc(), diag::unwrap_with_default_value);
9115-
9116-
// Figure out what we need to parenthesize.
9117-
bool needsParensInside =
9118-
exprNeedsParensBeforeAddingNilCoalescing(TC, DC, expr);
9119-
bool needsParensOutside =
9120-
exprNeedsParensAfterAddingNilCoalescing(TC, DC, expr, expr);
9121-
9122-
llvm::SmallString<2> insertBefore;
9123-
llvm::SmallString<32> insertAfter;
9124-
if (needsParensOutside) {
9125-
insertBefore += "(";
9126-
}
9127-
if (needsParensInside) {
9128-
insertBefore += "(";
9129-
insertAfter += ")";
9130-
}
9131-
insertAfter += " ?? <" "#default value#" ">";
9132-
if (needsParensOutside)
9133-
insertAfter += ")";
9134-
9135-
if (!insertBefore.empty()) {
9136-
diag.fixItInsert(expr->getStartLoc(), insertBefore);
9137-
}
9138-
diag.fixItInsertAfter(expr->getEndLoc(), insertAfter);
9139-
}
9140-
9141-
// Suggest a force-unwrap.
9142-
static void offerForceUnwrapFixit(ConstraintSystem &CS, Expr *expr) {
9143-
auto diag = CS.TC.diagnose(expr->getLoc(), diag::unwrap_with_force_value);
9144-
9145-
// If expr is optional as the result of an optional chain and this last
9146-
// dot isn't a member returning optional, then offer to force the last
9147-
// link in the chain, rather than an ugly parenthesized postfix force.
9148-
if (auto optionalChain = dyn_cast<OptionalEvaluationExpr>(expr)) {
9149-
if (auto dotExpr =
9150-
dyn_cast<UnresolvedDotExpr>(optionalChain->getSubExpr())) {
9151-
auto bind = dyn_cast<BindOptionalExpr>(dotExpr->getBase());
9152-
if (bind && !CS.getType(dotExpr)->getOptionalObjectType()) {
9153-
diag.fixItReplace(SourceRange(bind->getLoc()), "!");
9154-
return;
9155-
}
9156-
}
9157-
}
9158-
9159-
if (expr->canAppendPostfixExpression(true)) {
9160-
diag.fixItInsertAfter(expr->getEndLoc(), "!");
9161-
} else {
9162-
diag.fixItInsert(expr->getStartLoc(), "(")
9163-
.fixItInsertAfter(expr->getEndLoc(), ")!");
9164-
}
9165-
}
9166-
9167-
class VarDeclMultipleReferencesChecker : public ASTWalker {
9168-
VarDecl *varDecl;
9169-
int count;
9170-
9171-
std::pair<bool, Expr *> walkToExprPre(Expr *E) {
9172-
if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
9173-
if (DRE->getDecl() == varDecl)
9174-
count++;
9175-
}
9176-
return { true, E };
9177-
}
9178-
9179-
public:
9180-
VarDeclMultipleReferencesChecker(VarDecl *varDecl) : varDecl(varDecl),count(0) {}
9181-
int referencesCount() { return count; }
9182-
};
9183-
9184-
bool swift::diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Type type) {
9185-
Type unwrappedType = type->getOptionalObjectType();
9186-
if (!unwrappedType)
9187-
return false;
9188-
9189-
CS.TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, type,
9190-
unwrappedType);
9191-
9192-
// If the expression we're unwrapping is the only reference to a
9193-
// local variable whose type isn't explicit in the source, then
9194-
// offer unwrapping fixits on the initializer as well.
9195-
if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
9196-
if (auto varDecl = dyn_cast<VarDecl>(declRef->getDecl())) {
9197-
9198-
bool singleUse = false;
9199-
AbstractFunctionDecl *AFD = nullptr;
9200-
if (auto contextDecl = varDecl->getDeclContext()->getAsDeclOrDeclExtensionContext()) {
9201-
if ((AFD = dyn_cast<AbstractFunctionDecl>(contextDecl))) {
9202-
auto checker = VarDeclMultipleReferencesChecker(varDecl);
9203-
AFD->getBody()->walk(checker);
9204-
singleUse = checker.referencesCount() == 1;
9205-
}
9206-
}
9207-
9208-
PatternBindingDecl *binding = varDecl->getParentPatternBinding();
9209-
if (singleUse && binding && binding->getNumPatternEntries() == 1 &&
9210-
varDecl->getTypeSourceRangeForDiagnostics().isInvalid()) {
9211-
9212-
Expr *initializer = varDecl->getParentInitializer();
9213-
if (auto declRefExpr = dyn_cast<DeclRefExpr>(initializer)) {
9214-
if (declRefExpr->getDecl()->getAttrs().hasAttribute<ImplicitlyUnwrappedOptionalAttr>()) {
9215-
CS.TC.diagnose(declRefExpr->getLoc(), diag::unwrap_iuo_initializer, type);
9216-
}
9217-
}
9218-
9219-
auto fnTy = AFD->getInterfaceType()->castTo<AnyFunctionType>();
9220-
bool voidReturn = fnTy->getResult()->isEqual(TupleType::getEmpty(CS.DC->getASTContext()));
9221-
9222-
auto diag = CS.TC.diagnose(varDecl->getLoc(), diag::unwrap_with_guard);
9223-
diag.fixItInsert(binding->getStartLoc(), "guard ");
9224-
if (voidReturn) {
9225-
diag.fixItInsertAfter(binding->getEndLoc(), " else { return }");
9226-
} else {
9227-
diag.fixItInsertAfter(binding->getEndLoc(), " else { return <"
9228-
"#default value#" "> }");
9229-
}
9230-
diag.flush();
9231-
9232-
offerDefaultValueUnwrapFixit(CS.TC, varDecl->getDeclContext(),
9233-
initializer);
9234-
offerForceUnwrapFixit(CS, initializer);
9235-
}
9236-
}
9237-
}
9238-
9239-
offerDefaultValueUnwrapFixit(CS.TC, CS.DC, expr);
9240-
offerForceUnwrapFixit(CS, expr);
9241-
return true;
9242-
}
9243-
92449111
bool swift::diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
92459112
DeclName memberName,
92469113
bool resultOptional,
@@ -9257,7 +9124,8 @@ bool swift::diagnoseBaseUnwrapForMemberAccess(Expr *baseExpr, Type baseType,
92579124
// FIXME: It would be nice to immediately offer "base?.member ?? defaultValue"
92589125
// for non-optional results where that would be appropriate. For the moment
92599126
// always offering "?" means that if the user chooses chaining, we'll end up
9260-
// in diagnoseUnwrap() to offer a default value during the next compile.
9127+
// in MissingOptionalUnwrapFailure:diagnose() to offer a default value during
9128+
// the next compile.
92619129
diags.diagnose(baseExpr->getLoc(), diag::optional_base_chain, memberName)
92629130
.fixItInsertAfter(baseExpr->getEndLoc(), "?");
92639131

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,139 @@ bool MemberAccessOnOptionalBaseFailure::diagnose() {
367367
resultIsOptional, SourceRange());
368368
}
369369

370+
// Suggest a default value via ?? <default value>
371+
static void offerDefaultValueUnwrapFixit(TypeChecker &TC, DeclContext *DC, Expr *expr) {
372+
auto diag =
373+
TC.diagnose(expr->getLoc(), diag::unwrap_with_default_value);
374+
375+
// Figure out what we need to parenthesize.
376+
bool needsParensInside =
377+
exprNeedsParensBeforeAddingNilCoalescing(TC, DC, expr);
378+
bool needsParensOutside =
379+
exprNeedsParensAfterAddingNilCoalescing(TC, DC, expr, expr);
380+
381+
llvm::SmallString<2> insertBefore;
382+
llvm::SmallString<32> insertAfter;
383+
if (needsParensOutside) {
384+
insertBefore += "(";
385+
}
386+
if (needsParensInside) {
387+
insertBefore += "(";
388+
insertAfter += ")";
389+
}
390+
insertAfter += " ?? <" "#default value#" ">";
391+
if (needsParensOutside)
392+
insertAfter += ")";
393+
394+
if (!insertBefore.empty()) {
395+
diag.fixItInsert(expr->getStartLoc(), insertBefore);
396+
}
397+
diag.fixItInsertAfter(expr->getEndLoc(), insertAfter);
398+
}
399+
400+
// Suggest a force-unwrap.
401+
static void offerForceUnwrapFixit(ConstraintSystem &CS, Expr *expr) {
402+
auto diag = CS.TC.diagnose(expr->getLoc(), diag::unwrap_with_force_value);
403+
404+
// If expr is optional as the result of an optional chain and this last
405+
// dot isn't a member returning optional, then offer to force the last
406+
// link in the chain, rather than an ugly parenthesized postfix force.
407+
if (auto optionalChain = dyn_cast<OptionalEvaluationExpr>(expr)) {
408+
if (auto dotExpr =
409+
dyn_cast<UnresolvedDotExpr>(optionalChain->getSubExpr())) {
410+
auto bind = dyn_cast<BindOptionalExpr>(dotExpr->getBase());
411+
if (bind && !CS.getType(dotExpr)->getOptionalObjectType()) {
412+
diag.fixItReplace(SourceRange(bind->getLoc()), "!");
413+
return;
414+
}
415+
}
416+
}
417+
418+
if (expr->canAppendPostfixExpression(true)) {
419+
diag.fixItInsertAfter(expr->getEndLoc(), "!");
420+
} else {
421+
diag.fixItInsert(expr->getStartLoc(), "(")
422+
.fixItInsertAfter(expr->getEndLoc(), ")!");
423+
}
424+
}
425+
426+
class VarDeclMultipleReferencesChecker : public ASTWalker {
427+
VarDecl *varDecl;
428+
int count;
429+
430+
std::pair<bool, Expr *> walkToExprPre(Expr *E) {
431+
if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
432+
if (DRE->getDecl() == varDecl)
433+
count++;
434+
}
435+
return { true, E };
436+
}
437+
438+
public:
439+
VarDeclMultipleReferencesChecker(VarDecl *varDecl) : varDecl(varDecl),count(0) {}
440+
int referencesCount() { return count; }
441+
};
442+
443+
static bool diagnoseUnwrap(ConstraintSystem &CS, Expr *expr, Type type) {
444+
Type unwrappedType = type->getOptionalObjectType();
445+
if (!unwrappedType)
446+
return false;
447+
448+
CS.TC.diagnose(expr->getLoc(), diag::optional_not_unwrapped, type,
449+
unwrappedType);
450+
451+
// If the expression we're unwrapping is the only reference to a
452+
// local variable whose type isn't explicit in the source, then
453+
// offer unwrapping fixits on the initializer as well.
454+
if (auto declRef = dyn_cast<DeclRefExpr>(expr)) {
455+
if (auto varDecl = dyn_cast<VarDecl>(declRef->getDecl())) {
456+
457+
bool singleUse = false;
458+
AbstractFunctionDecl *AFD = nullptr;
459+
if (auto contextDecl = varDecl->getDeclContext()->getAsDeclOrDeclExtensionContext()) {
460+
if ((AFD = dyn_cast<AbstractFunctionDecl>(contextDecl))) {
461+
auto checker = VarDeclMultipleReferencesChecker(varDecl);
462+
AFD->getBody()->walk(checker);
463+
singleUse = checker.referencesCount() == 1;
464+
}
465+
}
466+
467+
PatternBindingDecl *binding = varDecl->getParentPatternBinding();
468+
if (singleUse && binding && binding->getNumPatternEntries() == 1 &&
469+
varDecl->getTypeSourceRangeForDiagnostics().isInvalid()) {
470+
471+
Expr *initializer = varDecl->getParentInitializer();
472+
if (auto declRefExpr = dyn_cast<DeclRefExpr>(initializer)) {
473+
if (declRefExpr->getDecl()->getAttrs().hasAttribute<ImplicitlyUnwrappedOptionalAttr>()) {
474+
CS.TC.diagnose(declRefExpr->getLoc(), diag::unwrap_iuo_initializer, type);
475+
}
476+
}
477+
478+
auto fnTy = AFD->getInterfaceType()->castTo<AnyFunctionType>();
479+
bool voidReturn = fnTy->getResult()->isEqual(TupleType::getEmpty(CS.DC->getASTContext()));
480+
481+
auto diag = CS.TC.diagnose(varDecl->getLoc(), diag::unwrap_with_guard);
482+
diag.fixItInsert(binding->getStartLoc(), "guard ");
483+
if (voidReturn) {
484+
diag.fixItInsertAfter(binding->getEndLoc(), " else { return }");
485+
} else {
486+
diag.fixItInsertAfter(binding->getEndLoc(), " else { return <"
487+
"#default value#" "> }");
488+
}
489+
diag.flush();
490+
491+
offerDefaultValueUnwrapFixit(CS.TC, varDecl->getDeclContext(),
492+
initializer);
493+
offerForceUnwrapFixit(CS, initializer);
494+
}
495+
}
496+
}
497+
498+
offerDefaultValueUnwrapFixit(CS.TC, CS.DC, expr);
499+
offerForceUnwrapFixit(CS, expr);
500+
return true;
501+
}
502+
370503
bool MissingOptionalUnwrapFailure::diagnose() {
371504
if (hasComplexLocator())
372505
return false;

‎lib/Sema/CSSolver.cpp

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,11 @@ bool ConstraintSystem::tryTypeVariableBindings(
713713
newBindings.push_back({subtype, binding.Kind, binding.BindingSource});
714714
}
715715

716-
if (binding.Kind == AllowedBindingKind::Subtypes) {
716+
// Allow solving for T even for a binding kind where that's invalid
717+
// if fixes are allowed, because that gives us the opportunity to
718+
// match T? values to the T binding by adding an unwrap fix.
719+
if (binding.Kind == AllowedBindingKind::Subtypes ||
720+
shouldAttemptFixes()) {
717721
// If we were unsuccessful solving for T?, try solving for T.
718722
if (auto objTy = type->getOptionalObjectType()) {
719723
if (exploredTypes.insert(objTy->getCanonicalType()).second) {

‎lib/Sema/CalleeCandidateInfo.cpp

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1032,16 +1032,6 @@ bool CalleeCandidateInfo::diagnoseGenericParameterErrors(Expr *badArgExpr) {
10321032

10331033
// FIXME: Add specific error for not subclass, if the archetype has a superclass?
10341034

1035-
// Check for optional near miss.
1036-
if (auto argOptType = substitution->getOptionalObjectType()) {
1037-
if (isSubstitutableFor(argOptType, paramArchetype, CS.DC)) {
1038-
if (diagnoseUnwrap(CS, badArgExpr, substitution)) {
1039-
foundFailure = true;
1040-
break;
1041-
}
1042-
}
1043-
}
1044-
10451035
for (auto proto : paramArchetype->getConformsTo()) {
10461036
if (!CS.TC.conformsToProtocol(substitution, proto, CS.DC,
10471037
ConformanceCheckFlags::InExpression)) {

‎lib/Sema/ConstraintSystem.h

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3559,12 +3559,6 @@ class InputMatcher {
35593559
size_t getNumSkippedParameters() const { return NumSkippedParameters; }
35603560
};
35613561

3562-
/// Diagnose an attempt to recover when we have a value of optional type
3563-
/// that needs to be unwrapped.
3564-
///
3565-
/// \returns true if a diagnostic was produced.
3566-
bool diagnoseUnwrap(constraints::ConstraintSystem &CS, Expr *expr, Type type);
3567-
35683562
/// Diagnose an attempt to recover from a member access into a value of
35693563
/// optional type which needed to be unwrapped for the member to be found.
35703564
///

‎test/Constraints/closures.swift

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,6 +288,20 @@ struct S<T> {
288288
}
289289
}
290290

291+
// Similar to SR1069 but with multiple generic arguments
292+
func simplified1069() {
293+
class C {}
294+
struct S {
295+
func genericallyNonOptional<T: AnyObject>(_ a: T, _ b: T, _ c: T) { }
296+
297+
func f(_ a: C?, _ b: C?, _ c: C) {
298+
genericallyNonOptional(a, b, c) // expected-error 2{{value of optional type 'C?' must be unwrapped to a value of type 'C'}}
299+
// expected-note @-1 2{{coalesce}}
300+
// expected-note @-2 2{{force-unwrap}}
301+
}
302+
}
303+
}
304+
291305
// Make sure we cannot infer an () argument from an empty parameter list.
292306
func acceptNothingToInt (_: () -> Int) {}
293307
func testAcceptNothingToInt(ac1: @autoclosure () -> Int) {

0 commit comments

Comments
 (0)