Skip to content
Prev Previous commit
Next Next commit
comment: fix examples and allow common side effects
  • Loading branch information
dmadisetti committed Nov 24, 2025
commit cc0d8dbd55c6838827d1952cd96fb8e9538c69c1
2 changes: 1 addition & 1 deletion docs/guides/lint_rules/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ These issues may cause runtime problems.
| Code | Name | Description | Fixable |
|------|------|-------------|----------|
| [MR001](rules/self_import.md) | self-import | Importing a module with the same name as the file | ❌ |
| [MR002](rules/branch_expression.md) | branch-expression | Branch statements with trailing expressions that won't be displayed | ❌ |
| [MR002](rules/branch_expression.md) | branch-expression | Branch statements with output expressions that won't be displayed | ❌ |

### ✨ Formatting Rules

Expand Down
22 changes: 11 additions & 11 deletions docs/guides/lint_rules/rules/branch_expression.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

⚠️ **Runtime** ❌ Not Fixable

MR002: Branch statements ending with expressions that won't be displayed.
MR002: Branch statements with output expressions that won't be displayed.

## Why is this bad?

When expressions are nested inside branches at the end of a cell:
When output expressions are nested inside branches at the end of a cell:
- The expressions execute but produce no visible output
- Users may expect to see the result (like `mo.md()` calls) but won't
- Users expect to see the result (like mo.md(), string literals, etc.)
- This can lead to confusion about whether code is running correctly
- It violates the principle of least surprise

Expand All @@ -34,10 +34,12 @@ match value:
"Just right" # Won't be displayed
```

**Problematic:**
**Not flagged (side effects only):**
```python
if invalid:
mo.md("Error message") # Won't be displayed even without else clause
if condition:
print("Debug message") # Side effect only, not flagged
else:
logger.info("Something") # Side effect only, not flagged
```

**Solution:**
Expand All @@ -58,15 +60,13 @@ else:
result
```

In the case where no output is expected:

**Alternative Solution:**
**Alternative Solution (if no output intended):**
```python
# Use a dummy variable to indicate intentional suppression
if condition:
_ = print("Result A")
_ = expr()
else:
_ = print("Result B")
_ = other()
```

## References
Expand Down
6 changes: 2 additions & 4 deletions examples/ai/chat/streaming_openai.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,16 +95,14 @@ def _(mo):
@app.cell
def _(chatbot, mo):
# Show chat history
if hasattr(chatbot, "value"):
mo.md(f"**Chat history:** {len(chatbot.value)} messages")
mo.md(f"**Chat history:** {len(chatbot.value)} messages") if hasattr(chatbot, "value") else None
return


@app.cell
def _(chatbot):
# Display full history
if hasattr(chatbot, "value"):
chatbot.value
chatbot.value if hasattr(chatbot, "value") else None
return


Expand Down
12 changes: 6 additions & 6 deletions examples/outputs/conditional_output.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,18 +34,18 @@ def _(checkbox):
@app.cell(hide_code=True)
def _(mo):
mo.md("""
The next cell does **not** show anything, since an if statement does not
The folling cell would **not** show anything, since an if statement does not
have a value:
""")
return


@app.cell
def _(checkbox, mo):
```python
# Intentionally demonstrates that if statements don't display expressions
# Using _ to suppress the lint warning while keeping the example
if checkbox.value:
mo.md("Checkbox is checked")
else:
mo.md("Checkbox is not checked")
```
""")
return


Expand Down
2 changes: 1 addition & 1 deletion examples/sql/misc/database_explorer.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def _(mo, os):
@app.cell
def _(database_url, duckdb):
if database_url.value:
duckdb.sql(
_ = duckdb.sql(
f"""
INSTALL postgres;
LOAD postgres;
Expand Down
2 changes: 1 addition & 1 deletion examples/third_party/cvxpy/signals/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ def __init__(self, sticky_bool):
[complib.Components.TREND_LINE, complib.Components.PERIODIC]
)
if solved.now:
solved_ever.set()
_ = solved_ever.set()
return (solved,)


Expand Down
127 changes: 99 additions & 28 deletions marimo/_lint/rules/runtime/branch_expression.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,19 @@


class BranchExpressionRule(LintRule):
"""MR002: Branch statements ending with expressions that won't be displayed.
"""MR002: Branch statements with output expressions that won't be displayed.

This rule detects when the last statement in a cell is a branch (if/elif/else,
match, or try/except/finally) where all branches end with expressions. In marimo,
only the last expression at the top level of a cell is displayed. Expressions
nested inside branches will execute but not be shown to the user, which can be
confusing if the user expected to see output.
match, or try/except/finally) containing expressions that produce output. In
marimo, only the last expression at the top level of a cell is displayed.
Expressions nested inside branches will execute but not be shown to the user,
which can be confusing if the user expected to see output.

## Why is this bad?

When expressions are nested inside branches at the end of a cell:
When output expressions are nested inside branches at the end of a cell:
- The expressions execute but produce no visible output
- Users may expect to see the result (like `mo.md()` calls) but won't
- Users expect to see the result (like mo.md(), string literals, etc.)
- This can lead to confusion about whether code is running correctly
- It violates the principle of least surprise

Expand All @@ -52,10 +52,12 @@ class BranchExpressionRule(LintRule):
"Just right" # Won't be displayed
```

**Problematic:**
**Not flagged (side effects only):**
```python
if invalid:
mo.md("Error message") # Won't be displayed even without else clause
if condition:
print("Debug message") # Side effect only, not flagged
else:
logger.info("Something") # Side effect only, not flagged
```

**Solution:**
Expand All @@ -76,15 +78,13 @@ class BranchExpressionRule(LintRule):
result
```

In the case where no output is expected:

**Alternative Solution:**
**Alternative Solution (if no output intended):**
```python
# Use a dummy variable to indicate intentional suppression
if condition:
_ = print("Result A")
_ = expr()
else:
_ = print("Result B")
_ = other()
```

## References
Expand All @@ -96,7 +96,7 @@ class BranchExpressionRule(LintRule):
code = "MR002"
name = "branch-expression"
description = (
"Branch statements with trailing expressions that won't be displayed"
"Branch statements with output expressions that won't be displayed"
)
severity = Severity.RUNTIME
fixable = False
Expand Down Expand Up @@ -133,33 +133,33 @@ async def check(self, ctx: RuleContext) -> None:
async def _check_if_statement(
self, node: ast.If, cell: CellDef, ctx: RuleContext
) -> None:
"""Check if an if statement has all branches ending with expressions."""
"""Check if an if statement has branches with output expressions."""
branches = self._collect_if_branches(node)

# Check if ALL branches end with expressions
# Check if ANY branch has an output expression
if not branches:
return

if all(self._ends_with_expression(branch) for branch in branches):
if any(self._has_output_expression(branch) for branch in branches):
await self._report_diagnostic(node, cell, ctx, "if statement")

async def _check_match_statement(
self, node: ast.Match, cell: CellDef, ctx: RuleContext
) -> None:
"""Check if a match statement has all cases ending with expressions."""
"""Check if a match statement has cases with output expressions."""
if not node.cases:
return

branches = [case.body for case in node.cases]

# Check if ALL cases end with expressions
if all(self._ends_with_expression(branch) for branch in branches):
# Check if ANY case has an output expression
if any(self._has_output_expression(branch) for branch in branches):
await self._report_diagnostic(node, cell, ctx, "match statement")

async def _check_try_statement(
self, node: ast.Try, cell: CellDef, ctx: RuleContext
) -> None:
"""Check if a try statement has all branches ending with expressions."""
"""Check if a try statement has branches with output expressions."""
branches = [node.body]

# Add exception handlers
Expand All @@ -174,8 +174,8 @@ async def _check_try_statement(
if node.finalbody:
branches.append(node.finalbody)

# Check if ALL branches end with expressions
if all(self._ends_with_expression(branch) for branch in branches):
# Check if ANY branch has an output expression
if any(self._has_output_expression(branch) for branch in branches):
await self._report_diagnostic(node, cell, ctx, "try statement")

def _collect_if_branches(self, node: ast.If) -> list[list[ast.stmt]]:
Expand All @@ -198,13 +198,84 @@ def _collect_if_branches(self, node: ast.If) -> list[list[ast.stmt]]:

return branches

def _ends_with_expression(self, stmts: list[ast.stmt]) -> bool:
"""Check if a list of statements ends with an expression statement."""
def _has_output_expression(self, stmts: list[ast.stmt]) -> bool:
"""Check if a branch has a trailing expression that produces output.

Returns True for any expression EXCEPT known side-effect-only calls
(print, logger.*, sys.*, mo.stop, mo.output.*).
"""
if not stmts:
return False

last_stmt = stmts[-1]
return isinstance(last_stmt, ast.Expr)
if not isinstance(last_stmt, ast.Expr):
return False

# Check if this is a side-effect call that shouldn't be flagged
return not self._is_side_effect_only(last_stmt.value)

def _is_side_effect_only(self, node: ast.expr) -> bool:
"""Check if an expression is a side-effect-only call.

Returns True for:
- print(), input(), exec(), eval()
- logger.*/log.*/logging.* calls
- sys.stdout.write(), sys.stderr.write()
- mo.stop()
- mo.output.append/replace/clear()

Returns False for everything else (including mo.md(), string literals, etc.)
"""
if not isinstance(node, ast.Call):
return False

func = node.func

# Builtin side-effect functions
if isinstance(func, ast.Name) and func.id in {
"print",
"input",
"exec",
"eval",
}:
return True

if isinstance(func, ast.Attribute):
# mo.stop()
if (
isinstance(func.value, ast.Name)
and func.value.id == "mo"
and func.attr == "stop"
):
return True

# mo.output.*
if isinstance(func.value, ast.Attribute):
if (
isinstance(func.value.value, ast.Name)
and func.value.value.id == "mo"
and func.value.attr == "output"
):
return True

# logger.*, log.*, logging.*
if isinstance(func.value, ast.Name) and func.value.id in {
"logger",
"log",
"logging",
}:
return True

# sys.stdout.*, sys.stderr.*
if isinstance(func.value, ast.Attribute):
if (
isinstance(func.value.value, ast.Name)
and func.value.value.id == "sys"
and func.value.attr in {"stdout", "stderr"}
):
return True

return False

async def _report_diagnostic(
self, node: ast.stmt, cell: CellDef, ctx: RuleContext, stmt_type: str
Expand Down
Loading