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

better error messages #184

Closed
ggrothendieck opened this Issue Sep 25, 2014 · 15 comments

Comments

Projects
None yet
4 participants
@ggrothendieck

There are a number of places where Rcpp can fail because TYPEOF(x) does not equal RTYPE. Since both of these are known at the point that the error is given it would help if these were displayed, e.g. rather than just "some message" use: std:string("some message TYPEOF(x): ") + CHAR(Rf_type2str(TYPEOF(x))) + " RTYPE: " + CHAR(Rf_type2str(RTYPE))
At least that will give some bit of clue as to what the problem might be.

Any other info that might be added to the error message to help locate the problem could be helpful too.

This applies to RcppEigen too.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Sep 25, 2014

Member

Maybe you could try this idea in a pull request?

Member

eddelbuettel commented Sep 25, 2014

Maybe you could try this idea in a pull request?

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Oct 3, 2014

Contributor

@ggrothendieck is something like this what you imagined? I would welcome you to modify r_cast.h to provide more appropriate error messages and send us a PR for review.

Contributor

kevinushey commented Oct 3, 2014

@ggrothendieck is something like this what you imagined? I would welcome you to modify r_cast.h to provide more appropriate error messages and send us a PR for review.

@ggrothendieck

This comment has been minimized.

Show comment
Hide comment
@ggrothendieck

ggrothendieck Oct 3, 2014

Yes, that looks good. The important part is to actually modify all points where such an error can occur to ensure that the expanded info is provided.

Yes, that looks good. The important part is to actually modify all points where such an error can occur to ensure that the expanded info is provided.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Nov 23, 2014

Contributor

Now that we have a better Rcpp::stop from @romainfrancois it would be sensible to revisit the Rcpp exceptions and use it to provide better error messages.

Contributor

kevinushey commented Nov 23, 2014

Now that we have a better Rcpp::stop from @romainfrancois it would be sensible to revisit the Rcpp exceptions and use it to provide better error messages.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Nov 24, 2014

Member

Sure. Do you think you may have spare cycles for that any time soon?

Member

eddelbuettel commented Nov 24, 2014

Sure. Do you think you may have spare cycles for that any time soon?

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 1, 2017

Contributor

As this sort of came up on SO today and its a bit problematic for beginners...

I glanced at the inst/include/Rcpp/r_cast.h. Seems like the commit was regressed. I think @kevinushey wanted to roll out the backend of ::Rcpp::stop() that uses /inst/include/Rcpp/utils/tinyformat.h in place of the exception macro generating ::Rcpp::not_compatible().

To maintain using the same class not_compatible -- but extend it so that it can hold multiple parameters -- I'm thinking just append to the file:

    template <typename T1>
    inline void NORET not_compatible(const char* fmt, const T1& arg1) {
        throw Rcpp::not_compatible( tfm::format(fmt, arg1 ).c_str() );
    }

    template <typename T1, typename T2>
    inline void NORET not_compatible(const char* fmt, const T1& arg1, const T2& arg2) {
        throw Rcpp::not_compatible( tfm::format(fmt, arg1, arg2 ).c_str() );
    }

    template <typename T1, typename T2, typename T3>
    inline void NORET not_compatible(const char* fmt, const T1& arg1, const T2& arg2, const T3& arg3) {
        throw Rcpp::not_compatible( tfm::format(fmt, arg1, arg2, arg3 ).c_str() );
    }

...

Label as "Enhancement" + "API"?

Contributor

coatless commented Apr 1, 2017

As this sort of came up on SO today and its a bit problematic for beginners...

I glanced at the inst/include/Rcpp/r_cast.h. Seems like the commit was regressed. I think @kevinushey wanted to roll out the backend of ::Rcpp::stop() that uses /inst/include/Rcpp/utils/tinyformat.h in place of the exception macro generating ::Rcpp::not_compatible().

To maintain using the same class not_compatible -- but extend it so that it can hold multiple parameters -- I'm thinking just append to the file:

    template <typename T1>
    inline void NORET not_compatible(const char* fmt, const T1& arg1) {
        throw Rcpp::not_compatible( tfm::format(fmt, arg1 ).c_str() );
    }

    template <typename T1, typename T2>
    inline void NORET not_compatible(const char* fmt, const T1& arg1, const T2& arg2) {
        throw Rcpp::not_compatible( tfm::format(fmt, arg1, arg2 ).c_str() );
    }

    template <typename T1, typename T2, typename T3>
    inline void NORET not_compatible(const char* fmt, const T1& arg1, const T2& arg2, const T3& arg3) {
        throw Rcpp::not_compatible( tfm::format(fmt, arg1, arg2, arg3 ).c_str() );
    }

...

Label as "Enhancement" + "API"?

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 5, 2017

Contributor

@eddelbuettel @kevinushey Thoughts on this? I would like to hopefully roll something out before next week.

Contributor

coatless commented Apr 5, 2017

@eddelbuettel @kevinushey Thoughts on this? I would like to hopefully roll something out before next week.

@kevinushey

This comment has been minimized.

Show comment
Hide comment
@kevinushey

kevinushey Apr 5, 2017

Contributor

I'm definitely on board for commits / PRs that improve the error messages reported by Rcpp.

Contributor

kevinushey commented Apr 5, 2017

I'm definitely on board for commits / PRs that improve the error messages reported by Rcpp.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 5, 2017

Member

Sure. If it works in that case and doesn't create trouble otherwise...

Member

eddelbuettel commented Apr 5, 2017

Sure. If it works in that case and doesn't create trouble otherwise...

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 10, 2017

Contributor

Proposal

  • Create a new RCPP_ADVANCED_EXCEPTION_CLASS exception generation macro to provide support for tfm::format(...) that mimics present implementation of Rcpp::stop() and Rcpp::warning().
  • Improve existing error throws by adding details on object structure (see list below)
  • Remove error codes not used

Enable Parameter Support / tfm::format(...)

Exception Class Current Proposed Rationale
no_such_slot SIMPLE EXCEPTION Provide the name of the slot being accessed.
not_a_closure SIMPLE EXCEPTION Provide the object type
index_out_of_bounds SIMPLE ADVANCED Provide the index requested that fell out of bounds alongside of the length of the vector.
not_compatible EXCEPTION ADVANCED Provide the starting class and requested end class.

Table Macro Use Key:

  • SIMPLE = RCPP_SIMPLE_EXCEPTION_CLASS
  • EXCEPTION = RCPP_EXCEPTION_CLASS
  • ADVANCED = RCPP_ADVANCED_EXCEPTION_CLASS (new mimics stop()/warning() )

Note: The difference between SIMPLE and EXCEPTION is the ability to pass a std::string to EXCEPTION that can dynamically change output whereas SIMPLE error codes are static.

Error Codes to Remove

The following error codes are not used (or are not detectable via grep / GitHub search):

Misc List of Error Code Class Usage

Below is a list of the current exception classes in Source (via GitHub)

RCPP_SIMPLE_EXCEPTION_CLASS

RCPP_EXCEPTION_CLASS

Contributor

coatless commented Apr 10, 2017

Proposal

  • Create a new RCPP_ADVANCED_EXCEPTION_CLASS exception generation macro to provide support for tfm::format(...) that mimics present implementation of Rcpp::stop() and Rcpp::warning().
  • Improve existing error throws by adding details on object structure (see list below)
  • Remove error codes not used

Enable Parameter Support / tfm::format(...)

Exception Class Current Proposed Rationale
no_such_slot SIMPLE EXCEPTION Provide the name of the slot being accessed.
not_a_closure SIMPLE EXCEPTION Provide the object type
index_out_of_bounds SIMPLE ADVANCED Provide the index requested that fell out of bounds alongside of the length of the vector.
not_compatible EXCEPTION ADVANCED Provide the starting class and requested end class.

Table Macro Use Key:

  • SIMPLE = RCPP_SIMPLE_EXCEPTION_CLASS
  • EXCEPTION = RCPP_EXCEPTION_CLASS
  • ADVANCED = RCPP_ADVANCED_EXCEPTION_CLASS (new mimics stop()/warning() )

Note: The difference between SIMPLE and EXCEPTION is the ability to pass a std::string to EXCEPTION that can dynamically change output whereas SIMPLE error codes are static.

Error Codes to Remove

The following error codes are not used (or are not detectable via grep / GitHub search):

Misc List of Error Code Class Usage

Below is a list of the current exception classes in Source (via GitHub)

RCPP_SIMPLE_EXCEPTION_CLASS

RCPP_EXCEPTION_CLASS

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 15, 2017

Contributor

By the way... I completely forgot to detail how the new exception message should be written.

Per feedback in #667, the goal here will be to state the error like before (in a similar syntax) and then follow it up with additional diagnostic information in the form of [key1=value1; key2=value2...]. You can see this take form in #667's ba4403d commit.

For simplicity, here is a list of the original exception messages contrasted with the new exception messages:

Old New
"cannot convert to environment" "Cannot convert object to an environment: [type:%s; target=ENVSXP]."
"cannot convert to function" "Cannot convert object to a function:
-- [type=%s; target=CLOSXP, SPECIALSXP, or BUILTINSXP]."
"not a promise" "Not a promise: [type = %s]."
"expecting a single value" "Expecting a single string value: [type=%s; extent=%i]."
"cannot convert to symbol (SYMSXP)" "Cannot convert object to a symbol:
-- [type=%s; target=SYMSXP]."
"expecting an external pointer" "Expecting an external pointer: [type=%s]."
"expecting a single value" "Expecting a single value: [extent=%i]."
"expecting a string" "Expecting a string: [type=%s]."
"expecting a string vector" "Expecting a string vector: [type=%s; required=STRSXP]."
std::string("could not convert using R function : ") + fun "Could not convert using R function: %s."
"not compatible" "Not compatible conversion to target: [type=%s; target=%s]."
"not compatible with requested type" "Not compatible with requested type: [type=%s; target=%s]."
"not compatible with STRSXP" "Not compatible with STRSXP: [type=%s]."
"index out of bounds" "Index is out of bounds: [index=%i; extent=%i]."
"index out of bounds" "Column index is out of bounds: [index=%i; column extent=%i]."
"index out of bounds" "Row index is out of bounds: [index=%i; row extent=%i]."
"index out of bounds" "Location index is out of bounds: [row index=%i; row extent=%i;
-- "column index=%i; column extent=%i]."
"index out of bounds" "Index is out of bounds: Object is missing names."
"index out of bounds" "Index is out of bounds: [index='%i']."
"index out of bounds" "Iterator index is out of bounds: [iterator index=%s; iterator extent=%i]"
"index out of bounds" "Iterator index is out of bounds: [iterator=%s; index=%i; extent=%i]"

Any thoughts would be welcome as I'd like to have this change ready to go after #674 is merged.

Contributor

coatless commented Apr 15, 2017

By the way... I completely forgot to detail how the new exception message should be written.

Per feedback in #667, the goal here will be to state the error like before (in a similar syntax) and then follow it up with additional diagnostic information in the form of [key1=value1; key2=value2...]. You can see this take form in #667's ba4403d commit.

For simplicity, here is a list of the original exception messages contrasted with the new exception messages:

Old New
"cannot convert to environment" "Cannot convert object to an environment: [type:%s; target=ENVSXP]."
"cannot convert to function" "Cannot convert object to a function:
-- [type=%s; target=CLOSXP, SPECIALSXP, or BUILTINSXP]."
"not a promise" "Not a promise: [type = %s]."
"expecting a single value" "Expecting a single string value: [type=%s; extent=%i]."
"cannot convert to symbol (SYMSXP)" "Cannot convert object to a symbol:
-- [type=%s; target=SYMSXP]."
"expecting an external pointer" "Expecting an external pointer: [type=%s]."
"expecting a single value" "Expecting a single value: [extent=%i]."
"expecting a string" "Expecting a string: [type=%s]."
"expecting a string vector" "Expecting a string vector: [type=%s; required=STRSXP]."
std::string("could not convert using R function : ") + fun "Could not convert using R function: %s."
"not compatible" "Not compatible conversion to target: [type=%s; target=%s]."
"not compatible with requested type" "Not compatible with requested type: [type=%s; target=%s]."
"not compatible with STRSXP" "Not compatible with STRSXP: [type=%s]."
"index out of bounds" "Index is out of bounds: [index=%i; extent=%i]."
"index out of bounds" "Column index is out of bounds: [index=%i; column extent=%i]."
"index out of bounds" "Row index is out of bounds: [index=%i; row extent=%i]."
"index out of bounds" "Location index is out of bounds: [row index=%i; row extent=%i;
-- "column index=%i; column extent=%i]."
"index out of bounds" "Index is out of bounds: Object is missing names."
"index out of bounds" "Index is out of bounds: [index='%i']."
"index out of bounds" "Iterator index is out of bounds: [iterator index=%s; iterator extent=%i]"
"index out of bounds" "Iterator index is out of bounds: [iterator=%s; index=%i; extent=%i]"

Any thoughts would be welcome as I'd like to have this change ready to go after #674 is merged.

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 17, 2017

Contributor

@eddelbuettel:

Would you prefer it if I staged the exception message changes in two parts?

e.g.

  • PR 1: Modification to exception.h to split into cpp98/cpp11 and adding a local mod in tinyformat to enable variadic templates?
  • PR 2: Exception message changes to the proposed above?

Or just do it all together?

Contributor

coatless commented Apr 17, 2017

@eddelbuettel:

Would you prefer it if I staged the exception message changes in two parts?

e.g.

  • PR 1: Modification to exception.h to split into cpp98/cpp11 and adding a local mod in tinyformat to enable variadic templates?
  • PR 2: Exception message changes to the proposed above?

Or just do it all together?

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 17, 2017

Member

Two sounds like a reasonable split.

"Bite-sized". Some folks of course swallow warthogs whole so it all depends...

Member

eddelbuettel commented Apr 17, 2017

Two sounds like a reasonable split.

"Bite-sized". Some folks of course swallow warthogs whole so it all depends...

@coatless

This comment has been minimized.

Show comment
Hide comment
@coatless

coatless Apr 19, 2017

Contributor

@eddelbuettel & @kevinushey

Any issue with the proposed table above? About to submit a PR.

Contributor

coatless commented Apr 19, 2017

@eddelbuettel & @kevinushey

Any issue with the proposed table above? About to submit a PR.

@eddelbuettel

This comment has been minimized.

Show comment
Hide comment
@eddelbuettel

eddelbuettel Apr 19, 2017

Member

So the PR would change from messages on the left to messages on the right?

Wow. Sounds useful. Worth a test too. PR away.

Member

eddelbuettel commented Apr 19, 2017

So the PR would change from messages on the left to messages on the right?

Wow. Sounds useful. Worth a test too. PR away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment