Skip to content

bpo-38530: Offer suggestions on AttributeError - #16856

Merged
pablogsal merged 7 commits into
python:masterfrom
pablogsal:suggestions2
Apr 14, 2021
Merged

bpo-38530: Offer suggestions on AttributeError#16856
pablogsal merged 7 commits into
python:masterfrom
pablogsal:suggestions2

Conversation

@pablogsal

@pablogsalpablogsal commented Oct 19, 2019•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@pablogsal
pablogsal requested a review from a team as a code ownerOctober 19, 2019 23:12
@pablogsal
pablogsalforce-pushed the suggestions2 branch 7 times, most recently from c812c91 to 2140445CompareOctober 20, 2019 01:32
@pablogsal
pablogsalforce-pushed the suggestions2 branch 2 times, most recently from d34f72e to 01a0bc1CompareOctober 28, 2019 02:03
@pablogsal

pablogsal commented Oct 28, 2019

Copy link
Copy Markdown
MemberAuthor

@serhiy-storchaka Would you like to take a look at this PR when you have some time? :)

@pablogsal
pablogsal requested review from 1st1 and methaneOctober 28, 2019 15:01
@pablogsal
bpo-38530: Offer suggestions on AttributeError
c9c85b5
Add News entry
Make 'name' and 'obj' public attributes of AttributeError
Fix various issues with the implementation
Add more tests
Simplify implementation and rename public function
More cosmetic changes
Add more tests
Add more tests
vstinner
vstinner reviewed Apr 10, 2021
View reviewed changes

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

First review. I like the overall approach (I expect a low overhead when raising AttributeError).

Comment threadInclude/cpython/pyerrors.h
PyException_HEAD
PyObject *obj;
PyObject *name;
} PyAttributeErrorObject;

@vstinnervstinnerApr 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

https://www.python.org/dev/peps/pep-0473/ was rejected because it was too broad, but this PR adds a single exception, which sounds ok according to the resolution: https://mail.python.org/pipermail/python-dev/2019-March/156692.html

My main worry is the risk of creating "more" and "worse" exception cycles, but Pablo says that it sounds unlikely: https://bugs.python.org/issue38530#msg354975

Comment threadInclude/pyerrors.h
Comment threadObjects/exceptions.c
Comment threadObjects/exceptions.c Outdated
Comment threadObjects/object.c Outdated
Comment threadPython/pythonrun.c Outdated
Comment threadPython/suggestions.c
Comment threadPython/suggestions.c Outdated
Comment threadPython/suggestions.c Outdated
Comment threadPython/suggestions.c Outdated
return_val = -1;
goto exit;
}
exc->args = new_args;

@vstinnervstinnerApr 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not storing suggestions in a new Exception field, rather than replacing existing args? I may break some applications.

@pablogsalpablogsalApr 10, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Because I don't want to touch the reporting facility to inspect said field or allowing users to inspect it manually

@pablogsalpablogsalApr 10, 2021•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Also, this substitution only happens before printing the value because it has been not caught. Nobody should be able to inspect this exception later, no? Notice this only happens in the default exception display, so if someone has a custom handler, they will not see this AFAIK.

@methanemethaneApr 11, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code level comment:
I don't understand why exc->args = new_args; Isn't it set by Py_SETREF?

Higher level comment:
Why do you replace exc->args instead of just printing suggestion after _PyErr_Display() or in _PyErr_Display()?

@pablogsalpablogsalApr 11, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I don't understand why exc->args = new_args; Isn't it set by Py_SETREF?

I forgot to remove that after updated the code after @vstinner suggestions.

Why do you replace exc->args instead of just printing suggestion after _PyErr_Display() or in _PyErr_Display()?

Actually, this is a very good point, this can make the code cleaner. I will implement this

vstinner
vstinner reviewed Apr 10, 2021
View reviewed changes
Comment threadObjects/object.c Outdated
@pablogsal
pablogsalforce-pushed the suggestions2 branch 4 times, most recently from d9b74da to 97bf9e4CompareApril 14, 2021 00:54
@pablogsal
pablogsal merged commit 37494b4 into python:masterApr 14, 2021
@pablogsal
pablogsal deleted the suggestions2 branch April 14, 2021 01:36
@pythonpython deleted a comment from bedevere-botApr 14, 2021
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 10, 2021
@Snowapril
Implement keyword suggestion routine
37075f4
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 11, 2021
@Snowapril
Implement keyword suggestion routine
9c5e9c1
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 11, 2021
@Snowapril
Implement keyword suggestion routine
be8f93c
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 11, 2021
@Snowapril
Implement keyword suggestion routine
3953327
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 11, 2021
@Snowapril
Implement keyword suggestion routine
2edd1d5
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 13, 2021
@Snowapril
Implement keyword suggestion routine
768a0da
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 13, 2021
@Snowapril
Implement keyword suggestion routine
19bab40
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 13, 2021
@Snowapril
Implement keyword suggestion routine
4affb59
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 13, 2021
@Snowapril
Implement keyword suggestion routine
8618864
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 14, 2021
@Snowapril
Implement keyword suggestion routine
2f51491
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 14, 2021
@Snowapril
Implement keyword suggestion routine
c30fa12
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 14, 2021
@Snowapril
Implement keyword suggestion routine
8aee5c2
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 15, 2021
@Snowapril
Implement keyword suggestion routine
24057aa
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 16, 2021
@Snowapril
Implement keyword suggestion routine
dd8ce45
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 18, 2021
@Snowapril
Implement keyword suggestion routine
860785d
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 18, 2021
@Snowapril
Implement keyword suggestion routine
da629a0
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 20, 2021
@Snowapril
Implement keyword suggestion routine
6ae5c3b
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
Snowapril added a commit to Snowapril/RustPython that referenced this pull request Oct 20, 2021
@Snowapril
Implement keyword suggestion routine
6339390
`suggestions.rs` is almost porting of implementation of
[this](python/cpython#16856) and
[this](python/cpython#25397).
Signed-off-by: snowapril <sinjihng@gmail.com>
@philg314philg314 mentioned this pull request Aug 28, 2022
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@methanemethanemethane left review comments
@vstinnervstinnervstinner approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pablogsal@vstinner@methane@the-knights-who-say-ni@bedevere-bot