-
Notifications
You must be signed in to change notification settings - Fork 597
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
Add generic params stand alone semantic model. #3360
Conversation
d8c79fc
to
a6adbd4
Compare
a6adbd4
to
d025589
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 7 files at r1, 5 of 11 files at r2, all commit messages.
Reviewable status: 8 of 14 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @spapinistarkware)
crates/cairo-lang-semantic/src/expr/test_data/generics
line 122 at r2 (raw file):
--> lib.cairo:1:35 fn bar<A, const B: usize, impl C: MyTrait::<felt252>>(){} ^*****^
remove
Code quote:
error: Not a trait.
--> lib.cairo:1:35
fn bar<A, const B: usize, impl C: MyTrait::<felt252>>(){}
^****************^
error: Trait not found.
--> lib.cairo:1:35
fn bar<A, const B: usize, impl C: MyTrait::<felt252>>(){}
^*****^
d025589
to
467da6f
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: 7 of 14 files reviewed, 1 unresolved discussion (waiting on @orizi and @spapinistarkware)
crates/cairo-lang-semantic/src/expr/test_data/generics
line 122 at r2 (raw file):
Previously, orizi wrote…
remove
Done, but not sure about the way I did it. It was raised twice because the path was evaluated both in the computation of the generic param itself, and now also when filtering the other required impls. In the latter I now collect the diagnostics into a temporary diagnostics object which is ignored as it is evaluated twice.
467da6f
to
45b1235
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 2 files at r3, all commit messages.
Reviewable status: 8 of 14 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @spapinistarkware)
crates/cairo-lang-semantic/src/items/generics.rs
line 209 at r3 (raw file):
} } let dummy_diagnostics = &mut SemanticDiagnostics::new(module_file_id);
doc why you don't need to propagate diags.
45b1235
to
cea1e7a
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: 8 of 14 files reviewed, 2 unresolved discussions (waiting on @orizi and @spapinistarkware)
crates/cairo-lang-semantic/src/items/generics.rs
line 209 at r3 (raw file):
Previously, orizi wrote…
doc why you don't need to propagate diags.
This was not relevant, removed. The other place is documented.
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 11 files at r2.
Reviewable status: 10 of 14 files reviewed, all discussions resolved (waiting on @spapinistarkware)
cea1e7a
to
67a5922
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: 9 of 15 files reviewed, all discussions resolved (waiting on @spapinistarkware)
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 r5, all commit messages.
Reviewable status: 10 of 15 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/items/generics.rs
line 165 at r5 (raw file):
) -> Maybe<GenericParam> { let syntax_db: &dyn SyntaxGroup = db.upcast(); let generic_item = generic_param_id.generic_item(db.upcast());
There is no need to go through the generic_item to get the generic_params syntax. If you look at the implementation of generic_item(), you can see it just goes to the parent a few times. Make a similar function that does to the parent less time.
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: 10 of 15 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/items/generics.rs
line 199 at r5 (raw file):
continue; } // Filter impls which are not required by the current impl to avoid cycles.
Do we need to do this logic to prevent cycles?
What about just adding the generic param id to the resolver, and there, fetch the semantic model only when requested? And add a cycle handling query.
Code quote:
avoid cycles
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: 10 of 15 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/expr/test_data/generics
line 307 at r5 (raw file):
struct B<impl Timpl1: ATrait<T>, T, impl Timpl2: ATrait2<T>> { t: T,
Remove whitespace
Code quote:
·
crates/cairo-lang-semantic/src/expr/test_data/generics
line 330 at r5 (raw file):
struct B<T, impl Timpl2: ATrait2<T>, impl Timpl1: ATrait<T>> { t: T,
Here too
Code quote:
·
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: 10 of 15 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)
tests/bug_samples/generic_cycles.cairo
line 5 at r5 (raw file):
struct B<T, impl Timpl1: ATrait<T>, impl Timpl2: ATrait2<T>> { t: T,
Here too
Code quote:
·
67a5922
to
032c6d9
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: 4 of 17 files reviewed, 5 unresolved discussions (waiting on @orizi and @spapinistarkware)
crates/cairo-lang-semantic/src/expr/test_data/generics
line 307 at r5 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Remove whitespace
Done.
crates/cairo-lang-semantic/src/expr/test_data/generics
line 330 at r5 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Here too
Done.
crates/cairo-lang-semantic/src/items/generics.rs
line 165 at r5 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
There is no need to go through the generic_item to get the generic_params syntax. If you look at the implementation of generic_item(), you can see it just goes to the parent a few times. Make a similar function that does to the parent less time.
Done.
crates/cairo-lang-semantic/src/items/generics.rs
line 199 at r5 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Do we need to do this logic to prevent cycles?
What about just adding the generic param id to the resolver, and there, fetch the semantic model only when requested? And add a cycle handling query.
Done.
tests/bug_samples/generic_cycles.cairo
line 5 at r5 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Here too
Done.
032c6d9
to
fbfc846
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: 4 of 17 files reviewed, 6 unresolved discussions (waiting on @orizi and @spapinistarkware)
crates/cairo-lang-semantic/src/resolve/mod.rs
line 783 at r6 (raw file):
ImplLookupContext::new( self.module_file_id.0, self.generic_params.values().copied().collect(),
In here, I ignore the unresolved generic params since trying to resolve all of then will cause unnecessary cycles.
fbfc846
to
6855c38
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.
Dismissed @spapinistarkware from a discussion.
Reviewable status: 4 of 17 files reviewed, 6 unresolved discussions (waiting on @orizi and @spapinistarkware)
crates/cairo-lang-semantic/src/items/generics.rs
line 199 at r5 (raw file):
Previously, gilbens-starkware (Gil Ben-Shachar) wrote…
Done.
I did this change but reverted it. The problem is the resolver impl_lookup_context
function. It returns all the generic params in context. Ignoring the unresolved generic params in this function cause some resolving failures, and fetching all the params semantic models naturally cause cycles.
I tried to add a filter similar to this to the impl_lookup_context
function, but it requires a similar logic to this in many places, and in some places it is not at all clear what the filter should be, while in here it is relatively simple and not scattered all over.
I think that maybe we should remove the impl_lookup_context
, and fetch the relevant generic params in each place separately and not rely on them already being in the resolver, wdyt?
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: 4 of 17 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/items/generics.rs
line 199 at r5 (raw file):
Previously, gilbens-starkware (Gil Ben-Shachar) wrote…
I did this change but reverted it. The problem is the resolver
impl_lookup_context
function. It returns all the generic params in context. Ignoring the unresolved generic params in this function cause some resolving failures, and fetching all the params semantic models naturally cause cycles.
I tried to add a filter similar to this to theimpl_lookup_context
function, but it requires a similar logic to this in many places, and in some places it is not at all clear what the filter should be, while in here it is relatively simple and not scattered all over.
I think that maybe we should remove theimpl_lookup_context
, and fetch the relevant generic params in each place separately and not rely on them already being in the resolver, wdyt?
Something similar, I think the ImplLookupContext should only contain
crates/cairo-lang-semantic/src/items/generics.rs
line 300 at r7 (raw file):
db.upcast(), &root, OptionWrappedGenericParamListPtr(parent),
Insetad of importing items under ast, qualify them with ast::
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: 0 of 19 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/resolve/mod.rs
line 94 at r11 (raw file):
// Generic parameters accessible to the resolver. generic_params: OrderedHashMap<SmolStr, GenericParam>, unresolved_generic_params: OrderedHashMap<SmolStr, GenericParamId>,
I don't understand why we ened this.
Resolver is only using the ids. Why can't we just make generic_params a map to ids?
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: 0 of 19 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/items/generics.rs
line 230 at r12 (raw file):
// cause a cycle. However the GenericParamId is needed for the impl lookup // context. resolver.add_unresolved_generic_param(cur_generic_param_id);
I don't thing we need this logic. Check my comment on resovler/mod.rs
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: 0 of 19 files reviewed, 5 unresolved discussions (waiting on @orizi and @spapinistarkware)
crates/cairo-lang-semantic/src/items/generics.rs
line 230 at r12 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
I don't thing we need this logic. Check my comment on resovler/mod.rs
You are right. Done.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 94 at r11 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
I don't understand why we ened this.
Resolver is only using the ids. Why can't we just make generic_params a map to ids?
There was another place where we used the generic params, not the ids, I removed it. Done.
cc52752
to
547c26b
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 11 files at r6, 11 of 16 files at r10, 1 of 1 files at r11, 4 of 7 files at r12, 5 of 5 files at r13, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @spapinistarkware)
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 16 files at r10, 2 of 5 files at r13.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware)
crates/cairo-lang-semantic/src/items/generics.rs
line 161 at r13 (raw file):
db: &dyn SemanticGroup, generic_param_id: GenericParamId, allow_consts: bool,
What is this allow_consts, why do we need it?
crates/cairo-lang-semantic/src/items/generics.rs
line 230 at r13 (raw file):
&generic_param_syntax, allow_consts, );
resolvers always have to be finalized, to make sure we are not left with uninferred variables
crates/cairo-lang-semantic/src/items/generics.rs
line 253 at r13 (raw file):
} /// Returns the resolver data of the parent generic item if exist.
It would be nice if this function was a method on LookupItemId.
crates/cairo-lang-semantic/src/items/generics.rs
line 360 at r13 (raw file):
)); let generic_param_data = db.generic_param_data(generic_param_id, allow_consts)?; let generic_param = generic_param_data.generic_param;
I think it's better to use the selector query for just the the param semantic model
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 @spapinistarkware)
crates/cairo-lang-semantic/src/items/generics.rs
line 161 at r13 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
What is this allow_consts, why do we need it?
Not sure TBH, it existed before, it seems that consts generics are allowed only in extern_functions
.
547c26b
to
58fa90e
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: 17 of 21 files reviewed, 5 unresolved discussions (waiting on @orizi and @spapinistarkware)
crates/cairo-lang-semantic/src/items/generics.rs
line 230 at r13 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
resolvers always have to be finalized, to make sure we are not left with uninferred variables
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.
Reviewable status: 17 of 21 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/items/generics.rs
line 161 at r13 (raw file):
Previously, gilbens-starkware (Gil Ben-Shachar) wrote…
Not sure TBH, it existed before, it seems that consts generics are allowed only in
extern_functions
.
That sounds right, but I'm not sure why this trickles all the way to the query
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: 17 of 21 files reviewed, 4 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/resolve/mod.rs
line 632 at r14 (raw file):
// If a generic param with this name is found, use it. if let Some(generic_param) = self.data.generic_params.get(&ident) {
Is this an id? if so, please rename
Suggestion:
generic_param_id
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 16 files at r10, 1 of 5 files at r13, 1 of 4 files at r14, all commit messages.
Reviewable status: 18 of 21 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/lookup_item.rs
line 50 at r14 (raw file):
Ok(Some(resolver_data)) } LookupItemId::ModuleItem(_) => Ok(None),
I suggest returning ResolverData::default() here.
crates/cairo-lang-semantic/src/expr/inference/infers.rs
line 108 at r14 (raw file):
let param = self .db .generic_param_semantic(param_id, false)
It's weird that I have to specify allow_consts: false, here. The semantic model of the parameter id doesn't actually depend on allow_consts.
WDYT about inferring allow_consts in the generic_param_semantic query itself, according to the parent item kind?
Code quote:
, false
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 21 files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/items/enm.rs
line 54 at r14 (raw file):
let generic_params_data = db.enum_generic_params_data(enum_id)?; let mut resolver = Resolver::with_data(db, (*generic_params_data.resolver_data).clone()); diagnostics.diagnostics.extend(generic_params_data.diagnostics);
Another approach would have been to not aggregate the diagnostic of the generic params here, but to collect it in module_diagnostics like other items. The benefit of this is that if only the diagnostics oc the generic params changed, we won't need to recompute the semantic model of the enum here.
Code quote:
generic_params_data.diagnostics
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 4 files at r14.
Reviewable status: 19 of 21 files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware and @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: 19 of 21 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/items/generics.rs
line 360 at r13 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
I think it's better to use the selector query for just the the param semantic model
nvm
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: 19 of 21 files reviewed, 5 unresolved discussions (waiting on @orizi and @spapinistarkware)
crates/cairo-lang-semantic/src/lookup_item.rs
line 50 at r14 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
I suggest returning ResolverData::default() here.
Done.
crates/cairo-lang-semantic/src/expr/inference/infers.rs
line 108 at r14 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
It's weird that I have to specify allow_consts: false, here. The semantic model of the parameter id doesn't actually depend on allow_consts.
WDYT about inferring allow_consts in the generic_param_semantic query itself, according to the parent item kind?
Done.
crates/cairo-lang-semantic/src/items/enm.rs
line 54 at r14 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Another approach would have been to not aggregate the diagnostic of the generic params here, but to collect it in module_diagnostics like other items. The benefit of this is that if only the diagnostics oc the generic params changed, we won't need to recompute the semantic model of the enum here.
Added a todo as discussed.
crates/cairo-lang-semantic/src/items/generics.rs
line 161 at r13 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
That sounds right, but I'm not sure why this trickles all the way to the query
Changed it as you suggested above.
crates/cairo-lang-semantic/src/resolve/mod.rs
line 632 at r14 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Is this an id? if so, please rename
Done.
f2afb06
to
6d2ac5e
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 16 files at r10, 12 of 14 files at r15, all commit messages.
Reviewable status: 19 of 21 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)
crates/cairo-lang-semantic/src/items/structure.rs
line 115 at r15 (raw file):
&struct_ast.generic_params(db.upcast()), )?; let generic_params = resolver.inference().rewrite(generic_params).no_err();
You are not finalizing the inference. Every place you create a Resolver, you should finalize it, sho we won't have uninferred variables. Maybe we can enforce it better in the future...
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 11 files at r6, 2 of 16 files at r10, 1 of 1 files at r11, 2 of 14 files at r15.
Reviewable status: all files reviewed, 1 unresolved discussion (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: all files reviewed, 1 unresolved discussion (waiting on @spapinistarkware)
crates/cairo-lang-semantic/src/items/structure.rs
line 115 at r15 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
You are not finalizing the inference. Every place you create a Resolver, you should finalize it, sho we won't have uninferred variables. Maybe we can enforce it better in the future...
Done.
6d2ac5e
to
858aa41
Compare
858aa41
to
89997ea
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 9 files at r16, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)
This change is