mirrors/Nim
1
0
Fork 0
mirror of https://github.com/nim-lang/Nim.git synced 2026-04-29 18:53:58 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix noreturn/implicit discard check logic (#23681)

Browse Source 
fixes #10440, fixes #13871, fixes #14665, fixes #19672, fixes #23677

The false positive in #23677 was caused by behavior in
`implicitlyDiscardable` where only the last node of `if`/`case`/`try`
etc expressions were considered, as in the final node of the final
branch (in this case `else`). To fix this we use the same iteration in
`implicitlyDiscardable` that we use in `endsInNoReturn`, with the
difference that for an `if`/`case`/`try` statement to be implicitly
discardable, all of its branches must be implicitly discardable.
`noreturn` calls are also considered implicitly discardable for this
reason, otherwise stuff like `if true: discardableCall() else: error()`
doesn't compile.

However `endsInNoReturn` also had bugs, one where `finally` was
considered in noreturn checking when it shouldn't, another where only
`nkIfStmt` was checked and not `nkIfExpr`, and the node given for the
error message was bad. So `endsInNoReturn` now skips over
`skipForDiscardable` which no longer contains
`nkIfStmt`/`nkCaseStmt`/`nkTryStmt`, stores the first encountered
returning node in a var parameter for the error message, and handles
`finally` and `nkIfExpr`.

Fixing #23677 already broke a line in `syncio` so some package code
might be affected.
This commit is contained in:
metagn
2024-06-05 21:53:05 +03:00
committed by GitHub
parent 77c04092e0
commit 42e8472ca6
6 changed files with 195 additions and 74 deletions
Download Patch File Download Diff File Expand all files Collapse all files

61
compiler/sem.nim
View File

@@ -222,66 +222,7 @@ proc shouldCheckCaseCovered(caseTyp: PType): bool =
else:
discard
proc endsInNoReturn(n: PNode): bool =
## check if expr ends the block like raising or call of noreturn procs do
result = false # assume it does return
template checkBranch(branch) =
if not endsInNoReturn(branch):
# proved a branch returns
return false
var it = n
# skip these beforehand, no special handling needed
while it.kind in {nkStmtList, nkStmtListExpr} and it.len > 0:
it = it.lastSon
case it.kind
of nkIfStmt:
var hasElse = false
for branch in it:
checkBranch:
if branch.len == 2:
branch[1]
elif branch.len == 1:
hasElse = true
branch[0]
else:
raiseAssert "Malformed `if` statement during endsInNoReturn"
# none of the branches returned
result = hasElse # Only truly a no-return when it's exhaustive
of nkCaseStmt:
let caseTyp = skipTypes(it[0].typ, abstractVar-{tyTypeDesc})
# semCase should already have checked for exhaustiveness in this case
# effectively the same as having an else
var hasElse = caseTyp.shouldCheckCaseCovered()
# actual noreturn checks
for i in 1 ..< it.len:
let branch = it[i]
checkBranch:
case branch.kind
of nkOfBranch:
branch[^1]
of nkElifBranch:
branch[1]
of nkElse:
hasElse = true
branch[0]
else:
raiseAssert "Malformed `case` statement in endsInNoReturn"
# Can only guarantee a noreturn if there is an else or it's exhaustive
result = hasElse
of nkTryStmt:
checkBranch(it[0])
for i in 1 ..< it.len:
let branch = it[i]
checkBranch(branch[^1])
# none of the branches returned
result = true
else:
result = it.kind in nkLastBlockStmts or
it.kind in nkCallKinds and it[0].kind == nkSym and sfNoReturn in it[0].sym.flags
proc endsInNoReturn(n: PNode): bool
proc commonType*(c: PContext; x: PType, y: PNode): PType =
# ignore exception raising branches in case/if expressions

145
compiler/semstmts.nim
View File

@@ -132,17 +132,140 @@ proc semExprBranchScope(c: PContext, n: PNode; expectedType: PType = nil): PNode
closeScope(c)
const
skipForDiscardable = {nkIfStmt, nkIfExpr, nkCaseStmt, nkOfBranch,
nkElse, nkStmtListExpr, nkTryStmt, nkFinally, nkExceptBranch,
skipForDiscardable = {nkStmtList, nkStmtListExpr,
nkOfBranch, nkElse, nkFinally, nkExceptBranch,
nkElifBranch, nkElifExpr, nkElseExpr, nkBlockStmt, nkBlockExpr,
nkHiddenStdConv, nkHiddenDeref}
proc implicitlyDiscardable(n: PNode): bool =
var n = n
while n.kind in skipForDiscardable: n = n.lastSon
result = n.kind in nkLastBlockStmts or
(isCallExpr(n) and n[0].kind == nkSym and
sfDiscardable in n[0].sym.flags)
# same traversal as endsInNoReturn
template checkBranch(branch) =
if not implicitlyDiscardable(branch):
return false
var it = n
# skip these beforehand, no special handling needed
while it.kind in skipForDiscardable and it.len > 0:
it = it.lastSon
case it.kind
of nkIfExpr, nkIfStmt:
for branch in it:
checkBranch:
if branch.len == 2:
branch[1]
elif branch.len == 1:
branch[0]
else:
raiseAssert "Malformed `if` statement during implicitlyDiscardable"
# all branches are discardable
result = true
of nkCaseStmt:
for i in 1 ..< it.len:
let branch = it[i]
checkBranch:
case branch.kind
of nkOfBranch:
branch[^1]
of nkElifBranch:
branch[1]
of nkElse:
branch[0]
else:
raiseAssert "Malformed `case` statement in endsInNoReturn"
# all branches are discardable
result = true
of nkTryStmt:
checkBranch(it[0])
for i in 1 ..< it.len:
let branch = it[i]
if branch.kind != nkFinally:
checkBranch(branch[^1])
# all branches are discardable
result = true
of nkCallKinds:
result = it[0].kind == nkSym and {sfDiscardable, sfNoReturn} * it[0].sym.flags != {}
of nkLastBlockStmts:
result = true
else:
result = false
proc endsInNoReturn(n: PNode, returningNode: var PNode): bool =
## check if expr ends the block like raising or call of noreturn procs do
result = false # assume it does return
template checkBranch(branch) =
if not endsInNoReturn(branch, returningNode):
# proved a branch returns
return false
var it = n
# skip these beforehand, no special handling needed
while it.kind in skipForDiscardable and it.len > 0:
it = it.lastSon
case it.kind
of nkIfExpr, nkIfStmt:
var hasElse = false
for branch in it:
checkBranch:
if branch.len == 2:
branch[1]
elif branch.len == 1:
hasElse = true
branch[0]
else:
raiseAssert "Malformed `if` statement during endsInNoReturn"
# none of the branches returned
result = hasElse # Only truly a no-return when it's exhaustive
of nkCaseStmt:
let caseTyp = skipTypes(it[0].typ, abstractVar-{tyTypeDesc})
# semCase should already have checked for exhaustiveness in this case
# effectively the same as having an else
var hasElse = caseTyp.shouldCheckCaseCovered()
# actual noreturn checks
for i in 1 ..< it.len:
let branch = it[i]
checkBranch:
case branch.kind
of nkOfBranch:
branch[^1]
of nkElifBranch:
branch[1]
of nkElse:
hasElse = true
branch[0]
else:
raiseAssert "Malformed `case` statement in endsInNoReturn"
# Can only guarantee a noreturn if there is an else or it's exhaustive
result = hasElse
of nkTryStmt:
checkBranch(it[0])
var lastIndex = it.len - 1
if it[lastIndex].kind == nkFinally:
# if finally is noreturn, then the entire statement is noreturn
if endsInNoReturn(it[lastIndex][^1], returningNode):
return true
dec lastIndex
for i in 1 .. lastIndex:
let branch = it[i]
checkBranch(branch[^1])
# none of the branches returned
result = true
of nkLastBlockStmts:
result = true
of nkCallKinds:
result = it[0].kind == nkSym and sfNoReturn in it[0].sym.flags
if not result:
returningNode = it
else:
result = false
returningNode = it
proc endsInNoReturn(n: PNode): bool =
var dummy: PNode = nil
result = endsInNoReturn(n, dummy)
proc fixNilType(c: PContext; n: PNode) =
if isAtom(n):
@@ -165,13 +288,9 @@ proc discardCheck(c: PContext, result: PNode, flags: TExprFlags) =
localError(c.config, result.info, "expression has no type: " &
renderTree(result, {renderNoComments}))
else:
var n = result
while n.kind in skipForDiscardable:
if n.kind == nkTryStmt: n = n[0]
else: n = n.lastSon
# Ignore noreturn procs since they don't have a type
if n.endsInNoReturn:
var n = result
if result.endsInNoReturn(n):
return
var s = "expression '" & $n & "' is of type '" &