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

[stdlib] Add Result type #2749

Closed
wants to merge 45 commits into from

Conversation

martinvuyk
Copy link
Contributor

@martinvuyk martinvuyk commented May 19, 2024

Add Result, a type modeling a value which may or may not be present. With an Error in the case of failure.

Closes #2748

martinvuyk and others added 27 commits May 18, 2024 20:24
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <110240700+martinvuyk@users.noreply.github.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
@martinvuyk martinvuyk marked this pull request as ready for review May 19, 2024 04:38
@martinvuyk martinvuyk requested a review from a team as a code owner May 19, 2024 04:38
martinvuyk and others added 10 commits May 19, 2024 09:38
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Copy link
Collaborator

@JoeLoser JoeLoser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result is definitely a useful type. I wouldn't eagerly create ResultReg or ErrorReg yet. In general, memory-only flavors of types are the way things are trending. If your counterexample is OptonalReg, I wouldn't base future decisions off of that. That is used in a few load-bearing places internally that aren't easy to move to Optional quite yet.

Given that, do you mind splitting out Result and its tests into a separate PR? Any thoughts here, @ConnorGray?

@martinvuyk
Copy link
Contributor Author

In general, memory-only flavors of types are the way things are trending

ok that makes sense.

do you mind splitting out Result and its tests into a separate PR?

Yeah np, I can also just delete everything else from this branch since I already have it in another repo

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
@martinvuyk martinvuyk changed the title [stdlib] Add Result, ResultReg, ErrorReg types [stdlib] Add Result type Jun 21, 2024
@remorses
Copy link

Mojo already has errors and Optional, you can combine both to get the same thing, why add a Result type? It adds another way to make the same thing, making the std code more confusing

@martinvuyk
Copy link
Contributor Author

@remorses Optional is for when the function can only return a value or nothing, the information about what went wrong inside the function is lost.

This is a pattern often used in python:

try:
    something()
except ValueError:
    pass
except IndexError:
    pass
...

That is the same as returning a Result with different errors.

If you still aren't convinced I suggest you have a look at Rust's Result and Option enums.

@remorses
Copy link

remorses commented Jun 21, 2024

The problem here is that in Python there is no way to handle an error in an expression, this is also the case for match and if statements.

This becomes a problem when you want to assign a variable directly with the result of an if, try or match.

The best solution here would be to add a way to create an expression out of a group of statements.

Zig has something similar and Rust too, but usually you don't need them because most things in Rust are expressions and not statements.

# with without anything becomes a way to turn a block into an expression
var result = with:
  try:
    something()
    break "success"
  except ValueError:
    break "ValueError"
  except IndexError:
    break "IndexError"

Same thing in Rust:

let result = match something() {
    Ok(_) => "success".to_string(),
    Err(e) => match e {
        "ValueError" | "IndexError" => "".to_string(),
        _ => "other error".to_string(),
    },
};

Created a proposal for this at #3093

@martinvuyk
Copy link
Contributor Author

create an expression out of a group of statements

That is a completely different discussion. And even then, it still needs a Result type to map to.

With the current implementation one can do:

fn func_that_can_err[A: CollectionElement]() -> Result[A]:
    return Error("failed")

fn return_early_if_err[T: CollectionElement, A: CollectionElement]() -> Result[T]:
    var result: Result[A] = func_that_can_err[A]()
    if not result:
        # the internal err gets transferred to a Result[T]
        return result
        # its also possible to do:
        # return None, Error("func_that_can_err failed")
    var val = result.value()
    var final_result: T
    ...
    return final_result

but you have to know the string message inside the error beforehand if you want to do any condition based on that.

I also have a proposal that I haven't formalized and I wanted to bring it up in the July 1 meeting that is having a parametrized pseudo-type for Error:

struct Error2[T: StringLiteral](Stringable, Boolable):
    """This type represents a parametric Error."""

    alias type = T
    """The type of Error."""
    var message: String
    """The Error message."""
    ...

with this result type one could do this

fn do_something(i: Int) -> Result2[Int, "IndexError"]:
    if i < 0:
        return None, Error2["IndexError"]("index out of bounds: " + str(i))
    return 1

fn do_some_other_thing() -> Result2[String, "OtherError"]:
    var a = do_something(-1)
    if a.err == "OtherError": # returns bool(err) and err.type == value
        return a # error gets transferred
    elif a.err == "IndexError":
        return a # error message gets transferred
    elif a.err: # some undefined error after an API change
        return a
    return "success"

@ConnorGray
Copy link
Collaborator

ConnorGray commented Jun 21, 2024

Hi @martinvuyk, thanks for taking the time to add this type, and think through what a Result type in Mojo 🔥 could look like.

A few comments on this PR:

  • The Result type in Rust is particularly ergonomic because of Rust language features like the ? (Try) operator, and Rust's general support for rich sum types (enum) and pattern matching (match).

    Mojo does not have those features today, and they aren't currently on our foreseeable roadmap.

  • However, the Result approach to error handling—the reification of "can succeed or fail" as "just another type" that programmers can manipulate and build upon—is a powerful idea in standard library and language design, and I think it'd be reasonable for us to enable the community to use their creativity to explore this design space 🧠

    • Can Result work ergonomically in Mojo today? 🤔
    • Can Result and raises-based error handling play nicely together; can you provide "adaptors" between them that programmers find useful?

    And I'm sure many other design questions are worth investigating 🔎 .

Therefore, although I think its very reasonable for folks to point out that what's right for Rust may not be right for Mojo, I think this is the kind of moment where what's possible in this space isn't something anyone really knows for certain. And, consequently, the most positive path forward is to unleash some creativity and see what a Result type in Mojo could look like 🙂

Perhaps it won't pan out, but the community has got the brain power and the enthusiasm that I don't want to bet against them and make that experimentation more difficult by siloing away a proven-useful pattern like this.

To that end, I suggest we:

  • Move this Result type to a new experimental/result.mojo top-level module in the standard library.

    This is something the standard library team has discussed and thinks will be generally useful for us to help the community coalesce around core utility types like this, that would otherwise be fragment and make it difficult for projects in the Mojo ecosystem to interoperate. (For example, if projects instead defined their own versions of a Result / std::expected-like type, duplicated across many folks code bases.)

  • Include a clear disclaimer that Result is not intended to be used in public parts of Mojo stdlib code outside the experimental module.

    I strongly agree with @remorses that we do not want to fragment the way Mojo standard library code represents errors.

    Although we're okay with including Result in a clearly-labeled experimental part of the stdlib, we're not endorsing this as a replacement or alternative to raises-based error handling. We don't want to find ourselves in the situation of having two common but different ways of indicating that a function can result in an error.

    To be crystal clear: folks contributing to the standard library should continue to write code that uses a raises / raise interface, not Result. Any PR introducing a public interface based on Result error handling will not be accepted (unless its part of the experimental module.)

Please let me know if you think this path forward sounds reasonable to you 🙂


P.S. regarding the language feature-related issues that @remorses was highlighting: I generally agree that this; the ergonomics for Result based error handling will likely struggle without more dedicated language features. However, I don't think those language features are entirely necessary for Result to be usable or even useful. For example, methods on Result will enable a "functional" style of working with errors (see all the methods on the Result type from Rust for an example of what I mean) that I think has potential advantages over raises even without dedicated language features.

That's my hypothesis at least, but I wouldn't say that's a certain outcome 🤷‍♂️. Hence why I think the right thing for Mojo and the community is experimentation. We're all fortunate to be working on Mojo relatively early in its life, so experimentation and making mistakes is relatively low risk but potentially very valuable work to see what Mojo can do 🏎️ 🔥 🙂

@martinvuyk
Copy link
Contributor Author

Move this Result type to a new experimental/result.mojo top-level module in the standard library

That is a great idea.

Can Result work ergonomically in Mojo today?

We are still missing:

  • A constructor in Optional[T] that takes a Result[T]
  • How to interact with raises function signatures or make raises become Result (or the other way around) under the hood
  • Pattern matching for different errors. And decide if the pattern matching should be obligatory for every possible error type (i.e. always having a default item.err == True case without specifying err message/kind).
  • When we have enums, decide if we will migrate Result to it.

we do not want to fragment the way Mojo standard library code represents errors

I understand, but I personally don't like finding out what kind of errors might come up only at runtime or only if the developer kept up with docstring best practices. Enforcing handling of every error type at compile time is one of the best things that Rust does IMO.

I think raises has its uses especially for pythonic scripting when just trowing a bunch of darts at a problem until it's solved, but for a standard library I think we should eventually use Result and find a way to make it so that if a function that returns a Result is called inside a raises function, then the error inside the result should be matched against the value True -> the Error is raised, and if not the value is unwraped (calling .value() automatically in the codegen) or an Optional[T] is constructed.

Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
Signed-off-by: martinvuyk <martin.vuyklop@gmail.com>
@gabrieldemarmiesse
Copy link
Contributor

gabrieldemarmiesse commented Jun 21, 2024

@ConnorGray would it make sense for this experimental namespace to be located in a repository modularml/stdlib-experimental ? With this setup, it would be possible for external contributors to become maintainers of this repository. We can see that the Modular staff is really busy and having them review features that may not end up in the stdlib can impact Mojo's development speed overall. Having external contributors be maintainers would increase the velocity.

Furthermore, some contributors like @martinvuyk clearly write code faster than the Mojo maintainers can review it. If we let the community take ownership of this new repository, we can share the review workload.

Finally, it would be a good way to give more responsabilities to the community, who currently has a quite low "glass ceiling" (cannot become maintainers, cannot publish packages, cannot participate in stdlib's meetings, can only contribute to a delayed view of an internal codebase), without impacting the development of internal products.

As the syntax is also changing, the work necessary to handle a breaking change in the Mojo syntax will be lower as we won't garantee that the nightly will always work with this modularml/stdlib-experimental repository.

To garantee that modularml/stdlib-experimental is actually used by the community, we can bundle it into each release, we'd just have a bit of work to sync them before each release, that seems managable.

I think the biggest pains of this repo, from a contributors' perspective, come from the fact that internal code depends on it, which would not be the case for modularml/stdlib-experimental and thus can become a "proper open-source first" Mojo codebase.

If you want to see examples of those kind of situations in the wild: TensorFlow had the exact same issue as Modular at the time, they made https://github.com/tensorflow/addons which had the source of truth in a public github repo, was managed by the community and it has been a huge success, contributing was much much much easier than in the TF repo. Sometime, the Tensorflow devs would grab a feature to make it "graduate", which is easier than starting a new feature from scratch.

@martinvuyk martinvuyk marked this pull request as draft September 19, 2024 22:08
@martinvuyk martinvuyk closed this Oct 11, 2024
@martinvuyk martinvuyk deleted the add-result-type branch October 11, 2024 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants