fix: local struct shadows included module
fixes: https://github.com/Thriftpy/thriftpy2/issues/323
@aisk not sure if you are the current maintainer but I see you have the most commits in the last half year so please take a look when you can 🙏
Friendly bump - @cocolato tagging you as well since I see some recent activity from you.
Some initial direction to this PR or the underlying issue would be great!
https://github.com/Thriftpy/thriftpy2/blob/8e82f1c091a9d9db30dc01af25431d06056c3d88/thriftpy2/parser/parser.py#L82-L84 https://github.com/Thriftpy/thriftpy2/blob/8e82f1c091a9d9db30dc01af25431d06056c3d88/thriftpy2/parser/parser.py#L244-L248 The root cause appears to be that the p_seen_struct method overwrites the contents of the originally imported module via setattr. Although the current fix resolves this issue, I suggest using a prefix with setattr to better distinguish between the imported module and the struct. @aisk please take a look.
https://github.com/Thriftpy/thriftpy2/blob/8e82f1c091a9d9db30dc01af25431d06056c3d88/thriftpy2/parser/parser.py#L82-L84
https://github.com/Thriftpy/thriftpy2/blob/8e82f1c091a9d9db30dc01af25431d06056c3d88/thriftpy2/parser/parser.py#L244-L248
The root cause appears to be that the
p_seen_structmethod overwrites the contents of the originally imported module viasetattr. Although the current fix resolves this issue, I suggest using a prefix with setattr to better distinguish between the imported module and the struct. @aisk please take a look.
Thank you for reviewing! You're correct that p_seen_struct overwrites the imported module - that's the root cause. However, using prefixes would be a breaking change since all existing code expects direct attribute access (e.g., thrift.MyStruct).
The current fix preserves backward compatibility by using the already-available __thrift_meta__['includes'] to correctly resolve qualified names when there's a collision. This follows the existing pattern where includes are already tracked separately in metadata.
If you'd prefer a different approach, I'm happy to adjust, but I believe this is the least disruptive fix for users.
Caveat - I am obviously very new to this project so it is possible I missed something in your recommendation. At the end of the day, any approach that fixes #323, either with this PR or a different PR, would be fine for me.