Remove CHECK from GetCurrentReturnSlot

[geoffromer]

GetCurrentReturnSlot is sometimes called when there is no return slot while checking incorrect Carbon code. This change also updates fail_returned_var_no_return_type.carbon to cover one such case.

[jonmeow]

It looks like you're trying to test the behavior for BuildReturnVar. What is the effect on BuildReturnWithExpr, which appears to have no handling for Invalid?

[jonmeow]

Why is this removing the test of return;? Should that be retained?

[geoffromer]

My original rationale was that this code has two distinct errors (forgetting the return type and forgetting to return the returned var), and it would be more orthogonal to have only one. But on reflection, arguably it's only one error (accidentally adding a returned var declaration), so it's probably worth keeping.

We do also need a test for the "forgot return type" case, because that's what would fail the CHECK that's being removed here, so I've added that below.

[jonmeow]

Thanks, I must be missing the change. Can you please link to it?

[jonmeow]

Ah, I see it now.

Would it make sense to use split files (// --- ...) in order to force each failure to be independently verified?

[geoffromer]

Sure, done.

[geoffromer]

It looks like you're trying to test the behavior for BuildReturnVar. What is the effect on BuildReturnWithExpr, which appears to have no handling for Invalid?

If you're asking whether BuildReturnWithExpr could ever benefit from the CHECK being removed here, I believe the answer is "not as it's currently written", because it only calls GetCurrentReturnSlot under conditions that ensure it will return a valid result. It also looks like it may implicitly handle invalid values, because there are other paths through that function that seem to leave return_slot_id invalid.

If that doesn't answer your question, can you rephrase it?

[geoffromer]

My original rationale was that this code has two distinct errors (forgetting the return type and forgetting to return the returned var), and it would be more orthogonal to have only one. But on reflection, arguably it's only one error (accidentally adding a returned var declaration), so it's probably worth keeping.

We do also need a test for the "forgot return type" case, because that's what would fail the CHECK that's being removed here, so I've added that below.

[jonmeow]

It looks like you're trying to test the behavior for BuildReturnVar. What is the effect on BuildReturnWithExpr, which appears to have no handling for Invalid?

If you're asking whether BuildReturnWithExpr could ever benefit from the CHECK being removed here, I believe the answer is "not as it's currently written", because it only calls GetCurrentReturnSlot under conditions that ensure it will return a valid result. It also looks like it may implicitly handle invalid values, because there are other paths through that function that seem to leave return_slot_id invalid.

If that doesn't answer your question, can you rephrase it?

Rather than assuming BuildReturnWithExpr would work, my leaning here would be to either:

1. Add a "valid" CHECK to the BuildReturnWithExpr code, to maintain the current behavior (essentially forcing a change/test if it's actually possible)
2. Add a test that BuildReturnWithExpr correctly handles invalid, to test that the new behavior doesn't lead to a bug.

Thoughts?

[jonmeow]

Thanks, I must be missing the change. Can you please link to it?

[geoffromer]

Rather than assuming BuildReturnWithExpr would work, my leaning here would be to either:

1. Add a "valid" CHECK to the BuildReturnWithExpr code, to maintain the current behavior (essentially forcing a change/test if it's actually possible)
2. Add a test that BuildReturnWithExpr correctly handles invalid, to test that the new behavior doesn't lead to a bug.

Thoughts?

#2 is infeasible, because AFAIK there's no input that can cause BuildReturnWithExpr to get an invalid result from its GetCurrentReturnSlot call. I've implemented #1 now, but it feels a bit redundant: in the space of three lines the code checks that there's a return slot to get, gets the return slot, and then checks that the result is valid.

[geoffromer]

Sure, done.

[jonmeow]

Rather than assuming BuildReturnWithExpr would work, my leaning here would be to either:

1. Add a "valid" CHECK to the BuildReturnWithExpr code, to maintain the current behavior (essentially forcing a change/test if it's actually possible)
2. Add a test that BuildReturnWithExpr correctly handles invalid, to test that the new behavior doesn't lead to a bug.

Thoughts?

#2 is infeasible, because AFAIK there's no input that can cause BuildReturnWithExpr to get an invalid result from its GetCurrentReturnSlot call. I've implemented #1 now, but it feels a bit redundant: in the space of three lines the code checks that there's a return slot to get, gets the return slot, and then checks that the result is valid.

Fair enough. My own thought here though is that it's getting the information in a different path, and assuming they'll match. But maybe this is an indicator that there's a cleaner fix? Had you considered changing BuildReturnVar to do an equivalent check before getting the return slot:

  auto return_slot_id = GetCurrentReturnSlot(context);
  if (!SemIR::ReturnTypeInfo::ForFunction(context.sem_ir(), function)
           .has_return_slot()) {
    // If we don't have a return slot, we're returning by value. Convert to a
    // value expression.
    returned_var_id = ConvertToValueExpr(context, returned_var_id);
    return_slot_id = SemIR::InstId::Invalid;
  }

->

  auto return_slot_id = SemIR::InstId::Invalid;
  if (SemIR::ReturnTypeInfo::ForFunction(context.sem_ir(), function)
           .has_return_slot()) {
    return_slot_id = GetCurrentReturnSlot(context);
  } else {
    // If we don't have a return slot, we're returning by value. Convert to a
    // value expression.
    returned_var_id = ConvertToValueExpr(context, returned_var_id);
  }

i.e., instead of the GetCurrentReturnSlot change. Note, approved either way.

[jonmeow]

Actually, maybe that approach is better since it avoids a name lookup in the case? Maybe the CHECK in GetCurrentReturnSlot is correct on that basis?

[geoffromer]

That's a good idea, and I tried it, but it turns out not to work, because GetCurrentReturnSlot can return a valid value even if ReturnTypeInfo::has_return_slot would return false (although not vice-versa). Specifically, GetCurrentReturnSlot will succeed if the function has a declared return type, but ReturnTypeInfo::has_return_slot only returns true if the return type has an in-place initializing representation, i.e. the return slot should actually be used.

(We really ought to get rid of that distinction; it's so confusing.)

I do think the logic in this whole area could be improved, but I think that's out of scope for this PR, so I'm merging this as-is.