[Sema] Improve type coercion fixits for optional-to-optional conversions. #18757
Conversation
|
@swift-ci Please smoke test. |
|
LGTM! |
|
@swift-ci Please smoke test and merge. |
lib/Sema/CSDiag.cpp
Outdated
| @@ -2883,7 +2887,8 @@ static bool addTypeCoerceFixit(InFlightDiagnostic &diag, ConstraintSystem &CS, | |||
| diag.fixItInsert( | |||
| Lexer::getLocForEndOfToken(CS.DC->getASTContext().SourceMgr, | |||
| expr->getEndLoc()), | |||
| (llvm::Twine(canUseAs ? " as " : " as! ") + OS.str()).str()); | |||
| (llvm::Twine(canUseAs ? " as " : " as! ") + OS.str() + | |||
| llvm::Twine(canUseAs && bothOptional ? "?" : "")).str()); | |||
There was a problem hiding this comment.
This is a little iffy; not all types can just have ? stuck on the end. (Consider () -> Void.) It'd be better to add one layer back in the both-optional case.
There was a problem hiding this comment.
Wait but both types already have to be optional to produce this fix, right?
There was a problem hiding this comment.
But then it was stripped the optional out (both for the coerce check and then for the type producing the fixit).
There was a problem hiding this comment.
Sorry what I mean is - if both types were optional before they could be again right?
There was a problem hiding this comment.
Doesn't () -> Void just need parens around it to be optional e.g. (() -> Void)?
There was a problem hiding this comment.
Right. Before Greg was adding the ? manually, but I pointed out that sometimes that's not good enough. So now he's rebuilding the optional type and having it print itself.
There was a problem hiding this comment.
Ah sorry for the noise, I totally misunderstood...
|
@swift-ci Please smoke test. |
| bool canUseAs = Kind == CheckedCastKind::Coercion || | ||
| Kind == CheckedCastKind::BridgingCoercion; | ||
| if (bothOptional && canUseAs) | ||
| toType = OptionalType::get(toType); | ||
| toType->print(OS); |
There was a problem hiding this comment.
…and then it's worth adding a test case.
There was a problem hiding this comment.
Hmm. Do you have any suggestions for a more complex test case? Function types can't actually be coerced this way (they end up as CheckedCastKind::ValueCast)
There was a problem hiding this comment.
Protocol types might be good enough? someOptionalInt as (NSObject & NSCopying)?
|
@swift-ci Please smoke test. |
This is a followup to @xedin's comments in #18324 about the less than ideal fixits for coercions between optionals. If from and to are both optional, and we could coerce the non-optional types to each other, suggest
as T?instead ofas! T.