Deal with unhandled exceptions the same way in all Dart api functions.

Deal with unhandled exceptions the same way in all Dart api functions.

--

Terminology change: invalid handles are now called error handles.

Unhandled exceptions used to be a kind of valid handle.  Now they are
a kind of error handle.

For the Dart_Invoke* functions, this means that the standard
error-checking blob drops from two tests to one test and is
harder to mess up.

Many other Dart api functions (Dart_ToString, for example) previously
dealt with unhandled exceptions by turning them into terse error
messages.  Now these functions preserve information about the
exceptions and generate better error messages.

Changed Dart_HandleMessage to return success/failure.  It seemed to fit.

--

Details:

Dart_IsValid becomes Dart_IsError (negated sense).

Dart_GetError now knows how to print a stack trace semi-nicely.

Dart_ExceptionOccurred -> Dart_IsUnhandledException.

Renamed ApiFailure class to ApiError to fit better.

ApiError now has a "data" pointer that points to either an error
message string or to an unhandled exception object.

Documentation changes aplenty.

[Anton Muhin, 2011-11-09 12:34:03]

I really like where it's heading, thanks a lot, Todd.

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:141: int line,
nit: you may want to fix indentation: cf. const char* file and int line.

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:846: * May generate an unhandled exception error.
I am slightly concerned with tagging API functions with this info: it may be
hard to maintain.  E.g. we recently changed List API to allow it to deal w/
classes implementing List.  The change would be somewhat more difficult if we
committed to not throwing before.

I don't want to say we cannot maintain this information, but maybe we should ask
embedders to be overparanoid and assume any method may throw an exception.  That
doesn't look like a heavy burden: I would expect many people to write one
function to handle error handles anyway.

Up to you though.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:45: failure ^= obj.raw();
may you used const ApiError& = ApiError::Handle(obj.raw()) instead?

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:110: Object& strtmp =
Object::Handle(DartLibraryCalls::ToString(exception));
this is a typical paranoia and DartLibraryCalls::ToString is not quite correct
in this regard, but technically .toString in Dart may return any kind of object
including null, so maybe you should have it differently: if
(!strtmp.IsUnhandledException() { ... }

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:126: int len = strlen(exc_str) + strlen(stack_str) +
strlen(format) - 2;
why - 2, to account for %s?  if yes, shouldn't it be -4 then?  and if still yes,
it might be more obvious to a reader if written as: - 2 * strlen("%s").

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:126: int len = strlen(exc_str) + strlen(stack_str) +
strlen(format) - 2;
maybe add + 1 into len as well not to duplicate it below.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:151: OS::SNPrint(msg_copy, len, "%s", msg);
I see you're keeping old code, but strncpy (unless forbidden by a style guide)
appears to be a more direct way to copy a string :)

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:51: " 0. Function: 'TestClass.testMain' url:
'dart:test-lib' line:3 col:5\n",
do you want to mandate this exactly output?

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/object.cc
File runtime/vm/object.cc (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/object.cc#newcode...
runtime/vm/object.cc:4777: return "ApiError";
I know it's not common, but maybe output data.ToCString() as well?

[turnidge, 2011-11-09 19:53:41]

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:141: int line,
On 2011/11/09 12:34:03, antonmuhin wrote:
> nit: you may want to fix indentation: cf. const char* file and int line.

Done.

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:846: * May generate an unhandled exception error.
On 2011/11/09 12:34:03, antonmuhin wrote:
> I am slightly concerned with tagging API functions with this info: it may be
> hard to maintain.  E.g. we recently changed List API to allow it to deal w/
> classes implementing List.  The change would be somewhat more difficult if we
> committed to not throwing before.
> 
> I don't want to say we cannot maintain this information, but maybe we should
ask
> embedders to be overparanoid and assume any method may throw an exception. 
That
> doesn't look like a heavy burden: I would expect many people to write one
> function to handle error handles anyway.
> 
> Up to you though.

I see your point.  We don't want to have our hands tied in the future with
regards to these tags.

I was thinking back to when I was using python's C api to embed python. 
Sometimes the api would fail to say what could happen and I'd end up needing to
go read the source to figure little details out.

Let's see what Ivan's take on it is.  I'm in the middle.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:45: failure ^= obj.raw();
On 2011/11/09 12:34:03, antonmuhin wrote:
> may you used const ApiError& = ApiError::Handle(obj.raw()) instead?

Done.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:110: Object& strtmp =
Object::Handle(DartLibraryCalls::ToString(exception));
On 2011/11/09 12:34:03, antonmuhin wrote:
> this is a typical paranoia and DartLibraryCalls::ToString is not quite correct
> in this regard, but technically .toString in Dart may return any kind of
object
> including null, so maybe you should have it differently: if
> (!strtmp.IsUnhandledException() { ... } 

Done.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:126: int len = strlen(exc_str) + strlen(stack_str) +
strlen(format) - 2;
On 2011/11/09 12:34:03, antonmuhin wrote:
> why - 2, to account for %s?  if yes, shouldn't it be -4 then?  and if still
yes,
> it might be more obvious to a reader if written as: - 2 * strlen("%s").

Sorry, brain spasm there.  Reworked this to be more correct and clear.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:151: OS::SNPrint(msg_copy, len, "%s", msg);
On 2011/11/09 12:34:03, antonmuhin wrote:
> I see you're keeping old code, but strncpy (unless forbidden by a style guide)
> appears to be a more direct way to copy a string :)

Um, fixed.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl_tes...
File runtime/vm/dart_api_impl_test.cc (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl_tes...
runtime/vm/dart_api_impl_test.cc:51: " 0. Function: 'TestClass.testMain' url:
'dart:test-lib' line:3 col:5\n",
On 2011/11/09 12:34:03, antonmuhin wrote:
> do you want to mandate this exactly output?

For now, yes.  I want to nail down exactly what the user will see in this case
so I know that it looks somewhat reasonable.  I'm willing to accept the need to
update the test if the stack trace format changes.

[Anton Muhin, 2011-11-10 08:28:38]

LGTM.  When LG to Ivan, let's coordinate on landing it.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:45: failure ^= obj.raw();
Thanks.  There are ways more instance of the same pattern, if you like, I can go
over CL and tag them.

On 2011/11/09 19:53:41, turnidge wrote:
> On 2011/11/09 12:34:03, antonmuhin wrote:
> > may you used const ApiError& = ApiError::Handle(obj.raw()) instead?
> 
> Done.

[Ivan Posva, 2011-11-10 18:05:46]

LGTM with comments.

Mostly it is slightly lacking/unclear documentation.

-Ivan

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:846: * May generate an unhandled exception error.
On 2011/11/09 19:53:41, turnidge wrote:
> On 2011/11/09 12:34:03, antonmuhin wrote:
> > I am slightly concerned with tagging API functions with this info: it may be
> > hard to maintain.  E.g. we recently changed List API to allow it to deal w/
> > classes implementing List.  The change would be somewhat more difficult if
we
> > committed to not throwing before.
> > 
> > I don't want to say we cannot maintain this information, but maybe we should
> ask
> > embedders to be overparanoid and assume any method may throw an exception. 
> That
> > doesn't look like a heavy burden: I would expect many people to write one
> > function to handle error handles anyway.
> > 
> > Up to you though.
> 
> I see your point.  We don't want to have our hands tied in the future with
> regards to these tags.
> 
> I was thinking back to when I was using python's C api to embed python. 
> Sometimes the api would fail to say what could happen and I'd end up needing
to
> go read the source to figure little details out.
> 
> Let's see what Ivan's take on it is.  I'm in the middle.

Until we have a better place to stick this kind of documentation we should leave
it here.

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:113: DART_EXPORT const char* Dart_GetError(const
Dart_Handle& handle);
What happens if you call Dart_GetError with an UnhandledException handle? As far
as I understand Dart_IsError returns true for both the error objects and
unhandled exceptions.

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:123: DART_EXPORT Dart_Handle
Dart_GetException(Dart_Handle handle);
Dart_ErrorGetException maybe to be in consistent with other calls such as
Dart_IntegerValue. Dart_UnhandledExceptionGetException which would the exact
one, is just a tad unwieldy.

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:128: DART_EXPORT Dart_Handle
Dart_GetStacktrace(Dart_Handle handle);
ditto

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:150: #define DART_CHECK_VALID(handle)                
                       \
This looks like a perfect candidate for a dart_api_utils.h. TODO maybe?

http://codereview.chromium.org/8501034/diff/9001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8501034/diff/9001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:154: 
extra empty lines?

http://codereview.chromium.org/8501034/diff/9001/runtime/vm/dart_api_impl.h
File runtime/vm/dart_api_impl.h (right):

http://codereview.chromium.org/8501034/diff/9001/runtime/vm/dart_api_impl.h#n...
runtime/vm/dart_api_impl.h:17: class UnhandledException;
I don't see why this is here.

[turnidge, 2011-11-10 20:59:12]

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:846: * May generate an unhandled exception error.
On 2011/11/10 18:05:46, Ivan Posva wrote:
> On 2011/11/09 19:53:41, turnidge wrote:
> > On 2011/11/09 12:34:03, antonmuhin wrote:
> > > I am slightly concerned with tagging API functions with this info: it may
be
> > > hard to maintain.  E.g. we recently changed List API to allow it to deal
w/
> > > classes implementing List.  The change would be somewhat more difficult if
> we
> > > committed to not throwing before.
> > > 
> > > I don't want to say we cannot maintain this information, but maybe we
should
> > ask
> > > embedders to be overparanoid and assume any method may throw an exception.

> > That
> > > doesn't look like a heavy burden: I would expect many people to write one
> > > function to handle error handles anyway.
> > > 
> > > Up to you though.
> > 
> > I see your point.  We don't want to have our hands tied in the future with
> > regards to these tags.
> > 
> > I was thinking back to when I was using python's C api to embed python. 
> > Sometimes the api would fail to say what could happen and I'd end up needing
> to
> > go read the source to figure little details out.
> > 
> > Let's see what Ivan's take on it is.  I'm in the middle.
> 
> Until we have a better place to stick this kind of documentation we should
leave
> it here.

Okay.  Leaving as is for now.

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8501034/diff/7006/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:45: failure ^= obj.raw();
On 2011/11/10 08:28:39, antonmuhin wrote:
> Thanks.  There are ways more instance of the same pattern, if you like, I can
go
> over CL and tag them.
> 
> On 2011/11/09 19:53:41, turnidge wrote:
> > On 2011/11/09 12:34:03, antonmuhin wrote:
> > > may you used const ApiError& = ApiError::Handle(obj.raw()) instead?
> > 
> > Done.
> 

To get this to work I need to use CheckedHandle.

Rather not fix all instances of this in this CL.  Follow-up?

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:113: DART_EXPORT const char* Dart_GetError(const
Dart_Handle& handle);
On 2011/11/10 18:05:46, Ivan Posva wrote:
> What happens if you call Dart_GetError with an UnhandledException handle? As
far
> as I understand Dart_IsError returns true for both the error objects and
> unhandled exceptions.

I never let bare UnhandledException objects out of the api.  Instead I wrap them
in ApiError object containers before the user sees them.

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:123: DART_EXPORT Dart_Handle
Dart_GetException(Dart_Handle handle);
On 2011/11/10 18:05:46, Ivan Posva wrote:
> Dart_ErrorGetException maybe to be in consistent with other calls such as
> Dart_IntegerValue. Dart_UnhandledExceptionGetException which would the exact
> one, is just a tad unwieldy.

Changed to Dart_ErrorGetException, and Dart_ErrorGetStacktrace.

I also changed Dart_IsUnhandledException to Dart_ErrorHasException.  Seems less
confusing to me.  WDYT?  Another possibility is Dart_ErrorIsException.

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:128: DART_EXPORT Dart_Handle
Dart_GetStacktrace(Dart_Handle handle);
On 2011/11/10 18:05:46, Ivan Posva wrote:
> ditto

Ok.

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:150: #define DART_CHECK_VALID(handle)                
                       \
On 2011/11/10 18:05:46, Ivan Posva wrote:
> This looks like a perfect candidate for a dart_api_utils.h. TODO maybe?

Yeah.  Added TODO.

http://codereview.chromium.org/8501034/diff/9001/runtime/vm/dart_api_impl.cc
File runtime/vm/dart_api_impl.cc (right):

http://codereview.chromium.org/8501034/diff/9001/runtime/vm/dart_api_impl.cc#...
runtime/vm/dart_api_impl.cc:154: 
On 2011/11/10 18:05:46, Ivan Posva wrote:
> extra empty lines?

The extra empty lines help me scan the code -- otherwise it all turns into a big
blob for me.  If you want still want me to remove them, I will.

http://codereview.chromium.org/8501034/diff/9001/runtime/vm/dart_api_impl.h
File runtime/vm/dart_api_impl.h (right):

http://codereview.chromium.org/8501034/diff/9001/runtime/vm/dart_api_impl.h#n...
runtime/vm/dart_api_impl.h:17: class UnhandledException;
On 2011/11/10 18:05:46, Ivan Posva wrote:
> I don't see why this is here.

Thanks.  It's a leftover.  Removed.

[Ivan Posva, 2011-11-10 21:44:06]

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h
File runtime/include/dart_api.h (right):

http://codereview.chromium.org/8501034/diff/9001/runtime/include/dart_api.h#n...
runtime/include/dart_api.h:123: DART_EXPORT Dart_Handle
Dart_GetException(Dart_Handle handle);
On 2011/11/10 20:59:13, turnidge wrote:
> On 2011/11/10 18:05:46, Ivan Posva wrote:
> > Dart_ErrorGetException maybe to be in consistent with other calls such as
> > Dart_IntegerValue. Dart_UnhandledExceptionGetException which would the exact
> > one, is just a tad unwieldy.
> 
> Changed to Dart_ErrorGetException, and Dart_ErrorGetStacktrace.
> 
> I also changed Dart_IsUnhandledException to Dart_ErrorHasException.  Seems
less
> confusing to me.  WDYT?  Another possibility is Dart_ErrorIsException.

Much clearer as it also cleans up confusing verbiage in the comment.