Skip to content

[CodeCompletion] Avoid re-typechecking pre-checked expressions #31774

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

Merged
merged 2 commits into from
May 14, 2020

Conversation

rintaro
Copy link
Member

@rintaro rintaro commented May 14, 2020

Implicit call expressions for "KeyPath expression as functions" weren't sanitized in SanitizeExpr. That leaves AutoClosureExpr in the AST before re-typechecking, and caused crashes.

Handle them in SanitizeExpr. (the first commit)

But we don't need to re-typecheck them in the first place. (the second commit)

rdar://problem/60982638

rintaro added 2 commits May 13, 2020 16:59
in expression context analysis. They are simply not necessary.

rdar://problem/60982638
@rintaro
Copy link
Member Author

rintaro commented May 14, 2020

@swift-ci Please smoke test

@rintaro rintaro requested review from xedin and benlangmuir May 14, 2020 00:03
@rintaro
Copy link
Member Author

rintaro commented May 14, 2020

@xedin Could you review SanitizeExpr part? This is now unreachable as far as we know, but it should avoid similar crashes in future.

Copy link
Contributor

@xedin xedin left a comment

Choose a reason for hiding this comment

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

LGTM!

@slavapestov
Copy link
Contributor

slavapestov commented May 14, 2020

If SanitizeExpr is unreachable (it's reachable because we call it every time we type check an expression! But if there's no re-checking happening, it should be a no-op), we should just remove it. There's no way to make it correct for the general case, and it has itself caused bugs in the past.

@xedin
Copy link
Contributor

xedin commented May 14, 2020

One we integrate code completion into solutions with fixes scheme we can finally deprecate SanitizeExpr since there is going to be no re-typechecking left.

@rintaro
Copy link
Member Author

rintaro commented May 14, 2020

@slavapestov Unfortunately we cannot remove SanitizeExpr yet because we still have some code path that calls re-typechecking. If we are 100% sure that normal compilations never do re-typechecking, we can move SanitizeExpr out of CSGen and call it manually.


auto baseTy = (*baseTyOpt)->getWithoutSpecifierType();
Type baseTy = baseExpr->getType();
if (!baseTy || baseTy->is<ErrorType>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it safe to re-check when it's ErrorType?

Copy link
Member Author

@rintaro rintaro May 14, 2020

Choose a reason for hiding this comment

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

It should be safe for any expression as long as SanitizeExpr works correctly.
This change is a kind of optimization which avoids unnecessary re-typechecking.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this change does not affect the correctness of this patch?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. The first patch fixes the crash. The second patch doesn't affect the result of it.

@rintaro rintaro merged commit b8f6471 into swiftlang:master May 14, 2020
@rintaro rintaro deleted the ide-completion-rdar60982638 branch May 14, 2020 19:48
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.

4 participants