-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(lint): warn on private state capture #9411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| # Copyright 2026 Marimo. All rights reserved. | ||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| from marimo._ast.variables import is_mangled_local, unmangle_local | ||
| from marimo._lint.diagnostic import Diagnostic, Severity | ||
| from marimo._lint.rules.breaking.graph import GraphRule | ||
|
|
||
| if TYPE_CHECKING: | ||
| from marimo._lint.context import RuleContext | ||
| from marimo._runtime.dataflow import DirectedGraph | ||
|
|
||
|
|
||
| class PrivateStateCaptureRule(GraphRule): | ||
| """MR004: Top-level functions/classes capture private cell-local state. | ||
|
|
||
| This rule warns when a top-level function or class closes over a private | ||
| variable from the same cell. Private variables are intentionally excluded | ||
| from the notebook dependency graph, so mutating them from a reusable | ||
| definition can make behavior depend on execution order. | ||
|
|
||
| ## Why is this bad? | ||
|
|
||
| A function that closes over private cell state can appear pure while still | ||
| hiding mutable state. When that function is called from different cells, | ||
| the observed result may depend on which cells ran first. | ||
|
|
||
| ## Example | ||
|
|
||
| ```python | ||
| _cache = {} | ||
|
|
||
|
|
||
| def square(x): | ||
| if x in _cache: | ||
| return _cache[x] + 1 | ||
| _cache[x] = x * x | ||
| return _cache[x] | ||
| ``` | ||
| """ | ||
|
|
||
| code = "MR004" | ||
| name = "private-state-capture" | ||
| description = "Top-level definitions capture private cell-local state" | ||
| severity = Severity.RUNTIME | ||
| fixable = False | ||
|
|
||
| async def _validate_graph( | ||
| self, graph: DirectedGraph, ctx: RuleContext | ||
| ) -> None: | ||
| for cell_id, cell in graph.cells.items(): | ||
| for ( | ||
| variable_name, | ||
| variable_data_list, | ||
| ) in cell.variable_data.items(): | ||
| for variable_data in variable_data_list: | ||
| if variable_data.kind not in {"function", "class"}: | ||
| continue | ||
|
|
||
| private_refs = sorted( | ||
| { | ||
| unmangle_local(ref, cell_id).name | ||
| for ref in variable_data.required_refs | ||
| if is_mangled_local(ref, cell_id) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Exclude self-references from Prompt for AI agents |
||
| } | ||
| ) | ||
| if not private_refs: | ||
| continue | ||
|
|
||
| line, column = self._get_variable_line_info( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Use unmangled definition names for diagnostics; current output can leak internal mangled names and degrade line/column accuracy for private defs. Prompt for AI agents |
||
| cell_id, variable_name, ctx | ||
| ) | ||
| kind = ( | ||
| "Function" | ||
| if variable_data.kind == "function" | ||
| else "Class" | ||
| ) | ||
| refs = ", ".join(f"`{ref}`" for ref in private_refs) | ||
| await ctx.add_diagnostic( | ||
| Diagnostic( | ||
| message=( | ||
| f"{kind} '{variable_name}' captures private " | ||
| f"cell-local variable(s): {refs}" | ||
| ), | ||
| cell_id=[cell_id], | ||
| line=line, | ||
| column=column, | ||
| code=self.code, | ||
| name=self.name, | ||
| severity=self.severity, | ||
| fixable=self.fixable, | ||
| fix=( | ||
| "Use explicit cell outputs for shared state, or " | ||
| "use @mo.cache for memoized values." | ||
| ), | ||
| ) | ||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,7 +101,9 @@ class UIElement(Html, Generic[S, T]): | |
|
|
||
| **Attributes.** | ||
|
|
||
| - value: The value of the `UIElement`. | ||
| - value: The current value of the `UIElement`. Read-only; it reflects | ||
| frontend state and can't be assigned directly. If you need to | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you revert these doc changes? |
||
| imperatively drive UI state, use `mo.state()`. | ||
|
|
||
| **Methods.** | ||
|
|
||
|
|
@@ -310,7 +312,12 @@ def _register_as_view(self, parent: UIElement[Any, Any], key: str) -> None: | |
|
|
||
| @property | ||
| def value(self) -> T: | ||
| """The element's current value.""" | ||
| """The element's current value. | ||
|
|
||
| Read-only; marimo updates it when the UI element changes in the | ||
| frontend. If you need to imperatively drive UI state, use | ||
| `mo.state()` instead of assigning to this property. | ||
| """ | ||
| if self._ctx is None: | ||
| return self._value | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from pathlib import Path | ||
|
|
||
| from marimo._lint import run_check | ||
|
|
||
|
|
||
| def test_private_state_capture_warns_on_captured_cache(tmp_path) -> None: | ||
| notebook_file = Path(tmp_path) / "cached.py" | ||
| notebook_file.write_text( | ||
| """import marimo | ||
|
|
||
| __generated_with = "0.23.2" | ||
| app = marimo.App() | ||
|
|
||
|
|
||
| @app.cell | ||
| def __(): | ||
| _cache = dict() | ||
|
|
||
| def square(x): | ||
| if x in _cache: | ||
| return _cache[x] + 1 | ||
| res = x * x | ||
| _cache[x] = res | ||
| return res | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't an issue if it's called within the cell |
||
|
|
||
| return (square,) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| app.run() | ||
| """ | ||
| ) | ||
|
|
||
| linter = run_check((str(notebook_file),), formatter="json") | ||
| result = linter.get_json_result() | ||
|
|
||
| assert result["summary"]["files_with_issues"] == 1 | ||
| issue = result["issues"][0] | ||
| assert issue["code"] == "MR004" | ||
| assert issue["severity"] == "runtime" | ||
| assert "private cell-local variable" in issue["message"] | ||
| assert "square" in issue["message"] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this is fixated on the specific report but not really addressing the general issue. I think that this in a single cell is actually fine.
Title should probably be:
MR004: Cross cell variable mutation.Here are things that the rule should catch:
Or mutation cross cell from closures