GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-98963: Restore the ability to have a dict-less property. by gpshead · Pull Request #105262 · python/cpython

Conversation

@gpshead

@gpshead gpshead commented Jun 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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.

  • Issue: property assumes that its subclasses have __dict__ #98963

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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
property assumes that its subclasses have __dict__ #98963
Closed
@gpshead
gpshead requested a review from JelleZijlstra June 3, 2023 00:44
@gpshead gpshead mentioned this pull request Jun 3, 2023
gh-98963: Add a note to the error for property subclasses without __doc__ #99058
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
@bedevere-bot bedevere-bot added the awaiting core review label Jun 3, 2023
@gpshead gpshead added the needs backport to 3.12 only security fixes label Jun 3, 2023
@gpshead gpshead mentioned this pull request Jun 3, 2023
bpo-41287: Handle doc argument of property.__init__ in subclasses #23205
Merged
@gpshead
gpshead requested a review from Yhg1s June 3, 2023 00:53
gpshead added 4 commits June 4, 2023 12:34
@gpshead
Fix ASAN issue, to many DECREFs :P
c6cdc7f
@gpshead
s/tabs/spaces/ (ugh editor configs)
3ceab53
@gpshead
Restore all historical behaviors.
95727f5
@gpshead
Reword comments and NEWS.
996e70b
@gpshead
gpshead enabled auto-merge (squash) June 4, 2023 21:14
@gpshead
restore the singular decref. (correctness)
517bd0d
JelleZijlstra
JelleZijlstra approved these changes Jun 5, 2023
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/descrobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jun 5, 2023
@gpshead
gpshead disabled auto-merge June 5, 2023 02:35
@gpshead
DECREF before return, not always.
0307832
@gpshead

gpshead commented Jun 5, 2023

Copy link
Copy Markdown
Member Author

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.
JelleZijlstra reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

miss-islington commented Jun 5, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 5, 2023
@bedevere-bot

bedevere-bot commented Jun 5, 2023

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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 <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 5, 2023
@gpshead

gpshead commented Jun 5, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 <[email protected]>
@JelleZijlstra

JelleZijlstra commented Jun 5, 2023

Copy link
Copy Markdown
Member

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

Sorry for that!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland erlend-aasland mentioned this pull request Jun 6, 2023
3.12 backport gh 105236 #105358
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@gpshead gpshead

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@gpshead @miss-islington @bedevere-bot @JelleZijlstra
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.