diagnosting same paths storage (#6099)

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<String, String>,
+    args: &OrderedHashMap<String, String>,
+) -> 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
+);

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<PluginDiagnostic> {
+        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<SmolStr, Vec<SmolStr>>,
+}
+
+impl StorageStructMembers {
+    fn handle(
+        &mut self,
+        member_name: SmolStr,
+        path_to_member: Vec<SmolStr>,
+        pointer_to_code: SyntaxStablePtrId,
+        diagnostics: &mut Vec<PluginDiagnostic>,
+    ) {
+        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<SmolStr>,
+    pointer_to_code: SyntaxStablePtrId,
+    diagnostics: &mut Vec<PluginDiagnostic>,
+) {
+    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();
+}

crates/cairo-lang-starknet/src/lib.rs

@@ -24,7 +24,8 @@ pub fn starknet_plugin_suite() -> PluginSuite {
         .add_inline_macro_plugin::<inline_macros::selector::SelectorMacro>()
         .add_inline_macro_plugin::<inline_macros::get_dep_component::GetDepComponentMacro>()
         .add_inline_macro_plugin::<inline_macros::get_dep_component::GetDepComponentMutMacro>()
-        .add_analyzer_plugin::<analyzer::ABIAnalyzer>();
+        .add_analyzer_plugin::<analyzer::ABIAnalyzer>()
+        .add_analyzer_plugin::<analyzer::StorageAnalyzer>();
     suite
 }
 

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";

crates/cairo-lang-starknet/src/plugin/plugin_test_data/contracts/with_component

@@ -433,7 +433,7 @@ impl StorageStorageBaseMutCopy of core::traits::Copy::<StorageStorageBaseMut>;
 //! > 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::<StorageStorageBaseMut>;
 impl StorageStorageBaseMutCopy of core::traits::Copy::<StorageStorageBaseMut>;
 
 //! > 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:

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: