[5.9] [CS] Allow ExprPatterns to be type-checked in the solver #65348
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
force-pushed
the
platypus-party-5.9
branch
from
f0c4fbd to
c441e03
Compare
AnthonyLatsis
added
🍒 release cherry pick
hamishknight
force-pushed
the
platypus-party-5.9
branch
3 times, most recently
from
d570f8a to
d5bd8c5
Compare
hamishknight
force-pushed
the
platypus-party-5.9
branch
from
d5bd8c5 to
3d1d5a3
Compare
hamishknight
force-pushed
the
platypus-party-5.9
branch
4 times, most recently
from
10ca9ff to
687bd25
Compare
hamishknight
force-pushed
the
platypus-party-5.9
branch
from
687bd25 to
8c759bd
Compare
This reverts commit 6c1642f.
Instead of assuming that `if let <expr>` is meant to be `if case <expr> = ...`, turn it into `if let _ = <expr>`, which is consistent with the fix-it we suggest. This currently doesn't have much of an effect on the diagnostics we produce, but will be important once we start doing bidirectional inference for ExprPatterns, as it avoids unhelpful diagnostics.
This seems better suited as its own fix, rather than as part of ContextualMismatch.
We shouldn't be allocating placeholders for type variables in the permanent arena, and we should be caching them such that equality works.
…`false` Returning `true` is wrong here as we could have the error diagnosed by another fix, which if not handled, would lead to us crashing as we assume we diagnosed the issue. Instead, return `false`, allowing us to at least bail with a bad error rather than a crash. We still need to go through and update argument list diagnostic logic to handle patterns, but I'm leaving that as future work for now. rdar://107724970 rdar://107651291
- Simplify the fix locator when looking for a fix already present in a pattern match, this avoids us emitting both a diagnostic for the argument conversion, and for a conformance failure. - Include tuples in the diagnostic logic where we emit a generic "operator cannot be applied" diagnostic, as a conformance diagnostic is unlikely to be helpful in that case.
We no longer need to check for a type variable here, it no longer regresses diagnostics. Also, while here, let's bump the impact for an Any/AnyObject missing conformance, as that's unlikely going to be helpful since they cannot conform to protocols even if the user wanted them to.
We can produce a hole here now without regressing diagnostics.
Rather than eagerly binding them to holes if the sequence element type ends up being Any, let's record the CollectionElementContextualMismatch fix, and then if the patterns end up becoming holes, skip penalizing them if we know the fix was recorded. This avoids prematurely turning type variables for ExprPatterns into holes, which should be able to get better bindings from the expression provided. Also this means we'll apply the logic to non-Any sequence types, which previously we would give a confusing diagnostic to.
This reverts commit 97be042.
We shouldn't be allocating placeholders for type variables in the permanent arena, and we should be caching them such that equality works. To achieve this, we need to introduce a new "solver allocated" type property. This is required because we don't want to mark placeholder types with type variable originators as themselves having type variables, as it's not part of their structural type. Also update ErrorType to use this bit, though I don't believe we currently create ErrorTypes with type variable originators.
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
hamishknight
force-pushed
the
platypus-party-5.9
branch
from
8c759bd to
59cd774
Compare
hamishknight
force-pushed
the
platypus-party-5.9
branch
from
59cd774 to
05ff1a4
Compare
|
@swift-ci please test |
airspeedswift
approved these changes
Jun 7, 2023
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.
5.9 cherry-pick of #64280 + #65744 + #65651 + #64566