fix: consistent mangling for private imports#9309
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dmadisetti
changed the title
for more information, see https://pre-commit.ci
mscolnick
left a comment
mscolnick
left a comment
There was a problem hiding this comment.
can we also test renaming the private import?
| # Previously we did not mangle. So this is technically breaking | ||
| # from 0.22.0 (#8762) but now consistent with private naming | ||
| # conventions. | ||
| mangled = self._if_local_then_mangle(basename) |
There was a problem hiding this comment.
why did this require actual code changes? I thought it was just to add a verification test?
There was a problem hiding this comment.
The current implementation is broken, The test I put in caught the edge case where a scoped import no longer works.
There was a problem hiding this comment.
got it, maybe worth thinking through some other edge cases then (lazy imports etc)
There was a problem hiding this comment.
Pull request overview
Fixes a regression introduced by private-name mangling changes (#8762) where underscore-prefixed imports referenced within scope (e.g., inside nested functions) could be mis-addressed, leading to runtime NameErrors. The PR updates the AST visitor’s import-alias handling and adds a runtime regression test to validate the intended mangling behavior.
Changes:
- Update
_get_alias_name()to mangle underscore-prefixed imports even when no explicitasalias is provided (by rewriting the AST alias name). - Add a runtime test covering underscore-prefixed
from ... import ...usage and ensuring such imports remain private and don’t leak into globals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/_runtime/test_runtime.py | Adds a regression test verifying behavior for underscore-prefixed imports across cells/scopes. |
| marimo/_ast/visitor.py | Changes AST import alias resolution to apply private-name mangling for non-aliased imports. |
akshayka
left a comment
akshayka
left a comment
There was a problem hiding this comment.
Can you make the PR description more specific?
and the edge case was not correctly handled.
Which edge case?
The change lead to a bug where imports referenced in scope were not correctly addressed. This PR handles the mangling behavior for these import cases.
What do you mean by not correctly addressed? How does this PR fix it?
| if mangled != basename: | ||
| node.asname = mangled |
There was a problem hiding this comment.
I don't understand this branch. Isn't node.asname None? Why are we assigning the mangled basename to it? Should it be name.name = mangled?
There was a problem hiding this comment.
We can't rename a module but:
import _private as _mangled_private is fine
I do think I'm missing the
import _private.submodule
case though. Will revise and add a comment.
@dylan can you address these comments? This PR is subtle enough that I would like the intended behavior to be very clear. |
|
Closing out for simpler method |
📝 Summary
A scoping change introduced in #8762 changed
_prefixed imports by turning then into privates.While this is consistent with the remainder of marimo's private pattern, it is technically a breaking change as of 0.22.0, and the edge case was not correctly handled. The change lead to a bug where imports referenced in scope were not correctly addressed. This PR handles the mangling behavior for these import cases.
An alternative PR, for retaining the behavior, is here: #9308 but this requires ref tracking a little differently such that the
_pattern is properly placed into globals.closes #9151