shish/rosettaboy

avoid generic exceptions

shish opened this issue · 10 comments

shish commented

rather than raise Exception("Invalid write to address %04X" % addr) we should use a specific exception like raise InvalidWriteAddress(addr)

  • c++
  • python
  • rust
  • go
  • php

For languages which support inheritance, I'd do something like:

  • Exception
    • ControlledExit - emulation should stop for some reason (exit 0, except for UnitTestFailed, which should exit 2)
      • Quit - the user hit the X button on the window
      • Timeout - we reached as far as we wanted to reach with the --profile flag
      • UnitTestPassed - Opcode 0xFC was executed
      • UnitTestFailed - Opcode 0xFD was executed
    • GameException - bugs in the game itself (exit 3)
      • InvalidOpcode
      • InvalidRomBank
      • InvalidRomRead
      • InvalidRomWrite
      • InvalidRamBank
      • InvalidRamRead
      • InvalidRamWrite
    • UserException - the user gave us some sort of invalid input (exit 4)
      • RomMissing - the .gb file specified on the command line doesn't exit
      • HeaderChecksumFailed - the .gb file specified has an invalid checksum
      • LogoChecksumFailed - the .gb file specified has an invalid checksum

Hey just wanted to check, regarding the python section for this, you're referring to line 297 of the ram.py file? And if so, perhaps I could go ahead to create another custom exception in the errors.py file to handle this?

shish commented

The if self.debug: print(...) parts in ram.py are things which are weird and noteworthy, but AFAIK wouldn't crash a real gameboy - in this case I'm mostly talking about the parts where we currently raise Exception(...), but there's no way to tell what went wrong (other than reading the exception message). I'd like those raise Exception(...) parts to be replaced with more specific Exception sub-classes, so we can do things like

try:
    [...]
except GameException:
    [...
    the game itself did something bad, like reading save data on
    a cart which doesn't support save data - so we should print
    out some useful data to help the game developer
    ...]
except UserException:
    [...
    the user did something bad like specifying a .gb file that
    doesn't exist, so we should print out a friendly error message
    ...]
except EmuException:
    [...
    the emulator itself did something bad, we should dump the
    state in a way that's useful for a rosettaboy developer to
    investigate
    ...]
except Exception:
    [...
    generic fallback for something totally unimaginable happening
    ...]
shish commented

(Right now we don't even have a clear hierarchy for game / user / emulator exceptions - everything is just a subclass of EmuError - having a useful tree of exception classes just came to me while I was writing that comment ^^)

I see, so am I right to say that the end goal would kinda be to have each Raise Exception section modified to be like the try/except structure you mentioned above to catch as many specialized errors as possible?

shish commented

The goal is for all the places we currently do a generic raise Exception(), to come up with a more specific kind of exception (ie replace the current raise Exception(...) with raise WriteToInvalidAddressException(address)) -- and then in main.py, we can catch the main categories of exceptions as in the try/except example above :)

Alright, so the first thing to do now would be to follow what you mentioned in your edited comment, which is to go to errors.py and create the three larger classes GameException, UserException, and EmuException so that their relevant smaller bugs can inherit from them? I wanted to also ask, right now EmuError simply exits with the exit code of 1, so would this similar, simple functionality be good enough for GameException, UserException, and EmuException too or would there need to be additional messages/functionality?

shish commented

For exit codes, everything should exit 1 except for Quit / Timeout / UnitTestPassed (which should exit 0) and UnitTestFailed (which should exit 2)

For messages, only Timeout is really important (because the message there includes performance details) - the rest can have nice messages if it's easy to add, but not really needed

shish commented

Actually, maybe different error codes for different categories of error...

After a bunch of experimentation, I think the C++ version is now in a state that I'm happy with -- so other languages should copy what's there (I've updated the original issue description to describe that)

Awesome, I'll just draw reference from what you've done with the C++ version and push up some code for the python version, and you can check if it looks good to you

hey, added some changes here #27. It's probably not complete/perfect. Do let me know if anything needs to be changed/added :)