Skip to content

Add ability to return errors from custom functions. #159

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

Merged
merged 1 commit into from
Aug 1, 2021
Merged

Add ability to return errors from custom functions. #159

merged 1 commit into from
Aug 1, 2021

Conversation

ffenix113
Copy link
Contributor

Also provide initial documentation on using and providing functions.

Also provide initial documentation on using and providing functions.
@antonmedv
Copy link
Member

Awesome Work! Nice with tests! But why current method is not okay? You can just panic in custom func.

@ffenix113
Copy link
Contributor Author

ffenix113 commented Jan 29, 2021

Thank you for your comment.

While true that panic's will also return errors it is not always useful. For example there could be cases when "panic" is forbidden by style-guide. Also checking for returned error message instead of it's type or behavior is not the best approach either.

I learned that it is useful to re-use already existing functions in the environment, but in some(if not most) cases such functions/methods can also return error to specify is something went wrong. Leaving out the error in such case could lead to unpredictable results of expression evaluation.

It will also be possible to check if the error is from compiling or running stage(like types mismatch) or from the functions that are being called in expression. In such case if error is *file.Error - then it is from compiling/runtime, while everything else is from functions from environment.

And most useful feature would be to preserve the context of the error, such as its type. Consider this example:

// Error1 and 2 are types like below, which also implement `error` interface
// type Error1 struct { Message string, Val interface{} }

env := map[string]interface{}{
  "err": func(i interface{}) (int, error) {
    switch typed := i.(type) {
    case int:
      return typed, nil
    case float64:
      return 0, Error1{Message: "value is float", Val: typed}
    default:
      return 0, Error2{Message: "value not numeric", Val: i}
    }
  },
}

program, _ := expr.Compile(`err(11.1)`)
res, err := expr.Run(program, env)
// err will be Error1, preserved from the return statement

While it is simple and crude example, it shows that this approach can be used to get error type and even capture context in a type that implements error interface.

@antonmedv
Copy link
Member

This is a great argument. Give me some time to review your PR.

@ffenix113
Copy link
Contributor Author

Hello,

Sorry to bother, but have you had any time to check this out?
We would like to use this feature in prod ASAP..

@antonmedv
Copy link
Member

I still need time to review it, as change is quite big.

@graineri
Copy link

Any progress on this PR?

@antonmedv
Copy link
Member

Will try to have a look on this weekend. Sorry for a delay.

@m-barthelemy
Copy link

Hi @antonmedv is there any issue with this PR?

@antonmedv antonmedv merged commit aad1dca into expr-lang:master Aug 1, 2021
@antonmedv
Copy link
Member

Sorry for a loooong delay))

@ffenix113 ffenix113 deleted the feature/return-function-errors branch August 1, 2021 15:42
vm.push(fn.(func(...interface{}) interface{})(in...))
if typed, ok := fn.(func(...interface{}) interface{}); ok {
vm.push(typed(in...))
} else if typed, ok := fn.(func(...interface{}) (interface{}, error)); ok {
Copy link
Member

Choose a reason for hiding this comment

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

CallFast is a special case. We don't want to change the signature of call fast methods. The idea for call fast funcs is what they are created manually for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't change it here, but rather allowing to add a new one, that also returns an error.

So both variants will work.

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.

4 participants