Ensure unconverted elements are converted when on LHS

[333fred]

Fixes #76444.

[AlekseyTs]

Done with review pass (commit 1)

[333fred]

@AlekseyTs addressed your feedback.

[AlekseyTs]

LGTM (commit 2)

[333fred]

@dotnet/roslyn-compiler for a second review of this fairly simple PR.

[cston]

HasAnyErrors

This check seems too broad since the error could in a nested expression with no error on op1 directly. In short, it seems surprising we're binding for error recovery in that situation.

If we're just trying to ensure op1 is converted, would it make sense to do that explicitly?

if (op1.NeedsToBeConverted())
{
    op1 = BindToNaturalType(op1, BindingDiagnosticBag.Discard, reportNoTargetType: false);
}

[333fred]

My original approach did effectively this. Aleksey was concerned, rightly so, that discarding diagnostics here is probably not safe, as there is no guarantee that errors have been reported for op1. I do not think that it is surprising that we bind for error recovery in this scenario when we know that there are errors; in such a scenario, we simply want to be able to give the most precise info that we can.

[cston]

discarding diagnostics here is probably not safe, as there is no guarantee that errors have been reported for op1.

In that case, we could pass in the diagnostics instance.

I do not think that it is surprising that we bind for error recovery in this scenario when we know that there are errors

HasAnyErrors indicates there is an error somewhere beneath op1, but the outermost node op1 might be otherwise valid.

[333fred]

If you would prefer, I can switch to just checking HasErrors, rather than HasAnyErrors, so it's only on the node itself.

[cston]

op1 = BindToTypeForErrorRecovery(op1);

Are we testing this change?

[333fred]

I cannot find a way to hit this line in a way that would fail without this binding; however, I believe that, since this is an error scenario anyways, more defensive coding ensuring that this goes through proper conversion checks is a good thing. If you want, I can add an assert to fail if an interesting case ever goes through here, ie Debug.Assert(op1 is BoundParameter or BoundLocal || !op1.NeedsToBeConverted(), "It is currently believed that this isn't hittable. If you trigger this assert, please an explicit test!");?

[cston]

Thanks for investigating. I don't think we need an assert.