fix(sem): noreturn analysis for void exprs

[saem]

Summary

void branches preceeding with no-return terminal branches are checked
with no-return analysis to avoid false positives.

Details

The analysis is updated to recursively check if/elif/of/except
non-terminal branches as well as the terminal branch.

Fixes #1453

[zerbina]

Ah, dang -- I thought about this case when reviewing the previous PR, but somehow convinced myself it isn't a problem.

I do think it makes sense to fully use recursion now -- in my opinion, it's easier to reason about:

proc endsInNoReturn*(n: PNode): bool =
  ## Checks if expression `n` ends in an unstructured exit (raise, return,
  ## etc.) or a call of a noreturn proc. This is meant to be called on a
  ## semmed `n`.
  case n.kind
  of nkStmtList, nkStmtListExpr:
    result = n.len > 0 and endsInNoReturn(n[^1])
  of nkBlockStmt, nkExceptBranch, nkElifBranch, nkElse, nkOfBranch:
    result = endsInNoReturn(n[^1])
  of nkIfStmt:
    for it in n.items:
      result = endsInNoReturn(it[^1])
      if not result:
        break
  of nkCaseStmt:
    # skip the selector expression
    for i in 1..<n.len:
      result = endsInNoReturn(n[i])
      if not result:
        break
  of nkTryStmt:
    # ignore the 'finally' -- it doesn't contribute to the type
    for i in 0..<(n.len - ord(n[^1].kind == nkFinally)):
      result = endsInNoReturn(n[i])
      if not result:
        break
  of nkLastBlockStmts:
    result = true
  of nkCallKinds:
    result = n[0].kind == nkSym and sfNoReturn in n[0].sym.flags
  else:
    result = false

(this code also fixes the tnoreturn_nested.nim test failure.)

[saem]

Ah, dang -- I thought about this case when reviewing the previous PR, but somehow convinced myself it isn't a problem.

You're not the only one. 😆

I do think it makes sense to fully use recursion now -- in my opinion, it's easier to reason about:

proc endsInNoReturn*(n: PNode): bool =
  ## Checks if expression `n` ends in an unstructured exit (raise, return,
  ## etc.) or a call of a noreturn proc. This is meant to be called on a
  ## semmed `n`.
  case n.kind
  of nkStmtList, nkStmtListExpr:
    result = n.len > 0 and endsInNoReturn(n[^1])
  of nkBlockStmt, nkExceptBranch, nkElifBranch, nkElse, nkOfBranch:
    result = endsInNoReturn(n[^1])
  of nkIfStmt:
    for it in n.items:
      result = endsInNoReturn(it[^1])
      if not result:
        break
  of nkCaseStmt:
    # skip the selector expression
    for i in 1..<n.len:
      result = endsInNoReturn(n[i])
      if not result:
        break
  of nkTryStmt:
    # ignore the 'finally' -- it doesn't contribute to the type
    for i in 0..<(n.len - ord(n[^1].kind == nkFinally)):
      result = endsInNoReturn(n[i])
      if not result:
        break
  of nkLastBlockStmts:
    result = true
  of nkCallKinds:
    result = n[0].kind == nkSym and sfNoReturn in n[0].sym.flags
  else:
    result = false

(this code also fixes the tnoreturn_nested.nim test failure.)

Yeah, I didn't want to go that route, but I don't think anything else will be pleasant to read.

[saem]

/merge

[github-actions]

Merge requested by: @saem

Contents after the first section break of the PR description has been removed and preserved below:

---

Notes for Reviewers

• yeesh, so much for the shortcuts I thought I could take with the analysis

[zerbina]

Ugh, the handling for non-exhaustive if/case statements is still missing.

if true:
  1
else:
  if true:   # <- `endsInNoReturn` would return true
    return

If added two suggestions for the fix. You might also want to add an additional test.

[zerbina]

@saem: Sorry for aborting the merge, but I just noticed another issue that's probably better fixed as part of this PR.

[saem]

@saem: Sorry for aborting the merge, but I just noticed another issue that's probably better fixed as part of this PR.

No problem, I'll add some more tests. I haven't been able to think about the testing for this change all that well, so I'll take a day or two with it.

[saem]

I expect the tests to pass, and we could consider this PR done, although see the caveat below.

---

Thinking about it more an area where this might not work is something nested inside a loop:

proc foo() =
  let x =
  while true:
    return

The chance of writing that code is very small, and thankfully it's only a false-negative, but it looks like I need to expand the traversal to include while and for loop bodies. If I'm being really pedantic, then I should also check for while (return; true): ..., but that applies to all predicate positions.

[zerbina]

Looks good to me.

The chance of writing that code is very small, and thankfully it's only a false-negative, but it looks like I need to expand the traversal to include while and for loop bodies

Given that neither while nor for can be used as expressions at the moment (outside of typeof expressions, but those are super special anyway), I don't think this is something to worry about.

It'd become relevant once introducing a unit type and making while and for real expressions, but at that point endsInNoReturn would be obsolete anyway.

[saem]

/merge