gh-153785: Generate AttributeError messages from context#153786
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
AttributeError messages from contextAttributeError messages from context
Documentation build overview
50 files changed ·
|
|
|
||
| When possible, :attr:`name` and :attr:`obj` are set automatically. | ||
|
|
||
| .. versionchanged:: 3.10 |
There was a problem hiding this comment.
We dropped the .. versionchanged:: next note that documented the generated message. This is a user-visible change to str(), so I think we should keep a versionchanged here, no?
There was a problem hiding this comment.
@serhiy-storchaka, do you agree on bringing it back?
There was a problem hiding this comment.
We usually do not add versionchanged for minor changes like changed error message. Otherwise our documentation would be overwhelmed by them.
vstinner
left a comment
There was a problem hiding this comment.
The following code writes 'lazy_import' object has no attribute 'missing1': it mentions lazy_import rather than module 'asyncio'. But I don't think that AttributeError_str() would be the right place to reify the asyncio module!
lazy import asyncio
class RaiseWithName:
def __getattr__(self, name):
raise AttributeError(name)
try:
getattr(globals()['asyncio'], "missing1")
except AttributeError as exc:
# assert exc.obj is ...
assert exc.name == "missing1"
print(str(exc))
"this" and "current one" is unclear here. Please update the PR description to describe the motivation. Also consider linking to the code for the trick in KeyError. |
I used Claude to infer the motivations and update the description. |
This trick has been applied to KeyError and AttributeError; should it be applied to other exceptions? Should it be implemented in such a way that the concern is shared across exceptions (explicitly or implicitly)? |
Short answer, not really. Details(Analysis by Claude, on my behalf.) A worthwhile question. Worth noting first that the two "tricks" only look alike at the surface (both override
Only the second is the interesting reusable pattern ("I have structured context attributes; render them lazily instead of forcing every raise site to build the string"). The natural cohort there is So I'd lean against building a shared framework here:
The one genuinely reusable piece is the guard predicate — "was this constructed with a default/absent message, or one that merely duplicates a structured field, so I'm safe to override?" (here: |
@vstinner, this code does not trigger the code path from this PR ( |
Co-authored-by: Victor Stinner <vstinner@python.org>
|
|
||
| When possible, :attr:`name` and :attr:`obj` are set automatically. | ||
|
|
||
| .. versionchanged:: 3.10 |
There was a problem hiding this comment.
We usually do not add versionchanged for minor changes like changed error message. Otherwise our documentation would be overwhelmed by them.
| if (modname && PyUnicode_Check(modname)) { | ||
| result = PyUnicode_FromFormat("module '%U' has no attribute '%U'", | ||
| modname, name); | ||
| Py_DECREF(modname); | ||
| } else { | ||
| Py_XDECREF(modname); | ||
| result = PyUnicode_FromFormat("module has no attribute '%U'", name); | ||
| } | ||
| } else if (PyType_Check(obj)) { | ||
| result = PyUnicode_FromFormat("type object '%N' has no attribute '%U'", | ||
| obj, name); | ||
| } else { | ||
| result = PyUnicode_FromFormat("'%T' object has no attribute '%U'", | ||
| obj, name); |
There was a problem hiding this comment.
The %U doesn't escape properly quote characters. Example:
exc=AttributeError()
exc.name = "a'b"
exc.obj = ["list"]
print(str(exc))Current output:
'list' object has no attribute 'a'b'
I suggesting using %R instead of '%U':
| if (modname && PyUnicode_Check(modname)) { | |
| result = PyUnicode_FromFormat("module '%U' has no attribute '%U'", | |
| modname, name); | |
| Py_DECREF(modname); | |
| } else { | |
| Py_XDECREF(modname); | |
| result = PyUnicode_FromFormat("module has no attribute '%U'", name); | |
| } | |
| } else if (PyType_Check(obj)) { | |
| result = PyUnicode_FromFormat("type object '%N' has no attribute '%U'", | |
| obj, name); | |
| } else { | |
| result = PyUnicode_FromFormat("'%T' object has no attribute '%U'", | |
| obj, name); | |
| if (modname && PyUnicode_Check(modname)) { | |
| result = PyUnicode_FromFormat("module '%U' has no attribute %R", | |
| modname, name); | |
| Py_DECREF(modname); | |
| } else { | |
| Py_XDECREF(modname); | |
| result = PyUnicode_FromFormat("module has no attribute %R", name); | |
| } | |
| } else if (PyType_Check(obj)) { | |
| result = PyUnicode_FromFormat("type object '%N' has no attribute %R", | |
| obj, name); | |
| } else { | |
| result = PyUnicode_FromFormat("'%T' object has no attribute %R", | |
| obj, name); |
There was a problem hiding this comment.
Sure, although we could say the same thing about module names and type names, no?
There was a problem hiding this comment.
Using import, I don't think that you can have a module name which contains quotes. You might hack the module name to insert quotes later.
For consistency, I would be fine with using %R to format the module name. It's also shorter 😁
There was a problem hiding this comment.
For the type name, quotes or other special characters are very unlikely. I don't think that it's worth it to care about these ones. '%T' looks like the right format.
There was a problem hiding this comment.
Thanks! Changes applied.
There was a problem hiding this comment.
First of all, we should be consistent with the original error messages raised in corresponding tp_getattro. We can change this, but we should do this globally, and this is a separate issue.
There was a problem hiding this comment.
I agree that it's worth it to use %R in similar code such as PyObject_GetAttr() and PyObject_SetAttr(). Currently, it also has the bug:
>>> getattr(object(), "a'b")
AttributeError: 'object' object has no attribute 'a'b'
We can change this, but we should do this globally, and this is a separate issue.
But I disagree with that. IMO it's fine to use %R in this PR. Similar code paths can be changed in a follow-up PR.
vstinner
left a comment
There was a problem hiding this comment.
LGTM. Thanks for addressing my latest comments.
I wanted to merge the change, but @serhiy-storchaka seems to have concerns about %R usage.
|
I think this deserves a separate discussion (What if the name is an instance of the str subclass like StrEnum?), but this should not be blocker for merging this PR. |
Do you mean that you're fine with |
|
I am fine with any. |
|
I merged your enhancement, thanks @johnslavik! |
|
Thanks everyone for reviewing! I've learned a ton |
I think this is the best place to do this. We do the same trick in
KeyError.This also fixed issue #143811 that motivated the current one.
AttributeErrormessages from their context #153785