Conversation
|
This change is |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
PR SummaryMedium Risk Overview Semantic A defs integration test asserts module-only, item-only, and combined feature sets via a reporting plugin. Reviewed by Cursor Bugbot for commit 61f97b4. 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 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).
crates/cairo-lang-defs/src/db.rs line 1505 at r1 (raw file):
} ModuleId::MacroCall { id, .. } => { module_allowed_features(db, (), id.parent_module(db)).clone()
We should consider supporting #[feature(...)] macro_creating_items!() which to my understanding will not work right now
crates/cairo-lang-defs/src/test.rs line 495 at r1 (raw file):
#[feature("inner4")] fn f4() {} }
Add nested modules and maybe a macro call inside the function
30760d2 to
a25d6f4
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a25d6f4. Configure here.
orizi
left a comment
There was a problem hiding this comment.
@orizi made 2 comments.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-defs/src/db.rs line 1505 at r1 (raw file):
Previously, eytan-starkware wrote…
We should consider supporting
#[feature(...)] macro_creating_items!()which to my understanding will not work right now
Done
crates/cairo-lang-defs/src/test.rs line 495 at r1 (raw file):
Previously, eytan-starkware wrote…
Add nested modules and maybe a macro call inside the function
macro-call isn't actually active here - but added the rest.
`module_sub_files` previously passed an empty `allowed_features` set to every
macro plugin (a `TODO`), so item-level macro plugins could not gate behavior on
`#[feature("...")]` the way inline-macro plugins already can.
Extract the features in scope for each item — the `#[feature(...)]` attributes on
the item plus those on its enclosing modules (walked via `module_allowed_features`,
reading the `mod` declarations straight from the syntax tree to avoid a
`module_data` -> `module_sub_files` query cycle) — and pass them in the metadata.
No behavior change today: no in-tree plugin reads `allowed_features` at the defs
layer yet. Adds a `defs` unit test covering the empty, item-level, module-scope,
and accumulated cases.
a25d6f4 to
61f97b4
Compare
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 4 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
Summary
MacroPluginMetadata::allowed_featuresnow reflects the full set of features in scope for each item: the#[feature("...")]attributes declared on the item itself plus those inherited from all ancestor modules up to the crate root.Previously,
allowed_featureswas always an emptyDefault::default(), meaning macro plugins had no visibility into which features were enabled for a given item. Now,module_allowed_features(a memoized#[salsa::tracked]function) walks the module hierarchy once perModuleIdand accumulates#[feature]attributes from each submodule declaration. Per-item,extract_allowed_featurescollects the item's own#[feature]attributes and merges them with the module-level set. When an item adds no features of its own, the module set is reused directly to avoid a per-item allocation.A new test (
test_allowed_features_accumulated_with_item) verifies the four cases: no features, module-only features, item-only features, and both combined.Type of change
Please check one:
Why is this change needed?
Macro plugins receive a
MacroPluginMetadatastruct that is supposed to communicate which features are allowed in scope. Theallowed_featuresfield was always empty, so plugins could not correctly gate behavior on feature availability. This made the feature-gating mechanism non-functional at the macro expansion level.What was the behavior or documentation before?
allowed_featuresinMacroPluginMetadatawas always an empty set, regardless of any#[feature("...")]attributes present on the item or its enclosing modules.What is the behavior or documentation after?
allowed_featurescontains the union of#[feature("...")]attributes from the item itself and all of its ancestor modules. Module-level feature sets are memoized perModuleIdso the ancestor walk is not repeated for every item in the same module.Related issue or discussion (if any)
Resolves the
TODO(orizi): Actually extract the allowed features per module.comment that was previously left inmodule_sub_files.Additional context
The
module_allowed_featuresfunction handles all threeModuleIdvariants:CrateRootreturns an empty set,Submoduleextracts features from the submodule declaration node and merges with the parent's set, andMacroCalldelegates directly to its parent module's set.