Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:93488
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain kelunik.com from 81.169.146.216 cause and error)
MIME-Version: 1.0
In-Reply-To: <CADqTB_jXnLZx-F_-08+F1APoUTd_9C-Gt+vuc2GAk=xWeXFw=A@mail.gmail.com>
References: <CADqTB_jC-EZ5p+c=q0dmRgaYAkyRVbdNFNUQRGhfaS7GaOTR9g@mail.gmail.com>
	<72f547d6-5fcf-3cd2-960d-cb612a429e47@php.net>
	<CANUQDCiEDeCV3RAr0bg+=-PHPDYkk-cP-ih4TnTGUzz7SNakMw@mail.gmail.com>
	<CADqTB_j67-ocNaWm5sRHbC4vJF2t7WBWUqrEXu9-Kk_MSi4CgA@mail.gmail.com>
	<CAF1yvAQozS1qpCbBNdAJu4gT5LbOEUimv2W-nWtkaUTbVZFubA@mail.gmail.com>
	<CANUQDCjnEgJwT5zEb50sWA5=OeF-LRzPDu_1+-E70as2N10oXw@mail.gmail.com>
	<fb93f245-a5e6-53f7-9fee-deaebb847b78@gmail.com>
	<CADqTB_inxTXkg+mVGyBVsP1drDqsWfaLMKe00R=veBQT7ZKedQ@mail.gmail.com>
	<CAMUwpuR=ES4eWTofrAcfC434nFQOrAyp5cy-wVdAykVr3BwE-A@mail.gmail.com>
	<CADqTB_jXnLZx-F_-08+F1APoUTd_9C-Gt+vuc2GAk=xWeXFw=A@mail.gmail.com>
Date: Tue, 24 May 2016 18:48:07 +0200
Message-ID: <CANUQDCipMNiLF3rxX6dYgs-a_ESU2PJcvxWmnv0oj-eG3buO8g@mail.gmail.com>
To: Rasmus Schultz <rasmus@mindplay.dk>
Cc: Julien Pauli <jpauli@php.net>, Rowan Collins <rowan.collins@gmail.com>, 
	PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a114b0cd44cb56c05339952dc
Subject: Re: [PHP-DEV] Exception::getLine()
From: me@kelunik.com (Niklas Keller)

--001a114b0cd44cb56c05339952dc
Content-Type: text/plain; charset=UTF-8

2016-05-24 17:32 GMT+02:00 Rasmus Schultz <rasmus@mindplay.dk>:

> > it would be more logical to collect the trace in the ZEND_THROW Opcode
> instead of in the create_handler of the Exception class
>
> I've been pondering a means of doing this with relatively few BC issues.
>
> There are four use-cases right now:
>
> 1. Direct: throw new Exception()
> 2. Indirect: throw $factory->createException()
> 3. Re-throwing: throw $exception
> 4. Not throwing: $exception = new Exception()
>

You missed the "5. Throw later" case, e.g. for coroutines using
Generator::throw.

http://amphp.org/docs/amp/managing-concurrency.html#generators

Regards, Niklas


> The first use-case (throw new) makes up the large majority, and would
> be unaffected by this change.
>
> The second use-case (indirect throw, factory) and third use-case
> (re-throwing) are both broken as-is - in either case, it isn't
> reporting the correct stack-trace now. This change corrects those
> cases. Even if that's a minor BC break, it corrects an error.
>
> By my analysis, that leaves only the 4th case (not throwing) as a
> potentially serious BC break. If somebody is presently constructing an
> Exception just as a means of extracting a stack-trace, this would no
> longer work. But there is a more direct way of doing that, which is
> debug_backtrace(), so likely the number of cases of this in the wild
> are few, and easy to correct, and likely with backwards-compatibility
> of the corrected code in all cases.
>
> I can't think of any other case where this change affects anything,
> but please correct me and point out any other case you can think of?
>
> As for Exception::getTrace(), I would propose the signature be changed
> to Exception::getTrace($index = 0) which would be backwards compatible
> in API terms - the default argument of 0 for $index would return the
> most recent stack trace, whereas an index of 1 would return the
> previous stack trace, e.g. after a re-throw.
>
> Exception::getTraceAsString() could either change in the same way, or
> render out all collected traces (which might be a bit much) or could
> simply render the most recently collected trace - I'm not sure which
> is best or most compatible.
>
> Exception::getLine() could either return the line-number of the first
> or last collected stack-trace - returning the first collected
> line-number is more compatible, consistent with "return new" and
> re-throwing, but returning the last line-number might be more correct,
> I'm not sure.
>
> Would a change like this require an RFC (and a vote) or is it arguably
> a bug? (I'm guessing this change is too substantial to be considered a
> "bug fix"?)
>
>
> On Mon, May 23, 2016 at 10:12 AM, Julien Pauli <jpauli@php.net> wrote:
> > On Sat, May 21, 2016 at 8:16 PM, Rasmus Schultz <rasmus@mindplay.dk>
> wrote:
> >> Alright, so forget my comparison with other languages.
> >>
> >> My other points remain:
> >>
> >> Presently, "throw new" is treated as though it was one statement.
> >> That's not the case. We have deferred throws and factory methods for
> >> exceptions, and we have re-throws, so collecting the stack-trace at
> >> construction time doesn't work.
> >>
> >> The construction site would only be relevant if "throw new" was in
> >> deed a single statement.
> >>
> >> Recording the actual throw site is clearly the goal - the current
> >> implementation is betting on "throw" and "new" happening at the same
> >> site, which is merely circumstance.
> >>
> >> Ideally, an Exception should collect another stack trace for each
> >> successive throw, which would enable you to trace not only the
> >> original site, but the flow through any exception-handlers that might
> >> have re-thrown the same Exception.
> >>
> >> As is, there is no information collected on throw, and thereby no
> >> evidence or record of possible re-throws - on top of the fact that you
> >> may be collecting and looking at bogus stack-traces from factory
> >> methods or exception mappers.
> >>
> >>
> >> On Fri, May 20, 2016 at 11:06 AM, Rowan Collins <
> rowan.collins@gmail.com> wrote:
> >>> On 20/05/2016 08:22, Niklas Keller wrote:
> >>>>
> >>>> 2016-05-20 4:13 GMT+02:00 Jesse Schalken <me@jesseschalken.com>:
> >>>>>
> >>>>> The top frame is the construction (get_error) and the site of the
> throw
> >>>>> (do_throw) doesn't appear in the stack at all.
> >>>>>
> >>>>
> >>>> The comparison with JavaScript isn't a good one, since you can throw
> >>>> everything in JS. If they didn't provide the stack trace upon throw,
> you
> >>>> would not have a stack trace at all if you throw a plain string.
> >>>>
> >>>
> >>>
> >>> That explanation justifies completely the opposite behaviour to what
> Jesse
> >>> described.
> >>>
> >>> According to MDN [1] the "stack" property is completley
> unstandardised, and
> >>> some engines may indeed populate it on throw, but there's no hint on
> that
> >>> page that they'll attach it to anything not constructed as an Error.
> >>>
> >>> So it's not a great comparison for either side (note that it was
> originally
> >>> brought up by Rasmus as an example where it *does* come from the throw
> site)
> >>> because the language doesn't actually guarantee you a stack trace at
> all.
> >>>
> >>> [1]
> >>>
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/stack
> >>>
> >>> Regards,
> >>> --
> >>> Rowan Collins
> >>> [IMSoP]
> >>>
> >>> --
> >>> PHP Internals - PHP Runtime Development Mailing List
> >>> To unsubscribe, visit: http://www.php.net/unsub.php
> >>>
> >>
> >> --
> >> PHP Internals - PHP Runtime Development Mailing List
> >> To unsubscribe, visit: http://www.php.net/unsub.php
> >>
> >
> > Hi.
> >
> > I explained that in my article detailing Exceptions from internal ,
> > http://jpauli.github.io/2015/04/09/exceptional-php.html
> >
> > I admit it would be more logical to collect the trace in the
> > ZEND_THROW Opcode instead of in the create_handler of the Exception
> > class.
> >
> > That would break backtraces, but we already broke them in PHP 7.0
> > So we could think about it for a 8.0 ideally , or a 7.1 ; I'm not
> > sure. Anyway, that needs more debate ;-)
> >
> >
> >
> > Julien.Pauli
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>

--001a114b0cd44cb56c05339952dc--