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.

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

Add support of PyStructSequence in copy.replace() #110222

Closed
XuehaiPan opened this issue Oct 2, 2023 · 4 comments · Fixed by #110223
Closed

Add support of PyStructSequence in copy.replace() #110222

XuehaiPan opened this issue Oct 2, 2023 · 4 comments · Fixed by #110223
Labels
type-feature A feature request or enhancement

Comments

@XuehaiPan
Copy link
Contributor

XuehaiPan commented Oct 2, 2023

Feature or enhancement

Proposal:

This issue is to make the concept of "named tuple" support the __replace__ protocol.

collections.namedtuple and typing.NamedTuple already support the __replace__ protocol in:

PyStructSequences are also named tuples but they do not support the __replace__ protocol yet.

It would be convenient if PyStructSequence be supported in copy.replace().

Has this already been discussed elsewhere?

I have already discussed this feature proposal on Discourse

Links to previous discussion of this feature:

Linked PRs

@XuehaiPan XuehaiPan added the type-feature A feature request or enhancement label Oct 2, 2023
@AlexWaygood
Copy link
Member

AlexWaygood commented Oct 2, 2023

cc. @serhiy-storchaka

@XuehaiPan
Copy link
Contributor Author

I created a PR to resolve this.

@serhiy-storchaka
Copy link
Member

PyStructSequence with unnamed fields is a special case. Currently the only such structure is os.stat_result. Accessible by name fields st_atime, st_mtime and st_ctime are float values. Corresponding unnamed fields 7-9 are integer values. It is an old trick for compatibility. __replace__() can replace named fields, but unnamed fields will keep the old value. Integer and float value will be de-synchronized, and the code that reads unnamed fields will work incorrectly.

The simplest solution is to disable __replace__() for PyStructSequence with unnamed fields. Such structures can even be deprecated, but this is a different issue.

There may be a special implementation of __replace__() for os.stat_result, which derives values for unnamed fields from corresponding values for named fields. But float value can be less precise than integer value, because it has limited precision. So it needs to take into account *_ns fields. It is separate very complex issue.

@XuehaiPan
Copy link
Contributor Author

PyStructSequence with unnamed fields is a special case. Currently the only such structure is os.stat_result. Accessible by name fields st_atime, st_mtime and st_ctime are float values. Corresponding unnamed fields 7-9 are integer values. It is an old trick for compatibility. __replace__() can replace named fields, but unnamed fields will keep the old value. Integer and float value will be de-synchronized, and the code that reads unnamed fields will work incorrectly.

I can confirm this is a bug for PyStructSequence with unnamed fields if we rely on __reduce__(). But I'm not sure why pickle.loads(pickle.dumps(stat)) works fine for os.stat_result.

The simplest solution is to disable __replace__() for PyStructSequence with unnamed fields. Such structures can even be deprecated, but this is a different issue.

I agree to raise an error for this (e.g., TypeError) since os.stat_result is the only case to have unnamed fields.

There may be a special implementation of __replace__() for os.stat_result, which derives values for unnamed fields from corresponding values for named fields. But float value can be less precise than integer value, because it has limited precision. So it needs to take into account *_ns fields. It is separate very complex issue.

I think it's better to disable __replace__() for PyStructSequence with unnamed fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants