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

Propose Result.try to avoid syntax changes #81

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arthurfiorette
Copy link
Owner

No description provided.

@arthurfiorette arthurfiorette self-assigned this Feb 28, 2025
*
* const [ok, error, value] = Result.try(func, arg1, arg2)
* const [ok, error, value] = await Result.try(asyncFunc, arg1, arg2)
* const [ok, error, value] = await Result.try(async (arg) => arg, 'pass')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Javascript typically uses arg1, arg2, ... to notate variables that pass through, so examples 1 and 2 already seem to obviously pass through. This third example confuses the issue. It looks like a instruction, but I think you're just saying it's a variable that passes through.

@DScheglov
Copy link
Contributor

So, you are going to introduce the Result type!
It must be monadic and errors must be typed.

@Arlen22
Copy link
Collaborator

Arlen22 commented Feb 28, 2025

I don't think the try method in this PR covers all the uses that the try operator would.

I've implemented the try operator in Typescript as a way to play around with it and see if we're missing anything. In order to handle every use case I had to use four separate helper functions.

I messed around with this for a bit and figured out that we can indeed combine all four uses cases into one function.

We can combine sync and async by just calling the callback and then checking if it returned a promise. We can also check if the callback returned an iterator or async iterator using the symbol. So we can combine them all into one helper function fairly deterministically it seems.

Now, keep in mind that the entire runtime specification of the try operator proposal amounts to the following four lines, and because of how await and yield are implemented in the spec, I don't think the try operator is even affected by their existence. As far as I can tell, it's literally this simple.

TryExpression : try AssignmentExpression

  1. Let A be Completion(Evaluation of Expression).
  2. If A is an abrupt completion, return TryExpressionResult(A).
  3. Let B be Completion(GetValue(A)).
  4. Return TryExpressionResult(B).

And TryExpressionResult(result) is just

  1. If result is a normal completion, return Result.ok(result.[[VALUE]]).
  2. If result is a throw completion, return Result.error(result.[[VALUE]]).

With that in mind, I now present:

The Comprehensive Try Function (edited)

function try_(callback, thisarg) {
  class Result {
    constructor(ok, error, value) {
      this.ok = ok
      this.error = error
      this.value = value
    }
    *[Symbol.iterator]() {
      yield this.ok
      yield this.error
      yield this.value
    }
    static ok(value) {
      return new Result(true, undefined, value)
    }
    static error(error) {
      return new Result(false, error, undefined)
    }
  }
  const syncCall = (function () {
    try {
      return Result.ok(callback.apply(thisarg));
    } catch (e) {
      return Result.error(e);
    }
  })();

  if (!syncCall.ok) return syncCall;

  if (isPromise(syncCall.value)) {
    return syncCall.value.then(
      (value) => Result.ok(value),
      (e) => Result.error(e)
    );
  }

  if (isIterable(syncCall.value)) {
    return (function* () {
      try {
        return Result.ok(yield* syncCall.value);
      } catch (e) {
        return Result.error(e);
      }
    })();
  }

  if (isAsyncIterable(syncCall.value)) {
    return (async function* () {
      try {
        return Result.ok(yield* syncCall.value);
      } catch (e) {
        return Result.error(e);
      }
    })();
  }
  
  return syncCall;

  function isPromise(result) { return result && typeof result.then === 'function'; }
  function isIterable(result) { return result && typeof result[Symbol.iterator] === 'function'; }
  function isAsyncIterable(result) { return result && typeof result[Symbol.asyncIterator] === 'function'; }

}

The usage isn't spectacular.

// try "hello"
const [ok, error, value] = try_("hello");

// try await "hello"
const [ok, error, value] = await try_(async () => await "hello");

// try yield "hello"
const [ok, error, value] = yield* try_(function* () { return yield "hello"; }, this);

// try yield await "hello"
const [ok, error, value] = yield* try_(async function* () { return await "hello"; }, this);

I'm being a bit dramatic, of course, but my point is that as far as I can tell, the try operator syntax is incredibly simple to implement, especially compared to some of the other proposals that we're seeing like the using directive, And the closest thing we can get to it currently is still rather annoying and easy to mess up.

Case in point: I forgot the yield inside the fourth example.

@DScheglov
Copy link
Contributor

DScheglov commented Mar 2, 2025

Hey, @Arlen22

these fragments:

  if (isIterable(syncCall.value)) {
    return (function* () {
      try {
        return TryResult.ok(yield* syncCall.value);
      } catch (e) {
        return TryResult.error(e);
      }
    }).apply(thisarg);
  }

  if (isAsyncIterable(syncCall.value)) {
    return (async function* () {
      try {
        return TryResult.ok(yield* syncCall.value);
      } catch (e) {
        return TryResult.error(e);
      }
    }).apply(thisarg);
  }

do very wierd things and they looks like excessive.

Because to reach the TryResult.ok will require manully call .next() while it doesn't return { done: true, value: TryResult.ok() }.

What do you want to achieve with it?

And the usage of thisarg looks very strange, becaue the only place where it must be used is callback.apply(thisarg), the rest code doesn't require it including this (fixed with passing thisarg to callback):

  const syncCall = (function () {
    try {
      return TryResult.ok(callback.apply(thisarg));
    } catch (e) {
      return TryResult.error(e);
    }
  }).apply() as TryResult<any>;

@Arlen22
Copy link
Collaborator

Arlen22 commented Mar 2, 2025

You're correct about thisArg. That's my mistake. Fixed.

You can think of yield* as the generator version of the spread syntax.

Since the code in the generator function isn't actually run until you call next, we can wrap the yield star invocation of it in the try catch, catch any error of the generator function throws, return the final return value of the function, and correctly forward all yields inside the function to their parent generator function that the yield was supposed to have been in if the try operator existed.

function* parent() {
  return yield* try_(function*() {
    return yield "hello"; //this yields from parent
  }, this);
}
const iter = parent();
console.log(iter.next().value); // prints "hello"
const { value: [good, error, value], done } = iter.next("some value");
console.log(done, value) // prints true "some value"

In my example usage:

const result = yield* try_(function* () { 
  return yield "hello"; // should yield a value in the parent generator
}, this);

yield* try_(... does indeed keep calling next until it returns.

@DScheglov
Copy link
Contributor

You can think of yield* as the generator version of the spread syntax.

@Arlen22

didn't get anything ... sorry.

Just try to call you function with callback that returns string or array.
In this case your try_ function returns Generator instead of the TryResult -- and it becomes really unexpected.

In general in try should avoid making any assumption regarding the packed value.

@Arlen22
Copy link
Collaborator

Arlen22 commented Mar 2, 2025

I don't exactly follow, but here's a gist with code that you can just run in the browser console to see how it works or test variations.

https://gist.github.com/Arlen22/3b17fc55c63354a4038f3638eecb1534

This code works for me in my browser.

@DScheglov
Copy link
Contributor

I don't exactly follow, but here's a gist with code that you can just run in the browser console to see how it works or test variations.

https://gist.github.com/Arlen22/3b17fc55c63354a4038f3638eecb1534

This code works for me in my browser.

Just run this:

const result = trt_(() => 'string');

And you got the following result (you can do it directly in browser console):

image

@Arlen22
Copy link
Collaborator

Arlen22 commented Mar 2, 2025

Oh, strings are iterable, as are a ton of other things the callback could return. Ok, maybe we do need a separate helper function for the yield keyword. I think we can still use this to check whether it is sync or async, in both cases.

@DScheglov
Copy link
Contributor

Oh, strings are iterable, as are a ton of other things the callback could return. Ok, maybe we do need a separate helper function for the yield keyword. I think we can still use this to check whether it is sync or async, in both cases.

Actually a lot of things are iterable, even the TryResult is. I really cannot get what do you want to reach with your example

@Arlen22
Copy link
Collaborator

Arlen22 commented Mar 2, 2025

What I'm trying to reach is the equivalent of the try operator. But you're right about iterables, and even the promise check has this problem (it would change the output value of any promise, not just those you intend to await).

So maybe we have to have four separate helpers, one for each scenario.

  • try_
  • try_await_
  • try_yield_
  • try_yield_await_

@Arlen22
Copy link
Collaborator

Arlen22 commented Mar 2, 2025

I'm pretty sure I'm trying to prove how badly we actually need the try operator.

I'm tempted to implement the try operator in the V8 engine just to prove how simple it would be compared to all this.

@Arlen22
Copy link
Collaborator

Arlen22 commented Mar 2, 2025

The simplest possible variation is this:

// try <expression>
try_(() => <expression>)

But if I want use a try function in an async generator, for instance, it would have to be this:

// try yield await <expression>
yield* try_yield_await_(async function* () { return (
  yield await <expression>
); })

In contrast, because await and yield both pause the execution context, for the try operator no matter which variation this is, the evaluation looks the same. At least on paper. I can't speak to actual engine code. But the spec literally has a Completion Type which it uses everywhere.

@arthurfiorette
Copy link
Owner Author

This feels a lot of over-engineering. Could you please present a compelling use case for a/sync iterables?

@Arlen22
Copy link
Collaborator

Arlen22 commented Mar 3, 2025

This feels a lot of over-engineering. Could you please present a compelling use case for a/sync iterables?

Since this is a PR for the try operator proposal, I'm trying to cover every case the try operator solves. The try operator actually has a significant use case in generator functions, where it may be used to protect the yield keyword in case it throws. However, you can't simply call yield inside a callback, so the simple sync case can't be used to protect a yield. The simplest usage that can be used is what I've already outlined.

I also think only covering the sync case is too simple and makes this proposal pointless. The entire class can be written by hand, in Typescript, in about 3 minutes.

class Result<O extends boolean, V> {

  constructor(public ok: O, public error: true extends O ? undefined : unknown, public value: V){

  }
  *[Symbol.iterator](){
    yield this.ok;
    yield this.error;
    yield this.value;
  }
  static try<T>(callback:() => T): Result<true, T> | Result<false, undefined> {
    try {
      return new Result(true, undefined, callback());
    } catch (e) {
      return new Result(false, e, undefined);
    }
  }
}

And as I've already demonstrated in previous comments, both the implementation and the usage of try_yield_ helpers is complex and error-prone. It requires over-engineering to get it right, and is definitely worth publishing to NPM, but I doubt even that would get included in JavaScript core.

The regular async case is already easy to do with Promise.try(() =>{...}), and I think the primary reason it was added to Promise was it's because a simpler alternative to new Promise((resolve,reject)=> Promise.resolve().then(() =>{...}).then(resolve, reject)),

So there's no point in this proposal unless we're asking to simplify all four helper functions into a simple try operator with simple instructions. I really think the syntax changes are an essential part of this proposal and we can't avoid them.

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.

3 participants