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

Documentation should be improved for propagate and debug/stacktrace. #1365

Closed
amano-kenji opened this issue Jan 22, 2024 · 20 comments
Closed

Comments

@amano-kenji
Copy link
Contributor

repl:2:> (doc propagate)


    function

    (propagate x fiber)

    Propagate a signal from a fiber to the current fiber. The resulting
    stack trace from the current fiber will include frames from fiber.
    If fiber is in a state that can be resumed, resuming the current
    fiber will first resume fiber. This function can be used to
    re-raise an error without losing the original stack trace.

Here, x should be the first element of a tuple passed to try's catch part. For example,

(try (error "ok") ([err fiber] (propagate err fiber)))

The doc doesn't explain what x is.

repl:1:> (doc debug/stacktrace)


    cfunction
    ../janet-1.33.0/src/core/debug.c on line 388, column 1

    (debug/stacktrace fiber &opt err prefix)

    Prints a nice looking stacktrace for a fiber. Can optionally
    provide an error value to print the stack trace with. If err is nil
    or not provided, and no prefix is given, will skip the error line.
    Returns the fiber.

Here, the doc says both err and prefix should be absent in order for debug/stacktrace to omit error, but if either err or prefix is absent, debug/stracktrace doesn't print error.

@sogaiu
Copy link
Contributor

sogaiu commented Jan 22, 2024

Below is a demo of the remark about debug/stacktrace (newlines added for readability):

$ janet
Janet 1.33.0-23b0fe9f linux/x64/gcc - '(doc)' for help

repl:1:> (try (error "hi") ([e f] (debug/stacktrace f)))
  in <anonymous> [repl] on line 1, column 6
<fiber 0x559C8B12EFA0>

repl:2:> (try (error "hi") ([e f] (debug/stacktrace f e)))
  in <anonymous> [repl] on line 2, column 6
<fiber 0x559C8B131690>

repl:3:> (try (error "hi") ([e f] (debug/stacktrace f e "")))
error: hi
  in <anonymous> [repl] on line 3, column 6
<fiber 0x559C8B133D30>

Strictly speaking, I think one can read the docstring as claiming to describe the repl:1:> case (and not making claims about the others), but I think it would be clearer / more complete to describe it differently...unless perhaps there is no guarantee being made for the other two cases?

@sogaiu
Copy link
Contributor

sogaiu commented Jan 24, 2024

Looking at debug.c's janet_stacktrace_ext, it appears that what determines whether the error line is printed is the value of prefix:

void janet_stacktrace_ext(/* elided */ const char *prefix) {
    // elided...
    int wrote_error = !prefix; // <-- NOTE 1

    // elided...

    for (fi = janet_v_count(fibers) - 1; fi >= 0; fi--) {
            // elided...

            /* Print prelude to stack frame */ // <-- NOTE 2
            if (!wrote_error) {
                JanetFiberStatus status = janet_fiber_status(fiber);
                janet_eprintf("%s%s: %s\n",
                              prefix ? prefix : "",
                              janet_status_names[status],
                              errstr ? errstr : janet_status_names[status]);
                wrote_error = 1;

For debug/stacktrace, prefix is populated like:

    const char *prefix = janet_optcstring(argv, argc, 2, NULL);
    janet_stacktrace_ext(fiber, x, prefix);

and janet_optcstring is:

const char *janet_optcstring(const Janet *argv, int32_t argc, int32_t n, const char *dflt) {
    if (n >= argc || janet_checktype(argv[n], JANET_NIL)) {
        return dflt;
    }
    return janet_getcstring(argv, n);
}

Perhaps then the docstring could be:

(debug/stacktrace fiber &opt err prefix)

Prints a nice looking stacktrace for a fiber. Can
optionally provide an error value to print the 
stack trace with. If `prefix` is nil or not provided, 
will skip the error line. Returns the fiber.

@sogaiu
Copy link
Contributor

sogaiu commented Jan 24, 2024

Investigating propagate, tried executing the following in a file using janet:

(var [f1 f2]
     [nil (coro :inside)])

(try 
  (error "a")
  ([e]
    (set f1 (fiber/current))
    (resume f2)
    (propagate :x f2)
    :unreached))

(pp [(fiber/last-value f1)
     (fiber/last-value f2)])

Output was:

(:x :inside)

So it looks like the x in propagate ends up as the "last value" for the fiber in which propagate is evaluated (usually not for the fiber it is passed) [1].

May be this gets us slightly closer to an explanation of what x is in propagate's docstring...


[1] Snooped from here.

@sogaiu
Copy link
Contributor

sogaiu commented Jan 25, 2024

Looking at the bytecode instruction execution implementation of propagate:

    VM_OP(JOP_PROPAGATE) {
        Janet fv = stack[C];
        vm_assert_type(fv, JANET_FIBER);
        JanetFiber *f = janet_unwrap_fiber(fv);
        JanetFiberStatus sub_status = janet_fiber_status(f);
        if (sub_status > JANET_STATUS_USER9) {
            vm_commit();
            janet_panicf("cannot propagate from fiber with status :%s",
                         janet_status_names[sub_status]);
        }
        fiber->child = f;
        vm_return((int) sub_status, stack[B]);
    }

If we assume an invocation of (propagate x fib), then roughly stack[C] from:

Janet fv = stack[C];

corresponds to fib, and stack[B] corresponds to x.

Then sub_status is roughly fiber/status for fib because:

        JanetFiber *f = janet_unwrap_fiber(fv);
        JanetFiberStatus sub_status = janet_fiber_status(f);

The code:

vm_return((int) sub_status, stack[B]);

leads to [1] the following bit of janet_continue_no_check:

        sig = run_vm(fiber, in);
    }

    /* Restore */
    if (janet_vm.root_fiber == fiber) janet_vm.root_fiber = NULL;
    janet_fiber_set_status(fiber, sig);
    janet_restore(&tstate);
    fiber->last_value = tstate.payload;
    *out = tstate.payload;

    return sig;

Here, sig corresponds to (int) sub_status from earlier.

Note that:

janet_fiber_set_status(fiber, sig);

takes the fiber status from fib and sets it as the status for fiber (the current fiber).

Also, stack[B] ends up as tstate.payload because prior to the call to run_vm, there is a call to janet_try which in turn executes janet_try_init, which has this line [2]:

    janet_vm.return_reg = &(state->payload);

Phew!


So in summary, a "signal" (sub_status aka fiber status) of fib (the fiber from (propagate x fib)) is obtained and then this is used to set the status of the current fiber (typically different from fib). Perhaps that's what is meant in the text of propagate's docstring:

Propagate a signal from a fiber to the current fiber.

[1] vm_return is:

#define vm_return(sig, val) do { \
    janet_vm.return_reg[0] = (val); \
    vm_commit(); \
    return (sig); \
} while (0)

[2] Since vm_return's second argument more or less ends up as janet_vm.return_reg and because:

    janet_vm.return_reg = &(state->payload);

state->payload / tstate.payload ends up with the value of the second argument.

bakpakin added a commit that referenced this issue Jan 28, 2024
@amano-kenji
Copy link
Contributor Author

amano-kenji commented Jan 30, 2024

After debug/stacktrace is done, propagate is remaining.

@sogaiu
Copy link
Contributor

sogaiu commented Jan 30, 2024

The following is from the website docs:

Different from traditional coroutines, Janet's fibers implement a signaling mechanism, which is used to differentiate different kinds of returns. When a fiber yields or throws an error, control is returned to the calling fiber. The parent fiber must then check what kind of state the fiber is in to differentiate errors from return values from user-defined signals.

So as I understand it, after calling resume for a fiber, one might want to:

  • check its state (aka "status", I presume) - this can be done via fiber/status
  • check its "last-value" - according to fiber/last-value's docstring, this is "the last value returned or signaled from the fiber"

So bringing some of the pieces together, examining a portion of the docstring:

(propagate x fiber)

Propagate a signal from a fiber to the current fiber.

  • fiber is some fiber which:
    • has a status
    • has a last value [1]
  • the current fiber (not typically fiber):
    • has its status set to the same as fiber's status
    • has its last value set to x

IIUC, "signal" refers to the status of the fiber (based on earlier investigation in this issue).


try is a typical place one might want to use propagate [2], so for the purpose of digesting the content above, consider a macro expansion of a try invocation:

(macex1 '(try (error :a) ([e f] (propagate e f))))

One can see (with a little massaging of the output):

(let [_00000r (fiber/new (fn [] (error :a)) :ie) 
      _00000s (resume _00000r)]
  (if (= (fiber/status _00000r) :error)
    (do
      (def e _00000s)
      (def f _00000r)
      (propagate e f))
    _00000s))

Here:

  • f (aka _00000r) is a fiber
  • e (aka _00000s) is a value returned from resuming the fiber

One can see that after f is created, it is resumed (which is when e is obtained). Then the status is checked:

  • If the status is :error, propagate is called
  • Otherwise, e is returned

Note that in general it's not necessary for the first argument to propagate be e -- other Janet values could work too (e.g. things that are based on or contain e).

For another example, see the code in this comment.


[1] fiber's typically have their "last value" this initialized to nil.

[2] Though propagate can be used outside of try as well.

@sogaiu
Copy link
Contributor

sogaiu commented Jan 31, 2024

Below is a candidate docstring that I think is more complete:

    (propagate x fiber)

    Propagate a signal (status) from `fiber` to the current fiber and
    set the last value of the current fiber to `x`.  

    The resulting stack trace from the current fiber will include 
    frames from `fiber`. If `fiber` is in a state that can be resumed,
    resuming the current fiber will first resume `fiber`. 

    This function can be used to re-raise an error without losing
    the original stack trace.

I think it's not unreasonable to expect people to have read relevant sections of the website docs. In the current case, I think the fibers portion is a likely place people might look given that the first sentence of the docstring mentions fibers.

Further, I think (as pointed out in an earlier comment) that section of the website docs has relevant and useful information that can help to make sense of this docstring.

However, I'm not sure fiber/status and fiber/last-value have good coverage in the main text of the website docs (not the API listing). They are mentioned at the end of the Multithreading page, but sort of only in passing:

In the above example, depending on whether or not the "oops!" error was triggered, event will be either (:error "oops!" nil) or (:ok nil nil). :error and :ok correspond to (fiber/status f), while "oops!" and nil to (fiber/last-value) of the main fiber of the child thread.

I might have missed other treatment in the main text, but if that is really missing, may be it would make sense to add something appropriate.

@amano-kenji
Copy link
Contributor Author

amano-kenji commented Jan 31, 2024

(doc propagate) should not be an IQ test puzzle. Right now, it is. If you want people to use open-source softwares, you should improve UX. In this case, UX means clear documentation.

As clear as a llama spitting in your face...

Nuances and passing remarks won't do. Llama spits will do... I don't want janet to be an esoteric language.

@sogaiu
Copy link
Contributor

sogaiu commented Feb 1, 2024

I'll start by saying that if you haven't read the main maintainer's comment here about the goals of the project, it might be worth trying to take that in and think over your most recent comment in light of it. Note that his comment is from a few years back and things can change over time, so grains of salt and all that. However, it might help in understanding the current state of things, so I still recommend reading it.


Regarding the documenation for propagate...

I will repeat a quote from Gerald Sussman here (which I have mentioned elsewhere to you) concerning his opinion on the reality of contemporary software development:

The work of engineers used to be about taking small parts that they understood entirely and using simple techniques to compose them into larger things that do what they want.

But programming now isn't so much like that. Nowadays you muck around with incomprehensible or nonexistent man pages for software you don't know who wrote. You have to do basic science on your libraries to see how they work, trying out different inputs and seeing how the code reacts. This is a fundamentally different job.

via: https://wingolog.org/archives/2018/01/11/spectre-and-the-end-of-langsec

This matches a fair bit of my experience of working with a variety of programming languages. I think it's something many developers are likely to be able to relate to. It is true that I experience a certain amount of that working with Janet, but I have found it to be less so than for a number of others (including another lisp-like that I spent some years with, that has a much bigger user base AND more maintainers).

As mentioned previously though, I don't think the docstrings are meant to be read in isolation. I think the expectation to consult the website docs is sound and reasonable because docstrings are not on their own a sufficient medium of expression for communicating certain kinds of information in a practical manner.

I do think Janet's documentation (both the website and the docstrings) can still be improved (can you tell from the effort I (and others) are putting into it?).

In the current case, I think the proposed change to propagate's docstring in this comment addresses one of the main issues with it:

The doc doesn't explain what x is.

As mentioned in this comment, I think some changes to the website docs regarding fiber/last-value and fiber/status would further help with understanding propagate, but that is probably better as another issue at the janet-lang.org repository [1].


[1] I wanted to get confirmation that those two functions aren't really covered that well in the main text before making an issue though.

@amano-kenji
Copy link
Contributor Author

the last value of the current fiber to x.

What? What is the value of a fiber?

@sogaiu
Copy link
Contributor

sogaiu commented Feb 1, 2024

What is the value of a fiber?

This would be the type of thing to address in the website docs as background information regarding fibers.

By the way it is the "last value", not the value of the fiber. That is one of the things I mentioned above about touching on fiber/last-value in the website docs.

@sogaiu
Copy link
Contributor

sogaiu commented Feb 1, 2024

I think the use of "What?" at the beginning of your comments as you have been doing on more than one occasion might be interpreted as being on the uncivil side. Further, I don't believe the contexts in which it has arisen have warranted that kind of expression.

Please be more considerate in your communication.

@pepe
Copy link
Member

pepe commented Feb 1, 2024

OT. I reread those three years old comments and am happy with what we did. Thank you, @sogaiu, for mentioning it and being part of the effort!

@amano-kenji
Copy link
Contributor Author

amano-kenji commented Feb 1, 2024

After finalizing j3blocks and j3blocks-extra, I finally had some attention left to read comments here.

It seems that :error fiber/status is a signal and the error value is actually a value returned from a fiber?

No documentation mentions something like that.

I thought the error value was the signal.

@sogaiu
Copy link
Contributor

sogaiu commented Feb 1, 2024

I think you are not digesting well what I have already written above. I suggest you read more carefully.

I believe I have covered all of the mentioned points.

@amano-kenji
Copy link
Contributor Author

I think that pull request in combination with #1365 (comment) can resolve this issue.

@sogaiu
Copy link
Contributor

sogaiu commented Feb 3, 2024

Based on some futher information, comments, and digestion of collected info [1], I'm considering the following alternative proposal for propagate's docstring:

    (propagate x fiber)

    Propagate a signal from `fiber` to the current fiber and
    set the last value of the current fiber to `x`.  The signal
    value is then available as the status of the current fiber.

    The resulting stack trace from the current fiber will include 
    frames from `fiber`. If `fiber` is in a state that can be resumed,
    resuming the current fiber will first resume `fiber`. 

    This function can be used to re-raise an error without losing
    the original stack trace.

[1] Here are some relevant bits:

@amano-kenji
Copy link
Contributor Author

I think that's good.

sogaiu pushed a commit to sogaiu/janet that referenced this issue Feb 3, 2024
@sogaiu
Copy link
Contributor

sogaiu commented Feb 3, 2024

Thanks for taking a look.

I've opened #1392 for a proposal to add to the docstring of propagate.

bakpakin added a commit that referenced this issue Feb 3, 2024
@sogaiu
Copy link
Contributor

sogaiu commented Feb 13, 2024

My impression is that this issue has been addressed.

Perhaps it can be closed.

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

No branches or pull requests

3 participants