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

Error handling overhaul, includes CC#1743 #29

Closed

Conversation

Projects
None yet
2 participants
@hostilefork
Copy link
Member

commented Jul 27, 2015

PROMPT REVIEW REQUESTED...a lot builds on this, and it's
taking a very long time as it is to try and ramp the code up to where
I'd like it to be.

(Note QUIT now takes an INTEGER! value only, and has no /NOW
refinement. A small step for now, but ideally EXIT will be shifted to
act as QUIT/RETURN, and QUIT will be arity zero meaning EXIT 0)

This is a major rewrite of the setjmp/longjmp-based error handling,
and it's not really that reasonable or feasible to break it up much
more than this. These things are all inter-related.

Central ideas are that there aren't two separate saved states
to worry about (Halt_State is integrated into Saved_State) and the
PUSH_CATCH/DROP_CATCH macros are clearer and simpler,
with less chance for making mistakes in usage.

The mechanism for including a REBVAL associated with an error
has been simplified and formalized a little more. Though simpler,
an attempt to further tighten the grip on this "sneaky" way of having
one error-per-task turned out to complicate the code a bit too much,
and so it could still be improved with some better asserts.

hostilefork added some commits Jul 27, 2015

Error handling overhaul, includes CC#1743
(Note QUIT now takes an INTEGER! value only, and has no /NOW
refinement.  A small step for now, but ideally EXIT will be shifted to
act as QUIT/RETURN, and QUIT will be arity zero meaning EXIT 0)

This is a major rewrite of the setjmp/longjmp-based error handling,
and it's not really that reasonable or feasible to break it up much
more than this.  These things are all inter-related.

Central ideas are that there aren't two separate saved states
to worry about (Halt_State is integrated into Saved_State) and the
PUSH_CATCH/DROP_CATCH macros are clearer and simpler,
with less chance for making mistakes in usage.

The mechanism for including a REBVAL associated with an error
has been simplified and formalized a little more.  Though simpler,
an attempt to further tighten the grip on this "sneaky" way of having
one error-per-task turned out to complicate the code a bit too much,
and so it could still be improved with some better asserts.
Comments and get rid of IS_XXX for errors
I specifically targeted IS_RETURN for deletion originally because there
was a new ANY-FUNCTION! class of value called RETURN!, and it was
easy enough to test VAL_ERR_NUM(error) against RE_RETURN to
remove the competition between the tests.  I'll probably not be adding
a new data type in the formal commit, but still it's better to be explicit
about the error number testing, especially given there are so few
instances of that.
@earl

This comment has been minimized.

Copy link

commented on src/core/a-lib.c in 7d19b08 Jul 27, 2015

The above comment seems to indicate, that the default toplevel error handling behaviour (cf CC#2215 is changed from what I call "halt-on-error" (previous/current behaviour) to "quit-on-error" (behaviour after this change). I appreciate the change per se, but suggest to add a remark that this is included in the set of changes to the commit message.

@earl

This comment has been minimized.

Copy link

commented on src/core/a-lib.c in 7d19b08 Jul 27, 2015

Absolutely minor stylistic Q: why is the assert after saving the error in the system object? I personally like to put assertions such as this one as far up in the guarded block as possible. (As consequence of this preference, whenever I see an assert midway in seemingly straight-line code, a "why?" flag is going up.)

This comment has been minimized.

Copy link
Owner Author

replied Jul 27, 2015

As a talking point on a minor style question, I'd say there's a difference between two things:

  • your main concern is the maintenance of the invariant, so establish it at as early as possible
  • your main concern is to communicate an invariant to someone modifying a piece of code at a location later than the absolute first point where the invariant is established.

In this case, it's more the second. I'm not terribly worried that zero error numbers are running amok in the system, rather making sure the return value is understood to not accidentally be registered as success. The text next to the return could say: ...error numbers can be unstable, but are never zero and that would be fine...as opposed to keeping the assert and moving it right next to return.

Yet the general principle remains: where a comment may be used to state something you're actually reasonably confident about, asserts can sometimes communicate better than prose what exactly the confident thing is you want to call out. And also, they have teeth and can serve as a doublecheck.

(In this case, would be serving as a doublecheck of some better ASSERT_ERROR integrity check done by the Catch/Throw, that would check more than just "error code isn't zero")

This comment has been minimized.

Copy link
Owner Author

replied Jul 29, 2015

And actually, as it turns out, I had not brought in my change to errors.r which brought about the invariant of no zero errors. (Being so long since I'd done it, I guess I'd come to think it had always been so.) So RE_BREAK was still zero. Moving the assert to a more central place caught that fact. Good to call it out...

@earl

This comment has been minimized.

Copy link

commented on src/core/a-lib.c in 7d19b08 Jul 27, 2015

Above you comment "!!! Through this interface we have no way to distinguish an error returned as a value from one that was thrown", yet here you add a way to do precisely this, if I am not mistaken:

  • (0) return value == 0: if result == NULL (i.e. caller didn't ask for the result to be saved)
  • (e) return value < 0: evaluation threw an error, negated return value is the error number
  • (v) return value > 0: evaluation did not throw an error, return value is the type of the returned value, value itself is pointed to by result

Unless my analysis is mistaken (which could very well be), why, for the error-thrown case (e), not save and return the error value in the same way as is done for the value in the no-error-thrown case (v)?

This comment has been minimized.

Copy link
Owner Author

replied Jul 27, 2015

"you add a way to do precisely this"

Perhaps yes, but not a way "existing clients expected" (to the extent there are any besides host-main.c...). Being the only client I know, it's written like:

while (TRUE) {
    Put_Str(prompt_str);
    if ((line = Get_Str())) {
        RL_Do_String(line, 0, 0);
        RL_Print_TOS(0, result_str);
        OS_FREE(line);
    }
    else break; // EOS
}

We could see more decision points in the loop above, to where it could be customized. Examples might be things like being able to print errors colorfully and make a beep if they are thrown (vs. just molded as values).

So far I've not focused on that aspect, but if someone (looks around for a show of hands from volunteers...?) wanted to, they could probably refactor just a little bit so that all the I/O for the interface happens in there. Get it to where Rebol doesn't print anything without giving an opportunity for host-main to have its say, even in RL_Api...

@earl

This comment has been minimized.

Copy link

commented on src/core/a-lib.c in 7d19b08 Jul 27, 2015

resolve is only used to later refer to the user context (system/contexts/user) and then resolve that against the lib context. Maybe user_ctx or user_context would be more descriptive names?

This comment has been minimized.

Copy link
Owner Author

replied Jul 28, 2015

The original name for this was rc, presumably for resolve context. I didn't like the name rc and at some point changed it to resolve as short but at least communicating what the r was for. I also hadn't quite gotten to the decision of using local scopes at that time, perhaps wondering if there was some reason they were being avoided. Nowadays we've accepted the "top of scope" C89-legal definition, so moving it into where it's used and just calling it user is probably enough to get the gist of what's going on...

This comment has been minimized.

Copy link

replied Jul 28, 2015

so moving it into where it's used and just calling it user is probably enough to get the gist of what's going on...

+1

@earl

This comment has been minimized.

Copy link

commented on src/core/a-lib.c in 7d19b08 Jul 27, 2015

Heh, elegant :) That (typically? always?) longjmps/throws to the setjmp/catch set up earlier in this very same function, right?

This comment has been minimized.

Copy link
Owner Author

replied Jul 28, 2015

Always...but because of the assert(DSP == 0). If it didn't enforce that, then a Halt would not offer an error to this. Instead the error macros would keep passing up to the enclosing level. (Or that's the idea anyway.)

If there were nested calls, then a catch/quit living in a higher call stack wouldn't get an opportunity...because this one would just OS_EXIT. So the reason for the assert is basically because the caller isn't doing any CATCH/QUIT-ting because it doesn't have an RL_API for doing so. Hence for quit to be caught, it has to be done in this routine.

Initially I tried to get around this, by putting the main quitting catch in RL_Init(). But if you notice the all-caps DROP_CATCH_SAME_STACKLEVEL_AS_PUSH, you'll see that I realized that does not work. Don't try returning across setjmp/longjmp. Anyway, this is why Ren/C clients like Ren/C++ need their hands on the CATCH macros.

hostilefork added some commits Jul 28, 2015

Add ASSERT_ERROR and use it in Throw()
@earl pointed out that the 'assert(VAL_ERR_NUM(error) != 0' was a
bit of a non-sequitur.  If error numbers aren't ever zero, that guarantee
should be established somewhere less random.  I started a basic
ASSERT_ERROR so that the verification is done at time of throw in
a debug build.
@hostilefork

This comment has been minimized.

Copy link
Owner Author

commented on src/core/n-control.c in 7d19b08 Jul 29, 2015

Got confused here thinking break always passed a value, but it sets it to "0" (NULL). Due to null tolerance that winds up getting interpreted as an unset value later on, when I actually had it in my head that breaking a loop would return NONE! by default.

Yet returning UNSET! is much better, and finesses the problem of "how would you break with an unset". Combining that with the reminder that "break/with is a lot better than break/return or break/value", it seems that perhaps this points to a consistent solution for RETURN as well: to be zero arity and return UNSET! by default, and then have a /WITH refinement.

Rambled in chat about it for a bit because I felt like it was a sort of a break/thru. But for the moment, I will patch this to return an UNSET!.

@hostilefork

This comment has been minimized.

Copy link
Owner Author

commented on src/core/u-parse.c in 7d19b08 Jul 29, 2015

This does work, and it was necessary (functions do not setjmp to catch a RETURN). Fixed, although it's a reminder that the categories need to be thought through a bit further. You don't throw breaks/returns/continues because they are thrown by their nature and don't have error objects...but then halt and quit are THROWN implicitly yet you throw them anyway.

All getting closer to manageable, in any case.

@hostilefork

This comment has been minimized.

Copy link
Owner Author

commented on src/core/n-control.c in 7d19b08 Jul 29, 2015

I don't know why the code I had put an error here when the goal is to suppress the error. This should be a none.

Some kind of inlining gone awry, maybe I missed a cue there somewhere because the original is wrong if this is how it's supposed to work:

error? attempt [make error! "Some error that hasn't been DO'd"]
none? attempt [do make error! "Some error we are DO-ing"]

Anyway, fixed.

hostilefork added some commits Jul 29, 2015

Correct handling of THROWN arguments in Do_Args
Fixes cases like 'print return 10', adds in some simple assertions
for sanity checking.

@earl earl force-pushed the metaeducation:master branch from 874a945 to 631bb4a Aug 2, 2015

@hostilefork

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2015

After the initial draft for commentary on methodology (prior to anything other than smoke testing of 'test suite did not crash'), going over the points found problems that were corrected. Having gotten back up to date on precisely why and when an error is "thrown" or just bubbled up, this could use another pass of improvements. But it seems to be working now.

Committed in rebased form as:

9b21568

Review comments still welcome of course, but the show must go on.

@hostilefork hostilefork closed this Aug 4, 2015

@hostilefork hostilefork deleted the hostilefork:error-handling-overhaul branch Aug 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.