Skip to content

Align symtable flag names with CPython#8184

Open
ShaharNaveh wants to merge 12 commits into
RustPython:mainfrom
ShaharNaveh:symtable-symbolflag-names
Open

Align symtable flag names with CPython#8184
ShaharNaveh wants to merge 12 commits into
RustPython:mainfrom
ShaharNaveh:symtable-symbolflag-names

Conversation

@ShaharNaveh

@ShaharNaveh ShaharNaveh commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Summary by CodeRabbit

  • Bug Fixes
    • Improved symbol tracking so imported names, parameters, globals, nonlocals, and comprehension variables are recognized more reliably.
    • Fixed several scope-related cases, including free-class handling and nested comprehension behavior.
    • Updated the Python symbol table API so flags and symbol properties report more consistent results.

@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

This PR renames symbol-table flags to the new DEF_*/USE scheme across codegen, symbol analysis, and Python symtable bindings. It updates comprehension, free-class, global/nonlocal, parameter, and annotation handling, plus matching tests and exported constants.

Changes

Symbol flag migration across analyzer, codegen, and symtable API

Layer / File(s) Summary
Flag contract and exported predicates
crates/codegen/src/symboltable.rs, crates/vm/src/stdlib/_symtable.rs
SymbolFlags constants and Symbol predicates now use the new DEF_*/USE bits, and DEF_BOUND is derived from those updated flags.
Synthesized symbols and closures
crates/codegen/src/symboltable.rs
.format symbols, comprehension inlining, free-class propagation, and related tests now use DEF_PARAM, DEF_COMP_CELL, USE, and DEF_FREE_CLASS.
Global and nonlocal registration
crates/codegen/src/symboltable.rs
analyze_symbol(), extend_namedexpr_scope(), and register_name() now validate and assign DEF_GLOBAL, DEF_NONLOCAL, DEF_LOCAL, DEF_PARAM, and DEF_TYPE_PARAM.
Compile-time flag consumers
crates/codegen/src/compile.rs
Compile-time symbol checks now read DEF_IMPORT, DEF_FREE_CLASS, DEF_COMP_CELL, DEF_GLOBAL, DEF_PARAM, and DEF_LOCAL in import, scope, and local-slot logic.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • RustPython/RustPython#7631: Overlaps on comprehension inlining and COMP_CELL/free-variable handling in crates/codegen/src/symboltable.rs.

Suggested reviewers

  • youknowone

Poem

🐇 I hopped through bits from old to new,
DEF_flags sparkled in the dew.
Comprehensions twitched, then ran quite free,
And globals bowed with symmetry.
A soft nose-nudge: hop, hop, hooray!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 76.92% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: renaming symtable flags to CPython-aligned names.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] lib: cpython/Lib/symtable.py
[x] test: cpython/Lib/test/test_symtable.py (TODO: 11)

dependencies:

  • symtable

dependent tests: (2 tests)

  • symtable: test_inspect test_symtable

Legend:

  • [+] path exists in CPython
  • [x] up-to-date, [ ] outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/codegen/src/symboltable.rs`:
- Around line 311-317: `SymbolFlags::DEF_BOUND` is currently mixing in the
RustPython-only `ITER` bit, which leaks into the public `_symtable.DEF_BOUND`
export and breaks CPython compatibility. Update the `DEF_BOUND` definition in
`symboltable.rs` to include only the CPython-aligned flags, and keep `ITER`
restricted to internal bound-check logic elsewhere in the symbol table handling.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 4c981d47-0be7-4e02-802a-f6f0517ef402

📥 Commits

Reviewing files that changed from the base of the PR and between 0009dd6 and 560b9cb.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_symtable.py is excluded by !Lib/**
📒 Files selected for processing (3)
  • crates/codegen/src/compile.rs
  • crates/codegen/src/symboltable.rs
  • crates/vm/src/stdlib/_symtable.rs

Comment on lines +311 to +317
const DEF_BOUND = (
Self::DEF_LOCAL.bits()
| Self::DEF_PARAM.bits()
| Self::DEF_IMPORT.bits()
| Self::ITER.bits()
| Self::DEF_TYPE_PARAM.bits()
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Keep RustPython-only ITER out of the CPython DEF_BOUND mask.

Line 315 folds the internal ITER bit into DEF_BOUND, and _symtable now exports SymbolFlags::DEF_BOUND.bits() directly. That makes the public _symtable.DEF_BOUND value include a RustPython-specific flag, breaking the CPython-aligned flag contract. Keep DEF_BOUND CPython-compatible and include ITER only in internal bound checks.

Suggested fix
         const DEF_BOUND = (
             Self::DEF_LOCAL.bits()
             | Self::DEF_PARAM.bits()
             | Self::DEF_IMPORT.bits()
-            | Self::ITER.bits()
             | Self::DEF_TYPE_PARAM.bits()
         );
     pub const fn is_bound(&self) -> bool {
-        self.flags.intersects(SymbolFlags::DEF_BOUND)
+        self.flags
+            .intersects(SymbolFlags::DEF_BOUND | SymbolFlags::ITER)
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const DEF_BOUND = (
Self::DEF_LOCAL.bits()
| Self::DEF_PARAM.bits()
| Self::DEF_IMPORT.bits()
| Self::ITER.bits()
| Self::DEF_TYPE_PARAM.bits()
);
const DEF_BOUND = (
Self::DEF_LOCAL.bits()
| Self::DEF_PARAM.bits()
| Self::DEF_IMPORT.bits()
| Self::DEF_TYPE_PARAM.bits()
);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/codegen/src/symboltable.rs` around lines 311 - 317,
`SymbolFlags::DEF_BOUND` is currently mixing in the RustPython-only `ITER` bit,
which leaks into the public `_symtable.DEF_BOUND` export and breaks CPython
compatibility. Update the `DEF_BOUND` definition in `symboltable.rs` to include
only the CPython-aligned flags, and keep `ITER` restricted to internal
bound-check logic elsewhere in the symbol table handling.

Comment on lines +292 to +296
const DEF_GLOBAL = 1;
const DEF_LOCAL = 2;
const DEF_PARAM = 2 << 1;
const DEF_NONLOCAL = 2 << 2;
const USE = 2 << 3;

@youknowone youknowone Jun 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const DEF_GLOBAL = 1;
const DEF_LOCAL = 2;
const DEF_PARAM = 2 << 1;
const DEF_NONLOCAL = 2 << 2;
const USE = 2 << 3;
const DEF_GLOBAL = 1 << 0;
const DEF_LOCAL = 1 << 1;
const DEF_PARAM = 1 << 2;
const DEF_NONLOCAL = 1 << 3;
const USE = 1 << 4;

if 2 << n better than this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've matched those values to be 1:1 like they're defined over at CPython for it to not be confusing. I'd rather keep it like this if you're okay with it

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.

2 participants