Skip to content

[Sema] Fix rdar://75200446 #36494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Conversation

ahoppen
Copy link
Member

@ahoppen ahoppen commented Mar 18, 2021

Similar to #36493, I don’t actually know what I’m changing here, but #36028 appears to have been to optimistic to declare the restored branch as unreachable, which causes a crash in the added test case. I’m just restoring the old behavior to prevent the crash. Please let me know if you think the code path should really be unreachable and we need to fix a deeper issue the way code completion sets up the constraint system.

Fixes rdar://75200446 [SR-14318]

It appears like the simplification in 9d3c8ca was a little over-optimistic to declare the code path unreachable and causes a crash in the test case added by this commit.  Restore the old code code path.

Fixes rdar://75200446
@ahoppen ahoppen requested a review from slavapestov March 18, 2021 16:53
@@ -7577,7 +7577,11 @@ Expr *ExprRewriter::finishApply(ApplyExpr *apply, Type openedType,
// FIXME: handle unwrapping everywhere else
assert(!unwrapResult);

assert(!cs.getType(fn)->is<UnresolvedType>());
// If this is an UnresolvedType in the system, preserve it.
if (cs.getType(fn)->is<UnresolvedType>()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@slavapestov It looks like some of the recently removed code paths are still used by the code completion. Maybe we should revert #36028?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is code completion still submitting UnresolvedType ASTs for type checking?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume so 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xedin could we do this with holes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all of the code completion requests has been ported to use typeCheckForCodeCompletion yet, but the plan is to avoid having to apply solution and use holes everywhere. Meanwhile I think the most sensible thing would be to revert #36028.

@ahoppen
Copy link
Member Author

ahoppen commented Mar 19, 2021

@swift-ci Please smoke test

@ahoppen
Copy link
Member Author

ahoppen commented Mar 19, 2021

Closing this to revert #36028 in #36523.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants