Skip to content

Commit

Permalink
fix: conversion of ipynb files with multiple definitions (#3458)
Browse files Browse the repository at this point in the history
The `NameTransformer` wasn't visiting node descendants, leading to
incomplete transformations.

Before, when remapping `x` to a local:

```python
x = 1
y = foo(x + 1)
```

was yielding

```python
x = 1
y = foo(x + 1)
```

With this change, it now yields

```python
_x = 1
y = foo(_x + 1)
```

This change also modifies an old test that was testing that the
`NameTransformer` respected scoping logic. But it never actually did, or
rather only did so because of a bug in which it wasn't visiting entire
subtrees. Nothing relies on the `NameTransformer` respecting scoping in
our codebase.
  • Loading branch information
akshayka authored Jan 16, 2025
1 parent 0cd1073 commit ede87f5
Show file tree
Hide file tree
Showing 3 changed files with 119 additions and 162 deletions.
10 changes: 10 additions & 0 deletions marimo/_ast/transformers.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,18 @@

class NameTransformer(ast.NodeTransformer):
def __init__(self, name_substitutions: dict[str, str]) -> None:
"""Remaps names in an AST.
Naively remaps all occurrences of names in an AST, given a substitution
dict mapping old names to new names. In particular does not take
scoping into account.
"""
self._name_substitutions = name_substitutions
self.made_changes = False
super().__init__()

def visit_Name(self, node: ast.Name) -> ast.Name:
self.generic_visit(node)
if node.id in self._name_substitutions:
self.made_changes = True
return ast.Name(
Expand All @@ -20,6 +27,7 @@ def visit_Name(self, node: ast.Name) -> ast.Name:
return node

def visit_FunctionDef(self, node: ast.FunctionDef) -> ast.FunctionDef:
self.generic_visit(node)
if node.name in self._name_substitutions:
self.made_changes = True
return ast.FunctionDef(
Expand All @@ -31,6 +39,7 @@ def visit_FunctionDef(self, node: ast.FunctionDef) -> ast.FunctionDef:
return node

def visit_ClassDef(self, node: ast.ClassDef) -> ast.ClassDef:
self.generic_visit(node)
if node.name in self._name_substitutions:
self.made_changes = True
return ast.ClassDef(
Expand All @@ -42,6 +51,7 @@ def visit_ClassDef(self, node: ast.ClassDef) -> ast.ClassDef:
return node

def visit_Assign(self, node: ast.Assign) -> ast.Assign:
self.generic_visit(node)
new_targets: list[Any] = []
for target in node.targets:
if (
Expand Down
9 changes: 6 additions & 3 deletions tests/_ast/test_transformers.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@
sys.version_info < (3, 9), reason="Feature not supported in python 3.8"
)
def test_name_transformer() -> None:
# Test code
# Name transformer should naively remap all occurrences of names in an AST,
# without taking scoping into account.
#
# It does not transform attributes.
code = """
def old_function():
old_variable = 42
Expand Down Expand Up @@ -47,8 +50,8 @@ def __init__(self):
# Expected transformed code
expected_code = """
def new_function():
old_variable = 42
return old_variable
new_variable = 42
return new_variable
class NewClass:
Expand Down
Loading

0 comments on commit ede87f5

Please sign in to comment.