Skip to content

Conversation

@FrankPortman
Copy link

@FrankPortman FrankPortman commented Sep 18, 2025

fixes: #323

@FrankPortman
Copy link
Author

@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
Copy link
Author

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!

@ethe ethe requested review from aisk, cocolato and ethe September 22, 2025 05:45
@cocolato
Copy link
Collaborator

cocolato commented Sep 22, 2025

setattr(child, '__thrift_module_name__', child_module_name)
setattr(thrift, child.__name__, child)
_add_thrift_meta('includes', child)
def p_seen_struct(p):
'''seen_struct : STRUCT IDENTIFIER '''
val = _make_empty_struct(p[2])
setattr(threadlocal.thrift_stack[-1], p[2], val)
p[0] = val
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.

@FrankPortman
Copy link
Author

FrankPortman commented Sep 22, 2025

setattr(child, '__thrift_module_name__', child_module_name)
setattr(thrift, child.__name__, child)
_add_thrift_meta('includes', child)

def p_seen_struct(p):
'''seen_struct : STRUCT IDENTIFIER '''
val = _make_empty_struct(p[2])
setattr(threadlocal.thrift_stack[-1], p[2], val)
p[0] = val

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.

Copy link
Member

@aisk aisk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to mark this as pending further discussion. Since we don’t have a mechanism to put a PR on hold, I’ve set it to Request Changes instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local struct shadows imported module

3 participants