statement types in resolver

[Tomer-StarkWare]

This change is 

[gilbens-starkware]

Reviewed 1 of 9 files at r1, all commit messages.
Reviewable status: 1 of 9 files reviewed, 6 unresolved discussions (waiting on @orizi and @Tomer-StarkWare)

---

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

                format!(r#"Multiple definitions of type "{}"."#, type_name)
            }
            SemanticDiagnosticKind::UnusedType => "Unused type.".into(),

You haven't actually added support of type definition in functions, just importing them, so the correct diagnostic is UnsedUse.
And for the multiple types, I think we want one diagnostic type, for multiple items with the same name, and as Ori mentioned there should be two groups of items, variable like (e.g.) and proper items, so probably one diagnostic for each group.

Code quote:

            SemanticDiagnosticKind::MultipleTypeDefinition(type_name) => {
                format!(r#"Multiple definitions of type "{}"."#, type_name)
            }
            SemanticDiagnosticKind::UnusedType => "Unused type.".into(),

---

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

// TODO(spapini): add a query wrapper.
/// Resolves a type given a module and a path.
/// Type called not inside a statement

Let the formatter break the lines.

Suggestion:

/// Resolves a type given a module and a path. Used for resolving from non-statement context.

---

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

    resolve_type_ex(db, diagnostics, resolver, ty_syntax, None)
}
/// Resolves a type given a module and a path.

Suggestion:

/// Resolves a type given a module and a path. `statement_env` should be provided if called from statement context.

---

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

}
/// Resolves a type given a module and a path.
pub fn resolve_type_ex(

Consider changing this function to resolve_type_with_enviorment and make the above function call directly to maybe_resolve_type.

Code quote:

/// Resolves a type given a module and a path.
pub fn resolve_type_ex(

---

crates/cairo-lang-semantic/src/expr/compute.rs line 238 at r2 (raw file):

            self.diagnostics.report(statement_ty.stable_ptr, UnusedType);
        }
    }

This function is called only once, inline it.

Code quote:

    /// Adds warning for unused types if required.
    fn add_unused_types_warning(&mut self, ty_name: &str, statement_ty: &StatementType) {
        if !self.environment.used_types.contains(ty_name) && !ty_name.starts_with('_') {
            self.diagnostics.report(statement_ty.stable_ptr, UnusedType);
        }
    }

---

crates/cairo-lang-semantic/src/expr/compute.rs line 297 at r2 (raw file):

#[derive(Clone, Debug, PartialEq, Eq, DebugWithDb)]
#[debug_db(dyn SemanticGroup + 'static)]
struct StatementType {

It isn't necessarily a type,

Code quote:

struct StatementType {

[Tomer-StarkWare]

Reviewable status: 1 of 9 files reviewed, 6 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

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

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

You haven't actually added support of type definition in functions, just importing them, so the correct diagnostic is UnsedUse.
And for the multiple types, I think we want one diagnostic type, for multiple items with the same name, and as Ori mentioned there should be two groups of items, variable like (e.g.) and proper items, so probably one diagnostic for each group.

I'll set the Unused Type in use to Unused Use, but regarding the variables and proper items, Rust allows both use X::R, Y::R for:
mod X {
type R = ...;
}
mod Y {
const R = ...;
}
Meaning Const and Types don't collide, to do the same here?

[orizi]

Reviewable status: 1 of 9 files reviewed, 7 unresolved discussions (waiting on @gilbens-starkware and @Tomer-StarkWare)

---

corelib/src/test/language_features/block_level_items_test.cairo line 51 at r2 (raw file):

pub mod SingleConstMod {
    pub const A: u8 = 1;
}

snake case all mods - and don't have the work Mod as part of it.

Suggestion:

pub mod single_const {
    pub const A: u8 = 1;
}

[gilbens-starkware]

Reviewable status: 1 of 9 files reviewed, 7 unresolved discussions (waiting on @orizi and @Tomer-StarkWare)

---

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

Previously, Tomer-StarkWare (TomerC-StarkWare) wrote…

I'll set the Unused Type in use to Unused Use, but regarding the variables and proper items, Rust allows both use X::R, Y::R for:
mod X {
type R = ...;
}
mod Y {
const R = ...;
}
Meaning Const and Types don't collide, to do the same here?

Yes, const and types shouldn't collide.

[Tomer-StarkWare]

Reviewable status: 1 of 9 files reviewed, 7 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

corelib/src/test/language_features/block_level_items_test.cairo line 51 at r2 (raw file):

Previously, orizi wrote…

snake case all mods - and don't have the work Mod as part of it.

Done.

---

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

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Yes, const and types shouldn't collide.

Done.

---

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

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Let the formatter break the lines.

Done.

---

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

    resolve_type_ex(db, diagnostics, resolver, ty_syntax, None)
}
/// Resolves a type given a module and a path.

Done.

---

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

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

Consider changing this function to resolve_type_with_enviorment and make the above function call directly to maybe_resolve_type.

Done.

---

crates/cairo-lang-semantic/src/expr/compute.rs line 238 at r2 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

This function is called only once, inline it.

Done.

---

crates/cairo-lang-semantic/src/expr/compute.rs line 297 at r2 (raw file):

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

It isn't necessarily a type,

Done.

[orizi]

Reviewed 3 of 4 files at r3, all commit messages.
Reviewable status: 3 of 9 files reviewed, 7 unresolved discussions (waiting on @gilbens-starkware and @Tomer-StarkWare)

---

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

#[derive(Clone, Debug, PartialEq, Eq, DebugWithDb)]
#[debug_db(dyn SemanticGroup + 'static)]
struct StatementGeneric {

rename both EnvTypes and StatementGeneric to relevant names.

Code quote:

type EnvTypes = OrderedHashMap<SmolStr, StatementGeneric>;

/// Struct that holds the resolved generic type of a statement item.
#[derive(Clone, Debug, PartialEq, Eq, DebugWithDb)]
#[debug_db(dyn SemanticGroup + 'static)]
struct StatementGeneric {

[Tomer-StarkWare]

Reviewable status: 3 of 9 files reviewed, 7 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

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

Previously, orizi wrote…

rename both EnvTypes and StatementGeneric to relevant names.

Done.

[gilbens-starkware]

Reviewable status: 3 of 9 files reviewed, 5 unresolved discussions (waiting on @orizi and @Tomer-StarkWare)

---

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

            }
            SemanticDiagnosticKind::MultipleGenericItemDefinition(type_name) => {
                format!(r#"Multiple definitions of generic item "{}"."#, type_name)

Suggestion:

            SemanticDiagnosticKind::MultipleItemDefinition(type_name) => {
                format!(r#"Multiple definitions of an item "{}"."#, type_name)

---

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

/// Resolves a type given a module and a path. `statement_env` should be provided if called from
/// statement context.
pub fn resolve_type_with_statement(

Suggestion:

with_enviorment

[Tomer-StarkWare]

Reviewable status: 3 of 9 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

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

            }
            SemanticDiagnosticKind::MultipleGenericItemDefinition(type_name) => {
                format!(r#"Multiple definitions of generic item "{}"."#, type_name)

There is also a Multiple Binding Definition, and Bindings are also Items. Should I merge them together?

[Tomer-StarkWare]

Reviewable status: 3 of 9 files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @orizi)

---

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

/// Resolves a type given a module and a path. `statement_env` should be provided if called from
/// statement context.
pub fn resolve_type_with_statement(

Done.

[mkaput]

Reviewed 2 of 6 files at r6.
Reviewable status: 3 of 12 files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @orizi, and @piotmag769)

[orizi]

Reviewed 1 of 6 files at r6, all commit messages.
Reviewable status: 4 of 12 files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @piotmag769, and @Tomer-StarkWare)

---

crates/cairo-lang-semantic/src/expr/compute.rs line 307 at r7 (raw file):

    used_variables: UnorderedHashSet<semantic::VarId>,
    use_types: EnvGenericItems,
    used_use_types: UnorderedHashSet<SmolStr>,

these aren't types.

i think you should use the word - Item all around - probably also drop the "generic".

Code quote:

    use_types: EnvGenericItems,
    used_use_types: UnorderedHashSet<SmolStr>,

[Tomer-StarkWare]

Reviewable status: 4 of 12 files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, @orizi, and @piotmag769)

---

crates/cairo-lang-semantic/src/expr/compute.rs line 307 at r7 (raw file):

Previously, orizi wrote…

these aren't types.

i think you should use the word - Item all around - probably also drop the "generic".

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 4 of 12 files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, and @Tomer-StarkWare)

---

crates/cairo-lang-semantic/src/expr/compute.rs line 343 at r8 (raw file):

/// Returns the requested type from the environment if it exists. Returns None otherwise.
pub fn get_statement_type_by_name(

fix name - type --> item.

[Tomer-StarkWare]

Reviewable status: 4 of 12 files reviewed, 5 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, and @orizi)

---

crates/cairo-lang-semantic/src/expr/compute.rs line 343 at r8 (raw file):

Previously, orizi wrote…

fix name - type --> item.

Done.

[orizi]

Reviewed 3 of 9 files at r1, 1 of 3 files at r9.
Reviewable status: 8 of 13 files reviewed, 10 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, and @Tomer-StarkWare)

---

crates/cairo-lang-semantic/src/expr/compute.rs line 207 at r9 (raw file):

            self.add_unused_binding_warning(&var_name, &var);
        }
        // Adds warning for unused types if required.

Suggestion:

// Adds warning for unused items if required.

---

crates/cairo-lang-semantic/src/expr/semantic_test_data/use line 201 at r9 (raw file):

warning[E0001]: Unused variable. Consider ignoring by prefixing with `_`.
 --> lib.cairo:11:9
    let x = R { a: 4 }; 

add _ remove irrelevant diagnostics.

everywhere.

Code quote:

warning[E0001]: Unused variable. Consider ignoring by prefixing with `_`.
 --> lib.cairo:9:9
    let y = RR { a: 3 };
        ^

warning[E0001]: Unused variable. Consider ignoring by prefixing with `_`.
 --> lib.cairo:11:9
    let x = R { a: 4 };

---

crates/cairo-lang-semantic/src/resolve/item.rs line 97 at r9 (raw file):

    Trait(ConcreteTraitId),
    Impl(ImplId),
    Statement(ResolvedGenericItem),

remove.

---

crates/cairo-lang-semantic/src/resolve/mod.rs line 423 at r9 (raw file):

                    }
                    ResolvedBase::StatementEnvironment(generic_item) => {
                        ResolvedConcreteItem::Statement(generic_item)

Suggestion:

                        segments.next();
                        function_that_adds_generic_args_to_concretize(generic_item, generics)

---

crates/cairo-lang-semantic/src/resolve/mod.rs line 450 at r9 (raw file):

                        ),
                        ResolvedBase::StatementEnvironment(generic_item) => {
                            ResolvedConcreteItem::Statement(generic_item)

Suggestion:

                        segments.next();
                        function_that_adds_generic_args_to_concretize(generic_item, generics)

---

crates/cairo-lang-semantic/src/resolve/mod.rs line 673 at r9 (raw file):

                    segment_stable_ptr,
                );
                Ok(specialized_item)

this is the logic of the function described above.

Code quote:

            ResolvedConcreteItem::Statement(inner_generic_item) => {
                let segment_stable_ptr = segment.stable_ptr().untyped();
                let specialized_item = self.specialize_generic_module_item(
                    diagnostics,
                    identifier,
                    inner_generic_item.clone(),
                    generic_args_syntax.clone(),
                )?;
                self.warn_same_impl_trait(
                    diagnostics,
                    &specialized_item,
                    &generic_args_syntax.unwrap_or_default(),
                    segment_stable_ptr,
                );
                Ok(specialized_item)

[Tomer-StarkWare]

Reviewable status: 6 of 15 files reviewed, 10 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, and @orizi)

---

crates/cairo-lang-semantic/src/expr/compute.rs line 207 at r9 (raw file):

            self.add_unused_binding_warning(&var_name, &var);
        }
        // Adds warning for unused types if required.

Done.

---

crates/cairo-lang-semantic/src/expr/semantic_test_data/use line 201 at r9 (raw file):

Previously, orizi wrote…

add _ remove irrelevant diagnostics.

everywhere.

Done.

---

crates/cairo-lang-semantic/src/resolve/mod.rs line 423 at r9 (raw file):

                    }
                    ResolvedBase::StatementEnvironment(generic_item) => {
                        ResolvedConcreteItem::Statement(generic_item)

Done.

---

crates/cairo-lang-semantic/src/resolve/mod.rs line 450 at r9 (raw file):

                        ),
                        ResolvedBase::StatementEnvironment(generic_item) => {
                            ResolvedConcreteItem::Statement(generic_item)

Done.

---

crates/cairo-lang-semantic/src/resolve/mod.rs line 673 at r9 (raw file):

Previously, orizi wrote…

this is the logic of the function described above.

Done.

---

crates/cairo-lang-semantic/src/resolve/item.rs line 97 at r9 (raw file):

Previously, orizi wrote…

remove.

Done.

[mkaput]

Reviewed 3 of 9 files at r1, 1 of 4 files at r3, 1 of 3 files at r5, 8 of 8 files at r11, 2 of 2 files at r12, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, and @orizi)

[orizi]

Reviewed 2 of 8 files at r11, 2 of 2 files at r12.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Arcticae, @Draggu, @gilbens-starkware, and @Tomer-StarkWare)

---

crates/cairo-lang-semantic/src/expr/semantic_test_data/use line 667 at r12 (raw file):

//! > ==========================================================================

//! > Test use type enum and const together

i don't understand the test.

---

crates/cairo-lang-semantic/src/resolve/mod.rs line 397 at r12 (raw file):

    }

    fn generic_arg_to_concrete(

doc.
move to the end - as this is a helper.