Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
a3f9afd to
5b8d3d4
Compare
PR SummaryMedium Risk Overview Glob Reviewed by Cursor Bugbot for commit aa086c9. Bugbot is set up for automated code reviews on this repo. Configure here. |
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).
crates/cairo-lang-semantic/src/resolve/mod.rs line 2036 at r1 (raw file):
ResolvedBase::FoundThroughGlobalUse { item_info: inner_module_item, containing_module,
What happens when it is deeply nested? It still brings the correct module?
…generic items `first_generic`'s `FoundThroughGlobalUse` arms resolved an item pulled in via `use *` without running the usability checks the concrete path performs. So re-exporting an `#[unstable]` (or otherwise gated) item through a glob (`use inner::*; use item as alias;`) skipped the E2065 warning that a direct `use inner::item as alias;` emits. Extract the shared "validate usability, record the use, resolve to a generic item" step into `resolve_module_item_as_generic` (subsuming the former `validate_module_item_usability`) and route every import→generic resolution through it — direct, glob, and `next_generic` — so the checks can't be skipped at a new site.
5b8d3d4 to
aa086c9
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-semantic/src/resolve/mod.rs line 2036 at r1 (raw file):
Previously, eytan-starkware wrote…
What happens when it is deeply nested? It still brings the correct module?
Done.
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
Summary
Consolidates the three separate call sites that convert an imported module item into a
ResolvedGenericIteminto a single method,resolve_module_item_as_generic. This method combines the visibility check (validate_module_item_usability), the feature-gate check (validate_feature_constraints), the used-import recording (insert_used_use), and the conversion (ResolvedGenericItem::from_module_item) into one place, ensuring none of these steps can be accidentally omitted.A new diagnostic test is added covering the case where an unstable item is re-exported through a glob
use *— previously, the feature-gate warning was not emitted in that path because theFoundThroughGlobalUsebranches were not callingvalidate_feature_constraints.Type of change
Please check one:
Why is this change needed?
The
FoundThroughGlobalUseresolution branches were callinginsert_used_useandResolvedGenericItem::from_module_itemdirectly, bypassingvalidate_module_item_usability(which includes the feature-gate check). This meant that accessing an unstable item via a glob re-export (use inner::*; pub use unstable as renamed;) would not produce the expected#[unstable]warning.What was the behavior or documentation before?
Accessing an unstable item through a glob re-export silently skipped the unstable-feature diagnostic. The visibility and feature checks were only reliably applied on the direct-import code path.
What is the behavior or documentation after?
All paths that resolve an imported item — direct imports and glob
use *— go throughresolve_module_item_as_generic, which unconditionally applies visibility checks, feature-gate checks, and used-import recording. A new test confirms that the unstable-feature warning is now emitted when an unstable item is re-exported via a glob import.Related issue or discussion (if any)
Additional context
The refactor is purely mechanical: no logic was changed, only consolidated. The new method is documented with a note that every import-to-generic-item conversion must go through it so these checks are never skipped in the future.