Skip to content
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

PEP 678: Enriching Exceptions with Notes #2201

Merged
merged 8 commits into from
Dec 22, 2021

Conversation

Zac-HD
Copy link
Contributor

@Zac-HD Zac-HD commented Dec 20, 2021

@iritkatriel mentioned to me that the addition of BaseException.__note__ is a change to a builtin and therefore - while it's already in CPython main - requires some further discussion. I've therefore drafted a PEP to explain the changes, motivation, and rejected alternatives.

I'm generally happy to help out with this around my holiday plans and amend HypothesisWorks/hypothesis#3191 to reflect any necessary changes, but would appreciate any advice people have - I've read the process docs but it's still my first PEP 😄

Draft by request of @iritkatriel describing the new BaseException.__note__ we co-designed.
Thanks again to Irit for helpful comments :-)
@Zac-HD Zac-HD requested a review from a team as a code owner December 20, 2021 12:22
pep-9999.rst Outdated Show resolved Hide resolved
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address the alternative proposed by Nathaniel, who suggested to attach the note to individual traceback entries; PyTracebackObject instances would get a new field ‘note’.

Otherwise LG.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of copyediting suggestions

.github/CODEOWNERS Outdated Show resolved Hide resolved
pep-9999.rst Outdated Show resolved Hide resolved
pep-9999.rst Outdated Show resolved Hide resolved
pep-9999.rst Outdated Show resolved Hide resolved
pep-9999.rst Outdated Show resolved Hide resolved
pep-9999.rst Outdated Show resolved Hide resolved
pep-9999.rst Outdated Show resolved Hide resolved
pep-9999.rst Outdated Show resolved Hide resolved
pep-9999.rst Outdated Show resolved Hide resolved
@iritkatriel
Copy link
Member

Please address the alternative proposed by Nathaniel, who suggested to attach the note to individual traceback entries; PyTracebackObject instances would get a new field ‘note’.

Otherwise LG.

Was this proposal made in public yet? (Otherwise Zac probably doesn't know about it).

@gvanrossum
Copy link
Member

gvanrossum commented Dec 20, 2021 via email

@brettcannon
Copy link
Member

Please take PEP 678 as your number (it should simply be the next available number in case I missed a PR w/ a proposed PEP that's already claimed that number).

@iritkatriel
Copy link
Member

Here's a paraphrase: https://docs.google.com/document/d/1ptNdtAe4vywW2jldOxbT0iYgRxhA0VjfCAVcYXohEH0/edit

I'd double check with him that this presents his idea correctly, and that he wants it quoted in the PEP (his remarks were not made in the public domain).

@Zac-HD Zac-HD changed the title PEP 9999: Enriching Exceptions with Notes PEP 678: Enriching Exceptions with Notes Dec 21, 2021
@JelleZijlstra
Copy link
Member

Unless you'd prefer me to wait, I'm planning to merge this PR once CI is green (there's a failure right now). That way, the number is claimed and I can stop pushing the approve CI button. Adding Nathaniel's idea can be done later.

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Dec 21, 2021

I did not know about Nathaniel's proposal, but as the doc notes it would not work for Hypothesis; nor in my understanding would it work for tools like friendly-traceback. (though Pytest might be interested!)

I'm happy for this to be merged whenever convenient; I can add a section in "Rejected Ideas" (or "related and deferred ideas", perhaps) once I have a better understanding of the traceback-notes proposal.

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Dec 22, 2021

@encukou asks in python/cpython#30158 (comment):

Complicated or not, is string hacking the right thing to do? And how much can Hypothesis assume about __note__?

Well, what's the alternative? "String or None" seems much simpler than e.g. a mutable list of sub-notes!

And all Hypothesis, or any other library, needs to assume is that the note is either None (and can be replaced), or some string - and can have another note joined to it. with one or more newlines if desired.

Consider this silly example, which could be refactored in various ways:

    def test_silly(self):
        for a in range(10):
            with self.subTest(a=a):
                for b in range(10):
                    with self.subTest(b=b):
                        self.assertNotEqual(a*b, 12)

Today's unittest has its own way of handling multiple exceptions, but if subTest it was being written today, would __note__ not be a good fit here? If it would, how would subTest be expected to work, and how should it interoperate with Hypothesis?

Yes, __note__ might be a good fit for subTest if it were being designed today.

Hypothesis sees test failures as "an exception raised from a callable", and would concatenate it's note to subTest's.

The result might look something like:
AssertionError: 12 is equal to 12
subTest: a=2
subTest: b=6

Falsifying example: test_silly(
    x=...,  # But what are we generating here, and why not include `a` and `b`?
)

Though note that Hypothesis is generally a replacement for rather than a complement to subTest: instead of looping over some data, I'd have Hypothesis generate examples directly and shrinking takes the place of labelling. That might look like:

@given(a=st.integers(0, 10), b=st.integers(0, 10))
def test_silly(self, a, b):
    self.assertNotEqual(a*b, 12)
AssertionError: 12 is equal to 12
Falsifying example: test_silly(
    a=2,
    b=6,
)

@iritkatriel
Copy link
Member

Is this another use case for note? I wonder why it's written like this instead of chaining. Maybe it predates chaining.

https://github.com/python/cpython/blob/f4c03484da59049eb62a9bf7777b963e2267d187/Lib/urllib/request.py#L1794

@iritkatriel
Copy link
Member

That example also points to an ambiguity in the note-on-traceback suggestion. Does the note belong to the exception or the traceback? In other words - should with_traceback copy over the notes, or create a deep copy of the traceback with the notes cleared?

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Dec 22, 2021

Is this another use case for note? I wonder why it's written like this instead of chaining. Maybe it predates chaining.

python/cpython@f4c0348/Lib/urllib/request.py#L1794

This goes right back to (at least) the creation of urllib in 2008, when exception chaining was PEP'd but not yet released in Python 3.0. In this particular case I'd be inclined to reach for exception chaining... or, since it's no longer changing the exception type, just take it out of a try/except block entirely.

That example also points to an ambiguity in the note-on-traceback suggestion. Does the note belong to the exception or the traceback? In other words - should with_traceback copy over the notes, or create a deep copy of the traceback with the notes cleared?

As I understand it, the proposal was to attach notes to particular traceback frames, not the exception. This would also mean that Trio's and Hypothesis' traceback-trimming logic would discard any notes that had (somehow!) been attached to frames we pop off the traceback. @njsmith might elaborate?

@JelleZijlstra
Copy link
Member

FYI CI is failing.

pep-0678.rst Outdated Show resolved Hide resolved
pep-0678.rst Outdated Show resolved Hide resolved
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
pep-0678.rst Outdated Show resolved Hide resolved
@JelleZijlstra JelleZijlstra merged commit 8c43285 into python:main Dec 22, 2021
@Zac-HD Zac-HD deleted the exception-note branch December 22, 2021 19:25
@warsaw
Copy link
Member

warsaw commented Dec 24, 2021 via email

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Dec 24, 2021

Did you think about setting __note__ to an object that has a __str__()? I also question whether string hacking is the right thing for this, given that coordination between except clauses within different frameworks might make that exceedingly difficult and fragile. If you could set __note__ to something more structured, then that object could provide a useful API and a __str__() for printing at trackback time.

I did consider this briefly, but didn't think the extra complexity was justified - I can't think of a scenario where frameworks would want to do anything but either concatenate or replace notes, and that wouldn't require any coordination with strings.

What kind of string hacking are you thinking of which would be fragile?

@warsaw
Copy link
Member

warsaw commented Dec 26, 2021

What kind of string hacking are you thinking of which would be fragile?

I don't have any specific use cases in mind, so it's just an intuition. Let's say I needed to catch an exception, roll back a transaction, and re-raise the exception. E.g. as in this code. What if I wanted to add a __note__ here saying "The transaction has been aborted". Where in the __note__ would I put that? Sure, I could just concatenate this, but maybe I wanted to be more clever and try to put this somewhere else in the exception note. Now I have to parse the string and figure out where to put it, but all that parsing will be pretty fragile. E.g. maybe there's some library layered in there that's putting in its own __note__ and that text changes between versions of the library. The mental model I have is something like doctests where you're printing out exceptions. You need to be very clever about that or your tests are closely tied to the exact language of the exception, which has known history of being easily broken.

As I said, I don't have a specific example in mind, so maybe none of this will be a problem in practice. I don't even have a better API in mind. I'm also not sure we can design the right API without some time with this feature released out in the wild. I'm just trying to raise some concerns based on previous, somewhat similar experiences. I think it might be sufficient enough to allow None, a str or an object with a __str__() and allow experimentation to guide us.

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

Successfully merging this pull request may close these issues.

7 participants