[CS] Allow ExprPatterns to be type-checked in the solver #64280
Merged
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
hamishknight
requested review from
hborla,
slavapestov,
xedin,
ahoppen,
bnbarham,
CodaFi,
DougGregor and
rintaro
as code owners
hamishknight
commented
Mar 10, 2023
hamishknight
commented
Mar 10, 2023
xedin
reviewed
Mar 13, 2023
xedin
approved these changes
Mar 13, 2023
There was a problem hiding this comment.
LGTM! It might make sense to have an alternative version of this where we either use a TypeJoinExpr to type-check expr patterns that convert to the same generic parameter or simply enable bi-directional checking for patterns since each pattern is solved separately anyway and it doesn't seem like they are too complex.
hamishknight
force-pushed
the
platypus
branch
2 times, most recently
from
18588c1 to
51d0721
Compare
xedin
approved these changes
Apr 12, 2023
| EEP->setType(patternTy); | ||
| return EEP; | ||
| } else { | ||
| // ...but for non-optional types it can never match! Diagnose it. |
There was a problem hiding this comment.
Can this be detected during solving?
There was a problem hiding this comment.
Yes, but we also need to diagnose for cases that don't yet go through the constraint system. We should be able to move this into the constraint system when we delete coercePatternToType
There was a problem hiding this comment.
Fair enough! We could start with cases that do go through the solver and expand later too :)
hamishknight
force-pushed
the
platypus
branch
2 times, most recently
from
b521f6f to
11a0e88
Compare
hamishknight
force-pushed
the
platypus
branch
4 times, most recently
from
2a3cab0 to
06bc822
Compare
hamishknight
force-pushed
the
platypus
branch
2 times, most recently
from
b370519 to
5ee2fc8
Compare
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
|
@swift-ci please SourceKit stress test |
xedin
approved these changes
Jun 6, 2023
The TypedPattern and IsPattern constraints were incorrectly written, with conversions propagating out of the patterns, when really conversions ought to propagate into patterns. In any case, it seems like we really want equality here. Fix the constraints to use equality, and have the cast constraint operate on the external pattern type instead of the subpattern type.
Order them such that if they were changed to conversions, they would be sound. This shouldn't make a difference, but unfortunately it turns out pattern equality is not symmetric. As such, tweak the pattern equality logic to account for the reversed types. This allows us to remove a special case from function matching.
This shouldn't be necessary, we should be able to solve with type variables instead. This makes sure we don't end up with weird special cases that only occur when an external type is present.
Push the only null case that can occur up into the caller.
We should never CSGen a null Type for patterns.
Instead of walking the single ASTNode from the target, walk all AST nodes associated with the target to find the completion expr. This is needed to find the completion expr in a pattern for an initialization target.
Previously we would wait until CSApply, which would trigger their type-checking in `coercePatternToType`. This caused a number of bugs, and hampered solver-based completion, which does not run CSApply. Instead, form a conjunction of all the ExprPatterns present, which preserves some of the previous isolation behavior (though does not provide complete isolation). We can then modify `coercePatternToType` to accept a closure, which allows the solver to take over rewriting the ExprPatterns it has already solved. This then sets the stage for the complete removal of `coercePatternToType`, and doing all pattern type-checking in the solver.
This is wrong because there's nowhere to put any conversion that is introduced, meaning that we'll likely crash in SILGen. Change the constraint to equality, which matches what we do outside of the constraint system. rdar://107709341
There's still plenty of more work to do here for pattern diagnostics, including introducing a bunch of new locator elements, and handling things like argument list mismatches. This at least lets us fall back to a generic mismatch diagnostic.
Previously if the cast was unresolved, we would emit a warning and bail with `nullptr`. This is wrong, because the caller expects a `nullptr` return to mean we emitted an error. Change the diagnostic to an error to fix this. This may appear source breaking, but in reality previously we were failing to add the cast at all in this case, which lead to a crash in SILGen. We really do want to reject these cases as errors, as this will give us a better opportunity to fall back to type-checking as ExprPatterns, and better matches the constraint solver type-checking. Also while we're here, change the diagnostic for the case where we don't have an existential context type from the confusing "enum doesn't have member" diagnostic to the pattern mismatch diagnostic. rdar://107420031
|
Going to stick to the conjunction implementation for now |
|
@swift-ci please smoke test and merge |
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.
Previously we would wait until CSApply, which would trigger their type-checking in
coercePatternToType. This caused a number of bugs, and hampered solver-based completion, which does not run CSApply. Instead, type-check ExprPatterns as a part of solving the overall pattern. We can then modifycoercePatternToTypeto accept a closure, which allows the solver to take over rewriting the ExprPatterns it has already solved.This then sets the stage for the complete removal of
coercePatternToType, and doing all pattern type-checking in the solver.This PR also includes a couple of cleanups/fixes for
getTypeForPattern, and some other pattern crasher fixes.Resolves #61850
Resolves #63374
rdar://104954155
rdar://105782480
rdar://106598067
rdar://107709341
rdar://107420031
rdar://109419240