Fix for issue #65913 - Compiler suggests a wrong insertion point for 'await' in if let ... #71716
Conversation
jameesbrown
requested review from
hborla,
slavapestov and
xedin
as code owners
|
@AnthonyLatsis I think this should resolve #65913, I added a test because It didn’t look like there was one for this diagnostic yet. I also reran all of the tests in Sema and smoke tested (manually) around optional binding/async compiler diagnostics. This is my first time contributing so please let me know if there’s something I didn’t do properly. |
|
@swift-ci Please smoke test |
|
Looks like the test failed because I did not add |
lib/Sema/TypeCheckEffects.cpp
Outdated
| // Check if we are looking at a shorthand optional binding. | ||
| // In that case, we'll want to supply a different fixIt. | ||
| const DeclRefExpr *declRefExpr; | ||
| if (const LoadExpr *loadExpr = dyn_cast<LoadExpr>(anchor)) { | ||
| declRefExpr = dyn_cast<DeclRefExpr>(loadExpr->getSubExpr()); | ||
| } else { | ||
| declRefExpr = dyn_cast<DeclRefExpr>(anchor); | ||
| } | ||
| if (declRefExpr && declRefExpr->isImplicit()) { | ||
| if (const VarDecl *varDecl = dyn_cast<VarDecl>(declRefExpr->getDecl())) { | ||
| fixIt = (varDecl->getNameStr() + " = await ").str(); | ||
| } | ||
| } |
There was a problem hiding this comment.
LoadExpr is not the only thing that can get in the way:
class A {}
class B: A {}
func test() async {
async let result: B? = nil
if let result: A {}
}Implicitness is a necessary condition for shorthand optional binding here, but I am not quite as certain it is sufficient too. You may have noticed by now that inserting result = await before a legal typed pattern as above will not work. The general solution is to insert after the pattern, so it seems we need to propagate the pattern one way or another for both a definitive check and the correct insertion location.
There was a problem hiding this comment.
Yes you're right, a more general solution that is aware of the actual pattern would be better (rather than inferring its existence like we are now). I had not thought of the case of inserting before a legal typed pattern, I may have been too focused on a specific set of cases. I will work on this some more and see what I can come up with. Thanks for taking a look at it and pointing me in the right direction!
There was a problem hiding this comment.
@AnthonyLatsis I was able to find a better solution, you'd given me a good hint. Even still, it took me a while to realize we probably have other fixItInsert functions. The new solution is to use fixItInsertAfter with the specialized fixIt given the conditions:
- The expression type is optional.
- Has a sub expression that is a DeclRefExpr and Implicit.
My reasoning is that I saw in ParseStmt.cpp, where we parse a shorthand optional binding we have:
else if (!ThePattern.getPtrOrNull()->getBoundName().empty()) {
auto bindingName = DeclNameRef(ThePattern.getPtrOrNull()->getBoundName());
auto loc = DeclNameLoc(ThePattern.getPtrOrNull()->getEndLoc());
auto declRefExpr = new (Context) UnresolvedDeclRefExpr(bindingName,
DeclRefKind::Ordinary,
loc);
declRefExpr->setImplicit();
Init = makeParserResult(declRefExpr);And I am thinking that implies 1 and 2, though I'm still unsure if it holds in the other direction.
There was a problem hiding this comment.
Hi @AnthonyLatsis, could you take another look at this for me when you get the chance?
There was a problem hiding this comment.
Hello, James. I’m sorry it took me so long to get back to you. What happened to the changes?
There was a problem hiding this comment.
Hi, sorry I was updating my checkout last night and switched the branch that I have this PR on without realizing it would close this. I reopened it with the changes on #72726.
jameesbrown
force-pushed
the
main
branch
3 times, most recently
from
a2e95eb to
5aad94d
Compare
Resolves #65913
I Added check in typeCheckEffects.cpp in the function diagnoseUncoveredAsyncSite(:) to see if we are looking at a shorthand version of optional binding. If so, we change the fix-it to be syntactically correct. Note that this still gives the correct fix-it for the non-shorthand version.