Skip to content

fix: avoid double-mangling names inside walrus comprehension#9276

Merged
dmadisetti merged 2 commits intomarimo-team:mainfrom
nojaf:fix-walrus
Apr 20, 2026
Merged

fix: avoid double-mangling names inside walrus comprehension#9276
dmadisetti merged 2 commits intomarimo-team:mainfrom
nojaf:fix-walrus

Conversation

@nojaf
Copy link
Copy Markdown
Contributor

@nojaf nojaf commented Apr 20, 2026

📝 Summary

Closes #9274

Walrus expressions at module scope that hold a comprehension as their value were having names inside the comprehension mangled twice, producing errors like:

NameError: name '_cell_MJUe_cell_MJUe_values' is not defined

Root cause

In visit_NamedExpr (marimo/_ast/visitor.py), node.value was visited explicitly, and then — when the walrus was not inside a comprehension — self.generic_visit(node) was called, which visits all children of the node and therefore re-visited node.value. For a walrus whose value is a comprehension (e.g. _matches := {k for k, v in _values.items() if v > 1}), this entered the comprehension's scope twice. After #8762 extended private-name mangling to nested-scope lookups, the second visit re-mangled the already-mangled private name, producing the double prefix.

Fix

In the non-comprehension branch of visit_NamedExpr, only visit node.target — the value was already visited earlier in the method.

Reproduction (from the issue)

items = {"a": {"x": 1, "y": 2}, "b": {"x": 3, "y": 4}}
for _key, _values in items.items():
    if _matches := {k for k, v in _values.items() if v > 1}:
        pass

Before: NameError: name '_cell_XXX_cell_XXX_values' is not defined. After: runs successfully.

📋 Pre-Review Checklist

  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
  • Video or media evidence is provided for any visual changes (optional).

✅ Merge Checklist

  • I have read the contributor guidelines.
  • Documentation has been updated where applicable, including docstrings for API changes.
  • Tests have been added for the changes made.

(marimo-team#9274)

  When a walrus expression at module scope held a comprehension as its
  value, the comprehension was visited twice by visit_NamedExpr: once
  explicitly, and again via generic_visit. After marimo-team#8762 extended mangling
  to nested-scope lookups, the second visit remangled already-mangled
  private names, producing errors like
  `NameError: _cell_XXX_cell_XXX_values`.

  Only visit the target in the non-comprehension branch, since value has
  already been visited.
Copilot AI review requested due to automatic review settings April 20, 2026 14:49
@vercel
Copy link
Copy Markdown

vercel Bot commented Apr 20, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment Apr 20, 2026 3:23pm

Request Review

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a scoping/mangling bug in the AST ScopedVisitor where module-scope walrus expressions whose value is a comprehension could cause names inside that comprehension to be mangled twice (regression for #9274).

Changes:

  • Adjust visit_NamedExpr traversal to avoid re-visiting node.value (and thus re-entering comprehension scope).
  • Add a regression test to ensure comprehension-in-walrus values do not produce double-mangled names.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
marimo/_ast/visitor.py Prevents double traversal of walrus value by visiting only target in the non-comprehension branch.
tests/_ast/test_visitor.py Adds regression coverage to ensure names in comprehension values are mangled exactly once.

@manzt
Copy link
Copy Markdown
Collaborator

manzt commented Apr 20, 2026

Thank you for fixing! I also put up the same fix in #9277. Would you mind copying over the other parameterized regression test from that PR and we can get yours merged? Added in 93dcbaf.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Bundle Report

Changes will increase total bundle size by 225.07kB (0.9%) ⬆️. This is within the configured threshold ✅

Detailed changes
Bundle name Size Change
marimo-esm 25.1MB 225.07kB (0.9%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: marimo-esm

Assets Changed:

Asset Name Size Change Total Size Change (%)
assets/cells-*.js 59 bytes 704.0kB 0.01%
assets/index-*.js 3.6kB 605.9kB 0.6%
assets/index-*.css 386 bytes 362.67kB 0.11%
assets/JsonOutput-*.js 2.15kB 342.27kB 0.63%
assets/edit-*.js 1 bytes 329.61kB 0.0%
assets/add-*.js 7 bytes 192.76kB 0.0%
assets/layout-*.js -311 bytes 185.6kB -0.17%
assets/cell-*.js 3 bytes 183.15kB 0.0%
assets/reveal-*.js (New) 165.6kB 165.6kB 100.0% 🚀
assets/swiper-*.js (New) 116.4kB 116.4kB 100.0% 🚀
assets/reveal-*.css (New) 54.08kB 54.08kB 100.0% 🚀
assets/file-*.js 50 bytes 49.4kB 0.1%
assets/panels-*.js 6 bytes 45.36kB 0.01%
assets/session-*.js -9 bytes 24.99kB -0.04%
assets/state-*.js 46 bytes 24.45kB 0.19%
assets/home-*.js 132 bytes 21.86kB 0.61%
assets/purify.es-*.js 16 bytes 21.05kB 0.08%
assets/swiper-*.css (New) 15.12kB 15.12kB 100.0% 🚀
assets/run-*.js -735 bytes 9.74kB -7.02%
assets/column-*.js -659 bytes 6.53kB -9.16%
assets/mermaid-*.core-B0EhnNjP.js (New) 2.38kB 2.38kB 100.0% 🚀
assets/slide-*.js (New) 615 bytes 615 bytes 100.0% 🚀
assets/slides-*.css -15.1kB 305 bytes -98.02%
assets/slides-*.js -116.37kB 0 bytes -100.0%
assets/mermaid-*.core-Blvy2Lmj.js (Deleted) -2.38kB 0 bytes -100.0% 🗑️

The bug in marimo-team#9274 is not specific to set comprehensions — any nested scope
inside a walrus value (list/gen/dict comps, lambda) trips the double
visit in visit_NamedExpr. Add a parametrized regression test so future
changes to named-expression handling surface the broader contract.
@manzt
Copy link
Copy Markdown
Collaborator

manzt commented Apr 20, 2026

Added the additional regression test from #9277

@nojaf
Copy link
Copy Markdown
Contributor Author

nojaf commented Apr 20, 2026

Thanks @manzt !

@manzt
Copy link
Copy Markdown
Collaborator

manzt commented Apr 20, 2026

thank you!

Copy link
Copy Markdown
Collaborator

@dmadisetti dmadisetti left a comment

Choose a reason for hiding this comment

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

CI is flaky, but the runtime and AST tests passed. Thanks @nojaf !

@dmadisetti dmadisetti merged commit c209752 into marimo-team:main Apr 20, 2026
27 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Walrus operator + list comprehensions mean names get mangled twice

4 participants