Skip to content

Commit

Permalink
fix(sem): missed discard in try/finally errors (nim-works#995)
Browse files Browse the repository at this point in the history
## Summary

A missing `discard` in a `try/finally` no longer crashes the compiler
and emits an error as it should.

## Details

The `discardCheck` used to ensure expressions are used or `void` did not
traverse `try` appropriately. Instead of looking at the last expression
in the last `except` or `try` branch, it would look at the last branch,
even if that was a `finally`. This led to incomplete data being sent for
error reporting, which ultimately led to an NPE.

Along with the fix a regression test has been added.
  • Loading branch information
saem authored Oct 24, 2023
1 parent 07f39ce commit 4a6621e
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 2 deletions.
14 changes: 12 additions & 2 deletions compiler/sem/semstmts.nim
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,18 @@ proc discardCheck(c: PContext, n: PNode, flags: TExprFlags): PNode =
elif n.typ.kind != tyError and c.config.cmd != cmdInteractive:
var m = n
while m.kind in skipForDiscardable:
m = m.lastSon
case m.kind
of nkTryStmt:
m =
case m[^1].kind
of nkFinally:
m[^2]
of nkExceptBranch:
m[^1]
of nkAllNodeKinds - {nkFinally, nkExceptBranch}:
unreachable()
else:
m = m.lastSon

result = newError(c.config, n,
PAstDiag(kind: adSemUseOrDiscardExpr, undiscarded: m))
Expand Down Expand Up @@ -299,7 +310,6 @@ proc semTry(c: PContext, n: PNode; flags: TExprFlags): PNode =
n[i][^1] = discardCheck(c, n[i][^1], flags)
if n[i][^1].isError:
return wrapError(c.config, n)

if typ == c.enforceVoidContext:
result.typ = c.enforceVoidContext
else:
Expand Down
14 changes: 14 additions & 0 deletions tests/lang_exprs/tdiscardcheck_try_except.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
discard """
description: "Ensure discard checks are done for try expressions"
errormsg: "expression '1' is of type 'int literal(1)' and has to be used (or discarded)"
line: 14
"""

## This was originally introduced as a regression test, where a fix to
## `semstmts.discardCheck` erroneously changed traversals in a manner where
## the test suite passed but this was not caught, except in code review.

try:
discard
except:
1
16 changes: 16 additions & 0 deletions tests/lang_exprs/tdiscardcheck_try_finally.nim
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
discard """
description: "Ensure discard checks are done for try expressions"
errormsg: "expression '1' is of type 'int literal(1)' and has to be used (or discarded)"
line: 14
"""

## This was originally introduced as a regression test, where
## `semstmts.discardCheck` erroneously checked the `finally` branch, instead
## of the last non-`finally` branch in the `try` expression. This generated an
## incomplete report, and resulted in an NPE within `cli_reporter` when
## attempting to render it.

try:
1
finally:
discard 1

0 comments on commit 4a6621e

Please sign in to comment.