gh-154387: Fix structseq_repr mislabeling of unnamed PyStructSequence fields#154434
gh-154387: Fix structseq_repr mislabeling of unnamed PyStructSequence fields#154434pranavchoudhary-tech wants to merge 9 commits into
structseq_repr mislabeling of unnamed PyStructSequence fields#154434Conversation
structseq_repr mislabeling of unnamed PyStructSequence fieldsstructseq_repr mislabeling of unnamed PyStructSequence fields
There was a problem hiding this comment.
The issue is not only about the repr but about labelling and accesses. I do not think this issue is easy for a newcomer as you need more testing here. In addition, not having anyone assigned to an issue does not mean it is free to pick as issues need first to be discussed.
| self.assertIn("st_ino=", rep) | ||
| self.assertIn("st_dev=", rep) | ||
|
|
||
| # Issue #154387: unnamed fields should not be mislabeled with named field names. |
There was a problem hiding this comment.
This does not test the same things as the issue. Please be more thorough here.
|
|
||
| By default, build.bat will build Python in Release configuration for | ||
| the 32-bit Win32 platform. It accepts several arguments to change | ||
| this behavior, try `build.bat -h` to learn more. |
There was a problem hiding this comment.
Unrelated changes please delete this.
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
| self.assertEqual( | ||
| rep, | ||
| "os.stat_result(st_mode=0, st_ino=1, st_dev=2, st_nlink=3, " | ||
| "st_uid=4, st_gid=5, st_size=6, 7, 8, 9)" |
There was a problem hiding this comment.
I do not think extra items should actually ne considered if they are un-named, or at least they should be formatted differently. We jeed to discuss the format first because here, this repr would not work (you cannot just use it again as an input).
I do not know whether it is better to simply ignore unnamed fields or do something else.
|
Hi @picnixz, thank you for taking the time to review this! You make a great point about the repr output resulting in invalid syntax if we just drop the labels. I completely agree that we need to align on the expected format before proceeding. Do you think we should completely omit the unnamed fields from the repr output, or is there another format the core team would prefer? I also apologize for jumping the gun and opening a PR before the issue was fully discussed. I have reverted the unrelated change to PCbuild/readme.txt as requested. Lastly, I saw your comment on the issue tracker about this also affecting "labelling and accesses". I did some digging into Objects/structseq.c and you're absolutely right—the root cause is that tp_members is constructed by omitting the unnamed fields, which breaks the index math (e.g. i - n_unnamed_fields) in structseq_reduce, structseq_new_impl, and structseq_replace. I have a complete fix prepared for all of those underlying access bugs that replaces the index math with an offset lookup. I will hold off on pushing that code until we reach consensus on what the repr format should be. Let me know how you'd like to proceed! |
First of all, I don't want to have a discussion with an LLM agent. If it's for translation, I'm ok, but I don't want an unedited agent answer, it's wasting my time. It's also not aligned with our AI policies: https://devguide.python.org/getting-started/ai-tools/#guidelines-for-using-ai-tools. I still think it's premature for a PR to be made so I'm clsoing it. In the future, don't rely on agents for discussion unless it's for translation or for understanding the codebase on your end or helping you write the code. |
Fixes #154387.
structseq_reprincorrectly assumed that all fields printed were sequentially named fields fromtp_members. When an unnamed field was encountered (like the timestamp float variants inos.stat_result), it would continue indexing intotp_membersarray, erroneously labeling unnamed field values with subsequent named field labels from the struct sequence definition.This PR fixes the bug by resolving the field name accurately based on the expected offset rather than the sequential index, correctly printing unnamed fields without labels and accurately matching named fields.
structseq_reprmislabels unnamedPyStructSequencefields (e.g.os.stat_resultslots 7–9) #154387