Use "raise from" where appropriate, all over the codebase

[Ram Rachum]

Hi guys,

I recently made a big ticket/PR to Django, and Shai Berger told me I should first talk about it in this mailing list.

This is the ticket: https://code.djangoproject.com/ticket/31177 and its PR: https://github.com/django/django/pull/12339

It's a generalization of this ticket that I opened and wrote a patch for a few days ago: https://code.djangoproject.com/ticket/31166 It was discussed and merged.

Basically, I think that in any place where we catch an exception, and then wrap it with our own exception for whatever reason, we should use the "raise new_exception from old_exception" syntax rather than just "raise new_exception".

This means that when Python displays the stacktrace (and when we display it in the debug page,) this will make it have a text of "this exception is the direct result of" instead of "During handling of the above exception, another exception occurred". This is more accurate, because the latter basically means "there was an error in the process of error-handling," which is absolutely not the case for us, and could mislead a user.

Note that we already started transitioning to "raise from", slowly:

 - Commit by Mariusz Felisiak: https://github.com/django/django/commit/84dcd1624784c2239f96a97b3151145a85dfbbe3

 - Commit by Diederik van der Boor: https://github.com/django/django/commit/25f21bd2376603c8e233a0a0e5a726a0fdfdd33e

 - Commit by Thomas Allison: https://github.com/django/django/commit/3e8b7333904f1ab6aa18eeb508659256f3644816

What do you think? 

Thanks,

Ram.

[Tom Forbes]

I agree with this change from a correctness standpoint but I would like to make the point that chained exceptions might be slightly annoying when displayed via console output, as you see the inner exception first and have to scroll up to see the exception you actually have to handle.

Tom

On 18 Jan 2020, at 09:55, Ram Rachum <ram.r...@gmail.com> wrote:



--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/25086cdc-24ab-4f0a-bdb9-9756551ac170%40googlegroups.com.

[Adam Johnson]

Agree with Tom here.

Is there anything we can do to control the way python displays them?

And how would we ensure the format is kept going forwards? Is there a flake8 rule/plugin we could activate to enforce it?

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/74BA6A05-D971-47EF-A0B6-4EF1149E83E6%40tomforb.es.

--

Adam

[Ram Rachum]

Hi Tom and Adam,

I do agree that Python's chained exceptions can be confusing. Of course, when you really need that exception information to troubleshoot something, it's an absolute godsend. In any case, the way Python chains exceptions when showing them is orthogonal to this proposed change. Python already displays the exceptions chained even if we don't use "raise from", the only thing that "raise from" changes is the text that Python puts between the 2 chained exceptions.

Regarding automatically enforcing this format going forward: I looked at the list of Flake8 rules and couldn't find anything about it. 

You received this message because you are subscribed to a topic in the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/ibEOt3A9c2M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAMyDDM0x0L%2Ba_r_gGL80qL3eqwMS4OV3%3Du-Hc0Wj3N1ruyQfuA%40mail.gmail.com.

[אורי‎]

Ram,

I noticed that 100 files changed in this commit. Did you edit each file manually before you committed, or was it some script doing it for you?

If it was a script or program, can I see it?

Uri.

אורי

u...@speedy.net

--

[Ram Rachum]

Hi Uri,

All the files were edited manually by me. I used a crude regex to find the relevant locations: "except .{3,100}raise"

I bet that there are a few cases that my regex didn't cover, but it probably covered 90%, so we can first decide whether we want this change, and later worry about the other 10%.

You received this message because you are subscribed to a topic in the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/ibEOt3A9c2M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CABD5YeHpYTz06M4ywKKmGB8P%3DAhj6Sv3P7K7%3DjB%3DYpCz6wpxLQ%40mail.gmail.com.

[Shai Berger]

Hi all,

On Sat, 18 Jan 2020 14:27:23 +0200
Ram Rachum <r...@rachum.com> wrote:

> [...] In any case, the

> way Python chains exceptions when showing them is orthogonal to this
> proposed change. Python already displays the exceptions chained even
> if we don't use "raise from", the only thing that "raise from"
> changes is the text that Python puts between the 2 chained exceptions.
>

Indeed. Well, almost: there is one more thing that addnig the `from`
changes, and that is the attributes on the exception (actually, it's
these attributes which really control the display): Without `from`, the
original exception is placed in a `__context__` attribute; with `from`,
it is placed in a `__cause__` attribute.

At first I thought this was a reason to object to this change -- I
thought code which catches exceptions and looks into their `__context__`
might break because of it. But as it turns out, `from` puts the
original exception on the `__cause__` in *addition* to `__context__`:

In [8]: try:
...: try:
...: 1/0
...: except ZeroDivisionError as e:
...: raise Exception("hi") from e
...: except Exception as ex:
...: exc = ex
...:

In [10]: exc
Out[10]: Exception('hi')

In [13]: exc.__cause__ is exc.__context__
Out[13]: True

So that is not a concern.

> Regarding automatically enforcing this format going forward: I looked

> at the list of Flake8 rules <https://www.flake8rules.com/> and

> couldn't find anything about it.
>

Indeed, I don't think there can be -- it cannot be a style violation to
run into an error while handling another error...

Shai.

[Ram Rachum]

On Sat, Jan 18, 2020 at 5:05 PM Shai Berger <sh...@platonix.com> wrote:

[snip] 

But as it turns out, `from` puts the
original exception on the `__cause__` in *addition* to `__context__`:

[snip]

So that is not a concern.

Awesome! I did not know that.  

 

> Regarding automatically enforcing this format going forward: I looked
> at the list of Flake8 rules <https://www.flake8rules.com/> and
> couldn't find anything about it.
>

Indeed, I don't think there can be -- it cannot be a style violation to
run into an error while handling another error...

I think it can be, as it's very rare to have a legitimate exception without a "raise from" inside an except clause. In almost any context in which "during handling of..." is correct, the raising is done deeper in the stack.

I think it's rare enough that personally, I would have liked to have a Flake8 / PyLint rule like that enforces it, and allow ignoring it with a comment. (As much as I hate Lint ignore comments.)

[Jon Dufresne]

+1 on chaining exceptions. I think the information is useful.

> Is there anything we can do to control the way python displays them?

I don't think we should do anything non-standard to display exceptions. Over time, Python programmers have become accustomed to how these exceptions are displayed. To alter that would be to introduce something unfamiliar from all their other Python debugging experiences.

If exception displaying should be improved, I think the effort should be upstream to benefit the larger Python community.

> And how would we ensure the format is kept going forwards? Is there a flake8 rule/plugin we could activate to enforce it?

At the very least, we could add a note in the code style guidelines that chained exceptions should normally be used.

Cheers

[Shai Berger]

That makes sense. And you know, flake8 supports plugins... a couple of
web searches and "pip search flake8 | grep {raise,from,chain}" didn't
pull anything which seems relevant, but if so, it can be written.

Shai.

[Adam Johnson]

I would like to make the point that chained exceptions might be slightly annoying when displayed via console output, as you see the inner exception first and have to scroll up to see the exception you actually have to handle.

Just coming back to this, Tom it's not quite true. Yes you see the "inner" exception first - meaning the one raised inside the 'except' block. But that is the one you, as a user, have to handle. This is not a concern for me.

I checked with a simple program that Python prints them in the same order and format whether or not "raise ... from" is used on the inner. The only change is the message reads "The above exception was the direct cause of the following exception:" which is pretty clear I think.

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CANXboVYxb%2BMeOHp%3De4gzqroKWkHzLjeGXy55qJyRLQQVcUPrWg%40mail.gmail.com.

--

Adam

[Jon Dufresne]

> I think it's rare enough that personally, I would have liked to have a
> Flake8 / PyLint rule like that enforces it, and allow ignoring it
> with a comment. (As much as I hate Lint ignore comments.)

That makes sense. And you know, flake8 supports plugins... a couple of
web searches and "pip search flake8 | grep {raise,from,chain}" didn't
pull anything which seems relevant, but if so, it can be written.

As a weekend project, I wrote a flake8 plugin to handle this at: https://pypi.org/project/flake8-raise/

 

I'm not advocating this necessarily included by the Django test suite, but it can be used to review the PR. Either way, it was fun to write.

[Adam Johnson]

Nice work Jon. I don't think we can generally apply R100 though - thre are definitely cases where "raise" inside an exception handler is not *caused* by the other exception - but maybe that's a sign refactoring is needed.

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CADhq2b4m40efQkorgji9-erXri9qSAb1VJBv%3DZ3HrOL6ksOJAQ%40mail.gmail.com.

--

Adam

[Ram Rachum]

Jon: That's awesome! I also liked R101. I didn't think of that. 

Adam: I thought so too, but after going over dozens of R100 cases, I didn't find even one where a raise without "from" inside an except clause was justified. I challenge you to show me even one such example.

[Ram Rachum]

I made a pull request for the style guide if anyone would like to review:  https://github.com/django/django/pull/12350

[Carlton Gibson]

> +1 on chaining exceptions. I think the information is useful.

Absolutely. But exceptions are **already** chained, regardless of whether we 

use the `from` syntax. 

Without the from clause exceptions are "implicitly" chained. With the from claus it's "explicit". 

Just the same tracebacks are presented, in just the same order. 

The only difference is the string that divides them: 

```

The above exception was the direct cause of the following exception:

```

vs

```

During handling of the above exception, another exception occurred:

```

That change in string isn't worth 380+ changes over 100 files. 

* We're obscuring the history for nothing, and...

* Whilst I **think** there's no danger in this case, bulk edits are by their nature 

  unconsidered, and there is the ever-present risk of regression. 

  

Beyond that, the from syntax is simply more verbose. In the PR the changes are 

all adding a unneeded `as e`, followed by an equally unneeded `from e`. 

Looking at the diff, the comparison that comes to mind is if we went through 

and replaced all percent formatting with format()

So I'm -1 on merging the PR. I think we should close the ticket as wontfix. 

I would ask everyone to clarify exactly what they think the benefits of the 

proposed change are, and to follow-up if there's a hidden benefit I've missed. 

I'm not anti the from clause in general. I use it myself, and in any given, 

considered case, I see no reason not to use it in Django. 

This is different from insisting it must be used, and rightly so, because, as I understand it, the target use case for from is when you want to provide more targeted exception logging. 

Take this case: 

    def inner():

        raise Exception("This is the one we really want to show the user.")

    def middle():

        try:

            inner()

        except:

            raise Exception("All sorts of stuff that's not so interesting.")

    def outer():

        try:

            middle()

        except Exception as e:

            msg = "We cut out the unimportant stuff, to focus on what's important."

            raise Exception(msg) from e.__context__

    if __name__ == '__main__':

        outer()

Here the use of from makes the traceback exactly how we want it:

    $ python exceptions.py 

    Traceback (most recent call last):

      File "exceptions.py", line 9, in middle

        inner()

      File "exceptions.py", line 4, in inner

        raise Exception("This is the one we really want to show the user.")

    Exception: This is the one we really want to show the user.

    The above exception was the direct cause of the following exception:

    Traceback (most recent call last):

      File "exceptions.py", line 23, in <module>

        outer()

      File "exceptions.py", line 19, in outer

        raise Exception(msg) from e.__context__

    Exception: We cut out the unimportant stuff, to focus on what's important.

 

Contrasted to not using from, where we just get everything: 

    $ python exceptions.py 

    Traceback (most recent call last):

      File "exceptions.py", line 9, in middle

        inner()

      File "exceptions.py", line 4, in inner

        raise Exception("This is the one we really want to show the user.")

    Exception: This is the one we really want to show the user.

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):

      File "exceptions.py", line 16, in outer

        middle()

      File "exceptions.py", line 11, in middle

        raise Exception("All sorts of stuff that's not so interesting.")

    Exception: All sorts of stuff that's not so interesting.

    During handling of the above exception, another exception occurred:

    Traceback (most recent call last):

      File "exceptions.py", line 23, in <module>

        outer()

      File "exceptions.py", line 19, in outer

        raise Exception(msg)

    Exception: We just output everything because we didn't use from.

 

That's where the real benefit lies. 

So, summary: 

* Exception chaining is the default. We already have that. 

* Short of something I missed (which is always possible 🙂) there's no reason for the bulk edit being proposed here. 

Kind Regards,

Carlton

[Mariusz Felisiak]

I agree with Carlton, I don't see much (if any) value in the bulk update.

Best,
Mariusz

[Ram Rachum]

Hi guys,

I'm disappointed that you're against this change... But I understand that you have a different perspective. Here's my last-ditch effort to convince you. 

If I understand correctly, you both agree that using "raise from" in this context is better than using plain raise, just that the benefits are not worth the price of a bulk update to Django. In other words, "raise from" is the inevitable future, it's just that we're not in a rush to get there.

Keep in mind that the thing I care about is not just usage of "raise from" in Django, but in any Python package which is relevant. Most Python developers don't know about this difference between "During the handling of" and "The above exception was the direct cause", because "raise from" isn't that prevalent yet. This is a chicken-and-egg problem, because as long as project maintainers don't use "raise from", the exception chaining wouldn't show the correct text, and people would get used to the fact that they can't rely on this text to be correct. 

Like any chicken-and-egg problem of changing people's habits, the best we could hope is to move it forward at a glacial pace-- A situation somewhat similar to the move to Python 3. If Django were to adopt this practice, it would help in getting other projects to do that too, and for people to pay attention to that line of text.

Thanks,

Ram.

--
You received this message because you are subscribed to a topic in the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/django-developers/ibEOt3A9c2M/unsubscribe.
To unsubscribe from this group and all its topics, send an email to django-develop...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/ab6abbb0-f651-4271-b84c-a3bacaba9482%40googlegroups.com.

[Aymeric Augustin]

Hello Ram,

On 6 Feb 2020, at 19:08, Ram Rachum <r...@rachum.com> wrote:

In other words, "raise from" is the inevitable future, it's just that we're not in a rush to get there.

I'm not sure how you came to this conclusion; I'm not seeing this in Carlton's and Mariusz' answers.

Carlton only said that `raise from` can be useful in specific cases where the default, implicit exception chaining doesn't do what you need.

On 6 Feb 2020, at 15:37, Carlton Gibson <carlton...@gmail.com> wrote:

Beyond that, the from syntax is simply more verbose. In the PR the changes are 

all adding a unneeded `as e`, followed by an equally unneeded `from e`. 

This wouldn't be an improvement.

Best regards,

-- 

Aymeric.

[Shai Berger]

On Thu, 6 Feb 2020 20:08:28 +0200
Ram Rachum <r...@rachum.com> wrote:

>
> If I understand correctly, you both agree that using "raise from" in
> this context is better than using plain raise, just that the benefits
> are not worth the price of a bulk update to Django. In other words,
> "raise from" is the inevitable future, it's just that we're not in a
> rush to get there.

As Aymeric pointed out, that's inaccurate; If I understood correctly,
Carlton's argument amounted to "it would be better to have the string
produced by `raise from` in the output, but it's not worth the extra
verbosity in exception handling code" -- besides the price of the bulk
update.

> Keep in mind that the thing I care about is not just usage of "raise
> from" in Django, but in any Python package which is relevant.

... and Carlton's argument applies to them all; when also considering:

> On Sat, 18 Jan 2020 17:18:41 +0200
> Ram Rachum <r...@rachum.com> wrote:
>
> >

> > it's very rare to have a legitimate exception
> > without a "raise from" inside an except clause. In almost any
> > context in which "during handling of..." is correct, the raising is
> > done deeper in the stack.
> >

I think the conclusion should be to ask for a change in Python, not
Django. The rule "if an exception is raised explicitly from an except
clause then it is considered raised-from" seems simple enough to me.

[Ryan Hiebert]

I think the conclusion should be to ask for a change in Python, not
Django. The rule "if an exception is raised explicitly from an except
clause then it is considered raised-from" seems simple enough to me.

I really like that. It makes perfect sense, and I can't think of a case where that rule would be incorrect.

[Ram Rachum]

On Fri, Feb 7, 2020 at 12:27 AM Aymeric Augustin <aymeric....@polytechnique.org> wrote:

Hello Ram,

On 6 Feb 2020, at 19:08, Ram Rachum <r...@rachum.com> wrote:

In other words, "raise from" is the inevitable future, it's just that we're not in a rush to get there.

I'm not sure how you came to this conclusion; I'm not seeing this in Carlton's and Mariusz' answers.

I'm basing it on the fact that Carlton approved this PR for the style guide: https://github.com/django/django/pull/12350

 

I'm not sure though whether he intends to merge it. 

In any case, I think it'll be sad if people will just get used to misleading exception chaining messages.

Shai Berger wrote:

> > > it's very rare to have a legitimate exception
> > > without a "raise from" inside an except clause. In almost any
> > > context in which "during handling of..." is correct, the raising is
> > > done deeper in the stack.
> > >

> I think the conclusion should be to ask for a change in Python, not
> Django. The rule "if an exception is raised explicitly from an except
> clause then it is considered raised-from" seems simple enough to me.  

That's an interesting idea, but there are 2-3 problems with it that make it close to impossible.

An alternative suggestion would be to allow syntax that automatically sets the cause to be the last-caught exception. I would suggest the `raise as` syntax, which was one of the rejected alternatives in PEP 409 for a different usecase

try:

    1/0

except ZeroDivisionError:

    raise as ValueError('Whatever')

My suggestion is that this `raise as` line would have the same effect as `raise ValueError('Whatever') from zero_division_error`, while also preventing the need to add `as zero_division_error` in the line above.

I think this solves most of the problems with the previous suggestion. What do you think? Want to write a PEP together? :)

[Carlton Gibson]

> I'm basing it on the fact that Carlton approved this PR for the style guide: https://github.com/django/django/pull/12350

No. I don't think we should merge that change. (It's "approved" qua itself before reviewing, and dependent on the main PR.)

To be clear. I think the default implicit chaining should be used unless there's specific reason not to. 

(In hindsight I should have come to this conclusion before agreeing to the original smaller cleanup.)

C.

[Ram Rachum]

I understand. Hypothetical question: If the syntax was `raise as NewException`, without having to give the old exception a name, would that change your decision?

[Carlton Gibson]

Maybe... it's still more verbose for no gain as I see it. 

I think the default implicit chaining is correct in the default case. It's only if you want to adjust that (or suppress is with `from None`) that the extra clause comes in handy. I think using the default unless there's a reason not to is, in general, a good policy. 

I know I'm -1 on this particular change, for the reasons in this thread, but thank you for your efforts nonetheless. :)

Kind Regards,

Carlton

On Friday, 7 February 2020 12:17:11 UTC+1, Ram Rachum wrote:

[Ram Rachum]

FYI: I opened a thread on Python-ideas where we continued the discussion on my `raise as` proposal, Shai's proposal, etc.:  https://mail.python.org/archives/list/python...@python.org/thread/KM7NRNFZHALOBKJUXVYQL2SLDP3MAANW/