-
Notifications
You must be signed in to change notification settings - Fork 605
Resolve items from macro generated code. #7835
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 12 files at r1, all commit messages.
Reviewable status: 10 of 12 files reviewed, 1 unresolved discussion
crates/cairo-lang-semantic/src/expr/test_data/inline_macros
line 1517 at r1 (raw file):
fn $name() {} }; }
Format.
6751a94
to
dfa68b5
Compare
7695a00
to
76bc873
Compare
76bc873
to
9be3e28
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 9 of 27 files at r3, 18 of 18 files at r4, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @orizi)
9be3e28
to
36da16f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r5, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @orizi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware)
crates/cairo-lang-defs/src/db.rs
line 429 at r5 (raw file):
} } ModuleId::MacroCall(_, file_id) => file_id,
a bit clearer for me.
probably have the fields of MacroCall
named.
Suggestion:
ModuleId::MacroCall(_, generated_file_id) => generated_file_id,
crates/cairo-lang-defs/src/db.rs
line 454 at r5 (raw file):
// This is a macro call, we return the directory where the macro call was defined. // It can be either the directory of the parent module or a plugin-generated virtual // file.
Suggestion:
// This is a macro call, we return the directory for the file that contained the macro call, as it is considered the location of the macro itself.
crates/cairo-lang-defs/src/ids.rs
line 288 at r5 (raw file):
// are being processed as if they were defined in a module with this id, however they are // being resolved in the context of the macro call's module. MacroCall(MacroCallId, FileId),
Suggestion:
// Macros at the item level are expanded into full files that can be considered an anonymous module.
MacroCall {
/// The id of the macro call.
id: MacroCallId,
/// The file id of the text generated by the macro call.
generated_file_od: FileId,
},
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)
crates/cairo-lang-defs/src/db.rs
line 429 at r5 (raw file):
Previously, orizi wrote…
a bit clearer for me.
probably have the fields of
MacroCall
named.
Done.
crates/cairo-lang-defs/src/db.rs
line 454 at r5 (raw file):
// This is a macro call, we return the directory where the macro call was defined. // It can be either the directory of the parent module or a plugin-generated virtual // file.
Done.
crates/cairo-lang-defs/src/ids.rs
line 288 at r5 (raw file):
// are being processed as if they were defined in a module with this id, however they are // being resolved in the context of the macro call's module. MacroCall(MacroCallId, FileId),
Done.
36da16f
to
6ba569a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 13 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)
dfa68b5
to
5c6983b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware)
-- commits
line 6 at r7:
can you change the target if this branch to the other PR - so we'd see only the relevant commit?
6ba569a
to
382dd91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 18 of 31 files reviewed, 3 unresolved discussions (waiting on @orizi)
Previously, orizi wrote…
can you change the target if this branch to the other PR - so we'd see only the relevant commit?
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 13 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @orizi)
5c6983b
to
4bbb3a4
Compare
382dd91
to
d28804d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 18 files at r4, 1 of 4 files at r5, 12 of 13 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware)
crates/cairo-lang-parser/src/macro_helpers.rs
line 45 at r8 (raw file):
/// result. The token tree iterator is consumed entirely. pub fn as_expr_macro_token_tree( mut token_tree: impl DoubleEndedIterator<Item = TokenTree>,
this seems unrelated.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 346 at r8 (raw file):
let owning_crate_id = data.module_file_id.0.owning_crate(db); // Check the data.module_file_id.module_id. If it's a macro call module, retrieve the macro // call data associated with this macro call and set it in the resolver below.
i don't understand what is this new comment describing.
Code quote:
// Check the data.module_file_id.module_id. If it's a macro call module, retrieve the macro
// call data associated with this macro call and set it in the resolver below.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 1605 at r8 (raw file):
if let Some((_, module_id)) = self.resolve_item_in_macro_calls(module_id, identifier) { return ResolvedBase::Module(module_id); }
this may be more correct to return something similar to FoundThroughGlobalUse
which contains both the item and its containing module, instead of just the module or the item.
this prevents the overhead of looking for the item twice.
(additionally, probably requires changing its name, as it is no longer just through global use
)
Code quote:
if let Some((_, module_id)) = self.resolve_item_in_macro_calls(module_id, identifier) {
return ResolvedBase::Module(module_id);
}
crates/cairo-lang-semantic/src/resolve/mod.rs
line 2489 at r8 (raw file):
/// The base module to address is the statement StatementEnvironment(ResolvedGenericItem), /// The item is imactive_module_file_idported using global use.
revert
Code quote:
/// The item is imactive_module_file_idported using global use.
crates/cairo-lang-semantic/src/test_utils.rs
line 182 at r8 (raw file):
cached_crate: Option<BlobId>, ) -> WithStringDiagnostics<TestModule> { let crate_id = setup_test_crate_ex(db, content, crate_settings, cached_crate);
seems unrelated as well.
4bbb3a4
to
5a68eba
Compare
d28804d
to
dc81407
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi)
crates/cairo-lang-parser/src/macro_helpers.rs
line 45 at r8 (raw file):
Previously, orizi wrote…
this seems unrelated.
Are you referring to the mut
?
If so- it is necessary here as we consume the iterator.
crates/cairo-lang-semantic/src/test_utils.rs
line 182 at r8 (raw file):
Previously, orizi wrote…
seems unrelated as well.
what does?
crates/cairo-lang-semantic/src/resolve/mod.rs
line 346 at r8 (raw file):
Previously, orizi wrote…
i don't understand what is this new comment describing.
I changed the comment to a TODO because we currently don’t handle macro context here yet.
The idea is- if the module was generated by a macro, we should fetch its macro context (ResolverMacroData
) and set it in the resolver, so path resolution inside macro expansions (like $callsite::foo
) works correctly.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 1605 at r8 (raw file):
Previously, orizi wrote…
this may be more correct to return something similar to
FoundThroughGlobalUse
which contains both the item and its containing module, instead of just the module or the item.this prevents the overhead of looking for the item twice.
(additionally, probably requires changing its name, as it is no longer justthrough global use
)
Done.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 2489 at r8 (raw file):
Previously, orizi wrote…
revert
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 14 files at r9, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi)
dc81407
to
a9f88fb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi)
9dede6f
to
96edb0f
Compare
5a68eba
to
9fada98
Compare
96edb0f
to
d9a3a75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r11, 10 of 10 files at r12, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 18 of 18 files at r13, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware)
corelib/src/test/language_features/macro_test.cairo
line 549 at r13 (raw file):
macro func_macro { () => { pub fn func_macro_fn() -> felt252 { 100 }
does this still work? (it should)
for all other pub
s as well.
Suggestion:
fn func_macro_fn() -> felt252 { 100 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 3 files at r11, 5 of 18 files at r13.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)
crates/cairo-lang-parser/src/macro_helpers.rs
line 45 at r8 (raw file):
Previously, dean-starkware wrote…
Are you referring to the
mut
?
If so- it is necessary here as we consume the iterator.
all changes here.
crates/cairo-lang-semantic/src/test_utils.rs
line 182 at r8 (raw file):
Previously, dean-starkware wrote…
what does?
all the changes in this file.
crates/cairo-lang-defs/src/db.rs
line 706 at r13 (raw file):
MacroCallLongId(module_file_id, inline_macro_ast.stable_ptr(db)).intern(db); macro_calls.insert(item_id, inline_macro_ast.clone()); // plugin_diagnostics.push((
?
crates/cairo-lang-parser/src/parser_test_data/partial_trees/item_inline_macro
line 586 at r13 (raw file):
└── child #1 (kind: FunctionWithBody) <ignored> //! > ==========================================================================
this is from a base PR - probably a missing rebase.
crates/cairo-lang-semantic/src/expr/compute.rs
line 120 at r13 (raw file):
.find(|mapping| mapping.span.start <= self.0 && self.0 <= mapping.span.end)?; Some(Self::new(match mapping.origin { cairo_lang_filesystem::ids::CodeOrigin::Start(text_offset) => {
not a part of this PR.
crates/cairo-lang-semantic/src/db.rs
line 1818 at r13 (raw file):
for macro_call in db.module_macro_calls_ids(module_id)?.iter() { diagnostics.extend(db.macro_call_diagnostics(*macro_call)); if let Ok(macro_call_module) = db.macro_call_module_id(*macro_call) {
a similar call is needed for lowering diagnostics.
and test.
crates/cairo-lang-semantic/src/db.rs
line 1818 at r13 (raw file):
for macro_call in db.module_macro_calls_ids(module_id)?.iter() { diagnostics.extend(db.macro_call_diagnostics(*macro_call)); if let Ok(macro_call_module) = db.macro_call_module_id(*macro_call) {
add test for getting the internal sematic diag.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 463 at r13 (raw file):
let segments_stable_ptr = segments.peek().expect("Trying to resolve an empty path.").stable_ptr(db); let mut cur_macro_call_data = self.macro_call_data.as_ref(); // there is a possible problem in here.
?
crates/cairo-lang-semantic/src/resolve/mod.rs
line 1651 at r13 (raw file):
let mut queue: VecDeque<_> = self.db.module_macro_calls_ids(module_id).ok()?.iter().copied().collect(); while let Some(macro_call_id) = queue.pop_front() {
test finding items through mix of macro-calls
and use path::*
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)
eeb9c96
to
b5a56ed
Compare
d9a3a75
to
0dcd5f8
Compare
0dcd5f8
to
f9233e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 20 of 20 files at r15, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gilbens-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 33 of 34 files reviewed, 9 unresolved discussions (waiting on @gilbens-starkware and @orizi)
corelib/src/test/language_features/macro_test.cairo
line 549 at r13 (raw file):
Previously, orizi wrote…
does this still work? (it should)
for all otherpub
s as well.
Done.
crates/cairo-lang-defs/src/db.rs
line 706 at r13 (raw file):
Previously, orizi wrote…
?
Done.
crates/cairo-lang-parser/src/parser_test_data/partial_trees/item_inline_macro
line 586 at r13 (raw file):
Previously, orizi wrote…
this is from a base PR - probably a missing rebase.
I don't think you're right. It's not taken from a base PR.
crates/cairo-lang-semantic/src/test_utils.rs
line 182 at r8 (raw file):
Previously, orizi wrote…
all the changes in this file.
Done.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 463 at r13 (raw file):
Previously, orizi wrote…
?
Done.
crates/cairo-lang-semantic/src/expr/compute.rs
line 120 at r13 (raw file):
Previously, orizi wrote…
not a part of this PR.
Done.
f9233e6
to
35e45a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 20 files at r15, 1 of 1 files at r16, 5 of 5 files at r17, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware)
corelib/src/test/language_features/macro_test.cairo
line 627 at r17 (raw file):
() => { mod generated_mod { pub fn mod_fn() -> felt252 { 77 }
add other tests where the pub
is required.
Code quote:
pub fn mod_fn() -> felt252 { 77 }
35e45a2
to
de8a98e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r16, 5 of 5 files at r17, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi)
corelib/src/test/language_features/macro_test.cairo
line 627 at r17 (raw file):
Previously, orizi wrote…
add other tests where the
pub
is required.
Done.
crates/cairo-lang-semantic/src/db.rs
line 1818 at r13 (raw file):
Previously, orizi wrote…
add test for getting the internal sematic diag.
Done.
crates/cairo-lang-semantic/src/db.rs
line 1818 at r13 (raw file):
Previously, orizi wrote…
a similar call is needed for lowering diagnostics.
and test.
Done.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 1651 at r13 (raw file):
Previously, orizi wrote…
test finding items through mix of
macro-calls
anduse path::*
s.
In a different (post) PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 5 of 5 files at r18, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @orizi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)
corelib/src/test/language_features/macro_test.cairo
line 627 at r17 (raw file):
Previously, dean-starkware wrote…
Done.
just the first was actually required.
the other test that i want to see is when the macro generates a function within a (non-generated) module.
and it is used out of the macro.
crates/cairo-lang-parser/src/parser_test_data/partial_trees/item_inline_macro
line 586 at r13 (raw file):
Previously, dean-starkware wrote…
I don't think you're right. It's not taken from a base PR.
you haven't done any parser updates here - you should not touch parser tests.
so either it is from a base PR - or it should be part of a base PR.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 1651 at r13 (raw file):
Previously, dean-starkware wrote…
In a different (post) PR.
continuing to block until i follow through - as this likely actually breaks this code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r18.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)
crates/cairo-lang-semantic/src/expr/test_data/inline_macros
line 1800 at r18 (raw file):
error: Unexpected argument type. Expected: "core::byte_array::ByteArray", found: "core::felt252". --> lib.cairo:8:27 let _num: ByteArray = item_level();
this is not propagated from the macro.
the error should be semantic within the macro,
return "100" instead of 100 for example.
Code quote:
let _num: ByteArray = item_level();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 5 files at r18.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @dean-starkware and @gilbens-starkware)
commit-id:fb7b0e76
de8a98e
to
044e923
Compare
Stack: