Skip to content

Fix forward function references at module scope#263

Closed
wsenn wants to merge 3 commits into
pydantic:mainfrom
wsenn:fix/forward-function-references
Closed

Fix forward function references at module scope#263
wsenn wants to merge 3 commits into
pydantic:mainfrom
wsenn:fix/forward-function-references

Conversation

@wsenn
Copy link
Copy Markdown

@wsenn wsenn commented Mar 10, 2026

Closes #183

Summary

  • Pre-scan all module-level names before the main prepare pass so function bodies can resolve forward references
  • In CPython, names inside function bodies are resolved at call time (via dictionary lookup), so def foo(): return bar() works even when bar is defined after foo
  • Monty's prepare phase was snapshotting the global name_map when processing each function def, but the snapshot only contained names processed so far — causing forward references to fail with NameError

Approach

Added a pre_scan_module_names method that reuses the existing collect_scope_info_from_node helper to collect all names that will be assigned at module level, then pre-allocates namespace slots for each. This runs before prepare_nodes, ensuring the global_name_map snapshot passed to each function body is complete.

By reusing collect_scope_info_from_node, the implementation:

  • Avoids duplicating the node traversal logic (~25 lines vs ~100 in the initial version)
  • Correctly handles walrus expressions, unpack targets, and control flow recursion
  • Correctly skips SubscriptAssign/SubscriptOpAssign (which modify containers, not create new names)
  • Stays in sync automatically when new node types are added

Test plan

  • Added 5 test cases to function__ops.py: basic forward ref, chained forward refs, forward ref with arguments, forward ref to global variable, mutual recursion
  • All 887 existing test cases pass (1 pre-existing failure in math__module.py unrelated to this change)
  • ref-count-panic tests pass with no leaks
  • cargo clippy clean

In CPython, names inside function bodies are resolved at call time,
so `def foo(): return bar()` works even when `bar` is defined after
`foo`. Monty's prepare phase takes a snapshot of the global name_map
when processing each function def, but this snapshot only contained
names processed so far — causing forward references to fail with
NameError.

Fix: add a pre-scan pass that collects all module-level names before
the main prepare pass, ensuring the global_name_map is complete when
any function body is prepared. Recurses into control flow blocks
(if/for/while/try) since Python has no block scoping.
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 10, 2026

Merging this PR will not alter performance

✅ 13 untouched benchmarks


Comparing wsenn:fix/forward-function-references (3aa3ea7) with main (e59c8fa)

Open in CodSpeed

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 10, 2026

Codecov Report

❌ Patch coverage is 41.25000% with 47 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/monty/src/prepare.rs 41.25% 17 Missing and 30 partials ⚠️

📢 Thoughts on this report? Let us know!

wsenn added 2 commits March 10, 2026 17:40
Replace hand-rolled AST traversal with the existing
collect_scope_info_from_node helper. This:
- Eliminates structural duplication (106 → 25 lines)
- Fixes incorrect SubscriptAssign/SubscriptOpAssign registration
- Adds walrus expression support for free
- Avoids unnecessary to_string() allocation via contains_key(&str)
@rsr5
Copy link
Copy Markdown
Contributor

rsr5 commented Apr 19, 2026

I created this issue (#183) and I've reviewed the approach. It looks clean and correct. The pre-scan reuses existing collect_scope_info_from_node infrastructure, keeps the change minimal, and the tests cover the important cases (chained refs, mutual recursion, etc.). Would be great to get this merged.

@davidhewitt would you be able to take a look please?

@rsr5
Copy link
Copy Markdown
Contributor

rsr5 commented Apr 19, 2026

Thanks for picking this up @wsenn!

@davidhewitt
Copy link
Copy Markdown
Collaborator

Thanks for the PR @wsenn and the ping @rsr5.

I have taken a look and I think this solves part of the problem but not the full picture when interacting with builtins. e.g. cpython handles the following code fine, but neither monty with this patch nor without works properly.

def call_sum():
    # this will resolve as the builtin sum until the `def sum` later on
    return sum([1, 2, 3])

# with this patch, monty fails on this assert with `NameError`
assert call_sum() == 6


def sum(*args):
    return 42

# previously, monty would fail on this assert because it would still call the builtin `sum`
assert call_sum() == 42

@davidhewitt
Copy link
Copy Markdown
Collaborator

I will explore possible solutions and either send review feedback to this PR, push to this PR, or push a new PR, whichever I think is easiest.

@davidhewitt
Copy link
Copy Markdown
Collaborator

I opened #469 to address this case with a more comprehensive set of adjustment. Thanks again both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forward function references at module scope cause NameError

3 participants