[Sema] More diagnostic improvement when an optional value is not unwrapped. #18094
Conversation
lib/Sema/CSSimplify.cpp
Outdated
| // (for '!' or optional members with '?'), or the original member type with | ||
| // one extra level of optionality ('?' with non-optional members). | ||
| auto innerTV = | ||
| createTypeVariable(locator, TVO_CanBindToLValue | TVO_CanBindToInOut); |
There was a problem hiding this comment.
This shouldn't be able to bind to inout.
lib/Sema/CSSimplify.cpp
Outdated
| @@ -3478,6 +3478,7 @@ ConstraintSystem::SolutionKind ConstraintSystem::simplifyMemberConstraint( | |||
| // Resolve the base type, if we can. If we can't resolve the base type, | |||
| // then we can't solve this constraint. | |||
| // FIXME: simplifyType() call here could be getFixedTypeRecursive? | |||
There was a problem hiding this comment.
Could you remove this comment (I know it's not yours)? The answer is no.
lib/Sema/CSSimplify.cpp
Outdated
| // implicit optional, don't try to fix it. The IUO will be forced instead. | ||
| auto resolvedOverload = getResolvedOverloadSets(); | ||
| while (resolvedOverload) { | ||
| if (resolvedOverload->BoundType->getCanonicalType() == |
There was a problem hiding this comment.
resolvedOverload->BoundType->isEqual(originalBaseTy)
|
Overall, the approach makes sense. I'm still unhappy with this whole arrangement - especially because CSSimplify is where the typing rules live and we should pull recovery modes like this out into CSDiag. I think you could shift this approach into CSDiag if you set up richer locators but I haven't looked into this deep enough to say more than that. Doug can say more here. |
lib/Sema/CSSimplify.cpp
Outdated
| // First type already appropriately set. | ||
| auto result = matchTypes(type1, type2, matchKind, subflags, locator); | ||
| if (result == SolutionKind::Solved) | ||
| if (recordFix(fix, locator)) |
There was a problem hiding this comment.
I suggest that you recordFix() first, then directly return the result of the call to matchTypes. That will handle the "unresolved" case appropriately, rather than losing the fix.
lib/Sema/CSSimplify.cpp
Outdated
| // implicit optional, don't try to fix it. The IUO will be forced instead. | ||
| auto resolvedOverload = getResolvedOverloadSets(); | ||
| while (resolvedOverload) { | ||
| if (resolvedOverload->BoundType->isEqual(originalBaseTy)) { |
There was a problem hiding this comment.
I don't think that checking BoundType for equality with originalBaseTy will suffice; it could match for unrelated cases. Could you check the locators here instead, like you're doing in CSApply.cpp?
|
|
||
| // Assumes that these fixes are always created in pairs, with the first | ||
| // created non-optional and the second with an added optional. | ||
| return (Data % 2) == 1; |
There was a problem hiding this comment.
That's very clever. I feel like the name of this method is misleading, though. Perhaps isUnwrapOptionalBaseByOptionalChaining()? I know it's long, but that's what we're looking for---the chaining case.
|
Thanks @CodaFi, @DougGregor! Made the requested changes. |
|
@swift-ci Please smoke test. |
|
@swift-ci Please smoke test and merge. |
|
@swift-ci Please smoke test. |
|
Would you mind rebasing before we kick off a testing cycle? |
|
Yep, my fault for messing around with github's web interface to mis-perform a trivial merge :) Rebased and squashed. |
member access if optional chaining is sure to be valid.
|
@swift-ci Please smoke test. |
|
Thanks! |
This builds on Doug's pull #17921 by bringing in a simpler non-recursing subset of my pull #15321.
The changes here create a disjunction for the result of a
FixKind::UnwrapOptionalBasesince the result can be either the original member type (for!or?on members that return optionals) or an optional of the member type (for?on members returning non-optional). Then which of the two is chosen determines the fixit notes produced.To be cautious about what the user intends (and because it only looks at a single step instead of a potential multi-step chain), this continues to always offer optional chaining as a possible fixit, but now it won't suggest force-unwrapping where that doesn't make sense.