From 4845d260bab10b2e5eb7f13c5a88202cd46a12e7 Mon Sep 17 00:00:00 2001 From: Eric Nordelo Date: Fri, 7 Aug 2026 14:32:15 +0200 Subject: [PATCH 1/2] fix: N-01 --- packages/macros/Cargo.lock | 1 - packages/macros/Cargo.toml | 1 - .../src/attribute/with_components/parser.rs | 437 ++++++++++++++---- .../macros/src/tests/test_with_components.rs | 229 +++++++++ 4 files changed, 578 insertions(+), 90 deletions(-) diff --git a/packages/macros/Cargo.lock b/packages/macros/Cargo.lock index 1b6fe5181..447a05307 100644 --- a/packages/macros/Cargo.lock +++ b/packages/macros/Cargo.lock @@ -1398,7 +1398,6 @@ dependencies = [ "nom", "proc-macro2", "quote", - "regex", ] [[package]] diff --git a/packages/macros/Cargo.toml b/packages/macros/Cargo.toml index ce5c68d48..9cf1bb8be 100644 --- a/packages/macros/Cargo.toml +++ b/packages/macros/Cargo.toml @@ -18,7 +18,6 @@ cairo-lang-starknet-classes = "2.18.0" cairo-lang-filesystem = "2.18.0" proc-macro2 = "1.0.87" indoc = "2.0.5" -regex = "1.11.1" insta = "1.42.0" convert_case = "0.8.0" nom = "8.0.0" diff --git a/packages/macros/src/attribute/with_components/parser.rs b/packages/macros/src/attribute/with_components/parser.rs index 8bda6764b..a892aa139 100644 --- a/packages/macros/src/attribute/with_components/parser.rs +++ b/packages/macros/src/attribute/with_components/parser.rs @@ -1,5 +1,7 @@ //! Parser utilities for the with_components macro. +use std::collections::HashSet; + use crate::{ constants::{ CONSTRUCTOR_ATTRIBUTE, CONTRACT_ATTRIBUTE, EVENT_ENUM_NAME, FLAT_ATTRIBUTE, @@ -15,15 +17,299 @@ use cairo_lang_syntax::node::{ db::SyntaxGroup, SyntaxNode, Terminal, TypedSyntaxNode, }; -use cairo_lang_syntax::node::{helpers::QueryAttrs, kind::SyntaxKind}; +use cairo_lang_syntax::node::{ + helpers::{GetIdentifier, QueryAttrs}, + kind::SyntaxKind, +}; use indoc::indoc; -use regex::Regex; use super::{ components::{AllowedComponents, ComponentInfo}, diagnostics::{errors, warnings}, }; +#[derive(Debug)] +struct ImportedName { + source_path: Vec, + local_name: String, +} + +/// Syntax-derived facts used by the lightweight component validation. +/// +/// All names and calls are collected from Cairo syntax terminals. Comments and string literals are +/// therefore excluded, while whitespace and path qualification do not affect matching. +#[derive(Default)] +struct ModuleFacts { + identifiers: HashSet, + imports: Vec, + implemented_traits: Vec>, + impl_alias_targets: Vec>, + calls: Vec>, + constructor_calls: Vec>, +} + +impl ModuleFacts { + fn collect<'db>(db: &'db dyn SyntaxGroup, body: &ast::ModuleBody<'db>) -> Self { + let body_node = body.as_syntax_node(); + let identifiers = body_node + .tokens(db) + .filter(|node| node.kind(db) == SyntaxKind::TerminalIdentifier) + .map(|node| terminal_text(db, node)) + .collect(); + let calls = collect_call_paths(db, body_node); + + let mut facts = Self { + identifiers, + calls, + ..Default::default() + }; + + for item in body.items(db).elements(db) { + match item { + ast::ModuleItem::Use(item_use) => { + collect_imports(db, item_use.use_path(db), &[], &mut facts.imports); + } + ast::ModuleItem::Impl(item_impl) => { + facts + .implemented_traits + .push(expr_path_segments(db, item_impl.trait_path(db))); + } + ast::ModuleItem::ImplAlias(item_impl_alias) => { + facts + .impl_alias_targets + .push(expr_path_segments(db, item_impl_alias.impl_path(db))); + } + ast::ModuleItem::FreeFunction(function) + if function.has_attr(db, CONSTRUCTOR_ATTRIBUTE) => + { + facts.constructor_calls = collect_call_paths(db, function.as_syntax_node()); + } + _ => {} + } + } + + facts + } + + fn has_identifier(&self, name: &str) -> bool { + self.identifiers.contains(name) + } + + fn imports_name(&self, name: &str) -> bool { + self.imports.iter().any(|import| { + import + .source_path + .last() + .is_some_and(|segment| segment == name) + }) + } + + fn imports_name_from(&self, parent_path: &[&str], name: &str) -> bool { + self.imports.iter().any(|import| { + import.source_path.len() == parent_path.len() + 1 + && path_starts_with(&import.source_path, parent_path) + && import + .source_path + .last() + .is_some_and(|segment| segment == name) + }) + } + + fn implements_trait(&self, trait_name: &str) -> bool { + self.implemented_traits.iter().any(|path| { + path.last().is_some_and(|segment| segment == trait_name) + || path.last().is_some_and(|local_name| { + self.imports.iter().any(|import| { + import.local_name == *local_name + && import + .source_path + .last() + .is_some_and(|segment| segment == trait_name) + }) + }) + }) + } + + fn implements_trait_path_suffix(&self, suffix: &[&str]) -> bool { + self.implemented_traits + .iter() + .any(|path| self.path_has_suffix(path, suffix)) + } + + fn implements_imported_trait_from(&self, parent_path: &[&str], trait_name: &str) -> bool { + self.imports.iter().any(|import| { + path_starts_with(&import.source_path, parent_path) + && import + .source_path + .last() + .is_some_and(|segment| segment == trait_name) + && self + .implemented_traits + .iter() + .any(|path| path.last() == Some(&import.local_name)) + }) + } + + fn has_impl_available(&self, impl_name: &str) -> bool { + self.imports_name(impl_name) + || self.impl_alias_targets.iter().any(|path| { + path.last().is_some_and(|segment| segment == impl_name) + || path.last().is_some_and(|local_name| { + self.imports.iter().any(|import| { + import.local_name == *local_name + && import + .source_path + .last() + .is_some_and(|segment| segment == impl_name) + }) + }) + }) + } + + fn has_call(&self, suffix: &[&str]) -> bool { + self.calls + .iter() + .any(|path| self.path_has_suffix(path, suffix)) + } + + fn has_constructor_call(&self, suffix: &[&str]) -> bool { + self.constructor_calls + .iter() + .any(|path| self.path_has_suffix(path, suffix)) + } + + fn path_has_suffix(&self, path: &[String], suffix: &[&str]) -> bool { + if path_has_suffix(path, suffix) { + return true; + } + + let Some((local_name, remaining_path)) = path.split_first() else { + return false; + }; + let Some(import) = self + .imports + .iter() + .find(|import| import.local_name == *local_name) + else { + return false; + }; + + // Resolve only the leading name imported into this module. The source path is deliberately + // not expanded again: ModuleFacts is a lexical, syntax-only view without name resolution. + let mut normalized_path = import.source_path.clone(); + normalized_path.extend_from_slice(remaining_path); + path_has_suffix(&normalized_path, suffix) + } +} + +fn terminal_text(db: &dyn SyntaxGroup, node: SyntaxNode<'_>) -> String { + node.get_text_without_trivia(db).long(db).to_string() +} + +fn expr_path_segments<'db>(db: &'db dyn SyntaxGroup, path: ast::ExprPath<'db>) -> Vec { + path.segments(db) + .elements(db) + .map(|segment| segment.identifier(db).long(db).to_string()) + .collect() +} + +fn collect_imports<'db>( + db: &'db dyn SyntaxGroup, + use_path: ast::UsePath<'db>, + prefix: &[String], + imports: &mut Vec, +) { + match use_path { + ast::UsePath::Leaf(leaf) => { + let source_name = leaf.ident(db).identifier(db).long(db).to_string(); + let mut source_path = prefix.to_vec(); + source_path.push(source_name.clone()); + let local_name = match leaf.alias_clause(db) { + ast::OptionAliasClause::AliasClause(alias) => { + alias.alias(db).text(db).long(db).to_string() + } + ast::OptionAliasClause::Empty(_) => source_name, + }; + imports.push(ImportedName { + source_path, + local_name, + }); + } + ast::UsePath::Single(single) => { + let mut nested_prefix = prefix.to_vec(); + nested_prefix.push(single.ident(db).identifier(db).long(db).to_string()); + collect_imports(db, single.use_path(db), &nested_prefix, imports); + } + ast::UsePath::Multi(multi) => { + for nested in multi.use_paths(db).elements(db) { + collect_imports(db, nested, prefix, imports); + } + } + ast::UsePath::Star(_) => {} + } +} + +fn collect_call_paths(db: &dyn SyntaxGroup, node: SyntaxNode<'_>) -> Vec> { + let terminals = node + .tokens(db) + .map(|terminal| (terminal.kind(db), terminal_text(db, terminal))) + .collect::>(); + let mut calls = vec![]; + + for (lparen_index, (kind, _)) in terminals.iter().enumerate() { + if *kind != SyntaxKind::TerminalLParen { + continue; + } + + let mut cursor = lparen_index; + let mut reversed_path = vec![]; + loop { + if cursor == 0 { + break; + } + cursor -= 1; + let (kind, text) = &terminals[cursor]; + if *kind != SyntaxKind::TerminalIdentifier { + break; + } + reversed_path.push(text.clone()); + + if cursor == 0 + || !matches!( + terminals[cursor - 1].0, + SyntaxKind::TerminalDot | SyntaxKind::TerminalColonColon + ) + { + break; + } + cursor -= 1; + } + + if !reversed_path.is_empty() { + reversed_path.reverse(); + calls.push(reversed_path); + } + } + + calls +} + +fn path_starts_with(path: &[String], prefix: &[&str]) -> bool { + path.len() >= prefix.len() + && path + .iter() + .zip(prefix) + .all(|(segment, expected)| segment == expected) +} + +fn path_has_suffix(path: &[String], suffix: &[&str]) -> bool { + path.len() >= suffix.len() + && path[path.len() - suffix.len()..] + .iter() + .zip(suffix) + .all(|(segment, expected)| segment == expected) +} + /// The parser for the with_components macro. pub struct WithComponentsParser<'a> { /// The base node. @@ -64,7 +350,7 @@ impl<'a> WithComponentsParser<'a> { }; // Validate the contract module - let (errors, mut warnings) = + let (errors, mut warnings, module_facts) = validate_contract_module(db, module_rnode, self.components_info); if !errors.is_empty() { return (String::new(), vec![], errors.into()); @@ -81,7 +367,7 @@ impl<'a> WithComponentsParser<'a> { // Add warnings for each component for component_info in self.components_info.iter() { - let component_warnings = add_per_component_warnings(&content, component_info); + let component_warnings = add_per_component_warnings(&module_facts, component_info); warnings.extend(component_warnings); } @@ -106,7 +392,7 @@ fn validate_contract_module<'db>( db: &'db dyn SyntaxGroup, node: &mut RewriteNode<'db>, components_info: &[ComponentInfo<'_>], -) -> (Vec, Vec) { +) -> (Vec, Vec, ModuleFacts) { let mut warnings = vec![]; if let RewriteNode::Copied(copied) = node { @@ -115,21 +401,14 @@ fn validate_contract_module<'db>( // 1. Check that the module has a body (error) let MaybeModuleBody::Some(body) = item.body(db) else { let error = Diagnostic::error(errors::NO_BODY); - return (vec![error], vec![]); + return (vec![error], vec![], ModuleFacts::default()); }; - - // Keep a stringified version of the module body around for validations below. - let body_ast = body.as_syntax_node(); - let typed = ast::ModuleBody::from_syntax_node(db, body_ast); - let body_rnode = RewriteNode::from_ast(&typed); - let mut builder = PatchBuilder::new_ex(db, &body_ast); - builder.add_modified(body_rnode); - let (body_code, _) = builder.build(); + let facts = ModuleFacts::collect(db, &body); // 2. Check that the module has the `#[starknet::contract]` attribute (error) if !item.has_attr(db, CONTRACT_ATTRIBUTE) { let error = Diagnostic::error(errors::NO_CONTRACT_ATTRIBUTE(CONTRACT_ATTRIBUTE)); - return (vec![error], vec![]); + return (vec![error], vec![], facts); } // 3. Ensure only one AccessControl component is used (error) @@ -147,21 +426,21 @@ fn validate_contract_module<'db>( let components_str = accesscontrol_components.join(", "); let error = Diagnostic::error(errors::MULTIPLE_ACCESS_CONTROL_COMPONENTS(&components_str)); - return (vec![error], vec![]); + return (vec![error], vec![], facts); } // 4. Disallow ERC721Enumerable and ERC721Consecutive being used together (error) let uses_erc721_enumerable = components_info .iter() .any(|c| matches!(c.kind(), AllowedComponents::ERC721Enumerable)) - || body_code.contains("ERC721Enumerable"); + || facts.has_identifier("ERC721EnumerableComponent"); let uses_erc721_consecutive = components_info .iter() .any(|c| matches!(c.kind(), AllowedComponents::ERC721Consecutive)) - || body_code.contains("ERC721Consecutive"); + || facts.has_identifier("ERC721ConsecutiveComponent"); if uses_erc721_enumerable && uses_erc721_consecutive { let error = Diagnostic::error(errors::ERC721_BALANCE_OF_INCOPATIBILITY); - return (vec![error], vec![]); + return (vec![error], vec![], facts); } // 5. Check that the module has the corresponding initializers (warning) @@ -171,24 +450,9 @@ fn validate_contract_module<'db>( .collect::>(); if !components_with_initializer.is_empty() { - let constructor = body.items(db).elements(db).find(|item| { - matches!(item, ast::ModuleItem::FreeFunction(function_ast) if function_ast.has_attr(db, CONSTRUCTOR_ATTRIBUTE)) - }); - let constructor_code = if let Some(constructor) = constructor { - // Get the constructor code (maybe we can do this without the builder) - let constructor_ast = constructor.as_syntax_node(); - let typed = ast::ModuleItem::from_syntax_node(db, constructor_ast); - let constructor_rnode = RewriteNode::from_ast(&typed); - let mut builder = PatchBuilder::new_ex(db, &constructor_ast); - builder.add_modified(constructor_rnode); - let (code, _) = builder.build(); - code - } else { - String::new() - }; let mut components_with_initializer_missing = vec![]; for component in components_with_initializer.iter() { - if !constructor_code.contains(&format!("self.{}.initializer(", component.storage)) { + if !facts.has_constructor_call(&["self", component.storage, "initializer"]) { components_with_initializer_missing.push(component.short_name()); } } @@ -210,35 +474,26 @@ fn validate_contract_module<'db>( .path .strip_suffix(&component.name) .expect("Component path must end with the component name"); - let default_config_import_re = Regex::new(&format!( - r"use {component_parent_path}[{{\w:, \n]*DefaultConfig(\s+as\s+\w+)?[{{\w}}, \n]*;" - )) - .unwrap(); - let default_config_used = default_config_import_re.is_match(&body_code); + let component_parent_segments = component_parent_path + .trim_end_matches("::") + .split("::") + .collect::>(); + let default_config_used = + facts.imports_name_from(&component_parent_segments, "DefaultConfig"); if default_config_used { continue; } // Case 2: ImmutableConfig is implemented with fully qualified path let immutable_config_implemented = - body_code.contains(&format!("of {}::ImmutableConfig", component.name)); + facts.implements_trait_path_suffix(&[component.name, "ImmutableConfig"]); if immutable_config_implemented { continue; } // Case 3: ImmutableConfig is imported (possibly aliased) and implemented - let immutable_config_import_re = Regex::new(&format!( - r"use {component_parent_path}[\w:]*\w+::[{{\w, \n]*ImmutableConfig(?:\s+as\s+(\w+))?[{{\w}}, \n]*;" - )) - .unwrap(); - if let Some(captures) = immutable_config_import_re.captures(&body_code) { - // Use the alias if present, otherwise use "ImmutableConfig" - let config_name = captures.get(1).map_or("ImmutableConfig", |m| m.as_str()); - let imported_immutable_config_implemented = - body_code.contains(&format!("of {config_name}")); - if imported_immutable_config_implemented { - continue; - } + if facts.implements_imported_trait_from(&component_parent_segments, "ImmutableConfig") { + continue; } // No valid config found - add warning @@ -248,20 +503,25 @@ fn validate_contract_module<'db>( )); warnings.push(warning); } + + return (vec![], warnings, facts); } - (vec![], warnings) + (vec![], warnings, ModuleFacts::default()) } /// Adds warnings that may be helpful for users. -fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec { +fn add_per_component_warnings( + facts: &ModuleFacts, + component_info: &ComponentInfo, +) -> Vec { let mut warnings = vec![]; match component_info.kind() { AllowedComponents::Vesting => { // Check that the VestingScheduleTrait is implemented - let linear_impl_used = code.contains("LinearVestingSchedule"); - let vesting_trait_used = code.contains("VestingScheduleTrait"); + let linear_impl_used = facts.has_impl_available("LinearVestingSchedule"); + let vesting_trait_used = facts.implements_trait("VestingScheduleTrait"); if !linear_impl_used && !vesting_trait_used { let warning = Diagnostic::warn(warnings::VESTING_SCHEDULE_IMPL_MISSING); warnings.push(warning); @@ -270,7 +530,7 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec AllowedComponents::Initializable => { // Check that the initialize internal function is called let initialize_internal_function_called = - code.contains("self.initializable.initialize()"); + facts.has_call(&["self", "initializable", "initialize"]); if !initialize_internal_function_called { let warning = Diagnostic::warn(warnings::INITIALIZABLE_NOT_USED); warnings.push(warning); @@ -278,8 +538,8 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec } AllowedComponents::Pausable => { // Check that the pause and unpause functions are called - let pause_function_called = code.contains("self.pausable.pause()"); - let unpause_function_called = code.contains("self.pausable.unpause()"); + let pause_function_called = facts.has_call(&["self", "pausable", "pause"]); + let unpause_function_called = facts.has_call(&["self", "pausable", "unpause"]); if !pause_function_called || !unpause_function_called { let warning = Diagnostic::warn(warnings::PAUSABLE_NOT_USED); warnings.push(warning); @@ -287,8 +547,8 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec } AllowedComponents::ERC20 => { // Check that the ERC20HooksTrait is implemented - let hooks_trait_used = code.contains("ERC20HooksTrait"); - let hooks_empty_impl_used = code.contains("ERC20HooksEmptyImpl"); + let hooks_trait_used = facts.implements_trait("ERC20HooksTrait"); + let hooks_empty_impl_used = facts.has_impl_available("ERC20HooksEmptyImpl"); if !hooks_trait_used && !hooks_empty_impl_used { let warning = Diagnostic::warn(warnings::ERC20_HOOKS_IMPL_MISSING); warnings.push(warning); @@ -296,29 +556,30 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec } AllowedComponents::ERC4626 => { // 1. Check that the ERC4626HooksTrait is implemented - let hooks_trait_used = code.contains("ERC4626HooksTrait"); - let hooks_empty_impl_used = code.contains("ERC4626EmptyHooks"); + let hooks_trait_used = facts.implements_trait("ERC4626HooksTrait"); + let hooks_empty_impl_used = facts.has_impl_available("ERC4626EmptyHooks"); if !hooks_trait_used && !hooks_empty_impl_used { let warning = Diagnostic::warn(warnings::ERC4626_HOOKS_IMPL_MISSING); warnings.push(warning); } // 2. Check that the FeeConfigTrait is implemented - let fee_config_trait_used = code.contains("FeeConfigTrait"); - let fee_config_empty_impl_used = code.contains("ERC4626DefaultNoFees"); + let fee_config_trait_used = facts.implements_trait("FeeConfigTrait"); + let fee_config_empty_impl_used = facts.has_impl_available("ERC4626DefaultNoFees"); if !fee_config_trait_used && !fee_config_empty_impl_used { let warning = Diagnostic::warn(warnings::ERC4626_FEE_CONFIG_IMPL_MISSING); warnings.push(warning); } // 3. Check that the LimitConfigTrait is implemented - let limit_config_trait_used = code.contains("LimitConfigTrait"); - let limit_config_empty_impl_used = code.contains("ERC4626DefaultNoLimits"); + let limit_config_trait_used = facts.implements_trait("LimitConfigTrait"); + let limit_config_empty_impl_used = facts.has_impl_available("ERC4626DefaultNoLimits"); if !limit_config_trait_used && !limit_config_empty_impl_used { let warning = Diagnostic::warn(warnings::ERC4626_LIMIT_CONFIG_IMPL_MISSING); warnings.push(warning); } // 4. Check that the AssetsManagementTrait is implemented - let assets_management_trait_used = code.contains("AssetsManagementTrait"); - let self_assets_management_impl_used = code.contains("ERC4626SelfAssetsManagement"); + let assets_management_trait_used = facts.implements_trait("AssetsManagementTrait"); + let self_assets_management_impl_used = + facts.has_impl_available("ERC4626SelfAssetsManagement"); if !assets_management_trait_used && !self_assets_management_impl_used { let warning = Diagnostic::warn(warnings::ERC4626_ASSETS_MANAGEMENT_IMPL_MISSING); warnings.push(warning); @@ -326,8 +587,8 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec } AllowedComponents::ERC721 => { // Check that the ERC721HooksTrait is implemented - let hooks_trait_used = code.contains("ERC721HooksTrait"); - let hooks_empty_impl_used = code.contains("ERC721HooksEmptyImpl"); + let hooks_trait_used = facts.implements_trait("ERC721HooksTrait"); + let hooks_empty_impl_used = facts.has_impl_available("ERC721HooksEmptyImpl"); if !hooks_trait_used && !hooks_empty_impl_used { let warning = Diagnostic::warn(warnings::ERC721_HOOKS_IMPL_MISSING); warnings.push(warning); @@ -335,8 +596,8 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec } AllowedComponents::ERC1155 => { // Check that the ERC1155HooksTrait is implemented - let hooks_trait_used = code.contains("ERC1155HooksTrait"); - let hooks_empty_impl_used = code.contains("ERC1155HooksEmptyImpl"); + let hooks_trait_used = facts.implements_trait("ERC1155HooksTrait"); + let hooks_empty_impl_used = facts.has_impl_available("ERC1155HooksEmptyImpl"); if !hooks_trait_used && !hooks_empty_impl_used { let warning = Diagnostic::warn(warnings::ERC1155_HOOKS_IMPL_MISSING); warnings.push(warning); @@ -344,16 +605,16 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec } AllowedComponents::ERC6909 => { // Check that the ERC6909HooksTrait is implemented - let hooks_trait_used = code.contains("ERC6909HooksTrait"); - let hooks_empty_impl_used = code.contains("ERC6909HooksEmptyImpl"); + let hooks_trait_used = facts.implements_trait("ERC6909HooksTrait"); + let hooks_empty_impl_used = facts.has_impl_available("ERC6909HooksEmptyImpl"); if !hooks_trait_used && !hooks_empty_impl_used { let warning = Diagnostic::warn(warnings::ERC6909_HOOKS_IMPL_MISSING); warnings.push(warning); } } AllowedComponents::ERC1155Supply => { - let hook_called = code.contains("erc1155_supply.after_update(") - || code.contains("ERC1155SupplyInternalImpl::after_update("); + let hook_called = facts.has_call(&["erc1155_supply", "after_update"]) + || facts.has_call(&["ERC1155SupplyInternalImpl", "after_update"]); if !hook_called { let warning = Diagnostic::warn(warnings::ERC1155_SUPPLY_HOOKS_MISSING); warnings.push(warning); @@ -361,7 +622,7 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec } AllowedComponents::Upgradeable => { // Check that the upgrade function is called - let upgrade_function_called = code.contains("self.upgradeable.upgrade"); + let upgrade_function_called = facts.has_call(&["self", "upgradeable", "upgrade"]); if !upgrade_function_called { let warning = Diagnostic::warn(warnings::UPGRADEABLE_NOT_USED); warnings.push(warning); @@ -369,33 +630,33 @@ fn add_per_component_warnings(code: &str, component_info: &ComponentInfo) -> Vec } AllowedComponents::Votes => { // Check that the SNIP12Metadata is implemented - let snip12_metadata_implemented = code.contains("of SNIP12Metadata"); + let snip12_metadata_implemented = facts.implements_trait("SNIP12Metadata"); if !snip12_metadata_implemented { let warning = Diagnostic::warn(warnings::SNIP12_METADATA_IMPL_MISSING); warnings.push(warning); } } AllowedComponents::ERC721Enumerable => { - let hook_called = code.contains("erc721_enumerable.before_update(") - || code.contains("ERC721EnumerableInternalImpl::before_update("); + let hook_called = facts.has_call(&["erc721_enumerable", "before_update"]) + || facts.has_call(&["ERC721EnumerableInternalImpl", "before_update"]); if !hook_called { let warning = Diagnostic::warn(warnings::ERC721_ENUMERABLE_HOOKS_MISSING); warnings.push(warning); } } AllowedComponents::ERC721URIStorage => { - let hook_called = code.contains("erc721_uri_storage.after_update(") - || code.contains("ERC721URIStorageInternalImpl::after_update("); + let hook_called = facts.has_call(&["erc721_uri_storage", "after_update"]) + || facts.has_call(&["ERC721URIStorageInternalImpl", "after_update"]); if !hook_called { let warning = Diagnostic::warn(warnings::ERC721_URI_STORAGE_HOOKS_MISSING); warnings.push(warning); } } AllowedComponents::ERC721Consecutive => { - let before_update_called = code.contains("erc721_consecutive.before_update(") - || code.contains("ERC721ConsecutiveInternalImpl::before_update("); - let after_update_called = code.contains("erc721_consecutive.after_update(") - || code.contains("ERC721ConsecutiveInternalImpl::after_update("); + let before_update_called = facts.has_call(&["erc721_consecutive", "before_update"]) + || facts.has_call(&["ERC721ConsecutiveInternalImpl", "before_update"]); + let after_update_called = facts.has_call(&["erc721_consecutive", "after_update"]) + || facts.has_call(&["ERC721ConsecutiveInternalImpl", "after_update"]); if !before_update_called || !after_update_called { let warning = Diagnostic::warn(warnings::ERC721_CONSECUTIVE_HOOKS_MISSING); warnings.push(warning); diff --git a/packages/macros/src/tests/test_with_components.rs b/packages/macros/src/tests/test_with_components.rs index 4d8d10f37..4cde664ab 100644 --- a/packages/macros/src/tests/test_with_components.rs +++ b/packages/macros/src/tests/test_with_components.rs @@ -1,4 +1,5 @@ use crate::attribute::with_components::definition::with_components_avevetedp5blk as with_components; +use crate::attribute::with_components::diagnostics::warnings; use cairo_lang_macro::{quote, TokenStream}; use insta::assert_snapshot; @@ -2210,6 +2211,226 @@ fn test_with_header_doc() { assert_snapshot!(result); } +#[test] +fn validation_ignores_hook_names_in_comments_and_longer_identifiers() { + let attribute = quote! { (ERC20) }; + let item = quote! { + #[starknet::contract] + pub mod MyToken { + use openzeppelin_token::erc20::DefaultConfig; + + // use openzeppelin_token::erc20::ERC20HooksEmptyImpl; + // impl Hooks of ERC20HooksTrait {} + struct ERC20HooksEmptyImplWrapper {} + + #[storage] + pub struct Storage {} + + #[constructor] + fn constructor(ref self: ContractState) { + self.erc20.initializer("MyToken", "MTK"); + } + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(diagnostics + .iter() + .any(|message| message == warnings::ERC20_HOOKS_IMPL_MISSING)); +} + +#[test] +fn validation_recognizes_qualified_hook_trait_path() { + let attribute = quote! { (ERC20) }; + let item = quote! { + #[starknet::contract] + pub mod MyToken { + use openzeppelin_token::erc20::DefaultConfig; + + impl Hooks of openzeppelin_token::erc20::ERC20Component::ERC20HooksTrait {} + + #[storage] + pub struct Storage {} + + #[constructor] + fn constructor(ref self: ContractState) { + self.erc20.initializer("MyToken", "MTK"); + } + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(!diagnostics + .iter() + .any(|message| message == warnings::ERC20_HOOKS_IMPL_MISSING)); +} + +#[test] +fn validation_recognizes_aliased_default_hook_impl() { + let attribute = quote! { (ERC20) }; + let item = quote! { + #[starknet::contract] + pub mod MyToken { + use openzeppelin_token::erc20::{ + DefaultConfig, ERC20HooksEmptyImpl as DefaultHooks, + }; + + #[storage] + pub struct Storage {} + + #[constructor] + fn constructor(ref self: ContractState) { + self.erc20.initializer("MyToken", "MTK"); + } + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(!diagnostics + .iter() + .any(|message| message == warnings::ERC20_HOOKS_IMPL_MISSING)); +} + +#[test] +fn validation_recognizes_qualified_snip12_metadata_trait() { + let attribute = quote! { (Votes) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + impl Metadata of openzeppelin_utils::cryptography::snip12::SNIP12Metadata { + fn name() -> felt252 { "DAPP_NAME" } + fn version() -> felt252 { "DAPP_VERSION" } + } + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(!diagnostics + .iter() + .any(|message| message == warnings::SNIP12_METADATA_IMPL_MISSING)); +} + +#[test] +fn validation_recognizes_aliased_snip12_metadata_trait() { + let attribute = quote! { (Votes) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + use openzeppelin_utils::cryptography::snip12::SNIP12Metadata as MetadataTrait; + + impl Metadata of MetadataTrait { + fn name() -> felt252 { "DAPP_NAME" } + fn version() -> felt252 { "DAPP_VERSION" } + } + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(!diagnostics + .iter() + .any(|message| message == warnings::SNIP12_METADATA_IMPL_MISSING)); +} + +#[test] +fn validation_ignores_component_calls_in_comments_and_longer_methods() { + let attribute = quote! { (ERC1155Supply) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + // ERC1155SupplyInternalImpl::after_update(); + fn update_supply_later(ref self: ContractState) { + self.erc1155_supply.after_update_later(); + } + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(diagnostics + .iter() + .any(|message| message == warnings::ERC1155_SUPPLY_HOOKS_MISSING)); +} + +#[test] +fn validation_recognizes_qualified_component_call() { + let attribute = quote! { (ERC1155Supply) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + fn update_supply(ref self: ContractState) { + openzeppelin_token::erc1155::extensions::ERC1155SupplyInternalImpl::after_update( + ref self.erc1155_supply, + ); + } + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(!diagnostics + .iter() + .any(|message| message == warnings::ERC1155_SUPPLY_HOOKS_MISSING)); +} + +#[test] +fn validation_recognizes_aliased_component_call() { + let attribute = quote! { (ERC1155Supply) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + use openzeppelin_token::erc1155::extensions::ERC1155SupplyInternalImpl as SupplyImpl; + + fn update_supply(ref self: ContractState) { + SupplyImpl::after_update(ref self.erc1155_supply); + } + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(!diagnostics + .iter() + .any(|message| message == warnings::ERC1155_SUPPLY_HOOKS_MISSING)); +} + +#[test] +fn validation_recognizes_aliased_component_immutable_config() { + let attribute = quote! { (ERC721Consecutive) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + use openzeppelin_token::erc721::extensions::ERC721ConsecutiveComponent as Consecutive; + + pub impl Config of Consecutive::ImmutableConfig { + const MAX_BATCH_SIZE: u64 = 4200; + const FIRST_CONSECUTIVE_ID: u64 = 42; + } + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + let missing_config = warnings::IMMUTABLE_CONFIG_MISSING( + "ERC721Consecutive", + "openzeppelin_token::erc721::extensions::DefaultConfig", + ); + assert!(!diagnostics.iter().any(|message| message == &missing_config)); +} + // // Helpers // @@ -2220,3 +2441,11 @@ fn get_string_result(attr_stream: TokenStream, item_stream: TokenStream) -> Stri let raw_result = with_components(attr_stream, item_stream); format_proc_macro_result(raw_result) } + +fn get_diagnostics(attr_stream: TokenStream, item_stream: TokenStream) -> Vec { + with_components(attr_stream, item_stream) + .diagnostics + .iter() + .map(|diagnostic| diagnostic.message().to_string()) + .collect() +} From 758e69429575d3bc0d87a3bc5cc835d17882eb3d Mon Sep 17 00:00:00 2001 From: Eric Nordelo Date: Mon, 10 Aug 2026 12:46:16 +0200 Subject: [PATCH 2/2] feat: apply review updates --- .../src/attribute/with_components/parser.rs | 42 ++++++- .../macros/src/tests/test_with_components.rs | 115 +++++++++++++++++- 2 files changed, 148 insertions(+), 9 deletions(-) diff --git a/packages/macros/src/attribute/with_components/parser.rs b/packages/macros/src/attribute/with_components/parser.rs index a892aa139..87a81f99b 100644 --- a/packages/macros/src/attribute/with_components/parser.rs +++ b/packages/macros/src/attribute/with_components/parser.rs @@ -138,7 +138,8 @@ impl ModuleFacts { fn implements_imported_trait_from(&self, parent_path: &[&str], trait_name: &str) -> bool { self.imports.iter().any(|import| { - path_starts_with(&import.source_path, parent_path) + import.source_path.len() == parent_path.len() + 1 + && path_starts_with(&import.source_path, parent_path) && import .source_path .last() @@ -264,6 +265,7 @@ fn collect_call_paths(db: &dyn SyntaxGroup, node: SyntaxNode<'_>) -> Vec) -> Vec` group immediately before `cursor` while scanning a call path +/// backwards. +fn skip_turbofish_arguments(terminals: &[(SyntaxKind, String)], cursor: &mut usize) { + if *cursor == 0 || terminals[*cursor - 1].0 != SyntaxKind::TerminalGT { + return; + } + + let original_cursor = *cursor; + let mut depth = 0; + while *cursor > 0 { + *cursor -= 1; + match terminals[*cursor].0 { + SyntaxKind::TerminalGT => depth += 1, + SyntaxKind::TerminalLT => { + depth -= 1; + if depth == 0 { + break; + } + } + _ => {} + } + } + + if depth != 0 || *cursor == 0 || terminals[*cursor - 1].0 != SyntaxKind::TerminalColonColon { + *cursor = original_cursor; + return; + } + + *cursor -= 1; +} + fn path_starts_with(path: &[String], prefix: &[&str]) -> bool { path.len() >= prefix.len() && path @@ -474,12 +507,12 @@ fn validate_contract_module<'db>( .path .strip_suffix(&component.name) .expect("Component path must end with the component name"); - let component_parent_segments = component_parent_path + let mut component_path_segments = component_parent_path .trim_end_matches("::") .split("::") .collect::>(); let default_config_used = - facts.imports_name_from(&component_parent_segments, "DefaultConfig"); + facts.imports_name_from(&component_path_segments, "DefaultConfig"); if default_config_used { continue; } @@ -492,7 +525,8 @@ fn validate_contract_module<'db>( } // Case 3: ImmutableConfig is imported (possibly aliased) and implemented - if facts.implements_imported_trait_from(&component_parent_segments, "ImmutableConfig") { + component_path_segments.push(component.name); + if facts.implements_imported_trait_from(&component_path_segments, "ImmutableConfig") { continue; } diff --git a/packages/macros/src/tests/test_with_components.rs b/packages/macros/src/tests/test_with_components.rs index 4cde664ab..b78a9c39b 100644 --- a/packages/macros/src/tests/test_with_components.rs +++ b/packages/macros/src/tests/test_with_components.rs @@ -1,6 +1,7 @@ use crate::attribute::with_components::definition::with_components_avevetedp5blk as with_components; use crate::attribute::with_components::diagnostics::warnings; -use cairo_lang_macro::{quote, TokenStream}; +use cairo_lang_macro::{quote, TextSpan, Token, TokenStream, TokenTree}; +use indoc::indoc; use insta::assert_snapshot; use super::common::format_proc_macro_result; @@ -2214,7 +2215,7 @@ fn test_with_header_doc() { #[test] fn validation_ignores_hook_names_in_comments_and_longer_identifiers() { let attribute = quote! { (ERC20) }; - let item = quote! { + let item = raw_token_stream(indoc! {r#" #[starknet::contract] pub mod MyToken { use openzeppelin_token::erc20::DefaultConfig; @@ -2231,7 +2232,7 @@ fn validation_ignores_hook_names_in_comments_and_longer_identifiers() { self.erc20.initializer("MyToken", "MTK"); } } - }; + "#}); let diagnostics = get_diagnostics(attribute, item); assert!(diagnostics @@ -2340,7 +2341,7 @@ fn validation_recognizes_aliased_snip12_metadata_trait() { #[test] fn validation_ignores_component_calls_in_comments_and_longer_methods() { let attribute = quote! { (ERC1155Supply) }; - let item = quote! { + let item = raw_token_stream(indoc! {r#" #[starknet::contract] pub mod MyContract { // ERC1155SupplyInternalImpl::after_update(); @@ -2351,7 +2352,7 @@ fn validation_ignores_component_calls_in_comments_and_longer_methods() { #[storage] pub struct Storage {} } - }; + "#}); let diagnostics = get_diagnostics(attribute, item); assert!(diagnostics @@ -2382,6 +2383,54 @@ fn validation_recognizes_qualified_component_call() { .any(|message| message == warnings::ERC1155_SUPPLY_HOOKS_MISSING)); } +#[test] +fn validation_recognizes_turbofish_component_call() { + let attribute = quote! { (ERC1155Supply) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + fn update_supply(ref self: ContractState) { + ERC1155SupplyInternalImpl::>>::after_update( + ref self.erc1155_supply, + ); + } + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + assert!(!diagnostics + .iter() + .any(|message| message == warnings::ERC1155_SUPPLY_HOOKS_MISSING)); +} + +#[test] +fn validation_recognizes_turbofish_initializer_call() { + let attribute = quote! { (ERC20) }; + let item = quote! { + #[starknet::contract] + pub mod MyToken { + use openzeppelin_token::erc20::DefaultConfig; + + #[storage] + pub struct Storage {} + + #[constructor] + fn constructor(ref self: ContractState) { + self.erc20.initializer::("MyToken", "MTK"); + } + } + }; + + let diagnostics = get_diagnostics(attribute, item); + let missing_initializer = warnings::INITIALIZERS_MISSING("ERC20"); + assert!(!diagnostics + .iter() + .any(|message| message == &missing_initializer)); +} + #[test] fn validation_recognizes_aliased_component_call() { let attribute = quote! { (ERC1155Supply) }; @@ -2431,6 +2480,55 @@ fn validation_recognizes_aliased_component_immutable_config() { assert!(!diagnostics.iter().any(|message| message == &missing_config)); } +#[test] +fn validation_recognizes_aliased_imported_component_immutable_config() { + let attribute = quote! { (ERC721Consecutive) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + use openzeppelin_token::erc721::extensions::ERC721ConsecutiveComponent::ImmutableConfig as ConsecutiveConfig; + + pub impl Config of ConsecutiveConfig { + const MAX_BATCH_SIZE: u64 = 4200; + const FIRST_CONSECUTIVE_ID: u64 = 42; + } + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + let missing_config = warnings::IMMUTABLE_CONFIG_MISSING( + "ERC721Consecutive", + "openzeppelin_token::erc721::extensions::DefaultConfig", + ); + assert!(!diagnostics.iter().any(|message| message == &missing_config)); +} + +#[test] +fn validation_rejects_sibling_component_immutable_config() { + let attribute = quote! { (ERC721Consecutive) }; + let item = quote! { + #[starknet::contract] + pub mod MyContract { + use openzeppelin_token::erc721::extensions::ERC721URIStorageComponent::ImmutableConfig; + + pub impl Config of ImmutableConfig {} + + #[storage] + pub struct Storage {} + } + }; + + let diagnostics = get_diagnostics(attribute, item); + let missing_config = warnings::IMMUTABLE_CONFIG_MISSING( + "ERC721Consecutive", + "openzeppelin_token::erc721::extensions::DefaultConfig", + ); + assert!(diagnostics.iter().any(|message| message == &missing_config)); +} + // // Helpers // @@ -2449,3 +2547,10 @@ fn get_diagnostics(attr_stream: TokenStream, item_stream: TokenStream) -> Vec TokenStream { + TokenStream::new(vec![TokenTree::Ident(Token::new( + source, + TextSpan::new(0, source.len() as u32), + ))]) +}