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

The dataclasses unit tests should record behavior of shadowed init vars #119581

Closed
stroxler opened this issue May 26, 2024 · 1 comment
Closed

The dataclasses unit tests should record behavior of shadowed init vars #119581

stroxler opened this issue May 26, 2024 · 1 comment
Labels
tests Tests in the Lib/test dir

Comments

@stroxler
Copy link
Contributor

stroxler commented May 26, 2024 •
edited by bedevere-app bot
Loading

Tests should verify that dataclasses allow no-default InitVars to use shadowed names.

I was surprised, when reviewing a change to MyPy that allows InitVar fields to be shadowed by property methods, that this even works since the runtime will drop the original InitVar value.

It turns out that __annotations__ does not forget about the original InitVar (__annotations__ preserves annotations from class attributes that came from assignment statements, ignoring any shadowing methods) and as a result this actually does work as long as no default value is provided.

See more discussion and context in python/mypy#17219.

@hauntsaninja mentioned that MyPy has had several reports of this as a false positives, so we know people are relying on this behavior (even if it may not have been intended originally) and we probably should not break it.

It probably makes sense to add a unit test, since this behavior is actually a bit surprising and could potentially be broken by a refactor. I'm not sure whether we should also discuss this in the documentation, I would lean toward "no" because I think this is supported more by accident than by design (both Pyright and Pyre reject it, which seems reasonable to me).

Linked PRs
@stroxler stroxler added the docs Documentation in the Doc dir label May 26, 2024
stroxler added a commit to stroxler/cpython that referenced this issue May 26, 2024
@stroxler
[pythongh-119581] Add a test of InitVar with name shadowing
0a8431d 
As originally discussed in
python/mypy#17219,
MyPy has had a false-positive bug report because it errors when a
dataclass has methods that shadow an `InitVar` field.

It is actually a bit surprising that this works, it turns out
that `__annotations__` "remembers" field assignments even if the
bound names are later overwritten by methods; it will *not* work
to provide a default value.

There have been multiple bug reports on MyPy so we know people are
actually relying on this in practice; most likely it comes up when
a dataclass wants to take a "raw" value as an InitVar and transform
it somehow in `__post_init__` into a different value before assigning
it to a field; in that case they may choose to make the actual field
private and provide a property for access.

I currently provide a test of the happy path where there is no default
value provided, but no tests of the behavior when no default is
provided (in which case the property will override the default) and no
documentation (because I'm not sure we want to consider this behavior
officially supported).

The main goal is to have a regression test since it would be easy for a
refactor to break this.
stroxler added a commit to stroxler/cpython that referenced this issue May 26, 2024
@stroxler
[pythongh-119581] Add a test of InitVar with name shadowing
2f5b3b8 
As originally discussed in
python/mypy#17219,
MyPy has had a false-positive bug report because it errors when a
dataclass has methods that shadow an `InitVar` field.

It is actually a bit surprising that this works, it turns out
that `__annotations__` "remembers" field assignments even if the
bound names are later overwritten by methods; it will *not* work
to provide a default value.

There have been multiple bug reports on MyPy so we know people are
actually relying on this in practice; most likely it comes up when
a dataclass wants to take a "raw" value as an InitVar and transform
it somehow in `__post_init__` into a different value before assigning
it to a field; in that case they may choose to make the actual field
private and provide a property for access.

I currently provide a test of the happy path where there is no default
value provided, but no tests of the behavior when no default is
provided (in which case the property will override the default) and no
documentation (because I'm not sure we want to consider this behavior
officially supported).

The main goal is to have a regression test since it would be easy for a
refactor to break this.
@bedevere-app bedevere-app bot mentioned this issue May 26, 2024
Merged
@Eclips4 Eclips4 added tests Tests in the Lib/test dir and removed docs Documentation in the Doc dir labels May 26, 2024
AlexWaygood pushed a commit that referenced this issue May 28, 2024
@stroxler
gh-119581: Add a test of InitVar with name shadowing (#119582)
6ec3712
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 28, 2024
@stroxler @miss-islington
pythongh-119581: Add a test of InitVar with name shadowing (pythonGH-…
2d42558 
…119582)

(cherry picked from commit 6ec3712)

Co-authored-by: Steven Troxler <[email protected]>
@bedevere-app bedevere-app bot mentioned this issue May 28, 2024
Merged
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 28, 2024
@stroxler @miss-islington
pythongh-119581: Add a test of InitVar with name shadowing (pythonGH-…
1fc07d3 
…119582)

(cherry picked from commit 6ec3712)

Co-authored-by: Steven Troxler <[email protected]>
@bedevere-app bedevere-app bot mentioned this issue May 28, 2024
Merged
AlexWaygood pushed a commit that referenced this issue May 28, 2024
@miss-islington @stroxler
[3.13] gh-119581: Add a test of InitVar with name shadowing (GH-119582)…
2b46253 
… (#119672)

gh-119581: Add a test of InitVar with name shadowing (GH-119582)
(cherry picked from commit 6ec3712)

Co-authored-by: Steven Troxler <[email protected]>
AlexWaygood pushed a commit that referenced this issue May 28, 2024
@miss-islington @stroxler
[3.12] gh-119581: Add a test of InitVar with name shadowing (GH-119582)…
08636c1 
… (#119673)

gh-119581: Add a test of InitVar with name shadowing (GH-119582)
(cherry picked from commit 6ec3712)

Co-authored-by: Steven Troxler <[email protected]>
@AlexWaygood
Copy link
Member

Thanks @stroxler! I agree with adding a test, but not documenting it. It feels obscure enough that we probably shouldn't devote space to it; doing so might confuse new users just as much as it clarifies things for advanced users

estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
@stroxler @estyxx
pythongh-119581: Add a test of InitVar with name shadowing (python#11…
2ca5185 
…9582)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@stroxler @AlexWaygood @Eclips4