@orizi reviewed 1 of 14 files at r1, all commit messages.
Reviewable status: 1 of 14 files reviewed, 5 unresolved discussions (waiting on @eytan-starkware, @giladchase, and @TomerStarkware)

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

            }
            SemanticDiagnosticKind::AmbiguousTrait {
                context_module,

maybe we should go the other way - getting the context externally - as we collect the diagnostics per module to begin with.

crates/cairo-lang-defs/src/ids.rs line 73 at r1 (raw file):

    fn full_path(&self, db: &'db dyn Database) -> String {
        self.path_segments(db).iter().map(|s| s.to_string(db)).collect::<Vec<_>>().join("::")

this seems less efficient.

crates/cairo-lang-defs/src/ids.rs line 328 at r1 (raw file):

    pub fn full_path(&self, db: &'db dyn Database) -> String {
        self.path_segments(db).iter().map(|s| s.to_string(db)).collect::<Vec<_>>().join("::")

ditto.

crates/cairo-lang-defs/src/ids.rs line 512 at r1 (raw file):

impl<'db> ImportableId<'db> {
    pub fn parent_module(&self, db: &'db dyn Database) -> Option<ModuleId<'db>> {

doc

crates/cairo-lang-defs/src/ids.rs line 531 at r1 (raw file):

    }

    pub fn name(&self, db: &'db dyn Database) -> SmolStrId<'db> {

doc

@eytan-starkware made 5 comments.
Reviewable status: 1 of 14 files reviewed, 5 unresolved discussions (waiting on @giladchase, @orizi, and @TomerStarkware).

crates/cairo-lang-defs/src/ids.rs line 73 at r1 (raw file):

Done.

crates/cairo-lang-defs/src/ids.rs line 328 at r1 (raw file):

Done.

crates/cairo-lang-defs/src/ids.rs line 512 at r1 (raw file):

Done.

crates/cairo-lang-defs/src/ids.rs line 531 at r1 (raw file):

Done.

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

Moved on top of a new PR that puts module as part of the SemanticDiagnostic.

@orizi reviewed 1 file and all commit messages, made 1 comment, and resolved 4 discussions.
Reviewable status: 1 of 17 files reviewed, 2 unresolved discussions (waiting on @eytan-starkware, @giladchase, and @TomerStarkware).

crates/cairo-lang-filesystem/src/ids.rs line 453 at r2 (raw file):

            result.push_str(&s.to_string(db));
        }
        result

my issue was the to-string and vector collection - you don't need them.

(and you probably don't need the function.

Suggestion:

        segments.iter().map(|s| s.long(db)).join(seperator)

@orizi reviewed 4 files, made 1 comment, and resolved 1 discussion.
Reviewable status: 5 of 17 files reviewed, 2 unresolved discussions (waiting on @eytan-starkware, @giladchase, and @TomerStarkware).

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

        TraitInferenceErrors<'db>,
    ) -> Option<SemanticDiagnosticKind<'db>>,
    multiple_trait_diagnostic: impl Fn(

still relevant?

@orizi reviewed 2 files and all commit messages, and made 5 comments.
Reviewable status: 5 of 24 files reviewed, 7 unresolved discussions (waiting on @eytan-starkware, @giladchase, and @TomerStarkware).

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

pub struct ItemAccessInfo<'db> {
    /// The shortest path segments to access the item from the context.
    pub path_segments: Vec<ImportableId<'db>>,

doc that it is reversed in relation to actual path (at least i assume so according to path())

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

            | ItemAccessKind::ViaGlobalUse
            | ItemAccessKind::FullPath(_) => "".to_string(),
        };

both are &' static - so no issue.

Suggestion:

        let prefix = match self.access_kind {
            ItemAccessKind::ViaCrate => "crate::",
            ItemAccessKind::DirectInModule
            | ItemAccessKind::ViaUse(_)
            | ItemAccessKind::ViaPrelude
            | ItemAccessKind::ViaGlobalUse
            | ItemAccessKind::FullPath(_) => "",
        };

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

            | ItemAccessKind::FullPath(_) => "".to_string(),
        };
        let mut path = self.path_segments.iter().rev().map(|id| id.name(db).long(db)).join("::");

comment about the rev.

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

        } else {
            path = format!("{path}::{}", self.name.long(db));
        }

Suggestion:

        let path = chain!(self.path_segments.iter().rev().map(|id|id.name(db)), [self.name]).map(|name| name.long(db)).join("::");

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

            path = format!("{path}::{}", self.name.long(db));
        }
        format!("{}{}", prefix, path)

Suggestion:

        format!("{prefix}{path}")

@orizi made 6 comments.
Reviewable status: 5 of 24 files reviewed, 13 unresolved discussions (waiting on @eytan-starkware, @giladchase, and @TomerStarkware).

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

///
/// Access strategies tried:
/// 1. Direct access (item in current module)

isn't prelude first?

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

}

fn check_prelude<'db>(

doc

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

    item: ImportableId<'db>,
    context_module: ModuleId<'db>,
) -> Result<Option<ItemAccessInfo<'db>>, cairo_lang_diagnostics::DiagnosticAdded> {

Suggestion:

) -> Maybe<Option<ItemAccessInfo<'db>>> {

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

) -> Result<Option<ItemAccessInfo<'db>>, cairo_lang_diagnostics::DiagnosticAdded> {
    let settings = db.crate_config(context_module.owning_crate(db)).map(|c| c.settings.clone());
    if let Some(prelude_module) = db.get_prelude_submodule(&settings.unwrap_or_default())

feels like this clone can be avoided.

Code quote:

    let settings = db.crate_config(context_module.owning_crate(db)).map(|c| c.settings.clone());
    if let Some(prelude_module) = db.get_prelude_submodule(&settings.unwrap_or_default())

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

}

trait IntoImportableMarker<'db>: Into<ImportableId<'db>> {}

doc

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

    for<'a> &'a T: IntoImportableMarker<'db>,
{
    fn contextualized_path(

doc

@orizi reviewed 19 files and resolved 1 discussion.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @eytan-starkware, @giladchase, and @TomerStarkware).

@eytan-starkware made 13 comments.
Reviewable status: 20 of 24 files reviewed, 12 unresolved discussions (waiting on @giladchase, @orizi, and @TomerStarkware).

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

Done.

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

Done.

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

Done.

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

It definitely is, but this whole comment is wrong by now.

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

Done.

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

Yes, but we need to create a default value somewhere leading to allocations either way (or a query we had from before)

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

Done.

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

The trait function is documented in the definition of the trait

crates/cairo-lang-filesystem/src/ids.rs line 453 at r2 (raw file):

Done

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

Apparently not

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

        } else {
            path = format!("{path}::{}", self.name.long(db));
        }

Done.

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

            path = format!("{path}::{}", self.name.long(db));
        }
        format!("{}{}", prefix, path)

Done.

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

    item: ImportableId<'db>,
    context_module: ModuleId<'db>,
) -> Result<Option<ItemAccessInfo<'db>>, cairo_lang_diagnostics::DiagnosticAdded> {

Done.

@orizi reviewed 4 files and all commit messages, made 1 comment, and resolved 12 discussions.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase and @TomerStarkware).

@orizi reviewed 2 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @giladchase and @TomerStarkware).