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

Added support for path types in contract/component storage members #3995

Merged
merged 1 commit into from
Sep 3, 2023

Conversation

yuvalsw
Copy link
Contributor

@yuvalsw yuvalsw commented Aug 31, 2023

This change is Reviewable

@yuvalsw yuvalsw requested a review from orizi August 31, 2023 12:34
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 4 files at r1.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @yuvalsw)


crates/cairo-lang-starknet/src/plugin/storage.rs line 183 at r1 (raw file):

        }
        None => {
            // TODO(yg): same for key_type and value_type in the legacy case.

unless done in this PR - or at a directly following PR - please elaborate more.


crates/cairo-lang-starknet/src/plugin/storage.rs line 216 at r1 (raw file):

        // brought to the inner module by a extra `use`).
        return type_node;
    }

wouldn't just adding an additional super in case we have started with super will work?
this solution won't work for core::interger::u8 which should be valid.

Code quote:

    let type_node = RewriteNode::new_trimmed(type_ast.as_syntax_node());
    let ast::Expr::Path(type_path) = type_ast else {
        return type_node;
    };
    if type_path.elements(db).len() == 1 {
        // No need to add `super::` as this is a corelib type or a locally defined type (which is
        // brought to the inner module by a extra `use`).
        return type_node;
    }

crates/cairo-lang-starknet/src/plugin/plugin_test_data/components/component line 22 at r1 (raw file):

        map: LegacyMap<u32, u32>,
        my_type_var: super::MyType,
    }

only relative paths did not work before - right?

@yuvalsw yuvalsw force-pushed the yuval/allow_path_types_contract branch from 0812307 to 2ce77bf Compare August 31, 2023 15:40
Copy link
Contributor Author

@yuvalsw yuvalsw 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 4 files reviewed, 3 unresolved discussions (waiting on @orizi)


crates/cairo-lang-starknet/src/plugin/storage.rs line 183 at r1 (raw file):

Previously, orizi wrote…

unless done in this PR - or at a directly following PR - please elaborate more.

Yes, forgot to do before sending.


crates/cairo-lang-starknet/src/plugin/storage.rs line 216 at r1 (raw file):

Previously, orizi wrote…

wouldn't just adding an additional super in case we have started with super will work?
this solution won't work for core::interger::u8 which should be valid.

Done.


crates/cairo-lang-starknet/src/plugin/plugin_test_data/components/component line 22 at r1 (raw file):

Previously, orizi wrote…

only relative paths did not work before - right?

Good catch! thanks, fixed.

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 2 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @yuvalsw)

@yuvalsw yuvalsw added this pull request to the merge queue Sep 3, 2023
Merged via the queue into main with commit a0d25a2 Sep 3, 2023
@orizi orizi deleted the yuval/allow_path_types_contract branch September 4, 2023 07:52
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.

2 participants