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

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

Open
sameerajayasoma opened this issue Dec 8, 2022 · 18 comments
Assignees
Labels
enhancement Enhancement to language design lang Relates to the Ballerina language specification status/pending Design is agreed and waiting to be added
Milestone

Comments

@sameerajayasoma
Copy link
Contributor

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.

@sameerajayasoma sameerajayasoma added the enhancement Enhancement to language design label Dec 8, 2022
@sameerajayasoma sameerajayasoma changed the title Making it easier to create an error value by wrapping another with additional context Improve check to return a new error value by wrapping underlying error with additional context Dec 8, 2022
@sameerajayasoma sameerajayasoma changed the title Improve check to return a new error value by wrapping underlying error with additional context Improve check to return a new error with additional context Dec 8, 2022
@jclark
Copy link
Collaborator

jclark commented Dec 9, 2022

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 jclark changed the title Improve check to return a new error with additional context Make it easier to check and return a new error with additional context Dec 9, 2022
@jclark
Copy link
Collaborator

jclark commented Dec 9, 2022

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
Copy link
Contributor

sanjiva commented Dec 9, 2022

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
Copy link
Collaborator

jclark commented Dec 9, 2022

@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
Copy link
Contributor

sanjiva commented Dec 9, 2022

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

@jclark
Copy link
Collaborator

jclark commented Dec 9, 2022

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

  • anonymous functions using => instead of function(...){...}
  • expression-bodied functions/methods using =>E instead of curly braces

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

@jclark jclark added lang Relates to the Ballerina language specification status/pending Design is agreed and waiting to be added labels Dec 9, 2022
@srinathperera
Copy link

srinathperera commented Dec 12, 2022

@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
Copy link
Contributor

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

@jclark
Copy link
Collaborator

jclark commented Dec 12, 2022

@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.

@jclark jclark added this to the 2023R1 milestone Dec 12, 2022
@sameerajayasoma
Copy link
Contributor Author

@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
Copy link
Contributor

hasithaa commented May 8, 2023

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
Copy link
Collaborator

jclark commented May 8, 2023

@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
Copy link
Contributor

hasithaa commented May 8, 2023

@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
Copy link

prakanth97 commented Jul 20, 2023

<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);
    }
}
  1. 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
Copy link

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
Copy link
Collaborator

jclark commented Aug 8, 2023

@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
Copy link
Contributor

hasithaa commented Aug 8, 2023

@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
Copy link
Collaborator

jclark commented Aug 9, 2023

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).

@jclark jclark modified the milestones: 2023R2, 2023R1 Oct 4, 2023
@jclark jclark modified the milestones: 2024R1, 2024R2 Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to language design lang Relates to the Ballerina language specification status/pending Design is agreed and waiting to be added
Projects
None yet
Development

No branches or pull requests

6 participants