Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misleading error in proc/template with try finally block #13871

Closed
Rekihyt opened this issue Apr 4, 2020 · 0 comments · Fixed by #23681
Closed

Misleading error in proc/template with try finally block #13871

Rekihyt opened this issue Apr 4, 2020 · 0 comments · Fixed by #23681

Comments

@Rekihyt
Copy link

Rekihyt commented Apr 4, 2020

Compiler references echo "expression" instead of 2 in the error message.

Example

template t*(body: int) =
  try:
    body
  finally:
    echo "expression"
  
t: 2

Current Output

Error: expression 'echo ["expression"]' is of type 'int literal(2)' and has to be discarded; start of expression here: ex.nim(7, 1)

Expected Output

This error on 2 instead of echo "expression":

expression '2' is of type 'int literal(2)' and has to be discarded

Possible Solution

Additional Information

Found when I forget to discard something in a withDir template.
Removing the finally fixes this, as well as of course adding a discard before 'body'

$ nim -v
Nim Compiler Version 1.2.0 [Linux: amd64]
@ghost ghost added the Error Messages label Jul 25, 2020
metagn added a commit to metagn/Nim that referenced this issue Jun 4, 2024
@Araq Araq closed this as completed in 42e8472 Jun 5, 2024
narimiran pushed a commit that referenced this issue Aug 29, 2024
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.

(cherry picked from commit 42e8472)
narimiran pushed a commit that referenced this issue Aug 31, 2024
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.

(cherry picked from commit 42e8472)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant