suppress "...at line..." in _croak if end newline

[mohawk2]

It can be very helpful to include line numbers etc for failure messages. Not having the choice when parameters are wrong is less helpful, though. This change makes F::P::_croak behave like die: the ...at line... is suppressed if last list element ends with "\n".

[mauke]

I don't understand why this is a good thing. What's the use case?

[mohawk2]

This shows a useful example: https://travis-ci.org/graphql-perl/graphql-perl/builds/278494197#L572-L590

The error messages show information that I would like to be able to choose to not show, as well as more generally that tests can be fragile if that information is in errors. More generally, the convention in Perl exceptions is to only add the information if there is no \n on the end.

Separately, I'm not sure why it's differing on "item 1" and "item 2", but that's a different problem which I will address.

[mauke]

The error message is for users of the function (who need to know which call is the problem). Why should the type of a single parameter get to decide to suppress the error location for the whole function?

Besides, the get_message() string is only part of the error message. It happens to be at the end right now (In method <foo>: parameter <n> ($<var>): <get_message>) , but if I move it around in the future (e.g. parameter <n> ($<var>): <get_message> in call to method <foo>), you're back to square one.

the convention in Perl exceptions is to only add the information if there is no \n on the end

This is not a generic "throw exception" mechanism. get_message() isn't supposed to return an exception (string or object), it's supposed to return a problem description that gets incorporated into a bigger string.

The behavior is consistent with perl's own runtime errors, which don't let you suppress the location either.

---

If you want to make tests more robust, there are a couple of ways.

You can set the filename and line number in eval by using a directive like

#line 1 "foobar"
do_stuff();

Then the do_stuff() call will be reported as being at foobar line 1.

Or you can make your message check more flexible (regex match) and allow \(eval \d+\) for the filename.

Or cut off the at \(eval \d+\) line \d+\.$ suffix before doing an exact string comparison.

[mohawk2]

Thanks for the observations.

1) perlapi says:

croak

This is an XS interface to Perl's die function.

Take a sprintf-style format pattern and argument list. These are used to generate a string message. If the message does not end with a newline, then it will be extended with some indication of the current location in the code, as described for mess_sv.

Therefore it seems like Perl thinks that croak (as called by XS functions) should only add location information if not terminated by \n. Are you saying that Function::Parameters (an XS module)'s _croak function is different and should not conform to that standard?

2) I read your remarks above as implying (with the suggestion of inserting #line) that my code is calling string-eval, and could fix the problem I am facing by simply and easily inserting #line statements to fix this! Unfortunately, graphql-perl is not in fact using string-eval. In fact, Carp::cluck shows in the stack-trace this (initial whitespace removed for readability):

Type::Tiny::get_message(Type::Tiny=HASH(0x2e6dbc8), ARRAY(0x38ae8a8)) called at /home/user/graphql-perl/blib/lib/GraphQL/Type/InputObject.pm line 83
GraphQL::Type::InputObject::graphql_to_perl(GraphQL::Type::InputObject=HASH(0x32636b0), ARRAY(0x38ae8a8)) called at (eval 252) line 11
GraphQL::Type::InputObject::graphql_to_perl(GraphQL::Type::InputObject=HASH(0x32636b0), ARRAY(0x38ae8a8)) called at /home/user/graphql-perl/blib/lib/GraphQL/Execution.pm line 496
eval {...} called at /home/user/graphql-perl/blib/lib/GraphQL/Execution.pm line 496

Note the second line refers to called at (eval 252) line 11. It looks most to me like the second and third lines are the same with the second line being a string-eval inserted by F::P. I am still digging to see how to trigger that for a repro case, but it currently seems possible that F::P is inserting a stack-frame it does not mean in the stack-trace. Can you help with this?

3. Finally, if a module author wants to dictate what exception messages are thrown from a method they write, using a type they choose to use and/or also write, and do not wish to get location information visible: are you saying you want to force them to put fragile interpretation code in place before re-throwing exceptions upward? Given that you said you might change the format in the future. I need to know so as to pick the correct way forward.

[mauke]

1. Function::Parameter's _croak function isn't XS. It's a normal Perl subroutine. It's also an undocumented implementation detail. I named it _croak because it was supposed to be similar to Carp::croak in that it adds a source location to the message. It doesn't conform to any standard. (In particular, it doesn't take an sprintf-style format string.)

2. Oh, hmm. I'll have to look into that. F:P is not supposed to be inserting any eval frames. That would be a problem.

3. I don't have a real plan, I'm mostly figuring things out as I go. My current stance is: A call that passes a wrong type is an error in the code and should be fixed. That's why the error message points to the location of the call.

   If I want to give more control over exceptions to function authors, I'll probably have to come up with a new interface, which would be a bit of work (but it should definitely be the function that decides what happens, not the type). However, if I had to use a module that suppresses error locations and just tells me I passed a wrong argument somewhere without saying where in my code, I'd find that highly annoying. It's kind of a dick move, hence I'm fine with the hypothetical module author having to jump through a few hoops to get this behavior. (Of course it's possible that I'm missing a use case here and I'm willing to be convinced otherwise.)

   The exact format of the message may change (it already did in version 2 with the new invocant system), but I don't want to change it in the future without a very good reason. That said, it was always meant as an informational message for a human programmer, not a machine readable object.

[mohawk2]

1. My thought was that your XS module is doing something croak-like, and as a user I was highly surprised by not being able to influence it in a completely croak-like way as referenced above. In the end, it's your module of course!

2. I have discovered what's adding these (eval) things and it's not your module! It's actually Return::Type:

{
package WithFP;
use Function::Parameters;
use Return::Type;
use Types::Standard -all;
method s(Str $s) { }
method s_rt(Str $s) :ReturnType(Any) { }
}

eval { WithFP->s(undef); }; print "no RT: $@" if $@;
eval { WithFP->s_rt(undef); }; print "with RT: $@" if $@;
__END__
no RT: In method s: parameter 1 ($s): Undef did not pass type constraint "Str" at tf line 10.
with RT: In method s_rt: parameter 1 ($s): Undef did not pass type constraint "Str" at (eval 72) line 15.

3. I was also thinking of a function. As you may have gathered, my current thinking is the relevant Type::Tiny's message function. I am open to other possibilities. What I would very much like is to make error messages that are actually customer-visible without an interpretation layer in between.

[mohawk2]

@mauke I thought you might have some insight into this: while investigating making Return::Type use its previous behaviour of using Scope::Upper to eliminate this problem, I found this interaction between Function::Parameters, Return::Type, Scope::Upper and Test::Fatal: https://github.com/rjbs/Test-Fatal/issues/15 which is completely beyond me!