Skip to content

Commit 43fd786

Browse files
LucianoPAlmeidaLucianoPAlmeida
authored
[SR-5688] [Sema] Handle key path component base type on MemberAccessOnOptionalBaseFailure (#32376)
* [CSDiagnostics] Adjusting MemberAccessOnOptionalBaseFailure to be able to handle key path component member base types * [tests] Adding regression tests for SR-5688 * [CSDiagnostics] Adjusting source range to diagnose/insert the fixes in correct location * [tests] Adjusting regression tests to handle the fixits * [AST] Creating an helper getSourceRange function for KeyPathExpr::Component * [ConstraintSystem] Store member base type when recording UnwrapOptionalBase fix * [AST] Creating a new diagnostic note for removing optional from written type * [CSDiagnostics] Adjusting logic around MemberAccessOnOptionalBaseFailure to emit the correct diagnostics and fixes * [tests] Adjusting regression tests to add subscript and key path root cases with respective diagnostics * [Diagnostics] Adjusting message to mention base type * [CSDiagnostics] Better naming for method/variable that represents base source range * [CSDiagnostics] Adjusting to use the stored base member only when member is a key path component. * [Diagnostics] Adjusting minor typos and code * [AST] Adjusting keypath root diagnostic note message for use unwrapped type * [CSDiagnostics] Adjusments in MemberAccessOnOptionalBaseFailure diagnostics as per suggestion * [tests] Adding more test cases for SR-5688 * [CSDiagnostics] Adjusting fixits for key path root and range for diagnostics * [CSSimplify] Attempt to diagnose InsertExplicitCall for optional function return types when possible on simplifyOptionalObjectConstraint * [tests] Adding TODO to improve the diagnostics refering to key path root infered as optional types * [CSDiagnostics] Adjusting comments * [CSSimplify] Adjusting logic on simplifyOptionalObjectConstraint to attempt InsertCall fix before remove unwrap * [CSDiagnostics] Adjust the logic to use resolveType on MemberAccessOnOptionalBaseFailure construction
1 parent 6aeca31 commit 43fd786

File tree

10 files changed

+216
-48
lines changed

10 files changed

+216
-48
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1075,6 +1075,9 @@ ERROR(optional_base_not_unwrapped,none,
10751075
NOTE(optional_base_chain,none,
10761076
"chain the optional using '?' to access member %0 only for non-'nil' "
10771077
"base values", (DeclNameRef))
1078+
NOTE(optional_base_remove_optional_for_keypath_root, none,
1079+
"use unwrapped type %0 as key path root", (Type))
1080+
10781081
ERROR(missing_unwrap_optional_try,none,
10791082
"value of optional type %0 not unwrapped; did you mean to use 'try!' "
10801083
"or chain with '?'?",

‎include/swift/AST/Expr.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5380,6 +5380,13 @@ class KeyPathExpr : public Expr {
53805380
return Loc;
53815381
}
53825382

5383+
SourceRange getSourceRange() const {
5384+
if (auto *indexExpr = getIndexExpr()) {
5385+
return indexExpr->getSourceRange();
5386+
}
5387+
return Loc;
5388+
}
5389+
53835390
Kind getKind() const {
53845391
return KindValue;
53855392
}

‎lib/Sema/CSDiagnostics.cpp

Lines changed: 53 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -984,37 +984,72 @@ bool MissingExplicitConversionFailure::diagnoseAsError() {
984984
return true;
985985
}
986986

987+
SourceRange MemberAccessOnOptionalBaseFailure::getSourceRange() const {
988+
if (auto componentPathElt =
989+
getLocator()->getLastElementAs<LocatorPathElt::KeyPathComponent>()) {
990+
auto anchor = getAnchor();
991+
auto keyPathExpr = castToExpr<KeyPathExpr>(anchor);
992+
if (componentPathElt->getIndex() == 0) {
993+
if (auto rootType = keyPathExpr->getRootType()) {
994+
return rootType->getSourceRange();
995+
}
996+
} else {
997+
auto componentIdx = componentPathElt->getIndex() - 1;
998+
auto component = keyPathExpr->getComponents()[componentIdx];
999+
return component.getSourceRange();
1000+
}
1001+
}
1002+
return FailureDiagnostic::getSourceRange();
1003+
}
1004+
9871005
bool MemberAccessOnOptionalBaseFailure::diagnoseAsError() {
988-
auto anchor = getAnchor();
989-
auto baseType = getType(anchor);
1006+
auto baseType = getMemberBaseType();
1007+
auto locator = getLocator();
1008+
9901009
bool resultIsOptional = ResultTypeIsOptional;
9911010

9921011
// If we've resolved the member overload to one that returns an optional
9931012
// type, then the result of the expression is optional (and we want to offer
9941013
// only a '?' fixit) even though the constraint system didn't need to add any
9951014
// additional optionality.
996-
auto overload = getOverloadChoiceIfAvailable(getLocator());
1015+
auto overload = getOverloadChoiceIfAvailable(locator);
9971016
if (overload && overload->openedType->getOptionalObjectType())
9981017
resultIsOptional = true;
9991018

10001019
auto unwrappedBaseType = baseType->getOptionalObjectType();
10011020
if (!unwrappedBaseType)
10021021
return false;
1003-
1004-
emitDiagnostic(diag::optional_base_not_unwrapped, baseType, Member,
1005-
unwrappedBaseType);
1006-
1007-
// FIXME: It would be nice to immediately offer "base?.member ?? defaultValue"
1008-
// for non-optional results where that would be appropriate. For the moment
1009-
// always offering "?" means that if the user chooses chaining, we'll end up
1010-
// in MissingOptionalUnwrapFailure:diagnose() to offer a default value during
1011-
// the next compile.
1012-
emitDiagnostic(diag::optional_base_chain, Member)
1013-
.fixItInsertAfter(getSourceRange().End, "?");
1014-
1015-
if (!resultIsOptional) {
1016-
emitDiagnostic(diag::unwrap_with_force_value)
1017-
.fixItInsertAfter(getSourceRange().End, "!");
1022+
1023+
auto sourceRange = getSourceRange();
1024+
1025+
emitDiagnostic(diag::optional_base_not_unwrapped,
1026+
baseType, Member, unwrappedBaseType);
1027+
1028+
auto componentPathElt =
1029+
locator->getLastElementAs<LocatorPathElt::KeyPathComponent>();
1030+
if (componentPathElt && componentPathElt->getIndex() == 0) {
1031+
// For members where the base type is an optional key path root
1032+
// let's emit a tailored note suggesting to use its unwrapped type.
1033+
auto *keyPathExpr = castToExpr<KeyPathExpr>(getAnchor());
1034+
if (auto rootType = keyPathExpr->getRootType()) {
1035+
emitDiagnostic(diag::optional_base_remove_optional_for_keypath_root,
1036+
unwrappedBaseType)
1037+
.fixItReplace(rootType->getSourceRange(),
1038+
unwrappedBaseType.getString());
1039+
}
1040+
} else {
1041+
// FIXME: It would be nice to immediately offer "base?.member ?? defaultValue"
1042+
// for non-optional results where that would be appropriate. For the moment
1043+
// always offering "?" means that if the user chooses chaining, we'll end up
1044+
// in MissingOptionalUnwrapFailure:diagnose() to offer a default value during
1045+
// the next compile.
1046+
emitDiagnostic(diag::optional_base_chain, Member)
1047+
.fixItInsertAfter(sourceRange.End, "?");
1048+
1049+
if (!resultIsOptional) {
1050+
emitDiagnostic(diag::unwrap_with_force_value)
1051+
.fixItInsertAfter(sourceRange.End, "!");
1052+
}
10181053
}
10191054

10201055
return true;

‎lib/Sema/CSDiagnostics.h

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -479,16 +479,32 @@ class LabelingFailure final : public FailureDiagnostic {
479479
/// type without optional chaining or force-unwrapping it first.
480480
class MemberAccessOnOptionalBaseFailure final : public FailureDiagnostic {
481481
DeclNameRef Member;
482+
Type MemberBaseType;
482483
bool ResultTypeIsOptional;
483484

484485
public:
485486
MemberAccessOnOptionalBaseFailure(const Solution &solution,
486487
ConstraintLocator *locator,
487-
DeclNameRef memberName, bool resultOptional)
488+
DeclNameRef memberName,
489+
Type memberBaseType,
490+
bool resultOptional)
488491
: FailureDiagnostic(solution, locator), Member(memberName),
492+
MemberBaseType(resolveType(memberBaseType)),
489493
ResultTypeIsOptional(resultOptional) {}
490494

491495
bool diagnoseAsError() override;
496+
497+
Type getMemberBaseType() const {
498+
return MemberBaseType;
499+
}
500+
501+
SourceLoc getLoc() const override {
502+
// The end location points to the dot in the member access.
503+
return getSourceRange().End;
504+
}
505+
506+
SourceRange getSourceRange() const override;
507+
492508
};
493509

494510
/// Diagnose errors associated with rvalues in positions

‎lib/Sema/CSFix.cpp

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,21 +83,24 @@ bool UnwrapOptionalBase::diagnose(const Solution &solution, bool asNote) const {
8383
bool resultIsOptional =
8484
getKind() == FixKind::UnwrapOptionalBaseWithOptionalResult;
8585
MemberAccessOnOptionalBaseFailure failure(solution, getLocator(), MemberName,
86-
resultIsOptional);
86+
MemberBaseType, resultIsOptional);
8787
return failure.diagnose(asNote);
8888
}
8989

9090
UnwrapOptionalBase *UnwrapOptionalBase::create(ConstraintSystem &cs,
9191
DeclNameRef member,
92+
Type memberBaseType,
9293
ConstraintLocator *locator) {
93-
return new (cs.getAllocator())
94-
UnwrapOptionalBase(cs, FixKind::UnwrapOptionalBase, member, locator);
94+
return new (cs.getAllocator()) UnwrapOptionalBase(
95+
cs, FixKind::UnwrapOptionalBase, member, memberBaseType, locator);
9596
}
9697

9798
UnwrapOptionalBase *UnwrapOptionalBase::createWithOptionalResult(
98-
ConstraintSystem &cs, DeclNameRef member, ConstraintLocator *locator) {
99-
return new (cs.getAllocator()) UnwrapOptionalBase(
100-
cs, FixKind::UnwrapOptionalBaseWithOptionalResult, member, locator);
99+
ConstraintSystem &cs, DeclNameRef member, Type memberBaseType,
100+
ConstraintLocator *locator) {
101+
return new (cs.getAllocator())
102+
UnwrapOptionalBase(cs, FixKind::UnwrapOptionalBaseWithOptionalResult,
103+
member, memberBaseType, locator);
101104
}
102105

103106
bool AddAddressOf::diagnose(const Solution &solution, bool asNote) const {

‎lib/Sema/CSFix.h

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -336,10 +336,12 @@ class ConstraintFix {
336336
/// Unwrap an optional base when we have a member access.
337337
class UnwrapOptionalBase final : public ConstraintFix {
338338
DeclNameRef MemberName;
339+
Type MemberBaseType;
339340

340341
UnwrapOptionalBase(ConstraintSystem &cs, FixKind kind, DeclNameRef member,
341-
ConstraintLocator *locator)
342-
: ConstraintFix(cs, kind, locator), MemberName(member) {
342+
Type memberBaseType, ConstraintLocator *locator)
343+
: ConstraintFix(cs, kind, locator), MemberName(member),
344+
MemberBaseType(memberBaseType) {
343345
assert(kind == FixKind::UnwrapOptionalBase ||
344346
kind == FixKind::UnwrapOptionalBaseWithOptionalResult);
345347
}
@@ -352,11 +354,12 @@ class UnwrapOptionalBase final : public ConstraintFix {
352354
bool diagnose(const Solution &solution, bool asNote = false) const override;
353355

354356
static UnwrapOptionalBase *create(ConstraintSystem &cs, DeclNameRef member,
357+
Type memberBaseType,
355358
ConstraintLocator *locator);
356359

357360
static UnwrapOptionalBase *
358361
createWithOptionalResult(ConstraintSystem &cs, DeclNameRef member,
359-
ConstraintLocator *locator);
362+
Type memberBaseType, ConstraintLocator *locator);
360363
};
361364

362365
// Treat rvalue as if it was an lvalue

‎lib/Sema/CSSimplify.cpp

Lines changed: 34 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -5697,26 +5697,41 @@ ConstraintSystem::simplifyOptionalObjectConstraint(
56975697
// If the base type is not optional, let's attempt a fix (if possible)
56985698
// and assume that `!` is just not there.
56995699
if (!objectTy) {
5700+
if (!shouldAttemptFixes())
5701+
return SolutionKind::Error;
5702+
57005703
// Let's see if we can apply a specific fix here.
5701-
if (shouldAttemptFixes()) {
5702-
if (optTy->isHole())
5703-
return SolutionKind::Solved;
5704-
5705-
auto *fix =
5706-
RemoveUnwrap::create(*this, optTy, getConstraintLocator(locator));
5704+
if (optTy->isHole())
5705+
return SolutionKind::Solved;
5706+
5707+
auto fnType = optTy->getAs<FunctionType>();
5708+
if (fnType && fnType->getNumParams() == 0) {
5709+
// For function types with no parameters, let's try to
5710+
// offer a "make it a call" fix if possible.
5711+
auto optionalResultType = fnType->getResult()->getOptionalObjectType();
5712+
if (optionalResultType) {
5713+
if (matchTypes(optionalResultType, second, ConstraintKind::Bind,
5714+
flags | TMF_ApplyingFix, locator)
5715+
.isSuccess()) {
5716+
auto *fix =
5717+
InsertExplicitCall::create(*this, getConstraintLocator(locator));
5718+
5719+
return recordFix(fix) ? SolutionKind::Error : SolutionKind::Solved;
5720+
}
5721+
}
5722+
}
57075723

5708-
if (recordFix(fix))
5709-
return SolutionKind::Error;
5724+
auto *fix =
5725+
RemoveUnwrap::create(*this, optTy, getConstraintLocator(locator));
57105726

5711-
// If the fix was successful let's record
5712-
// "fixed" object type and continue.
5713-
objectTy = optTy;
5714-
} else {
5715-
// If fixes are not allowed, no choice but to fail.
5727+
if (recordFix(fix))
57165728
return SolutionKind::Error;
5717-
}
5729+
5730+
// If the fix was successful let's record
5731+
// "fixed" object type and continue.
5732+
objectTy = optTy;
57185733
}
5719-
5734+
57205735
// The object type is an lvalue if the optional was.
57215736
if (optLValueTy->is<LValueType>())
57225737
objectTy = LValueType::get(objectTy);
@@ -6950,11 +6965,12 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint(
69506965
SmallVector<Constraint *, 2> optionalities;
69516966
auto nonoptionalResult = Constraint::createFixed(
69526967
*this, ConstraintKind::Bind,
6953-
UnwrapOptionalBase::create(*this, member, locator), memberTy, innerTV,
6954-
locator);
6968+
UnwrapOptionalBase::create(*this, member, baseObjTy, locator),
6969+
memberTy, innerTV, locator);
69556970
auto optionalResult = Constraint::createFixed(
69566971
*this, ConstraintKind::Bind,
6957-
UnwrapOptionalBase::createWithOptionalResult(*this, member, locator),
6972+
UnwrapOptionalBase::createWithOptionalResult(*this, member,
6973+
baseObjTy, locator),
69586974
optTy, memberTy, locator);
69596975
optionalities.push_back(nonoptionalResult);
69606976
optionalities.push_back(optionalResult);

‎test/Constraints/diagnostics.swift

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1342,3 +1342,10 @@ func rdar62989214() {
13421342
arr[flag].isTrue // expected-error {{cannot convert value of type 'Flag' to expected argument type 'Int'}}
13431343
}
13441344
}
1345+
1346+
// SR-5688
1347+
func SR5688_1() -> String? { "" }
1348+
SR5688_1!.count // expected-error {{function 'SR5688_1' was used as a property; add () to call it}} {{9-9=()}}
1349+
1350+
func SR5688_2() -> Int? { 0 }
1351+
let _: Int = SR5688_2! // expected-error {{function 'SR5688_2' was used as a property; add () to call it}} {{22-22=()}}

‎test/Constraints/fixes.swift

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,7 @@ class T {
110110
func m1() {
111111
// <rdar://problem/17741575>
112112
let l = self.m2!.prop1
113-
// expected-error@-1 {{cannot force unwrap value of non-optional type '() -> U?'}} {{22-23=}}
114-
// expected-error@-2 {{method 'm2' was used as a property; add () to call it}} {{22-22=()}}
113+
// expected-error@-1 {{method 'm2' was used as a property; add () to call it}} {{22-22=()}}
115114
}
116115

117116
func m2() -> U! {

‎test/expr/unary/keypath/keypath.swift

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -930,6 +930,85 @@ func testMissingMember() {
930930
_ = \String.x.y // expected-error {{value of type 'String' has no member 'x'}}
931931
}
932932

933+
// SR-5688
934+
struct SR5688_A {
935+
var b: SR5688_B?
936+
}
937+
938+
struct SR5688_AA {
939+
var b: SR5688_B
940+
}
941+
942+
struct SR5688_B {
943+
var m: Int
944+
var c: SR5688_C?
945+
}
946+
947+
struct SR5688_C {
948+
var d: Int
949+
}
950+
951+
struct SR5688_S {
952+
subscript(_ x: Int) -> String? { "" }
953+
}
954+
955+
struct SR5688_O {
956+
struct Nested {
957+
var foo = ""
958+
}
959+
}
960+
961+
func SR5688_KP(_ kp: KeyPath<String?, Int>) {}
962+
963+
func testMemberAccessOnOptionalKeyPathComponent() {
964+
965+
_ = \SR5688_A.b.m
966+
// expected-error@-1 {{value of optional type 'SR5688_B?' must be unwrapped to refer to member 'm' of wrapped base type 'SR5688_B'}}
967+
// expected-note@-2 {{chain the optional using '?' to access member 'm' only for non-'nil' base values}} {{18-18=?}}
968+
// expected-note@-3 {{force-unwrap using '!' to abort execution if the optional value contains 'nil'}} {{18-18=!}}
969+
970+
_ = \SR5688_A.b.c.d
971+
// expected-error@-1 {{value of optional type 'SR5688_B?' must be unwrapped to refer to member 'c' of wrapped base type 'SR5688_B'}}
972+
// expected-note@-2 {{chain the optional using '?' to access member 'c' only for non-'nil' base values}} {{18-18=?}}
973+
// expected-error@-3 {{value of optional type 'SR5688_C?' must be unwrapped to refer to member 'd' of wrapped base type 'SR5688_C'}}
974+
// expected-note@-4 {{chain the optional using '?' to access member 'd' only for non-'nil' base values}} {{20-20=?}}
975+
// expected-note@-5 {{force-unwrap using '!' to abort execution if the optional value contains 'nil'}} {{20-20=!}}
976+
_ = \SR5688_A.b?.c.d
977+
// expected-error@-1 {{value of optional type 'SR5688_C?' must be unwrapped to refer to member 'd' of wrapped base type 'SR5688_C'}}
978+
// expected-note@-2 {{chain the optional using '?' to access member 'd' only for non-'nil' base values}} {{21-21=?}}
979+
980+
_ = \SR5688_AA.b.c.d
981+
// expected-error@-1 {{value of optional type 'SR5688_C?' must be unwrapped to refer to member 'd' of wrapped base type 'SR5688_C'}}
982+
// expected-note@-2 {{chain the optional using '?' to access member 'd' only for non-'nil' base values}} {{21-21=?}}
983+
// expected-note@-3 {{force-unwrap using '!' to abort execution if the optional value contains 'nil'}} {{21-21=!}}
984+
985+
\String?.count
986+
// expected-error@-1 {{value of optional type 'String?' must be unwrapped to refer to member 'count' of wrapped base type 'String'}}
987+
// expected-note@-2 {{use unwrapped type 'String' as key path root}} {{4-11=String}}
988+
989+
\Optional<String>.count
990+
// expected-error@-1 {{value of optional type 'Optional<String>' must be unwrapped to refer to member 'count' of wrapped base type 'String'}}
991+
// expected-note@-2 {{use unwrapped type 'String' as key path root}} {{4-20=String}}
992+
993+
\SR5688_S.[5].count
994+
// expected-error@-1 {{value of optional type 'String?' must be unwrapped to refer to member 'count' of wrapped base type 'String'}}
995+
// expected-note@-2 {{chain the optional using '?' to access member 'count' only for non-'nil' base values}}{{16-16=?}}
996+
// expected-note@-3 {{force-unwrap using '!' to abort execution if the optional value contains 'nil'}}{{16-16=!}}
997+
998+
999+
\SR5688_O.Nested?.foo.count
1000+
// expected-error@-1 {{value of optional type 'SR5688_O.Nested?' must be unwrapped to refer to member 'foo' of wrapped base type 'SR5688_O.Nested'}}
1001+
// expected-note@-2 {{use unwrapped type 'SR5688_O.Nested' as key path root}}{{4-20=SR5688_O.Nested}}
1002+
1003+
\(Int, Int)?.0
1004+
// expected-error@-1 {{value of optional type '(Int, Int)?' must be unwrapped to refer to member '0' of wrapped base type '(Int, Int)'}}
1005+
// expected-note@-2 {{use unwrapped type '(Int, Int)' as key path root}}{{4-15=(Int, Int)}}
1006+
1007+
// TODO(diagnostics) Improve diagnostics refering to key path root not able to be infered as an optional type.
1008+
SR5688_KP(\.count)
1009+
// expected-error@-1 {{value of optional type 'String?' must be unwrapped to refer to member 'count' of wrapped base type 'String'}}
1010+
}
1011+
9331012
func testSyntaxErrors() { // expected-note{{}}
9341013
_ = \. ; // expected-error{{expected member name following '.'}}
9351014
_ = \.a ;

0 commit comments

Comments
 (0)