fix: mo.cache returning stale values#8411
Conversation
The RemoveReturns AST transformer was converting 'return expr' to
just 'expr' (expression statement). Python's compiler constant-folds numeric
expressions ('11 + 19' becomes '30') and since the result is unused (expression statement), it optimizes
them away entirely. This caused different numeric expressions to compile to
identical bytecode, producing identical hashes.
Changed RemoveReturns to convert 'return expr' to '_ = expr' (assignment
statement) instead of just 'expr'. This preserves the constant values in the
compiled bytecode, ensuring different expressions produce different hashes.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical cache invalidation bug where @mo.cache was returning stale values when decorated function bodies changed, specifically for numeric return types. The root cause was that Python's compiler optimizes away unused expression statements after constant-folding (e.g., 11 + 19 becomes 30, but as an unused expression it gets removed), resulting in identical bytecode hashes for different numeric expressions.
Changes:
- Modified the
RemoveReturnsAST transformer to convertreturn exprinto_ = expr(assignment statement) instead of justexpr(expression statement), ensuring constant values are preserved in the compiled bytecode - Added comprehensive test coverage for cache invalidation with numeric (int), string, and float return types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| marimo/_ast/transformers.py | Changed RemoveReturns transformer to use assignment statements instead of expression statements to preserve constants in bytecode |
| tests/_save/test_cache_invalidation.py | Added new test suite for cache invalidation with int, string, and float return types |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
marimo/_ast/transformers.py
Outdated
| expr.col_offset = node.col_offset | ||
| return expr | ||
| def visit_Return(self, node: ast.Return) -> ast.Assign: | ||
| # Convert "return expr" to "_ = expr" to preserve constant values in bytecode. |
There was a problem hiding this comment.
Nice approach!
I think this is fairly cache-busting for many, can we check for particular statement forms to lower the chance of existing invalidation?
… robust test cases
The RemoveReturns AST transformer was converting 'return expr' to just 'expr' (expression statement). Python's compiler constant-folds numeric expressions ('11 + 19' becomes '30') and since the result is unused (expression statement), it optimizes them away entirely. This caused different numeric expressions to compile to identical bytecode, producing identical hashes.
Changed RemoveReturns to convert 'return expr' to '_ = expr' (assignment statement) instead of just 'expr'. This preserves the constant values in the compiled bytecode, ensuring different expressions produce different hashes.
Fixes #8364