[CS] Eagerly produce holes in a couple of places #85527
Conversation
hamishknight
requested review from
CodaFi,
DougGregor,
bnbarham,
hborla,
rintaro,
slavapestov and
xedin
as code owners
| @@ -1,4 +1,4 @@ | |||
| // {"kind":"typecheck","signature":"(anonymous namespace)::ConnectedComponents::unionSets(swift::TypeVariableType*, swift::TypeVariableType*)","signatureAssert":"Assertion failed: (validComponentCount > 0), function unionSets"} | |||
| // RUN: not --crash %target-swift-frontend -typecheck %s | |||
| // RUN: not %target-swift-frontend -typecheck %s | |||
There was a problem hiding this comment.
The underlying crashes here aren't really fixed, I've added some similar variants as part of #85407, and the fuzzer should hopefully pick up more cases for the rest
xedin
left a comment
xedin
left a comment
There was a problem hiding this comment.
I think the next step here is to be more aggressively with holes in ApplicableFunction constraint. I was thinking that we can bind right-hand side of that constraint to a placeholder if one of the arguments on the left is and skip attempting a disjunction in such case. That would help with performance for situations when a missing member is involved in a complex expression like an operator chain because it would propagate the holes to the right of the missing member.
| // Eagerly turn the member type variable into a hole since we know | ||
| // this is where the issue is and we've recorded a fix for it. This | ||
| // avoids producing unnecessary holes for e.g generic parameters. | ||
| recordTypeVariablesAsHoles(memberTypeVar); |
There was a problem hiding this comment.
We didn't do this originally because sometimes it still helpful to wait and infer member type from the context to help avoid _ in diagnostics but that indeed leads to some unnecessary hole propagation. But we do want to be more aggressive with missing members so I think this is okay to try and see how it goes.
There was a problem hiding this comment.
Yeah I think we ought to be able to handle those cases in repairFailures instead
We know this is where the issue is so we ought to always produce a concrete hole.
We know this is where the issue is so we can immediately bind to a hole, ensuring we don't produce unnecessary downstream diagnostics from things we can't infer.
hamishknight
force-pushed
the
the-whole-hole
branch
from
68d22b0 to
61a5ae8
Compare
|
@swift-ci please test |
| let _ = [i, j, k].reduce(0 as Int?) { // expected-error {{missing argument label 'into:' in call}} | ||
| // expected-error@-1 {{cannot convert value of type 'Int?' to expected argument type '(inout (Bool, Bool) -> Bool?, Int?) throws -> ()'}} | ||
| $0 && $1 ? $0 + $1 : ($0 ? $0 : ($1 ? $1 : nil)) | ||
| // expected-error@-1 {{binary operator '+' cannot be applied to two 'Bool' operands}} |
There was a problem hiding this comment.
Missed the fact that the diagnostic changed here, this is because the extra check for hole in repairFailures, previously we were recording a redundant fix and then declining to emit a diagnostic for it, so now we're picking a different overload of +. The original diagnostic wasn't very good anyway, so I'm going to leave this for a follow-up since it doesn't immediately look like a straightforward fix
There was a problem hiding this comment.
Yeah, this expression is very tricky to diagnose properly…
|
@swift-ci please test Windows |
|
@swift-ci please smoke test Linux |
2 participants
Split this out of #83724 since it's also needed for another patch I'm working on. If we have a missing member or invalid expression, eagerly produce a hole since we know that's where the issue is.