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.

Sign up for GitHub

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

Jump to bottom

gh-98963: Restore the ability to have a dict-less property. #105262

Merged
merged 7 commits into from
Jun 5, 2023
Merged

gh-98963: Restore the ability to have a dict-less property. #105262

merged 7 commits into from
Jun 5, 2023

Conversation

gpshead
Copy link
Member

@gpshead gpshead commented Jun 3, 2023 •
edited
Loading

Ignore doc string assignment failures in property as has been the behavior of all past Python releases.

Preserves the one situation in which the AttributeError has always been raised for this property subclass situation:
If the docstring would be applied from a getter function it raises rather than remaining silent. (see the existing test and code comments)

This undoes a behavior regression present in 3.12beta1 that was causing existing widely used library code (Google protobuf) to fail.

@gpshead
pythongh-98963: Restore the ability to have a dict-less property.
93276d2 
Ignore doc string assignment failures in `property` as has been the
behavior of all past Python releases.

One behavior change: The longstanding tested behavior of raising an
`AttributeError` when using a slotted no-dict property subclass as a
decorator on a getter sporting a docstring is now consistent with the
subclassing behavior and does not produce an error.
@gpshead gpshead self-assigned this Jun 3, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jun 3, 2023
Closed
@gpshead gpshead requested a review from JelleZijlstra June 3, 2023 00:44
@gpshead gpshead mentioned this pull request Jun 3, 2023
Closed
@gpshead gpshead requested a review from encukou June 3, 2023 00:46
@gpshead

This comment was marked as outdated.

Sign in to view
@gpshead gpshead marked this pull request as ready for review June 3, 2023 00:48
@gpshead gpshead added the needs backport to 3.12 bug and security fixes label Jun 3, 2023
@gpshead gpshead mentioned this pull request Jun 3, 2023
Merged
@gpshead gpshead requested a review from Yhg1s June 3, 2023 00:53
@gpshead gpshead enabled auto-merge (squash) June 4, 2023 21:14
JelleZijlstra
JelleZijlstra approved these changes Jun 5, 2023
View reviewed changes
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.

This behavior is weird but it's consistent with how 3.11 works, so let's merge it. (Apart from what I think is an extraneous DECREF.)

Objects/descrobject.c Outdated Show resolved Hide resolved
@gpshead gpshead disabled auto-merge June 5, 2023 02:35
@gpshead
Copy link
Member Author

gpshead commented Jun 5, 2023

This behavior is weird but it's consistent with how 3.11 works, so let's merge it.

Yeah... I can't say this is the overall set of behavior anyone would ever choose. I'm mostly aiming for minimum behavior change vs older Pythons so if we ever want to bother with changing this semi-esoteric edge case we can do it in a planned API change fashion.

adding that extra code to retain the AttributeError only when the docstring comes from getter.__doc__ feels weird, but that behavior might honestly be what we want in all cases in the long run so removing it as my initial version of this PR did felt unwise as it could lead to people unintentionally writing code that doesn't work across multiple versions if we ever choose to do that.

@gpshead gpshead enabled auto-merge (squash) June 5, 2023 03:01
@JelleZijlstra JelleZijlstra disabled auto-merge June 5, 2023 03:05
@JelleZijlstra JelleZijlstra enabled auto-merge (squash) June 5, 2023 03:15
@JelleZijlstra JelleZijlstra merged commit 418befd into python:main Jun 5, 2023
@miss-islington
Copy link
Contributor

Thanks @gpshead for the PR, and @JelleZijlstra for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-105297 is a backport of this pull request to the 3.12 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 5, 2023
@gpshead @miss-islington
pythongh-98963: Restore the ability to have a dict-less property. (py…
544efd0 
…thonGH-105262)

Ignore doc string assignment failures in `property` as has been the
behavior of all past Python releases.
(cherry picked from commit 418befd)

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jun 5, 2023
@gpshead
Copy link
Member Author

gpshead commented Jun 5, 2023 •
edited
Loading

I wish github would've preserved the much longer commit message I authored for later when auto merge was disabled. :(

@gpshead gpshead deleted the property-nodict-docstr-error branch June 5, 2023 03:24
gpshead added a commit that referenced this pull request Jun 5, 2023
@miss-islington @gpshead
[3.12] gh-98963: Restore the ability to have a dict-less property. (G…
3e7ddc2 
…H-105262) (#105297)

gh-98963: Restore the ability to have a dict-less property. (GH-105262)

Ignore doc string assignment failures in `property` as has been the
behavior of all past Python releases.  (the docstring is discarded)
(cherry picked from commit 418befd)

This fixes a behavior regression in 3.12beta1 where an AttributeError was being raised in a situation it has never been in the past. It keeps the existing unusual single situation where AttributeError does get raised.

Existing widely deployed projects depend on this not raising an exception.

Co-authored-by: Gregory P. Smith <greg@krypto.org>
@JelleZijlstra
Copy link
Member

I wish github would've preserved the much longer commit message I authored for later when auto merge was disabled. :(

Sorry for that!

@erlend-aasland erlend-aasland mentioned this pull request Jun 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

@encukou encukou Awaiting requested review from encukou

@Yhg1s Yhg1s Awaiting requested review from Yhg1s

Assignees

@gpshead gpshead

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gpshead @miss-islington @bedevere-bot @JelleZijlstra