ballerina-platform / ballerina-spec

Ballerina Language and Platform Specifications
Other
167 stars 54 forks source link

Make it easier to `check` and return a new error with additional context #1197

Open sameerajayasoma opened 1 year ago

sameerajayasoma commented 1 year ago

There can be one or more entry points in a typical real-world application: the main function, resource, or remote service methods. It is common to handle errors in these entry points and log them before exiting the program or sending responses to clients. Usually, many more functions are involved between the function where an error value is created and the entry point where the error is eventually handled and logged.

Our recommendation so far is to use check to bubble up the error value to the entry point. But, it is common in real-world applications to wrap an underlying error with a new error as it bubbles up. Typically new errors are created with additional contextual details. Also, the underlying error is set as the cause so that the complete error chain can be logged with their stack traces at the entry point.

Ballerina provides a way to create a new error by wrapping another with additional context. This pattern is visible in most enterprise applications written in Ballerina. e.g., WSO2 internal apps and Choreo services.

error? e = doA();
if e is error {
    return error ("Something happened", c1 = c1, c2 = c2, cause = e);
}

But if you use check, it simply returns the error to the caller. check is concise and easy, but it's not enough in specific scenarios.

check doA();

I created this issue to discuss the possibility of improving the check to make it easy to return with a new error.

jclark commented 1 year ago

This is one of the cases that do/on fail was designed to handle:

do {
  check doA();
  check doB();
} on fail var e {
   return error ("Something happened", e, c1 = c1, c2 = c2);
}

It will often be the case that multiple errors need to be augmented in the same way.

I could imagine adding a langlib/stdlib function to make the construction of the new error easier.

I could imagine add syntax to on fail for handling the case where the body is return expr (as with function definitions):

on fail var e => error ("Something happened", e, c1 = c1, c2 = c2);
jclark commented 1 year ago

We could add an expression-oriented on fail clause to check, so

check doA() on fail e => error("Something happened", e, c1 = c1, c2 = c2);

desugars to

do {
  check doA();
} on fail var e {
   fail error ("Something happened", e, c1 = c1, c2 = c2);
}

except that this is an expression.

(The variable e doesn't have an explicit type by analogy with arrow functions.)

sanjiva commented 1 year ago

It seems to me this is logically incorrect: T t = check expr-returning-T-or-error on fail e => expr-of-type-error because check is supposed to remove the error.

Would this work? T t = check expr on fail [var e] { return error("Something happened", e); }

That then desugars to

T|error t = expr;
if t is error 
   return error(...);

Same as what you suggested but without using arrow functions and using a statement block (which must return).

I guess one can argue return is heavy handed there because if this is inside a do { .. } on fail { .. } then it only returns to the fail block.

jclark commented 1 year ago

@sanjiva The semantic of check E are

  1. Evaluate E to get value v
  2. If v is an error, the expression completes abruptly with a check-fail with associated value v
  3. Otherwise, the expression completes normally with value v

With check E on fail x => F step 2 would be modified:

  1. Evaluate E to get value v
  2. If v is an error:
    1. bind x to v
    2. evaluate F to get a value e (which must be an error)
    3. the expression completes completes abruptly with a check-fail with associated value e
  3. Otherwise, the expression completes normally with value v
sanjiva commented 1 year ago

Ack .. that works. I was trying to avoid using the expression function syntax. But it solves the need!

jclark commented 1 year ago

Given that check is an expression (you can do check doA(check foo(), check bar())), if we want to enhance check we need to do this an expression. The language doesn't generally allow statements inside expressions because that would break the ability to create a sequence diagram.

So what we are doing here is creating an expression-oriented lightweight syntax for something that can already be done more generally with a more heavy-weight statement-oriented syntax. We have a couple of cases of this already in the language

I think the syntax I have proposed is consistent with this.

srinathperera commented 1 year ago

@sameerajayasoma can we get this into the next release coming in next month? It will make code people are writing less verbose and easy to read, so the faster we can get them out the better.

hasithaa commented 1 year ago

@srinathperera We already passed, next month release train for language features. We can make it part of Update 5.

jclark commented 1 year ago

@srinathperera @sameerajayasoma Are there common patterns for how the new wrapping error is created? I am wondering whether we can do anything to improve this aspect.

sameerajayasoma commented 1 year ago

@srinathperera Update 5 will be out in February last week as per the current state.

@jclark There is a requirement to print the complete error, including stack traces of all causes. I am unsure whether this should be part of the langlib, but we need a utility that the log module can use as well.

hasithaa commented 1 year ago

With check E on fail x => F step 2 would be modified:

@jclark, the syntax should be

check expression on fail [typed-binding-pattern] => expression

Usually, we have variables in the expression context with a type-binding-pattern. isn't it?

jclark commented 1 year ago

@hasithaa Usually we do, but we don't with https://ballerina.io/spec/lang/2022R4/#infer-anonymous-function-expr which seems analogous to me.

hasithaa commented 1 year ago

@hasithaa Usually we do, but we don't with https://ballerina.io/spec/lang/2022R4/#infer-anonymous-function-expr which seems analogous to me.

+1.

prakanth97 commented 1 year ago

<This can be ignored>

For the module-level [2] we can wrap inside the generated init function. .....

  1. If we take the below case,
    
    public function main() returns error? {
    _ = 1 + check doA() on fail e => error("Something happened", e);
    }

function doA() returns int { panic error("Error"); };


Shall we desugar it as below?

public function main() returns error? { do { _ = 1 + check doA(); } on fail var e { fail error("Something happened", e); } }


2. If we have defined it as a global variable, how should we desugar it?

var x = check doA() on fail e => error("Something happened", e);

prakanth97 commented 1 year ago
public function main() {
    _ = check 1 + 1 on fail e => error("Error occurred", e);
}

Currently, we are getting a parser error for the above case because the precedence for check x is higher than binary-expr. Refer: https://ballerina.io/spec/lang/master/#expressions

According to the syntax check E on fail x => F, users may write cases like the one mentioned above, which will be restricted by the parser. Is this acceptable, or do we need to improve the syntax? @jclark @hasithaa

jclark commented 1 year ago

@prakanth97 That is a serious problem, I think.

We need the precedence of check ... on fail ... to be like let ... in ... or from ... select ..., but I don't think we can make check ... have a different precedence from check ... on fail ..., and we can't change the existing precedence of check in Swan Lake (or perhaps in any release) without an unacceptable incompatibility.

hasithaa commented 1 year ago

@jclark I too agree with James, Since we can't change the precedence of check, It is better to have different precedence for check ... on fail .... @lochana-chathura @prakanth97 Do you see any problem with this approach?

jclark commented 1 year ago

I think the way to approach this is to make the expression version of on fail more like the current statement version. In the statement version of on fail the on fail is completely separate, both semantically and syntactically, from the check; any number of checks can be handled by a single on fail.

To do this with expressions, we leave check expressions unchanged, and simply add an on fail expression with the syntax

expr1 on fail [typed-binding-pattern] => expr2

Then the semantics are:

  1. Evaluate expr1
  2. If expr1 completes normally with value v, then the result is v.
  3. If expr1 completes abruptly with associated error value e, then bind e using the typed-binding-pattern and evaluate expr2

on fail can then have whatever precedence we want (lower than binary-expr).

So you can do

check foo() + check bar()
on fail e => error("something happened", e)

(assuming foo() and bar() can both return errors)

Note that

check 1 + 1 on fail ...

makes no sense since 1 does not return an error.

check foo() + bar() on fail ... 

will produce a semantic error if bar() returns an error (since + cannot be applied to a left hand operand of type error).

We could potentially require that the static type of expr1 does not allow error to reduce confusion (i.e. it must use check to catch all errors that can occur in the left operand).