@orizi reviewed 1 of 6 files at r1, 1 of 5 files at r2, all commit messages.
Reviewable status: 2 of 7 files reviewed, 4 unresolved discussions (waiting on @TomerStarkware)

crates/cairo-lang-semantic/src/types.rs line 215 at r2 (raw file):

    }

    pub fn extract_generic_params(

doc

crates/cairo-lang-semantic/src/types.rs line 218 at r2 (raw file):

        &self,
        db: &'db dyn Database,
        generic_parameters: &mut HashSet<GenericParamId<'db>>,

Suggestion:

        generic_parameters: &mut OrderedHashSet<GenericParamId<'db>>,

crates/cairo-lang-semantic/src/types.rs line 246 at r2 (raw file):

                    }
                }
                // TODO(ilya): Do we need to extract generic params from the impl.ty?

what generic params?

crates/cairo-lang-semantic/src/types.rs line 249 at r2 (raw file):

            }
            TypeLongId::Closure(_) => {
                // TODO(ilya): Do we need to extract generic params from the closure??

can it be a closure? isn't it about the signature of the taken impl?

Reviewable status: 2 of 7 files reviewed, 4 unresolved discussions (waiting on @orizi and @TomerStarkware)

crates/cairo-lang-semantic/src/types.rs line 246 at r2 (raw file):

impl.ty is a type, so it might include generic params, no?

Reviewable status: 2 of 7 files reviewed, 4 unresolved discussions (waiting on @orizi and @TomerStarkware)

crates/cairo-lang-semantic/src/types.rs line 215 at r2 (raw file):

Done.

crates/cairo-lang-semantic/src/types.rs line 218 at r2 (raw file):

        &self,
        db: &'db dyn Database,
        generic_parameters: &mut HashSet<GenericParamId<'db>>,

Done.

@orizi reviewed all commit messages.
Reviewable status: 2 of 7 files reviewed, 4 unresolved discussions (waiting on @ilyalesokhin-starkware and @TomerStarkware)

crates/cairo-lang-semantic/src/types.rs line 246 at r2 (raw file):

i think only the impl or trait itself may have params - not the type itself.

crates/cairo-lang-semantic/src/expr/inference.rs line 1193 at r3 (raw file):

        }

        let mut generic_params = OrderedHashSet::default();

or something? without the specificity it is kinda unclear.

Suggestion:

negatic_impl_generic_params

crates/cairo-lang-semantic/src/expr/inference.rs line 1198 at r3 (raw file):

                // If we fail to extract the generic parameters, return that the negative impl is
                // invalid.
                return Ok(SolutionSet::None);

or something?

Suggestion:

                return Ok(SolutionSet::Ambiguous(
                    Ambiguity::NegativeImplWithUnsupportedExtractedArgs { concrete_trait_id, ty },
                ));

crates/cairo-lang-semantic/src/types.rs line 215 at r3 (raw file):

    }

    /// A utility function for extractingthe generic paramaters arguments from TypeLongId.

Suggestion:

    /// A utility function for extracting the generic paramaters arguments from TypeLongId.

Reviewable status: 2 of 7 files reviewed, 5 unresolved discussions (waiting on @orizi and @TomerStarkware)

crates/cairo-lang-semantic/src/types.rs line 246 at r2 (raw file):

@TomerStarkware said:
"No, I think we have a todo for it"

What do you want to do about my TODO here?

crates/cairo-lang-semantic/src/expr/inference.rs line 1193 at r3 (raw file):

Done

crates/cairo-lang-semantic/src/expr/inference.rs line 1198 at r3 (raw file):

Done.

crates/cairo-lang-semantic/src/types.rs line 215 at r3 (raw file):

    }

    /// A utility function for extractingthe generic paramaters arguments from TypeLongId.

Done.

@orizi reviewed 1 of 5 files at r2, 3 of 4 files at r4, all commit messages.
Reviewable status: 6 of 7 files reviewed, 5 unresolved discussions (waiting on @TomerStarkware)

crates/cairo-lang-semantic/src/types.rs line 246 at r2 (raw file):

i think it won't have a useful effect - remove IMO.

crates/cairo-lang-semantic/src/types.rs line 260 at r4 (raw file):

                }
            }
            TypeLongId::Missing(diag_added) => return Maybe::Err(*diag_added),

it seems error can only happen if the system is broken?

crates/cairo-lang-semantic/src/types.rs line 260 at r4 (raw file):

                }
            }
            TypeLongId::Missing(diag_added) => return Maybe::Err(*diag_added),

Suggestion:

            TypeLongId::Missing(diag_added) => return Err(*diag_added),

crates/cairo-lang-semantic/src/expr/inference/solver.rs line 97 at r4 (raw file):

            }
            Ambiguity::NegativeImplWithUnsupportedExtractedArgs(garg) => {
                format!("Negative impl has an unsupported {:?}.", garg.debug(db),)

Suggestion:

                format!("Negative impl has an unsupported arg {:?}.", garg.debug(db),)

Reviewable status: 3 of 7 files reviewed, 4 unresolved discussions (waiting on @orizi and @TomerStarkware)

crates/cairo-lang-semantic/src/types.rs line 246 at r2 (raw file):

Done.

crates/cairo-lang-semantic/src/expr/inference/solver.rs line 97 at r4 (raw file):

            }
            Ambiguity::NegativeImplWithUnsupportedExtractedArgs(garg) => {
                format!("Negative impl has an unsupported {:?}.", garg.debug(db),)

Done.

crates/cairo-lang-semantic/src/types.rs line 260 at r4 (raw file):

                }
            }
            TypeLongId::Missing(diag_added) => return Maybe::Err(*diag_added),

Done.

@orizi reviewed 1 of 4 files at r4, 3 of 3 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware)

crates/cairo-lang-semantic/src/types.rs line 260 at r4 (raw file):

this is probably better if we think there may be issues.
but we can probably change to "unexpected behavior - report" or something.

Reviewable status: all files reviewed, 1 unresolved discussion

crates/cairo-lang-semantic/src/types.rs line 241 at r5 (raw file):

            TypeLongId::Coupon(_) => {}
            TypeLongId::FixedSizeArray { type_id, .. } => {
                type_id.long(db).extract_generic_params(db, generic_parameters)?

The size of the array can be generic as well,