Skip to content
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

Handling use star in Semantics. #6592

Merged
merged 1 commit into from
Nov 24, 2024
Merged

Conversation

Tomer-StarkWare
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a 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 8 files reviewed, 1 unresolved discussion (waiting on @orizi and @Tomer-StarkWare)


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

/// Returns all the path star under a given use item.
/// Notice that UsePath::Star is not returned here.

Copy link
Collaborator

@orizi orizi left a 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 8 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @Tomer-StarkWare)


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

)

//! > expected_diagnostics

add a proper test.

this doesn;t actually use a::b::*;

add a test that gets diagnostics if the use path is bad.

Code quote:

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

//! > Testing use star

//! > test_runner_name
test_expr_semantics(expect_diagnostics: false)

//! > module_code
pub mod generic_type {
    pub struct S {
        pub x: u8,
    }
    pub enum E {
        A: u8,
        B: u16,
    }
}

use generic_type::*;

//! > function_body

//! > expr_code
{

}

//! > expected_semantics
Block(
    ExprBlock {
        statements: [],
        tail: None,
        ty: (),
    },
)

//! > expected_diagnostics

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-semantic branch from fe47217 to e4048a1 Compare November 10, 2024 08:28
Copy link
Collaborator Author

@Tomer-StarkWare Tomer-StarkWare left a 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 8 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @orizi)


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

Previously, orizi wrote…

add a proper test.

this doesn;t actually use a::b::*;

add a test that gets diagnostics if the use path is bad.

Done.


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

/// Returns all the path star under a given use item.
/// Notice that UsePath::Star is not returned here.

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 6 of 8 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gilbens-starkware and @Tomer-StarkWare)


crates/cairo-lang-defs/src/db.rs line 801 at r2 (raw file):

/// Returns all the path star under a given use item.
/// Notice that UsePath::Leaf is not returned here.

how are the Notice thing relevant?
remove/

Suggestion:

/// Returns all the path leaves under a given use item.
pub fn get_all_path_leaves(db: &dyn SyntaxGroup, use_item: &ast::ItemUse) -> Vec<ast::UsePathLeaf> {
    let mut res = vec![];
    let mut stack = vec![use_item.use_path(db)];
    while let Some(use_path) = stack.pop() {
        match use_path {
            ast::UsePath::Leaf(use_path) => res.push(use_path),
            ast::UsePath::Single(use_path) => stack.push(use_path.use_path(db)),
            ast::UsePath::Multi(use_path) => {
                stack.extend(use_path.use_paths(db).elements(db).into_iter().rev())
            }
            ast::UsePath::Star(_) => {}
        }
    }
    res
}

/// Returns all the path stars under a given use item.

crates/cairo-lang-defs/src/db.rs line 892 at r2 (raw file):

}
pub fn module_global_use_by_id(
    db: &dyn DefsGroup,

Suggestion:

    let module_uses = db.module_uses(use_id.module_file_id(db.upcast()).0)?;
    Ok(module_uses.get(&use_id).cloned())
}

pub fn module_global_use_by_id(
    db: &dyn DefsGroup,

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

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

//! > Testing use star without implementation in Semantic

Add a TODO stating this gives a bad result for now.


crates/cairo-lang-semantic/src/items/module.rs line 115 at r2 (raw file):

        }
    }
    let global_uses = db.module_global_uses(module_id)?.keys().copied().collect::<Vec<_>>();

the visiblity of global uses is also required to be part of the semantic data.

you probably need a pub global_uses: OrderedHashMap<GlobalUseId, Visibility>,

Code quote:

    let global_uses = db.module_global_uses(module_id)?.keys().copied().collect::<Vec<_>>();

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-semantic branch from e4048a1 to 5436296 Compare November 10, 2024 14:21
@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-formatter branch from 552e6fe to a2b87c5 Compare November 10, 2024 15:02
@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-semantic branch from 5436296 to 3867f96 Compare November 10, 2024 15:04
Copy link
Collaborator Author

@Tomer-StarkWare Tomer-StarkWare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


crates/cairo-lang-defs/src/db.rs line 801 at r2 (raw file):

Previously, orizi wrote…

how are the Notice thing relevant?
remove/

Done.


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

Previously, orizi wrote…

Add a TODO stating this gives a bad result for now.

Done.


crates/cairo-lang-semantic/src/items/module.rs line 115 at r2 (raw file):

Previously, orizi wrote…

the visiblity of global uses is also required to be part of the semantic data.

you probably need a pub global_uses: OrderedHashMap<GlobalUseId, Visibility>,

Done.


crates/cairo-lang-defs/src/db.rs line 892 at r2 (raw file):

}
pub fn module_global_use_by_id(
    db: &dyn DefsGroup,

Done.

Copy link
Collaborator

@orizi orizi left a 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 r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @Tomer-StarkWare)


crates/cairo-lang-semantic/src/items/module.rs line 38 at r3 (raw file):

    pub global_uses: OrderedHashMap<GlobalUseId, cairo_lang_syntax::node::ast::Visibility>,
    pub diagnostics: Diagnostics<SemanticDiagnostic>,
}

Suggestion:

    pub items: OrderedHashMap<SmolStr, ModuleItemInfo>,
    pub global_uses: OrderedHashMap<GlobalUseId, Visibility>,
    pub diagnostics: Diagnostics<SemanticDiagnostic>,
}

crates/cairo-lang-semantic/src/items/module.rs line 123 at r3 (raw file):

            (*global_use_id, item.visibility(syntax_db))
        })
        .collect();

Suggestion:

    let global_uses = db
        .module_global_uses(module_id)?
        .iter()
        .map(|(global_use_id, use_path_star)| {
            let item = ast::UsePath::Star(use_path_star.clone()).get_item(syntax_db);
            (*global_use_id, Visibility::from_ast(db.upcast(), &mut diagnostics, &item.visibility(syntax_db)))
        })
        .collect();

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-semantic branch from 3867f96 to 46e971a Compare November 10, 2024 15:31
Copy link
Collaborator Author

@Tomer-StarkWare Tomer-StarkWare left a 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 @gilbens-starkware and @orizi)


crates/cairo-lang-semantic/src/items/module.rs line 38 at r3 (raw file):

    pub global_uses: OrderedHashMap<GlobalUseId, cairo_lang_syntax::node::ast::Visibility>,
    pub diagnostics: Diagnostics<SemanticDiagnostic>,
}

Done.


crates/cairo-lang-semantic/src/items/module.rs line 123 at r3 (raw file):

            (*global_use_id, item.visibility(syntax_db))
        })
        .collect();

Done.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware)

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-semantic branch from 46e971a to a632be5 Compare November 11, 2024 12:58
Copy link
Collaborator

@orizi orizi left a 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 r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @Tomer-StarkWare)


crates/cairo-lang-semantic/src/items/module.rs line 122 at r5 (raw file):

            let item = ast::UsePath::Star(use_path_star.clone()).get_item(syntax_db);
            let visibility = item.visibility(syntax_db);
            if !matches!(visibility, ast::Visibility::Default(_)) {

add TODO to consider adding these.

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

:lgtm:

Reviewed 3 of 8 files at r1, 3 of 4 files at r3, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Tomer-StarkWare)

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-semantic branch from a632be5 to a0d581a Compare November 13, 2024 15:18
Copy link
Collaborator Author

@Tomer-StarkWare Tomer-StarkWare left a 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 @orizi)


crates/cairo-lang-semantic/src/items/module.rs line 122 at r5 (raw file):

Previously, orizi wrote…

add TODO to consider adding these.

Done.

Copy link
Collaborator

@orizi orizi left a 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 r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Tomer-StarkWare)

@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-formatter branch 2 times, most recently from 2e75753 to 6c83f7b Compare November 24, 2024 14:06
@Tomer-StarkWare Tomer-StarkWare force-pushed the tomerc/use-star-semantic branch from a0d581a to 028038e Compare November 24, 2024 14:27
@Tomer-StarkWare Tomer-StarkWare changed the base branch from tomerc/use-star-formatter to main November 24, 2024 14:27
Copy link
Collaborator

@orizi orizi left a 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 r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Tomer-StarkWare)

@Tomer-StarkWare Tomer-StarkWare added this pull request to the merge queue Nov 24, 2024
Merged via the queue into main with commit bdacb58 Nov 24, 2024
48 checks passed
@orizi orizi deleted the tomerc/use-star-semantic branch November 28, 2024 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants