diff --git a/crates/cairo-lang-starknet/src/abi_test.rs b/crates/cairo-lang-starknet/src/abi_test.rs index 022cc79f863..34050078f4d 100644 --- a/crates/cairo-lang-starknet/src/abi_test.rs +++ b/crates/cairo-lang-starknet/src/abi_test.rs @@ -6,8 +6,7 @@ use cairo_lang_test_utils::parse_test_file::TestRunnerResult; use cairo_lang_test_utils::{get_direct_or_file_content, verify_diagnostics_expectation}; use cairo_lang_utils::ordered_hash_map::OrderedHashMap; -use super::BuilderConfig; -use crate::abi::AbiBuilder; +use super::{AbiBuilder, BuilderConfig}; use crate::plugin::consts::CONTRACT_ATTR; use crate::starknet_plugin_suite; @@ -57,3 +56,33 @@ cairo_lang_test_utils::test_file_test!( }, test_abi_failure ); + +/// Helper function for testing multiple Storage path accesses to the same place. +pub fn test_storage_path_check( + inputs: &OrderedHashMap, + args: &OrderedHashMap, +) -> TestRunnerResult { + let db = &mut RootDatabase::builder() + .detect_corelib() + .with_plugin_suite(starknet_plugin_suite()) + .build() + .unwrap(); + let (_, cairo_code) = get_direct_or_file_content(&inputs["cairo_code"]); + let (_, diagnostics) = setup_test_module(db, &cairo_code).split(); + + let test_error = verify_diagnostics_expectation(args, &diagnostics); + + TestRunnerResult { + outputs: OrderedHashMap::from([("diagnostics".into(), diagnostics)]), + error: test_error, + } +} + +cairo_lang_test_utils::test_file_test!( + storage_path_check, + "src/test_data", + { + storage_path_check: "storage_path_check", + }, + test_storage_path_check +); diff --git a/crates/cairo-lang-starknet/src/analyzer.rs b/crates/cairo-lang-starknet/src/analyzer.rs index 6f07d4e8adc..3d584fe040c 100644 --- a/crates/cairo-lang-starknet/src/analyzer.rs +++ b/crates/cairo-lang-starknet/src/analyzer.rs @@ -2,14 +2,23 @@ use cairo_lang_defs::ids::ModuleId; use cairo_lang_defs::plugin::PluginDiagnostic; use cairo_lang_semantic::db::SemanticGroup; use cairo_lang_semantic::items::attribute::SemanticQueryAttrs; +use cairo_lang_semantic::items::structure::Member; use cairo_lang_semantic::plugin::AnalyzerPlugin; +use cairo_lang_semantic::{ConcreteTypeId, TypeLongId}; use cairo_lang_syntax::attribute::consts::STARKNET_INTERFACE_ATTR; use cairo_lang_syntax::node::helpers::QueryAttrs; -use cairo_lang_syntax::node::{TypedStablePtr, TypedSyntaxNode}; +use cairo_lang_syntax::node::ids::SyntaxStablePtrId; +use cairo_lang_syntax::node::{Terminal, TypedStablePtr, TypedSyntaxNode}; +use cairo_lang_utils::ordered_hash_map::OrderedHashMap; +use cairo_lang_utils::LookupIntern; +use smol_str::SmolStr; use crate::abi::{ABIError, AbiBuilder, BuilderConfig}; use crate::contract::module_contract; -use crate::plugin::consts::EMBEDDABLE_ATTR; +use crate::plugin::consts::{ + COMPONENT_ATTR, CONTRACT_ATTR, EMBEDDABLE_ATTR, FLAT_ATTR, STORAGE_ATTR, STORAGE_NODE_ATTR, + STORAGE_STRUCT_NAME, SUBSTORAGE_ATTR, +}; /// Plugin to add diagnostics for contracts for bad ABI generation. #[derive(Default, Debug)] @@ -74,3 +83,129 @@ fn add_abi_diagnostics( } } } + +/// Plugin to add diagnostics for contracts with multiple paths to the same location in storage. +#[derive(Default, Debug)] +pub struct StorageAnalyzer; + +impl AnalyzerPlugin for StorageAnalyzer { + fn diagnostics(&self, db: &dyn SemanticGroup, module_id: ModuleId) -> Vec { + let mut diagnostics = vec![]; + let Ok(module_structs) = db.module_structs(module_id) else { + return vec![]; + }; + + // Analyze all the members of the struct. + for (id, item) in module_structs.iter() { + if !item.has_attr(db.upcast(), STORAGE_NODE_ATTR) + && !item.has_attr(db.upcast(), STORAGE_ATTR) + && (item.name(db.upcast()).text(db.upcast()) != STORAGE_STRUCT_NAME + || (!module_id.has_attr(db, CONTRACT_ATTR).unwrap_or_default() + && !module_id.has_attr(db, COMPONENT_ATTR).unwrap_or_default())) + { + continue; + } + let paths_data = &mut StorageStructMembers { name_to_paths: OrderedHashMap::default() }; + let Ok(members) = db.struct_members(*id) else { continue }; + for (member_name, member) in members.iter() { + member_analyze( + db, + member, + member_name.clone(), + paths_data, + &mut vec![], + member + .id + .stable_ptr(db.upcast()) + .lookup(db.upcast()) + .name(db.upcast()) + .stable_ptr() + .untyped(), + &mut diagnostics, + ); + } + } + diagnostics + } +} + +/// Helper for the storage analyzer. +pub struct StorageStructMembers { + /// Maps the name in actual storage to the path in actual user code. + pub name_to_paths: OrderedHashMap>, +} + +impl StorageStructMembers { + fn handle( + &mut self, + member_name: SmolStr, + path_to_member: Vec, + pointer_to_code: SyntaxStablePtrId, + diagnostics: &mut Vec, + ) { + if let Some(existing_path) = self.name_to_paths.get(&member_name) { + diagnostics.push(PluginDiagnostic::warning( + pointer_to_code, + format!( + "The path `{}` collides with existing path `{}`.", + path_to_member.join("."), + existing_path.join(".") + ), + )); + } else { + self.name_to_paths.insert(member_name.clone(), path_to_member); + } + } +} + +/// This function is called recursively to analyze all the members of a struct. +/// For every member, it checks if it has the `flat` attribute. If it does, it +/// recursively analyzes all the members of the struct that the member points to. +/// Otherwise, it adds the path to the `StorageStructMembers` struct. +/// For example, if the path is: +/// member1.member2.member3 +/// where member1 is flat with a member member2 which is flat and a member member3 which is not +/// flat, The function will iterate until it reaches member3 and add the path +/// member1.member2.member3 to the `StorageStructMembers` struct. +fn member_analyze( + db: &dyn SemanticGroup, + member: &Member, + member_name: SmolStr, + paths_data: &mut StorageStructMembers, + user_data_path: &mut Vec, + pointer_to_code: SyntaxStablePtrId, + diagnostics: &mut Vec, +) { + user_data_path.push(member_name.clone()); + if !(member.id.stable_ptr(db.upcast()).lookup(db.upcast()).has_attr(db.upcast(), FLAT_ATTR) + || member + .id + .stable_ptr(db.upcast()) + .lookup(db.upcast()) + .has_attr(db.upcast(), SUBSTORAGE_ATTR)) + { + paths_data.handle(member_name, user_data_path.clone(), pointer_to_code, diagnostics); + user_data_path.pop(); + return; + } + let TypeLongId::Concrete(ConcreteTypeId::Struct(member_struct)) = member.ty.lookup_intern(db) + else { + paths_data.handle(member_name, user_data_path.clone(), pointer_to_code, diagnostics); + user_data_path.pop(); + return; + }; + for (inner_member_name, inner_member) in + db.struct_members(member_struct.lookup_intern(db).struct_id).unwrap().iter() + { + member_analyze( + db, + inner_member, + inner_member_name.clone(), + paths_data, + user_data_path, + pointer_to_code, + diagnostics, + ); + } + user_data_path.pop(); +} diff --git a/crates/cairo-lang-starknet/src/lib.rs b/crates/cairo-lang-starknet/src/lib.rs index 0827e8bc25b..44b83234f0f 100644 --- a/crates/cairo-lang-starknet/src/lib.rs +++ b/crates/cairo-lang-starknet/src/lib.rs @@ -24,7 +24,8 @@ pub fn starknet_plugin_suite() -> PluginSuite { .add_inline_macro_plugin::() .add_inline_macro_plugin::() .add_inline_macro_plugin::() - .add_analyzer_plugin::(); + .add_analyzer_plugin::() + .add_analyzer_plugin::(); suite } diff --git a/crates/cairo-lang-starknet/src/plugin/consts.rs b/crates/cairo-lang-starknet/src/plugin/consts.rs index 971865e727a..cca1c56a928 100644 --- a/crates/cairo-lang-starknet/src/plugin/consts.rs +++ b/crates/cairo-lang-starknet/src/plugin/consts.rs @@ -30,7 +30,7 @@ pub const INTERFACE_ATTR: &str = "starknet::interface"; pub(super) const DEPRECATED_CONTRACT_ATTR: &str = "contract"; pub const CONTRACT_ATTR: &str = "starknet::contract"; pub const CONTRACT_ATTR_ACCOUNT_ARG: &str = "account"; -pub(super) const COMPONENT_ATTR: &str = "starknet::component"; +pub const COMPONENT_ATTR: &str = "starknet::component"; pub const STORAGE_ATTR: &str = "storage"; pub const EXTERNAL_ATTR: &str = "external"; pub const EMBEDDABLE_ATTR: &str = "starknet::embeddable"; diff --git a/crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component b/crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component index 16a1cf6c9d1..7ceda6b9451 100644 --- a/crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component +++ b/crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component @@ -433,7 +433,7 @@ impl StorageStorageBaseMutCopy of core::traits::Copy::; //! > 2 components in a contract //! > test_runner_name -ExpandContractTestRunner(expect_diagnostics: false) +ExpandContractTestRunner(expect_diagnostics: warnings_only) //! > cairo_code #[starknet::component] @@ -1045,13 +1045,17 @@ impl StorageStorageBaseMutDrop of core::traits::Drop::; impl StorageStorageBaseMutCopy of core::traits::Copy::; //! > expected_diagnostics +warning: Plugin diagnostic: The path `component2_storage.data` collides with existing path `component1_storage.data`. + --> lib.cairo:26:9 + component2_storage: super::component2::Storage, + ^****************^ //! > ========================================================================== //! > 2 components in a contract. //! > test_runner_name -ExpandContractTestRunner(expect_diagnostics: false) +ExpandContractTestRunner(expect_diagnostics: warnings_only) //! > cairo_code #[starknet::component] @@ -1096,6 +1100,10 @@ mod test_contract { } //! > expected_diagnostics +warning: Plugin diagnostic: The path `component2_storage.data` collides with existing path `component1_storage.data`. + --> lib.cairo:26:9 + component2_storage: super::component2::Storage, + ^****************^ //! > generated_cairo_code lib.cairo: diff --git a/crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component_diagnostics b/crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component_diagnostics index e23f164d52b..9f2e22cd092 100644 --- a/crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component_diagnostics +++ b/crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component_diagnostics @@ -3493,6 +3493,11 @@ Note: currently with components, only an enum Event directly in the contract is component!(path: super::component2, storage: component2_storage, event: Comp2Event); ^********^ +warning: Plugin diagnostic: The path `component2_storage.data` collides with existing path `component1_storage.data`. + --> lib.cairo:27:9 + component2_storage: super::component2::Storage, + ^****************^ + //! > generated_cairo_code lib.cairo: diff --git a/crates/cairo-lang-starknet/src/test_data/storage_path_check b/crates/cairo-lang-starknet/src/test_data/storage_path_check new file mode 100644 index 00000000000..1cb75d2bead --- /dev/null +++ b/crates/cairo-lang-starknet/src/test_data/storage_path_check @@ -0,0 +1,185 @@ +//! > Test flat storage path check. + +//! > test_runner_name +test_storage_path_check(expect_diagnostics: true) + +//! > cairo_code +#[starknet::storage_node] +struct HasA { + a: felt252, +} + +#[starknet::storage_node] +struct ComponentNewStorage { + a: felt252, + #[flat] + has_a: HasA, +} + +#[starknet::storage_node] +struct Storage { + a: felt252, + #[flat] + pub has_a: HasA, +} + +//! > expected_diagnostics + +//! > diagnostics +warning: Plugin diagnostic: The path `has_a.a` collides with existing path `a`. + --> lib.cairo:10:5 + has_a: HasA, + ^***^ + +warning: Plugin diagnostic: The path `has_a.a` collides with existing path `a`. + --> lib.cairo:17:9 + pub has_a: HasA, + ^***^ + +//! > ========================================================================== + +//! > Test contract flat path check. + +//! > test_runner_name +test_storage_path_check(expect_diagnostics: true) + +//! > cairo_code +#[starknet::storage_node] +struct Struct1 { + pub member1: felt252, +} + +#[starknet::storage_node] +struct Struct0 { + pub member0: Struct1, +} + +#[starknet::storage_node] +struct IgnoredMemberStruct { + #[flat] + pub ignored_member: Struct0, +} + +#[starknet::contract] +mod contract_with_and_without_ignored { + use super::{IgnoredMemberStruct, Struct0}; + #[storage] + pub struct Storage { + #[flat] + pub x: IgnoredMemberStruct, + #[flat] + pub y: Struct0, + } +} + +//! > expected_diagnostics + +//! > diagnostics +warning: Plugin diagnostic: The path `y.member0` collides with existing path `x.ignored_member.member0`. + --> lib.cairo:25:13 + pub y: Struct0, + ^ + +//! > ========================================================================== + +//! > Test component flat path check. + +//! > test_runner_name +test_storage_path_check(expect_diagnostics: true) + +//! > cairo_code +#[starknet::storage_node] +struct HasA { + a: felt252, +} + +#[starknet::storage_node] +struct HasHasA { + #[flat] + has_a: HasA, +} + +#[starknet::component] +mod component_with_and_without_ignored { + use super::{HasA, HasHasA}; + #[storage] + pub struct Storage { + pub a: felt252, + #[flat] + pub has_a: HasA, + #[flat] + pub has_has_a: HasHasA, + } +} + +//! > expected_diagnostics + +//! > diagnostics +warning: Plugin diagnostic: The path `has_a.a` collides with existing path `a`. + --> lib.cairo:19:13 + pub has_a: HasA, + ^***^ + +warning: Plugin diagnostic: The path `has_has_a.has_a.a` collides with existing path `a`. + --> lib.cairo:21:13 + pub has_has_a: HasHasA, + ^*******^ + +//! > ========================================================================== + +//! > Test contract and component flat path check. + +//! > test_runner_name +test_storage_path_check(expect_diagnostics: true) + +//! > cairo_code +#[starknet::storage_node] +struct Struct1 { + pub member1: felt252, +} + +#[starknet::storage_node] +struct Struct0 { + pub member0: Struct1, +} + +#[starknet::storage_node] +struct IgnoredMemberStruct { + #[flat] + pub ignored_member: Struct0, +} + +#[starknet::component] +mod component { + use super::{Struct0, IgnoredMemberStruct}; + #[storage] + pub struct Storage { + #[flat] + pub x: Struct0, + #[flat] + pub y: IgnoredMemberStruct, + } +} + +#[starknet::contract] +mod contract_with_component { + use super::component; + #[storage] + struct Storage { + #[substorage(v0)] + member: component::Storage, + } +} + +//! > expected_diagnostics + +//! > diagnostics +warning: Plugin diagnostic: The path `y.ignored_member.member0` collides with existing path `x.member0`. + --> lib.cairo:25:13 + pub y: IgnoredMemberStruct, + ^ + +warning: Plugin diagnostic: The path `member.y.ignored_member.member0` collides with existing path `member.x.member0`. + --> lib.cairo:35:9 + member: component::Storage, + ^****^