Repository navigation
import: __main__ identity, circular-import diagnostics, module getattr precedence, imp completeness - #764
Conversation
|
Warning Review limit reached
Next review available in: 45 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24392d184d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // path does not. `__file__` is absolutized (os.path.abspath, 3.9+) while | ||
| // `sys.argv[0]` keeps the literal command-line path. | ||
| if filename != "<string>" { | ||
| let abs_file = match std::path::absolute(filename) { |
There was a problem hiding this comment.
In the sandbox Script path, startup deliberately reads cwd through host_seam so the guest sees the controller's virtual cwd rather than the trusted process cwd, but this new absolutization uses std::path::absolute in the child. For a relative script run with --features sandbox, __file__ is now built from the host cwd (and can leak it / point outside the virtual filesystem) instead of the sandbox path; use the seam-provided cwd or keep the literal path under sandbox.
Useful? React with 👍 / 👎.
| pub(crate) fn get_spec(module: PyObjectRef) -> PyObjectRef { | ||
| match crate::baseobjspace::getattr_str(module, "__spec__") { | ||
| Ok(v) => v, | ||
| Err(_) => pyre_object::w_none(), |
There was a problem hiding this comment.
The ported get_spec helper should only treat a missing __spec__ as None; with this wildcard, a module whose __getattr__ or descriptor raises (for example RuntimeError) while formatting a failed from m import x has that user exception masked and converted into a normal ImportError. PyPy's helper re-raises non-AttributeError, so please keep that behavior instead of swallowing every lookup failure.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| // subclass `__repr__` override wins over the native module | ||
| // formatting. | ||
| if let Some(r) = module_user_dunder_obj(obj, "__repr__")? { | ||
| pyre_object::w_str_get_value(r).to_string() |
There was a problem hiding this comment.
When a ModuleType subclass __repr__ returns a Python str containing a lone surrogate, repr() reaches this new branch through py_repr_wtf8, whose purpose is to avoid w_str_get_value panics for surrogate-bearing dunder results. Folding the returned string through w_str_get_value can panic user code instead of returning the Python string; read the result via w_str_get_wtf8 on the WTF-8 path (and mirror the same care for the new module __str__ branch).
Useful? React with 👍 / 👎.
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit e19980e). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patch
2. Other mismatches introduced by this patch
3. Pre-existing mismatches (already present before this patch)
4. Structural adaptations
|
|
Follow-up: implemented the previously-deferred items too, each verified against CPython 3.14 and PyPy3 (respecting PyPy source structure, matching CPython 3.14 behavior).
— commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae02bc96fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| module_ns_store( | ||
| ns, | ||
| "dont_write_bytecode", | ||
| w_bool_from(crate::importing::dont_write_bytecode_flag()), |
There was a problem hiding this comment.
With the default launcher flags this now publishes sys.dont_write_bytecode == False, but the import path still documents that pyre has no .pyc cache and passes cpathname=None for source imports (importing.rs around exec_code_module/load_source_module). Any code or stdlib tests guarded by if not sys.dont_write_bytecode will now expect __pycache__ files to be created even though imports never write them; keep reporting true until cache writing exists, or wire the flag to actual bytecode-cache generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d34abff4d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| return Ok(None); | ||
| } | ||
| let r = crate::builtins::call_and_check(method, &[obj])?; |
There was a problem hiding this comment.
When a types.ModuleType subclass defines __repr__ or __str__ as a staticmethod, classmethod, or custom descriptor, this calls the raw class-dict value with the module as an extra positional argument. The generic special-method path binds through get_and_call_function, so these valid overrides otherwise raise TypeError or receive the wrong receiver instead of controlling repr()/str().
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| Ok(v) => crate::baseobjspace::is_true(v).unwrap_or(false), | ||
| Err(_) => false, |
There was a problem hiding this comment.
When a module has a custom __spec__ whose _initializing descriptor or truthiness raises during a failed from m import x or missing module attribute, this helper converts the exception to false. PyPy only treats a missing _initializing attribute as false and otherwise re-raises, so pyre masks user exceptions and reports the generic import/attribute error instead.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
|
Addressed the automated review (Codex inline + Codex parity), each verified against CPython 3.14 and PyPy3. Rebased onto the fixed Fixed (3):
Declined (2), with reasoning:
Codex parity §3/§4 (import-lock atomic counter, immutable-constant rebuild in
— commented by Claude |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c1f5109d40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // circular-import cause, as does one whose submodule slot is unset. | ||
| let w_spec = get_spec(module)?; | ||
| let initializing = is_spec_initializing(w_spec)?; | ||
| let uninit_submodule = !initializing && is_spec_uninitialized_submodule(w_spec, &pkgname)?; |
There was a problem hiding this comment.
When from pkg import child misses while pkg.__spec__._uninitialized_submodules contains the leaf name (child), this passes the package name (pkg) into the containment check, so the new circular-import submodule diagnostic branch is skipped and pyre falls back to the generic cannot import name error; if the branch ever does fire, the message below also swaps the submodule and module names. Pass name to the check and format it as the submodule of pkgname.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/18967027556f28408747c8149510e4c677d0cc55/pyre-interpreter/src/importing.rs#L3183-L3185
Here w_pkgpath can be a freshly allocated default string (or a value returned by __file__), but only w_pkgname is pinned. The new get_spec / is_spec_* calls can allocate or run user descriptors before utf8_w(w_pkgpath) and before the ImportError stores the path, so a nursery collection can move the path object and leave this local pointer stale; pin w_pkgpath as well or read/own it before these lookups.
https://github.com/youknowone/pyre/blob/18967027556f28408747c8149510e4c677d0cc55/pyre-interpreter/src/module/imp/interp_imp.rs#L231-L232
This uses check_sys_modules, which falls back to pyre's private SYS_MODULES cache. If user code imports a builtin and then deletes sys.modules[name], _imp.is_builtin(name) still returns -1 because the private cache retains the module, whereas the ported PyPy logic checks the Python-visible sys.modules entry directly and should report the builtin as re-initializable (1); use the dict membership/value here instead of the import cache.
AGENTS.md reference: AGENTS.md:L194-L195
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A `types.ModuleType` subclass instance keeps `ob_type` at the canonical module type and carries its class in `w_class`, so the `ob_type`-keyed formatting in `py_repr`/`py_str` bypassed a subclass override and always ran the native module repr. Resolve `__repr__`/`__str__` through `w_class` (via `module_user_dunder_obj`) before the native path, mirroring the tuple/structseq handling; a plain module or a subclass without the override keeps the fast native formatting. Assisted-by: Claude
os.chmod read mode with the unchecked w_int_get_value, so a non-integer argument reinterpreted its object layout as a W_IntObject and reached libc::chmod with an arbitrary value instead of raising TypeError. Unwrap mode through c_int_w, matching the c_int unwrap in posix.chmod. Assisted-by: Claude
Add get_spec / is_spec_initializing / is_spec_uninitialized_submodule (importing.py:430-458) and consult them when import_from cannot resolve a name: a module still executing reports "partially initialized module ... (most likely due to a circular import)" and an unset submodule reports "cannot access submodule ...". Expose check_sys_modules as pub(crate) for the imp.is_builtin already-imported check. Assisted-by: Claude
A module receiver now runs the generic lookup with the __getattr__ fallback suppressed, then its own dict's PEP 562 __getattr__, and only then the receiver type's class-level __getattr__ (module.py:130-162). A class-level __getattr__'s own AttributeError reaches the caller instead of being overwritten by the generic module-miss message. The module miss also reports the circular-import cause via __spec__._initializing / _uninitialized_submodules. Assisted-by: Claude
is_builtin now returns -1 for a builtin already present in sys.modules and thus not re-initializable, 1 for a not-yet-imported builtin, 0 otherwise (interp_imp.py:77-87). Register init_frozen, a legacy entry point that returns None (interp_imp.py:74). Assisted-by: Claude
Report False by default, matching sys.flags.dont_write_bytecode (0); no bytecode cache is written regardless. Assisted-by: Claude
Seed __spec__ / __package__ / __doc__ = None on the __main__ namespace before executing user code so a bare reference no longer falls through to builtins, and set __file__ (absolutized) / __cached__ = None for a script run by path. Assisted-by: Claude
acquire_lock / release_lock / lock_held now track a reentrant depth counter (importing.py:159 ImportRLock collapsed to the observable count); release_lock raises RuntimeError "not holding the import lock" when the count is zero, and lock_held reports True during an import. create_dynamic raises ImportError "Not implemented" (interp_imp.py:49, the has_so_extension()==False branch) so the meta-path hasattr probe matches while except ImportError still falls back to pure Python. _fix_co_filename replaces co_filename in place, recursing through nested code constants whose filename matches the root's original (importing.py:379 update_code_filenames); argument-type errors match the code/str clinic messages. Assisted-by: Claude
Execute importlib._bootstrap / _bootstrap_external with __name__ set to
_frozen_importlib / _frozen_importlib_external during exec, then restore the
public name (_frozen_importlib moduledef.py:17-49 install()). Classes defined
in the bodies capture __module__ = the frozen name, matching the frozen build,
while module.__name__ stays importlib._bootstrap{,_external}.
Assisted-by: Claude
Parse -B, -O (counting), and the PYTHONDONTWRITEBYTECODE / PYTHONOPTIMIZE environment variables (folded via _Py_get_env_flag semantics, gated on -E/-I), and record them as runtime flags. sys.flags.optimize / sys.flags.dont_write_bytecode and sys.dont_write_bytecode now reflect them, the default script / -c / import compile carries the optimize level (stripping asserts and __debug__, and at level 2 docstrings), and compile()'s optimize=-1 resolves to sys.flags.optimize. Assisted-by: Claude
get_spec, is_spec_initializing, and is_spec_uninitialized_submodule returned a bare bool/None and swallowed every error. Return Result and suppress only a missing attribute (AttributeError), propagating any other lookup error, the truth test, and the containment test. Route the two callers — import_from message construction and the module getattr miss — to propagate. Assisted-by: Claude
py_repr_wtf8 and py_str_wtf8 had no branch for a types.ModuleType subclass, so a surrogate-bearing __repr__/__str__ override result fell through to py_repr/py_str and panicked in w_str_get_value. Read the override result through w_str_get_wtf8, and route a module str() without a __str__ override back through py_repr_wtf8. Assisted-by: Claude
A relative script path was absolutized with std::path::absolute, which joins the trusted process cwd. Join it against sys_path_cwd() — the seam-provided virtual cwd under sandbox — before normalizing, so the process cwd is never read or leaked. Off sandbox the result is unchanged. Assisted-by: Claude
The optimization level was stored as u8, so a PYTHONOPTIMIZE value above 255 was capped (999 -> 255) while both CPython 3.14 and PyPy report it verbatim. Widen the stored level to i64 through the launcher and the sys flag; the compiler-facing optimize_flag() clamps it into a byte (levels above 2 behave as 2), so compilation is unchanged. Assisted-by: Claude
module_getattr and import_from classify a failed lookup through the module spec: a file whose directory matches the startup sys.path[0] and whose name is a standard-library module reports the "consider renaming ... standard library module" hint; a non-stdlib match reports the "consider renaming ... library you intended to import" hint while the module initializes; a still-initializing module reports its origin in the circular-import message. - Add spec_file_origin, is_possibly_shadowing, and module_shadow_info helpers keyed on a startup sys.path[0] recorded by init_sys_path. - Populate sys.stdlib_module_names with the full standard-library set. - import_from reports "unknown location" for a non-str __file__ instead of raising TypeError. Assisted-by: Claude
create_dynamic reads the spec's name and origin and rejects an embedded null with ValueError before reporting the unsupported dynamic load. Assisted-by: Claude
…the attribute import_from now looks up sys.modules under `<__name__>.<name>` using the object's __name__ rather than requiring a module, so a submodule bound in sys.modules that _handle_fromlist could not setattr onto a non-module parent is still returned. Assisted-by: Claude
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e19980e2d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Short('O') => {} // no-op | ||
| // `-O` / `-OO` raise the optimization level; each occurrence counts | ||
| // (app_main.py `optimize`). PYTHONOPTIMIZE folds in during finalize. | ||
| Short('O') => flags.optimize = flags.optimize.saturating_add(1), |
There was a problem hiding this comment.
When -O or PYTHONOPTIMIZE now becomes effective for compilation, the builtins module still has the only __debug__ registration hard-coded to true. This leaves import builtins; builtins.__debug__ true under optimized runs even though PyPy/CPython publish it as false, so code that inspects the builtins namespace instead of a compiled __debug__ constant observes the wrong optimization mode.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| Err(crate::PyError::new( | ||
| crate::PyErrorKind::ImportError, | ||
| "Not implemented".to_string(), | ||
| )) |
There was a problem hiding this comment.
For unsupported extension modules, PyPy's _imp.create_dynamic(FakeSpec('foo')) raises ImportError with .name == spec.name and .path == spec.origin; this code validates those attributes and then discards them by constructing a plain ImportError. Any importer or test that catches this fallback error and inspects e.name/e.path now loses the module identity it was supposed to carry.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
| other => other.clone(), | ||
| }) | ||
| .collect(); | ||
| code.constants = new_consts; |
There was a problem hiding this comment.
_imp._fix_co_filename promises to recurse through nested code constants, but rebuilding code.constants leaves any already-realized entries in the enclosing PyCode.co_consts_w cache pointing at wrappers whose co_filename is still the old value; later LOAD_CONST reuses that stale wrapper. This shows up if code.co_consts/a nested function code was realized before calling the public helper, unlike PyPy's direct walk over co_consts_w.
AGENTS.md reference: AGENTS.md:L194-L195
Useful? React with 👍 / 👎.
Continues the import-compatibility work. Each change was verified against
PyPy3 (the compat oracle) and CPython 3.14; every case below now matches
PyPy3 exactly.
Changes
__main__identity attributes — seed__spec__/__package__/__doc__=Noneon the__main__namespace before user code runs, so abare reference no longer falls through to
builtins. Set__file__(absolutized) /
__cached__=Nonefor a script run by path. Fixesif __spec__ is Nonemain-detection (multiprocessing spawn, runpy, pytest)and
NameError: __doc__.Circular-import diagnostics —
from a import xanda.xon a modulestill executing now report "partially initialized module 'a' ... (most
likely due to a circular import)", and an unset submodule reports "cannot
access submodule ...", driven by
__spec__._initializing/_uninitialized_submodules.Module
__getattr__precedence — a module consults its own dict's PEP562
__getattr__before the receiver type's class-level__getattr__, anda class-level
__getattr__'s ownAttributeErrorreaches the callerinstead of being overwritten by the generic module-miss message.
_imp.is_builtin— returns-1for a builtin already insys.modules(not re-initializable),
1for a not-yet-imported builtin,0otherwise._imp.init_frozen— added as the legacy no-op returningNone.sys.dont_write_bytecode— defaults toFalse, consistent withsys.flags.dont_write_bytecode(no bytecode cache is written regardless).Also included (earlier on this branch): module subclass
__repr__/__str__override dispatch in
repr/str, andchmodmode validation asc_int.Verification
wording follows PyPy3 per the faithful-error-message convention).
check.pydynasm 300/300 ×2 and cranelift 300/300 ×2, all green.— commented by Claude