Callbacks marked with raises pragma break exception tracking analysis #1631

Closed
gradha opened this Issue Nov 4, 2014 · 6 comments

Comments

Projects
None yet
3 participants
@gradha
Contributor

gradha commented Nov 4, 2014

The following parent.nim file should compile:

type
  TCallback = proc(a: int) {.nimcall, raises: [EInvalidValue].}

proc bad_callback*(a: int) {.procvar, raises: [EInvalidValue].} =
  if a > 0: raise newException(EInvalidValue, "Too positive")
  else: discard

proc do_something*(data: int,
    callback: TCallback = bad_callback) {.raises: [].} =
  echo "Doing something with ", data
  try:
    callback(data)
  except:
    echo "Avoided mishap!"

proc test() {.raises: [].} =
  do_something(5)

when isMainModule: test()

However the compiler disagrees despite the callback being wrapped inside a try/except block:

parent.nim(16, 22) Info: instantiation from here
parent.nim(4, 47) Error: can raise an unlisted exception: EInvalidValue

It seems that the exception tracking of the callback is being added to the proc it is being used in regardless of usage. Also, if I change the example to remove most of the exception tracking except for the test proc like:

type
  TCallback = proc(a: int) {.nimcall.}

proc bad_callback*(a: int) {.procvar.} =
  if a > 0: raise newException(EInvalidValue, "Too positive")
  else: discard

proc do_something*(data: int,
    callback: TCallback = bad_callback) =
  echo "Doing something with ", data

proc test() {.raises: [].} =
  do_something(5)

when isMainModule: test()

Then the compiler still shows errors:

parent.nim(12, 22) Info: instantiation from here
parent.nim(9, 26) Error: can raise an unlisted exception: E_Base

Which now looks more like a duplicate of #1628. But is it?

@Araq

This comment has been minimized.

Show comment
Hide comment
@Araq

Araq Nov 5, 2014

Member

It seems that the exception tracking of the callback is being added to the proc it is being used in regardless of usage.

Yes. That's an undesired side effect of the "proc var parameters are special" rule. But it's not a bug. We can't do better and still keep it sound.

Member

Araq commented Nov 5, 2014

It seems that the exception tracking of the callback is being added to the proc it is being used in regardless of usage.

Yes. That's an undesired side effect of the "proc var parameters are special" rule. But it's not a bug. We can't do better and still keep it sound.

@Araq Araq closed this Nov 5, 2014

@gradha

This comment has been minimized.

Show comment
Hide comment
@gradha

gradha Nov 5, 2014

Contributor

It is quite unfortunate that callbacks conflict with exception tracking, the addition of such a callback to an API which was using the raises pragma has forced me to disable exception tracking for the whole module or nothing would compile any more.

Contributor

gradha commented Nov 5, 2014

It is quite unfortunate that callbacks conflict with exception tracking, the addition of such a callback to an API which was using the raises pragma has forced me to disable exception tracking for the whole module or nothing would compile any more.

@reactormonk

This comment has been minimized.

Show comment
Hide comment
@reactormonk

reactormonk Nov 5, 2014

Contributor

Could the callback problem be solved by also passing an exception handler for any exception that may be thrown by the callback?

Contributor

reactormonk commented Nov 5, 2014

Could the callback problem be solved by also passing an exception handler for any exception that may be thrown by the callback?

@Araq

This comment has been minimized.

Show comment
Hide comment
@Araq

Araq Nov 5, 2014

Member

@gradha This makes no sense to me. Can you post your real world example?

Member

Araq commented Nov 5, 2014

@gradha This makes no sense to me. Can you post your real world example?

@Araq Araq reopened this Nov 5, 2014

@gradha

This comment has been minimized.

Show comment
Hide comment
@gradha

gradha Nov 5, 2014

Contributor

Here would be my real world example:

git clone --recursive https://github.com/gradha/lazy_rest.git
cd lazy_rest
git checkout review_callbacks
nimrod c lazy_rest # <-- works
nimrod c lazy_rest_c_api # <-- fails

In this case I had wrapped the Nimrod API to C and was in the process of adding callbacks to the Nimrod version. However you don't need to look at all this mountain of code. Take the example from the exception tracking section in the manual and change it like this:

proc noRaise(x: proc()) {.raises: [].} =
  # unknown call that is protected to raise nothing
  try: x()
  except: discard

proc doRaise() {.raises: [EIO].} =
  raise newException(EIO, "IO")

proc use() {.raises: [].} =
  # doesn't compile even though nothign can be raised!
  noRaise(doRaise)

The compiler complaints with:

private/tmp/ex/ex.nim(9, 21) Info: instantiation from here
private/tmp/ex/ex.nim(6, 26) Error: can raise an unlisted exception: EIO

Rule 2 is being applied at the level of the use() proc, ignoring that the static usage of the callback inside noRaise() is inside a try/catch block. That's why the example I posted initially doesn't compile: the compiler looks at the callsite instead of looking at the usage of the callback (where the second version doesn't even call the callback!).

In any case you don't need to spend more time about this, while I've had to disable exception tracking to make the C module compile I have decided to modify the callback API so that nothing can be raised and instead errors are passed in back via string/cstring or nil if there were none. This should appease exception tracking.

Contributor

gradha commented Nov 5, 2014

Here would be my real world example:

git clone --recursive https://github.com/gradha/lazy_rest.git
cd lazy_rest
git checkout review_callbacks
nimrod c lazy_rest # <-- works
nimrod c lazy_rest_c_api # <-- fails

In this case I had wrapped the Nimrod API to C and was in the process of adding callbacks to the Nimrod version. However you don't need to look at all this mountain of code. Take the example from the exception tracking section in the manual and change it like this:

proc noRaise(x: proc()) {.raises: [].} =
  # unknown call that is protected to raise nothing
  try: x()
  except: discard

proc doRaise() {.raises: [EIO].} =
  raise newException(EIO, "IO")

proc use() {.raises: [].} =
  # doesn't compile even though nothign can be raised!
  noRaise(doRaise)

The compiler complaints with:

private/tmp/ex/ex.nim(9, 21) Info: instantiation from here
private/tmp/ex/ex.nim(6, 26) Error: can raise an unlisted exception: EIO

Rule 2 is being applied at the level of the use() proc, ignoring that the static usage of the callback inside noRaise() is inside a try/catch block. That's why the example I posted initially doesn't compile: the compiler looks at the callsite instead of looking at the usage of the callback (where the second version doesn't even call the callback!).

In any case you don't need to spend more time about this, while I've had to disable exception tracking to make the C module compile I have decided to modify the callback API so that nothing can be raised and instead errors are passed in back via string/cstring or nil if there were none. This should appease exception tracking.

@Araq

This comment has been minimized.

Show comment
Hide comment
@Araq

Araq Nov 5, 2014

Member

I still cannot see why you need to rewrite whole modules. One workaround is:

proc noRaise(x: proc()) {.raises: [].} = 
  x()

proc doRaise() {.raises: [EIO].} = raise newException(EIO, "IO") 
proc wrapper() {.raises: [].} =
  try:
     doRaise()
   except: discard

proc use() {.raises: [].} = noRaise(wrapper)
Member

Araq commented Nov 5, 2014

I still cannot see why you need to rewrite whole modules. One workaround is:

proc noRaise(x: proc()) {.raises: [].} = 
  x()

proc doRaise() {.raises: [EIO].} = raise newException(EIO, "IO") 
proc wrapper() {.raises: [].} =
  try:
     doRaise()
   except: discard

proc use() {.raises: [].} = noRaise(wrapper)

@Araq Araq closed this Nov 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment