Skip to content

Remove private comprehension flags - #8537

Open
YangSiJun528 wants to merge 3 commits into
RustPython:mainfrom
YangSiJun528:remove-private-comprehension-flags
Open

Remove private comprehension flags#8537
YangSiJun528 wants to merge 3 commits into
RustPython:mainfrom
YangSiJun528:remove-private-comprehension-flags

Conversation

@YangSiJun528

@YangSiJun528 YangSiJun528 commented Aug 15, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Remove the RustPython-only ASSIGNED_IN_COMPREHENSION and ITER flags in favor of existing definition flags.
  • Preserve both assignment-expression/iterator conflicts and inlined-comprehension behavior.
  • Fix class-mangled conflict diagnostics and enable the existing test.

Rationale

ASSIGNED_IN_COMPREHENSION and ITER duplicate facts already represented by the existing definition flags.
They also occupy bit positions reserved for packed scope information.
Removing these private flags makes that range available for scope packing without losing the comprehension conflict checks.

CPython reference

CPython 3.14.7 handles both conflict directions using existing flags:

  • symtable_add_def_helper() rejects an iterator target already marked DEF_GLOBAL or DEF_NONLOCAL, then records valid targets with DEF_COMP_ITER.
  • symtable_extend_namedexpr_scope() rejects a named-expression target already marked DEF_LOCAL | DEF_COMP_ITER, otherwise propagating DEF_GLOBAL or DEF_NONLOCAL into the comprehension scope.

The inlined-comprehension path is unchanged: every iterator target marked with ITER was already DEF_LOCAL, so checking DEF_LOCAL alone selects the same locals for save/restore.

Additionally, this PR fixes a small diagnostic bug found while updating these checks: lookup still uses the mangled key, while errors now show the identifier as written in the source. This enables the existing mangled-name diagnostic test.

Validation

Passed pre-commit, workspace and C-API tests, and all 74 test_named_expressions tests.

Follow-up

A follow-up PR will build on this change to enable more test_symtable tests.

AI assistance

Codex (GPT-5.6 Sol) was used to investigate the symbol-table control flow, compare it with CPython, and implement this change. I reviewed the diff and the validation results above.

Summary by CodeRabbit

  • Bug Fixes
    • Improved scope handling for comprehension iteration variables.
    • Fixed name resolution for named expressions used within comprehensions.
    • Improved detection of conflicting declarations in nested loops.
    • Prevented internal symbol classification from causing incorrect compilation behavior.

@YangSiJun528
Remove the private assignment-expression marker
9cf06c1 
Why:
extend_namedexpr_scope() already propagates assignment-expression
targets and records DEF_GLOBAL or DEF_NONLOCAL in comprehension scopes.
ASSIGNED_IN_COMPREHENSION remained only for conflict checks. Its private
bit overlaps CPython's packed LOCAL scope bit.

Changes:
- Register assignment-expression targets as ordinary assignments.
- Detect later target conflicts from existing declaration flags.

Assisted-by: Codex:gpt-5.6-sol
@YangSiJun528
Remove the private comprehension iterator flag
a7385d2 
Why:
ITER duplicates the DEF_LOCAL and DEF_COMP_ITER facts already recorded
for comprehension targets. Its private bit also overlaps CPython's
packed scope field.

Changes:
- Use those flags for rebinding checks and local restoration around
  inlined comprehensions.

Assisted-by: Codex:gpt-5.6-sol
@YangSiJun528
Preserve source names in named-expression diagnostics
b7f4952 
Why:
Assignment-expression diagnostics exposed the mangled keys used for
symbol-table lookup. CPython reports identifiers as written in source.

Changes:
- Use source names in both comprehension conflict diagnostics.
- Enable test_named_expression_invalid_mangled_class_variables.

Assisted-by: Codex:gpt-5.6-sol
@github-actions github-actions Bot added the z-ca-2026 Tag to track Contribution Academy 2026 label Aug 15, 2026
@coderabbitai

coderabbitai Bot commented Aug 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1c7706f9-31e9-4c34-8282-492a0471bf6a

📥 Commits

Reviewing files that changed from the base of the PR and between 70b47dd and b7f4952.

⛔ Files ignored due to path filters (1)
  • Lib/test/test_named_expressions.py is excluded by !Lib/**
📒 Files selected for processing (2)
  • crates/codegen/src/compile.rs
  • crates/codegen/src/symboltable.rs
📝 Walkthrough

Walkthrough

The code generator now represents comprehension iterators with DEF_LOCAL | DEF_COMP_ITER. It removes dedicated iterator flags and tracking state, updates named-expression and inner-loop conflict checks, and narrows local-symbol detection to DEF_LOCAL.

Changes

Comprehension Symbol Analysis

Layer / File(s) Summary
Iterator flag and assignment registration
crates/codegen/src/symboltable.rs		 Removed dedicated iterator flags, named-expression usage, and inner-loop tracking state. Comprehension iterators now register as `DEF_LOCAL
Scope and conflict validation
crates/codegen/src/symboltable.rs, crates/codegen/src/compile.rs		 Updated rebinding and global/nonlocal conflict checks. Local-symbol detection now checks DEF_LOCAL only.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to b7f49

The change preserves comprehension conflict behavior, keeps inlined-comprehension handling equivalent, and corrects class-mangled diagnostics while removing redundant internal flags. The listed validation passes, so no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related PRs

Suggested reviewers: shaharnaveh, youknowone

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: removing the internal comprehension flags from the symbol-table implementation.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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

github-actions Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

📦 Library Dependencies

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

[x] test: cpython/Lib/test/test_named_expressions.py (TODO: 1)

dependencies:

dependent tests: (no tests depend on named_expressions)

Legend:

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

ShaharNaveh
ShaharNaveh approved these changes Aug 15, 2026
View reviewed changes

@ShaharNaveh ShaharNaveh 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.

TYSM!!!
I wanted to remove these for so long

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

Reviewers

@ShaharNaveh ShaharNaveh ShaharNaveh approved these changes

Assignees

@YangSiJun528 YangSiJun528

Labels

z-ca-2026 Tag to track Contribution Academy 2026

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@YangSiJun528 @ShaharNaveh