Function builders: pre-check the original closure body in-place #26286
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@swift-ci Please test. |
DougGregor
approved these changes
Jul 23, 2019
|
Build failed |
|
Build failed |
rjmccall
force-pushed
the
precheck-function-builders
branch
from
136685a to
3891274
Compare
|
@swift-ci Please test. |
|
Build failed |
|
Build failed |
xedin
approved these changes
Jul 23, 2019
lib/Sema/BuilderTransform.cpp
Outdated
| return FunctionBuilderClosurePreCheck::Okay; | ||
| } | ||
|
|
||
| virtual std::pair<bool, Expr *> walkToExprPre(Expr *E) { |
There was a problem hiding this comment.
Nit: did you mean override instead of virtual here and in walkToStmtPre?
There was a problem hiding this comment.
Yes, I'll fix that. Thanks!
Prior to this patch, we pre-checked the result of applying the function-builder transformation, but only when we hadn't already pre-checked the closure before. This causes two problems to arise when the transformation is applied to the same closure along multiple branches of a disjunction. The first is that any expressions that are synthesized by the transformation will not be pre-checked the second time through, which is a problem if we try to apply different builder types to the same closure (we do cache expressions for identical builder types). The second is that the pre-check will rewrite sub-expressions in place *in the synthesized expression*, which means that top-level expressions in the original closure body (including `if` conditions) that are now nested in the synthesized expression will not be rewritten in the original closure and therefore will be encountered in their raw state the second time through. This patch causes all expressions in the original closure body to be pre-checked before doing any other work. We then pre-check the synthesized expression immediately before generating constraints for it in order to set up the AST appropriately for CSGen; this could be skipped if we just synthesized expressions the way that CSGen wants them, but that seems to be somewhat involved. Pre-checking is safe to apply to an expression multiple times, so it's fine if we take this path and then decide not to use a function builder. I've also merged the check for `return` statements into this same walk, which was convenient. Fixes rdar://53325810 at least, and probably also some bugs with applying different function builders to the same closure.
rjmccall
force-pushed
the
precheck-function-builders
branch
from
3891274 to
33fbfaf
Compare
|
@swift-ci Please test. |
|
@swift-ci Please test source compatibility. |
|
Build failed |
|
Build failed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this patch, we pre-checked the result of applying the function-builder transformation, but only when we hadn't already pre-checked the closure before. This causes two problems to arise when the transformation is applied to the same closure along multiple branches of a disjunction. The first is that any expressions that are synthesized by the transformation will not be pre-checked the second time through, which is a problem if we try to apply different builder types to the same closure (we do cache expressions for identical builder types). The second is that the pre-check will rewrite sub-expressions in place in the synthesized expression, which means that top-level expressions in the original closure body (including
ifconditions) that are now nested in the synthesized expression will not be rewritten in the original closure and therefore will be encountered in their raw state the second time through.This patch causes all expressions in the original closure body to be pre-checked before doing any other work. We then pre-check the synthesized expression immediately before generating constraints for it in order to set up the AST appropriately for CSGen; this could be skipped if we just synthesized expressions the way that CSGen wants them, but that seems to be somewhat involved.
I've also merged the check for
returnstatements into this same walk, which was convenient.Pre-checking is safe to apply to an expression multiple times, so it's fine if we take this path and then decide not to use a function builder.
Fixes rdar://53325810 at least, and probably also some bugs with applying different function builders to the same closure.