thriftpy2 icon indicating copy to clipboard operation
thriftpy2 copied to clipboard

fix: local struct shadows included module

Open FrankPortman opened this issue 3 months ago • 4 comments

fixes: https://github.com/Thriftpy/thriftpy2/issues/323

FrankPortman avatar Sep 18 '25 17:09 FrankPortman

@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 🙏

FrankPortman avatar Sep 18 '25 18:09 FrankPortman

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!

FrankPortman avatar Sep 21 '25 20:09 FrankPortman

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.

cocolato avatar Sep 22 '25 09:09 cocolato

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.

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.

FrankPortman avatar Sep 22 '25 16:09 FrankPortman