[Feature Request] Improved Error Management

gr4ph0s commented 6 months ago

Review Mojo's priorities

[X] I have read the roadmap and priorities and I believe this request falls within the priorities.

What is your request?

Instead of introducing a "raises" keyword in the fn signature which is a bit weird and not really "lookable" from the caller, explicitly return a Variant[Error, T]. Ideally we would be able to write it fn myFunc() -> Result[T]: .... Where a Result is either an error or something that hold a value of type T.

We could define a Result as something like:

struct Result[T]:
  var T # Holds the value, can be a reference, or a pointer or any DtypePointer
  var ErrorType # Similar than DType we should have poll of error, this is only required until mojo support inheritance, then everyone will be able to implement its own error type.
  var message # String containing a message

If the current caller scope also output an error then for each method that could raise an error we have multiple choice. the simple one:

fn callerScope() -> Result[T]
  let myVar: T = myFunc().iferr_return() # If MyFunc fail, then the function callerScope return with the original error.
  return myVar

Or if you do not want to propagate the error and manually handle it the caller have the opportunity to do with a custom if statement only for error so something like

fn callerScope() -> Result[T]
  iferr (let myVar: T = myFunc())
    print(err.Message) # Within an iferr scope you have access to the error generated via the err variable (this is hidden I admit)
    return T()
  return myVar

We could also have a ifnoerr which will do the inverse of the iferr.

It would be nice to be able to define custom error handling for this scope. e.g. you need to raise the error to the user. You can do that with the iferr but instead of doing that everywhere you should be able to define a custom iferr_scope handler that will be present for the current scope as I describe in https://github.com/modularml/mojo/issues/1746#issuecomment-1938627453

Here the main things I would like to see for an error system in mojo. Of course this is a proposal and maybe another approach is better.

What is your motivation for this change?

Try/expect block are really painful to manage and if you want to do it properly then you need a lot of cases. Moreover it kind of hide which function can fail and which can't fail and lack customization. If you have 2 calls that can raises but you want 2 different error handling you need in total 8 lines... While with my proposal you will just need 4 lines.

Moreover I think having a good and extensible error system is mandatory for any language and one of the fundamental of it. I personally think one of a good code base signal is a good error handling though since the beginning of it. As most of the time system that have been built with incomplete error system then fail sooner or later and then adding proper error handling everywhere is a difficult and time consuming task.

Any other details?

In the company I work, we use mainly C++ and we have our own Error Management without exception which is really a joy to works with. My request summarize it but you can find more information it in our documentation https://developers.maxon.net/docs/cpp/2024_2_0/page_maxonapi_error.html#page_maxonapi_error_scope.

gryznar commented 6 months ago

+1

gr4ph0s commented 6 months ago

I agree with you the current C++ split from exception and no exception world is a big error and both should be treated teh same way. If the same error system can't express two different error level (the recoverable one and the one you cant), then that's an issue. And I think my proposal allow an unified error system.

But while rethinking about my proposal, I see two issues with my previous iferr_scope_handler proposal. It does not really comply to the Mojo language, so I guess something like that would be more meaningful

fn callerScope() -> T
  @parameter
  fn iferr_scope_handler(err: Result) -> T:
    print(err.message)
    return T()

  # Here the current callerScope does not return a Result object, any error will stop the execution and execute the iferr_scope_handler.
  let myVar: T = myFunc().iferr_return[iferr_scope_handler]() 

  return myVar

The second issue I see is that currently it can be dangerous for allocated memory until we have proper finally closure support (and that can be stacked, so you could have multiple finally and you can disable them)

So until Mojo have proper finally closure maybe it make more sense, when you want a more generalized approach to use the try/except block. This will give us something like:

fn callerScope() -> T:

  var allocatedData = Pointer[SomeType].alloc()

  @paramter
  fn CustomErrorHandling(err: Result) -> T:
    allocatedData.free()
    print(err.message)
    return T()

  # In this case we specify which error handler to call. There is no default error handling since the current function does not return an error so its mandatory to provide one.
  let myVar: T = myFunc().iferr_return[CustomErrorHandling]() 

  # We start an Try/except block, any non-handled error will land into the except block.
  try:

     # This function is handled as the previous one
     myFunc().iferr_return[CustomErrorHandling]() 

     # If this call fail, it will go to the except block
     myFunc()
  except:
   allocatedData.free()
   print("An error occurred")

   # the code will stop at the end of the except scope because the except code does not return a valid value
   # But if you uncomment the next line, then the code continue and will return a default T()
   # return T()

  return myVar

In order to be safe, the compiler should be able to track pointer allocations within a scope, and detect if the owner of that Pointer is freed. Otherwise the compiler will have the duty to free the allocated data. Doing such a thing, can be quiet complex to implement but is mandatory if we want Mojo to be safe.

melodyogonna commented 6 months ago

How is the current implementation not "lookable" from the caller? I have also not found it weird, the opposite in fact, I found it pretty sensible, especially as Mojo tries to be immediately familiar to Python developers.

gr4ph0s commented 6 months ago

How is the current implementation not "lookable" from the caller? I have also not found it weird, the opposite in fact, I found it pretty sensible, especially as Mojo tries to be immediately familiar to Python developers.

Look only at the main function and tell me what can fail? https://github.com/taalhaataahir0102/Jpeg-Decoder/blob/main/Mojo/main.mojo#L987

Inspired from https://mojodojo.dev/guides/benchmarks/sudoku.html scroll to the last code snippet don't read other. Now let's imagine you have something like that:

from Benchmark import Benchmark
from custom_module import init_board

alias board_size = 9 
fn bench(python_secs: Float64):
    fn solve():
        try:
            let board = init_board()
            _ = board.solve()
        except:
            pass

    let mojo_secs = Benchmark().run[solve]() / 1e9
    print("mojo seconds:", mojo_secs)
    print("speedup:", python_secs / mojo_secs)

bench(python_secs.to_float64())

Does the init can fail or its the solving part that can fail? You have no clue. This lead to increase time on code review. As you have no clue which method should absolutely not fail and have an extra care and the one that are able to fail. Moreover this allow people to not think about what can fail in their code. Having something that remind you this can fail or this cant fail, force the developer to ask himself, what should I do if this particular call fail? Should I de-allocate anything?

Look at the project mentioned https://github.com/mojicians/awesome-mojo for the moment most of them consist of a main file that looks like

import my_feature

fn main():
    try:
        print(MyFeature())
    except e:
        print("Error: ", e)

And when you look at my_feature all functions or almost all are marked with raises keyword because anything in them can fail but you dont really know what just few examples:

My proposal btw is to keep the try/except because for some small project it is fine enough, and its fine to not always to have to thinks about errors. But when you work on large code base (not just hundreds or few thousands but million of lines) you want to quickly know what can fail and what can't/should not fail.

melodyogonna commented 6 months ago

I do not program with a limitation that says I can't check function signatures, every IDE or text editor has LSP hover functionality that shows what function raises or doesn't.

As for reviews, try/catch has been the de-facto error-handling mechanism in most popular languages for decades, reviews are done without issues... and I would imagine the point of a code review is explicitly checking for things like this.

Finally, Mojo aims to be immediately familiar to Python developers, try/catch is immediately familiar to Python developers, Rust-style error handling is not.

melodyogonna commented 6 months ago

I mean, we can have both, but there is this from the Zen of Python: There should be one-- and preferably only one --obvious way to do it.

Anyway, Modular may already have plans to include the Result type, there's this from the roadmap: Many standard library types, including Optional[T] and Result[T, Error] types when we have algebraic datatypes and basic traits. https://docs.modular.com/mojo/roadmap.html#small-independent-features

gryznar commented 6 months ago

I am also almost sure, that compiler internally handles that as Result[T, Error]. This is only strangely exposed as raises with need to use try...except instead of -> Result[T, Error] as return type and match to handle in monadic way

gamendez98 commented 5 months ago

@melodyogonna I understand that this is not a priority but there is one case for which this answers to the specific intentions of mojo, and that is python integration. A Result like type would make it so much easier to breach the distance between a dynamically typed and statically type context

laszlokindrat commented 5 months ago

Thanks for opening the ticket! The Mojo Language Feature Work Group considered the question of dropping the raises keyword from functions in favor of explicit error propagation in the return type. For now, our decision is to retain the existing semantics for raising functions due to two main reasons: (1) we want implicit error propagation because calling fallible functions and immediately returning if their result is erroneous is a very common pattern (and requires a lot of boilerplate in languages like C++), and (2) we don’t want to break python semantics completely (and essentially abandon try...except). If one desires, it is already possible to opt out of this by explicitly using Variant or some custom type that wraps an optional result or an error (and not marking the function with raises). Moreover, there is currently work being done to improve and extend error propagation capabilities, and we intend to revisit this question in the future as the language matures.

melodyogonna commented 5 months ago

That is very nice to hear.

pckim93 commented 1 month ago

I second this. It would be very nice to be able to avoid dealing with exception handling. This might be just a personal opinion, but monadic error handling used in Rust feels beautiful, and I think this is pretty much what the author of this ticket is proposing.

modularml / mojo