Simplify Condition Resolution #6547
Conversation
CodaFi
force-pushed
the
i-have-a-little-shadow-that-goes-in-and-out-with-me
branch
from
b71ea2b to
588a5d5
Compare
|
@swift-ci please test. |
|
Build failed |
|
Build failed |
|
@swift-ci please smoke test. |
CodaFi
force-pushed
the
i-have-a-little-shadow-that-goes-in-and-out-with-me
branch
2 times, most recently
from
06322f5 to
3490a88
Compare
|
@swift-ci please test. |
|
There. @jrose-apple given the recent spate of issues, this is the version of this patch I'm most comfortable with. |
| @@ -5,6 +5,6 @@ | |||
| // See https://swift.org/LICENSE.txt for license information | |||
| // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | |||
|
|
|||
| // RUN: not --crash %target-swift-frontend %s -emit-ir | |||
| // RUN: not %target-swift-frontend %s -emit-ir | |||
| // REQUIRES: asserts | |||
There was a problem hiding this comment.
No need for the REQUIRES: asserts.
|
I guess this is the best we can do. Would still like @benlangmuir's opinion as well—it's annoying that we've added a language feature that means we can't parse without search paths set up. Maybe we can recover this later by taking @DougGregor's idea—the contents of active conditional blocks not having to be splatted into their parent. But the first priority is to restore source compat. |
|
I still strongly disagree with making parsing depend on (a) search paths, and (b) what is on disk in those search paths. Not only is this a performance issue for syntax-only parsing, but also leaves us unable to reliably reproduce syntax-only SourceKit bugs without knowing the state of the filesystem or command-line argument, both of which are harder to find out and are less likely to be provided in bug reports than the source code. We are stuck with that for semantic issues, but we shouldn't have to be for parsing. |
|
Then we're stuck with parse-only producing a slightly different AST for now. Or we turn the feature back off for 3.1 and give ourselves a little more time to plan. |
|
Trouble is we're also stuck with it for Parse issues. Parse depends on being able to have total knowledge of DeclRefs, classes introduced that depend on Foundation, and local type declarations in addition to structure in general. Condition Resolution demonstrated that at least the last two could be shifted to Sema, but without ASTScope or NameLookup taking the first Parse is going to wire up nonsense. Modeling conditions dynamically would make querying compound parts of the AST have to go through condition resolution which is still subject to at least one of the linked SRs (the one about deinitializers being especially concerning). |
|
The deinitializer one isn't so bad; the syntax-only parsing doesn't need to include members synthesized on demand. Sema can take care of that. We also don't actually care about resolving decl refs at all in a syntax-only parse. But we're out of time. If @benlangmuir isn't happy with this approach, we should…well, we should still take this patch, so that we go back to what we had before, but we should disable the support for |
SGTM |
|
OK, I'm going to remove support for the |
|
Thanks, Robert. I feel really bad that you've put all this work in (multiple implementations, effectively!) and we can't ship it yet. Ben and I talked a little more about what we think the syntax-only parser wants out of this and I'll send you and swift-dev an email on it later. |
|
Don't worry about it, I'm most concerned about keeping the compiler and language stable around the feature change. |
This reverts the contents of swiftlang#5778 and replaces it with a far simpler implementation of condition resolution along with canImport. When combined with the optimizations in swiftlang#6279 we get the best of both worlds with a performance win and a simpler implementation.
CodaFi
force-pushed
the
i-have-a-little-shadow-that-goes-in-and-out-with-me
branch
from
3490a88 to
d2570cb
Compare
|
@swift-ci please smoke test. |
jrose-apple
changed the title
|
I can't find the test anywhere, so @swift-ci please smoke test Linux platform. |
|
⛵️ |
This reverts the major content of #5778, the Condition Resolution pass, and replaces it with a far simpler implementation of condition resolution along with
canImport. When combined with the optimizations in #6279 we get the best of both worlds with a performance win and a simpler implementation.This is one of two ways towards a stable implementation of this feature. The concern here is that even with optimizations Parse will have to hit the disk at least once per
canImportstatement which could potentially slow down a very hot path. On the other hand, that cost cannot simply be deferred to Sema without significantly complicatingDeclRefresolution, semantic analysis, and condition resolution.@benlangmuir, @jrose-apple thoughts?
Resolves SR-3438, SR-3544, and SR-3546