[Diagnostics] When building a subscript don't assume that overload is always present #5819
Conversation
|
/cc @rudkx @DougGregor |
| if (!selected.hasValue()) { | ||
| cs.TC.diagnose(base->getLoc(), diag::type_of_expression_is_ambiguous) | ||
| .highlight(base->getSourceRange()); | ||
| return nullptr; |
There was a problem hiding this comment.
This doesn't seem like a very helpful diagnostic in this case.
Consider this example I just made up:
var a = [1]
let b = { $0[$1] }(1, 1)
We hit the assert in getOverloadChoice(), but there's nothing ambiguous here.
It seems like instead we should be checking if base or index has ErrorType or UnresolvedType and providing more specific diagnostics in those cases.
There was a problem hiding this comment.
I think this example illustrates why we probably shouldn't get this far at all, too. I've looked at this bug in the past though and couldn't find a clean fix that avoided getting this far.
There was a problem hiding this comment.
@rudkx I agree that sometimes that diagnostic doesn't make much sense but the problem was is I simply return nullptr without diagnostic compiler would just keep going and fail with UnresolvedMemberExpr in wrong phase assertion, let me dig dipper into this, maybe I can make it better, I'll add your example to the closures.swift test case as well.
There was a problem hiding this comment.
@rudkx I've poked it a bit more from different angle, and it looks like even example with both parameters specified is going to be ambiguous, because original system would not produce any solutions for TupleExpr which contains parameters and failure diagnostics would start from re-checking "subscript" because it's a "function" in that case (when type-checking closure separately), it wouldn't be able to resolve $0 nor $1 (because there TupleExpr is not available in that context) and would fail with ambiguous result.
What I did to archive that is - i've added additional parameter to typeCheckChildIndependently which identifies if we are trying to re-check a closure and if so - don't allow unresolved types - such avoids failing with subscript too early and actually going and running failure diagnostics for sub-expressions of the closure containing subscript. Such allows to avoid diagnosing problem in buildSubscript directly, do you think it would be useful for you to take a look at that?
|
@rudkx I've improved error messages a bit e.g. it would look through error type to figure out what was the original one and changed message to be centered on subscript itself instead of simply saying that expression is ambiguous... I think it might be possible to improve situation related to closure calls (instead of trying to solve closure functions in the regular way in FailureDiagnosis::visitApplyExpr we can try to solve arguments first and then visitClosureExpr) but at the same time we can do that later on via separate PR... WDYT? |
|
@xedin Can you fix the conflicts when you have a moment and then I'll run tests and review? |
|
@rudkx Done! |
|
@swift-ci Please smoke test |
… always present This handles situation when overload for the subscript hasn't been resolved by constraint solver, such might happen, for example, if solver was allowed to produce solutions with free or unresolved type variables (e.g. when running diagnostics). Resolves: <rdar://problem/27329076>, <rdar://problem/28619118>, <rdar://problem/2778734>.
| @@ -1233,11 +1233,32 @@ namespace { | |||
| ConstraintLocatorBuilder locator, | |||
| bool isImplicit, AccessSemantics semantics) { | |||
| // Determine the declaration selected for this subscript operation. | |||
| auto selected = getOverloadChoice( | |||
| auto selected = getOverloadChoiceIfAvailable( | |||
There was a problem hiding this comment.
Huh, this makes me wonder: are all direct uses of getOverloadChoice within CSApply.cpp suspect because of this issue?
There was a problem hiding this comment.
They might be, I saw couple of valid ones too, do you want to be to double-check and maybe create separate PR to address any issues I find?
There was a problem hiding this comment.
That would be greatly appreciated!
| errorType->getOriginalType()) | ||
| .highlight(base->getSourceRange()); | ||
| } else { | ||
| tc.diagnose(base->getLoc(), diag::cannot_subscript_ambiguous_base) |
There was a problem hiding this comment.
Should we assert that baseType->hasError() or is an UnresolvedType? Are there other cases?
There was a problem hiding this comment.
I think depending if free type variables are allowed it might be a type variable as well, not sure how much sense would it make it add assertion here...
There was a problem hiding this comment.
I brought it up because I saw similar assertions in other places that used getOverloadChoiceIfAvailable, and I do think it's useful to have this assertion: it's fairly easy to make a mistake when trying to line up locators in CSGen, CSSimplify, and CSApply. In such cases, it's programmer error on our part, and we would much rather have it assert to fail fast ("cannot find a locator when we found a complete solution!") than produce a mysterious "ambiguous" error for a constraint system that actually succeeded.
It'd be fine to do this as a follow-up.
|
@swift-ci please smoke test and merge |
|
@swift-ci please smoke test |
This handles situation when overload for the subscript hasn't been resolved
by constraint solver, such might happen, for example, if solver was allowed to
produce solutions with free or unresolved type variables (e.g. when running diagnostics).
Resolves: rdar://problem/27329076, rdar://problem/28619118, rdar://problem/2778734.