ryo_suggest/pattern/

pattern_suggest.rs

//! PatternBasedSuggest - Rule → Suggest adapter
//!
//! Bridges `ryo-pattern` rules with the Suggest framework.

use ryo_analysis::context::AnalysisContext;
use ryo_analysis::{SymbolId, SymbolKind, SymbolPath};
use ryo_pattern::{BodyScanner, MatchResult, Rule, Severity as PatternSeverity};
use ryo_source::pure::{
    PureBlock, PureExpr, PureItem, PureMatchArm, PurePattern, PureStmt, PureVis,
};

use crate::lint::{LintDetails, LintSuggest};
use crate::suggest::SymbolScope;
use crate::{
    EnumToTraitStrategy, LintSeverity, MatchHandling, MutationSpec, OpportunityId, SafetyLevel,
    Suggest, SuggestCategory, SuggestLocation, SuggestOpportunity, SuggestResult,
};

/// Adapter: Rule → Suggest trait
///
/// Wraps a `ryo-pattern` Rule and implements the Suggest trait,
/// enabling pattern-based lint rules to integrate with the suggestion system.
///
/// # Example
///
/// ```ignore
/// use ryo_suggest::pattern::{RuleStore, PatternBasedSuggest};
///
/// let store = RuleStore::builtin_only()?;
/// for rule in store.all_rules() {
///     let suggest = PatternBasedSuggest::new(rule.clone());
///     registry.register(Box::new(suggest));
/// }
/// ```
pub struct PatternBasedSuggest {
    rule: Rule,
    /// Cached name for Suggest trait (leaked &'static str)
    name: &'static str,
    /// Cached description string (rule.name)
    description: String,
}

impl PatternBasedSuggest {
    /// Create a new PatternBasedSuggest from a Rule
    pub fn new(rule: Rule) -> Self {
        // Leak the rule ID as &'static str for Suggest::name()
        // This is acceptable for long-lived rule objects
        let name = Box::leak(rule.id.clone().into_boxed_str());
        let description = rule.name.clone();
        Self {
            rule,
            name,
            description,
        }
    }

    /// Get the underlying rule
    pub fn rule(&self) -> &Rule {
        &self.rule
    }

    /// Convert PatternSeverity to LintSeverity
    fn to_lint_severity(severity: PatternSeverity) -> LintSeverity {
        match severity {
            PatternSeverity::Error => LintSeverity::Error,
            PatternSeverity::Warning => LintSeverity::Warning,
            PatternSeverity::Info | PatternSeverity::Hint => LintSeverity::Info,
        }
    }

    /// Convert PatternSeverity to SafetyLevel
    fn to_safety_level(severity: PatternSeverity) -> SafetyLevel {
        match severity {
            PatternSeverity::Error => SafetyLevel::Manual,
            PatternSeverity::Warning => SafetyLevel::Confirm,
            PatternSeverity::Info | PatternSeverity::Hint => SafetyLevel::Auto,
        }
    }

    /// Check if a symbol matches the query's kind filter
    fn matches_kind(&self, ctx: &AnalysisContext, symbol_id: SymbolId) -> bool {
        let query_kind = match &self.rule.query.kind {
            Some(k) => k,
            None => return true, // No kind filter = match all
        };

        let symbol_kind = match ctx.registry.kind(symbol_id) {
            Some(k) => k,
            None => return false,
        };

        // Convert ryo_pattern::SymbolKind to ryo_symbol::SymbolKind
        use ryo_pattern::SymbolKind as PatternKind;
        matches!(
            (query_kind, symbol_kind),
            (PatternKind::Function, SymbolKind::Function)
                | (PatternKind::Struct, SymbolKind::Struct)
                | (PatternKind::Enum, SymbolKind::Enum)
                | (PatternKind::Trait, SymbolKind::Trait)
                | (PatternKind::Impl, SymbolKind::Impl)
                | (PatternKind::Mod, SymbolKind::Mod)
                | (PatternKind::Const, SymbolKind::Const)
                | (PatternKind::Static, SymbolKind::Static)
                | (PatternKind::TypeAlias, SymbolKind::TypeAlias)
                | (PatternKind::Field, SymbolKind::Field)
                | (PatternKind::Variant, SymbolKind::Variant)
        )
    }

    /// Check if a symbol matches the query's attribute filter (vis, name, etc.)
    fn matches_attrs(&self, ctx: &AnalysisContext, symbol_id: SymbolId) -> bool {
        let attrs = match &self.rule.query.r#match {
            Some(a) => a,
            None => return true, // No attr filter = match all
        };

        // Check name pattern
        if let Some(ref name_pattern) = attrs.name {
            if let Some(path) = ctx.registry.path(symbol_id) {
                let name = path.name();
                if name != name_pattern {
                    return false;
                }
            }
        }

        // Check glob pattern
        if let Some(ref pattern) = attrs.pattern {
            if let Some(path) = ctx.registry.path(symbol_id) {
                let name = path.name();
                if !self.matches_glob(pattern, name) {
                    return false;
                }
            }
        }

        // Check visibility using the actual AST
        if let Some(ref vis) = attrs.vis {
            let ast_vis = Self::get_ast_visibility(ctx, symbol_id);
            use ryo_pattern::Visibility;
            match vis {
                Visibility::Public => {
                    if ast_vis != PureVis::Public {
                        return false;
                    }
                }
                Visibility::Crate => {
                    if ast_vis != PureVis::Crate {
                        return false;
                    }
                }
                Visibility::Super => {
                    if ast_vis != PureVis::Super {
                        return false;
                    }
                }
                Visibility::Private => {
                    if ast_vis != PureVis::Private {
                        return false;
                    }
                }
            }
        }

        true
    }

    /// Extract visibility from the AST for a given symbol.
    /// Falls back to Private if the symbol is not found or has no visibility field.
    fn get_ast_visibility(ctx: &AnalysisContext, symbol_id: SymbolId) -> PureVis {
        if let Some(item) = ctx.ast_registry.get(symbol_id) {
            match item {
                PureItem::Fn(f) => return f.vis.clone(),
                PureItem::Struct(s) => return s.vis.clone(),
                PureItem::Enum(e) => return e.vis.clone(),
                PureItem::Const(c) => return c.vis.clone(),
                PureItem::Static(s) => return s.vis.clone(),
                PureItem::Trait(t) => return t.vis.clone(),
                PureItem::Type(t) => return t.vis.clone(),
                PureItem::Mod(m) => return m.vis.clone(),
                _ => {}
            }
        }
        PureVis::Private
    }

    /// Simple glob matching (supports * wildcard)
    fn matches_glob(&self, pattern: &str, name: &str) -> bool {
        if pattern == "*" {
            return true;
        }
        if let Some(prefix) = pattern.strip_suffix('*') {
            return name.starts_with(prefix);
        }
        if let Some(suffix) = pattern.strip_prefix('*') {
            return name.ends_with(suffix);
        }
        pattern == name
    }

    /// Dynamic checks for rules that need runtime analysis.
    ///
    /// Returns (passes_check, custom_message) where custom_message overrides
    /// the default rule message with more specific information.
    fn matches_dynamic_checks(
        &self,
        ctx: &AnalysisContext,
        symbol_id: SymbolId,
    ) -> (bool, Option<String>) {
        match self.rule.id.as_str() {
            // RL060: trait-inline-candidate - only suggest if exactly 1 implementor
            "RL060" => {
                let impl_count = ctx.code_graph().impl_count(symbol_id);
                if impl_count == 1 {
                    let impl_id = match ctx.code_graph().implementors_of(symbol_id).next() {
                        Some(id) => id,
                        None => return (false, None),
                    };

                    // Skip blanket impls (e.g. `impl<T: Foo> Bar for T`)
                    // where self_ty is a generic type parameter.
                    if let Some(detail) = ctx.detail_store().impl_(impl_id) {
                        if !detail.generics.type_params.is_empty()
                            && detail
                                .generics
                                .type_params
                                .iter()
                                .any(|tp| tp == &detail.self_ty)
                        {
                            return (false, None);
                        }
                    }

                    let implementor_name = ctx
                        .registry
                        .path(impl_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let trait_name = ctx
                        .registry
                        .path(symbol_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let message = format!(
                        "Trait `{}` has exactly 1 implementor (`{}`). Consider InlineTrait to simplify.",
                        trait_name, implementor_name
                    );
                    (true, Some(message))
                } else {
                    // Multiple implementors - not a good candidate for inlining
                    (false, None)
                }
            }

            // RL061: impl-extract-candidate
            // Only suggest for inherent impls with >= 5 methods AND no existing trait impls
            "RL061" => {
                const MIN_METHODS: usize = 5;

                let detail = ctx.detail_store().impl_(symbol_id);

                // Must be an inherent impl (trait_ is None)
                let is_inherent = detail.map(|d| d.trait_.is_none()).unwrap_or(false);
                if !is_inherent {
                    return (false, None);
                }

                let self_ty = detail.map(|d| d.self_ty.as_str()).unwrap_or("unknown");
                let method_count = detail.map(|d| d.methods.len()).unwrap_or(0);

                if method_count < MIN_METHODS {
                    return (false, None);
                }

                // Skip if the type already has trait implementations
                let has_trait_impl = ctx
                    .registry
                    .iter_by_kind(SymbolKind::Impl)
                    .filter(|&id| id != symbol_id)
                    .any(|id| {
                        ctx.detail_store()
                            .impl_(id)
                            .map(|d| d.trait_.is_some() && d.self_ty == self_ty)
                            .unwrap_or(false)
                    });

                if has_trait_impl {
                    return (false, None);
                }

                let message = format!(
                    "Inherent impl for `{}` with {} methods, no trait impls. Consider ExtractTrait.",
                    self_ty, method_count
                );
                (true, Some(message))
            }

            // RL043: needless-return - only suggest if last statement is a return
            "RL043" => {
                let has_trailing_return = ctx
                    .ast_registry
                    .get(symbol_id)
                    .and_then(|item| match item {
                        PureItem::Fn(f) => Some(is_trailing_return(&f.body)),
                        _ => None,
                    })
                    .unwrap_or(false);

                if has_trailing_return {
                    let fn_name = ctx
                        .registry
                        .path(symbol_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let message = format!(
                        "Function `{}` has a redundant return at the end. Remove for idiomatic Rust.",
                        fn_name
                    );
                    (true, Some(message))
                } else {
                    (false, None)
                }
            }

            // RL090: large-function - only suggest if function has many statements
            "RL090" => {
                const STMT_THRESHOLD: usize = 30;

                let stmt_count = ctx
                    .ast_registry
                    .get(symbol_id)
                    .and_then(|item| match item {
                        PureItem::Fn(f) => Some(count_statements(&f.body)),
                        _ => None,
                    })
                    .unwrap_or(0);

                if stmt_count >= STMT_THRESHOLD {
                    let fn_name = ctx
                        .registry
                        .path(symbol_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let message = format!(
                        "Function `{}` has {} statements (threshold: {}). Consider splitting.",
                        fn_name, stmt_count, STMT_THRESHOLD
                    );
                    (true, Some(message))
                } else {
                    (false, None)
                }
            }

            // RL091: too-many-args - only suggest if function has many parameters
            "RL091" => {
                const PARAM_THRESHOLD: usize = 5;

                let param_count = ctx
                    .ast_registry
                    .get(symbol_id)
                    .and_then(|item| match item {
                        PureItem::Fn(f) => Some(f.params.len()),
                        _ => None,
                    })
                    .unwrap_or(0);

                if param_count >= PARAM_THRESHOLD {
                    let fn_name = ctx
                        .registry
                        .path(symbol_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let message = format!(
                        "Function `{}` has {} parameters (threshold: {}). Consider grouping into a struct.",
                        fn_name, param_count, PARAM_THRESHOLD
                    );
                    (true, Some(message))
                } else {
                    (false, None)
                }
            }

            // RL004: unwrap-or-default - only suggest if unwrap_or(Default::default()) exists
            "RL004" => {
                let unwrap_or_default_count = ctx
                    .ast_registry
                    .get(symbol_id)
                    .and_then(|item| match item {
                        PureItem::Fn(f) => Some(count_unwrap_or_default_calls(&f.body)),
                        _ => None,
                    })
                    .unwrap_or(0);

                if unwrap_or_default_count > 0 {
                    let fn_name = ctx
                        .registry
                        .path(symbol_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let message = format!(
                        "`{}` has {} unwrap_or(Default::default()) call{} that can be simplified to unwrap_or_default()",
                        fn_name,
                        unwrap_or_default_count,
                        if unwrap_or_default_count == 1 { "" } else { "s" }
                    );
                    (true, Some(message))
                } else {
                    (false, None)
                }
            }

            // RL040: collapsible-if - only suggest if actually collapsible nested ifs exist
            "RL040" => {
                let collapsible_count = ctx
                    .ast_registry
                    .get(symbol_id)
                    .and_then(|item| match item {
                        PureItem::Fn(f) => Some(count_collapsible_ifs(&f.body)),
                        _ => None,
                    })
                    .unwrap_or(0);

                if collapsible_count > 0 {
                    let fn_name = ctx
                        .registry
                        .path(symbol_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let message = format!(
                        "Nested if statements may be collapsible in `{}` ({} occurrence{})",
                        fn_name,
                        collapsible_count,
                        if collapsible_count == 1 { "" } else { "s" }
                    );
                    (true, Some(message))
                } else {
                    (false, None)
                }
            }

            // RL050: underscore-noop-arm - only suggest if match actually has `_ => {}` arm
            "RL050" => {
                let empty_wild_count = ctx
                    .ast_registry
                    .get(symbol_id)
                    .and_then(|item| match item {
                        PureItem::Fn(f) => Some(count_empty_wildcard_arms(&f.body)),
                        _ => None,
                    })
                    .unwrap_or(0);

                if empty_wild_count > 0 {
                    let fn_name = ctx
                        .registry
                        .path(symbol_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let message = format!(
                        "`{}` has {} empty wildcard arm{} (`_ => {{}}`). Expand into explicit variant arms.",
                        fn_name,
                        empty_wild_count,
                        if empty_wild_count == 1 { "" } else { "s" }
                    );
                    (true, Some(message))
                } else {
                    (false, None)
                }
            }

            // RL070: enum-to-trait-candidate - only suggest for enums with enough variants and impl blocks
            "RL070" => {
                const MIN_VARIANTS: usize = 4;

                let variant_count = ctx
                    .detail_store()
                    .enum_(symbol_id)
                    .map(|d| d.variants.len())
                    .unwrap_or(0);

                if variant_count < MIN_VARIANTS {
                    return (false, None);
                }

                // Check if the enum has inherent impls (= has behavior, not just data)
                let impl_count = ctx.code_graph().impl_count(symbol_id);
                if impl_count == 0 {
                    return (false, None);
                }

                let enum_name = ctx
                    .registry
                    .path(symbol_id)
                    .map(|p| p.name().to_string())
                    .unwrap_or_else(|| "unknown".to_string());

                let message = format!(
                    "Enum `{}` has {} variants and {} impl block{} - consider EnumToTrait for extensibility",
                    enum_name,
                    variant_count,
                    impl_count,
                    if impl_count == 1 { "" } else { "s" }
                );
                (true, Some(message))
            }

            // RL003: no-direct-indexing - skip if all index ops are bounds-guarded
            "RL003" => {
                let func = match ctx.ast_registry.get(symbol_id) {
                    Some(PureItem::Fn(f)) => f,
                    _ => return (true, None),
                };

                let mut indexed_collections = Vec::new();
                collect_indexed_collections(&func.body, &mut indexed_collections);

                if indexed_collections.is_empty() {
                    return (false, None);
                }

                // Build len-alias map: `let len = x.len()` → "len" → "x"
                let mut len_aliases = std::collections::HashMap::new();
                collect_len_aliases(&func.body, &mut len_aliases);

                let mut len_checked_collections = Vec::new();
                collect_len_checked_collections(
                    &func.body,
                    &mut len_checked_collections,
                    &len_aliases,
                );

                // Check if every indexed collection has a corresponding bounds check
                let all_guarded = indexed_collections
                    .iter()
                    .all(|coll| len_checked_collections.contains(coll));

                if all_guarded {
                    (false, None)
                } else {
                    let fn_name = ctx
                        .registry
                        .path(symbol_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_else(|| "unknown".to_string());

                    let unguarded: Vec<_> = indexed_collections
                        .iter()
                        .filter(|c| !len_checked_collections.contains(c))
                        .collect();

                    let message = format!(
                        "Function `{}` has unguarded index on: {}",
                        fn_name,
                        unguarded
                            .iter()
                            .map(|s| s.as_str())
                            .collect::<Vec<_>>()
                            .join(", "),
                    );
                    (true, Some(message))
                }
            }

            // Other rules: no dynamic checks needed
            _ => (true, None),
        }
    }

    /// Scan function body for pattern matches
    fn scan_body(&self, ctx: &AnalysisContext, symbol_id: SymbolId) -> Vec<MatchResult> {
        // RL003: per-collection filtering (only flag unguarded collections)
        if self.rule.id == "RL003" {
            return self.scan_body_rl003(ctx, symbol_id);
        }

        let body_match = match &self.rule.query.body {
            Some(b) => b,
            None => return vec![], // No body pattern = no matches
        };

        // Get function from AST registry
        let func = match ctx.ast_registry.get(symbol_id) {
            Some(PureItem::Fn(f)) => f,
            _ => return vec![],
        };

        let mut results = Vec::new();

        // Check contains patterns
        if let Some(ref patterns) = body_match.contains {
            for pattern in patterns {
                let scanner = BodyScanner::new(pattern);
                let matches = scanner.scan_fn(func);
                results.extend(matches);
            }
        }

        // Check all_of patterns (all must match)
        if let Some(ref patterns) = body_match.all_of {
            let all_matched = patterns.iter().all(|pattern| {
                let scanner = BodyScanner::new(pattern);
                !scanner.scan_fn(func).is_empty()
            });
            if !all_matched {
                // If not all patterns matched, clear results
                return vec![];
            }
        }

        // Check not_contains patterns (none should match)
        if let Some(ref patterns) = body_match.not_contains {
            for pattern in patterns {
                let scanner = BodyScanner::new(pattern);
                if !scanner.scan_fn(func).is_empty() {
                    // Found a pattern that should not exist
                    return vec![];
                }
            }
        }

        results
    }

    /// RL003-specific: scan body and only return Index matches on unguarded collections.
    fn scan_body_rl003(&self, ctx: &AnalysisContext, symbol_id: SymbolId) -> Vec<MatchResult> {
        let func = match ctx.ast_registry.get(symbol_id) {
            Some(PureItem::Fn(f)) => f,
            _ => return vec![],
        };

        let mut indexed = Vec::new();
        collect_indexed_collections(&func.body, &mut indexed);
        if indexed.is_empty() {
            return vec![];
        }

        let mut aliases = std::collections::HashMap::new();
        collect_len_aliases(&func.body, &mut aliases);

        let mut checked = Vec::new();
        collect_len_checked_collections(&func.body, &mut checked, &aliases);

        // split().collect() results always have ≥1 element
        collect_split_derived(&func.body, &mut checked);

        let unguarded: Vec<String> = indexed
            .into_iter()
            .filter(|c| !checked.contains(c))
            .collect();
        if unguarded.is_empty() {
            return vec![];
        }

        let mut results = Vec::new();
        scan_unguarded_indexes_block(&func.body, &unguarded, &mut results);
        results
    }

    /// Interpolate message with captures
    fn interpolate_message(&self, match_result: &MatchResult) -> String {
        let mut message = self.rule.message.clone();

        // Replace {$VAR.text} with captured text
        for (var_name, captured) in &match_result.captures {
            let placeholder = format!("{{{}.text}}", var_name);
            message = message.replace(&placeholder, &captured.text);

            // Also support {$VAR} shorthand
            message = message.replace(&format!("{{{}}}", var_name), &captured.text);
        }

        message
    }

    /// Get location for a symbol
    fn get_location(&self, ctx: &AnalysisContext, symbol_id: SymbolId) -> Option<SuggestLocation> {
        SuggestLocation::from_context(ctx, symbol_id)
    }

    /// Get the module SymbolPath containing the primary target symbol.
    ///
    /// Returns the parent module path for targeting MutationSpecs.
    fn get_target_module_path(
        &self,
        ctx: &AnalysisContext,
        opportunity: &SuggestOpportunity,
    ) -> Option<SymbolPath> {
        let symbol_id = opportunity.primary_target()?;
        let symbol_path = ctx.registry.path(symbol_id)?;

        // Get parent module path (remove the function/item name)
        symbol_path.parent()
    }

    /// Generate MutationSpecs from Rule.fix or dynamic logic.
    ///
    /// Priority:
    /// 1. Rule.fix (YAML-defined MutationSpec) - filled with runtime context
    /// 2. Dynamic generation for rules requiring runtime info (RL060, RL061, RL070)
    fn generate_specs_for_rule(
        &self,
        ctx: &AnalysisContext,
        opportunity: &SuggestOpportunity,
    ) -> SuggestResult<Vec<MutationSpec>> {
        let target_module = self.get_target_module_path(ctx, opportunity);
        let target_fn = Some(opportunity.location.symbol_name().to_string());

        // Try Rule.fix first (YAML-defined MutationSpec)
        if let Some(fix_json) = &self.rule.fix {
            if let Ok(mut spec) = serde_json::from_value::<MutationSpec>(fix_json.clone()) {
                // Fill in runtime context (target module and function)
                Self::fill_mutation_spec_context(&mut spec, target_module, target_fn);
                return Ok(vec![spec]);
            }
        }

        // Dynamic generation for rules that need runtime info
        match self.rule.id.as_str() {
            // RL060: trait-inline-candidate → InlineTrait
            // Dynamic check already verified exactly 1 implementor exists
            "RL060" => {
                let symbol_id = opportunity.primary_target();
                if let Some(trait_id) = symbol_id {
                    // Get trait name for diagnostics
                    let _trait_name = ctx
                        .registry
                        .path(trait_id)
                        .map(|p| p.name().to_string())
                        .unwrap_or_default();

                    // Get the single implementor (we know there's exactly 1 from dynamic check)
                    let implementor = ctx.code_graph().implementors_of(trait_id).next();
                    let struct_name = implementor
                        .and_then(|id| ctx.registry.path(id))
                        .map(|p| p.name().to_string())
                        .unwrap_or_default();

                    if !struct_name.is_empty() {
                        Ok(vec![MutationSpec::InlineTrait {
                            target: ryo_executor::MutationTargetSymbol::ById(trait_id),
                            struct_name,
                            remove_trait: true,
                        }])
                    } else {
                        Ok(Vec::new())
                    }
                } else {
                    Ok(Vec::new())
                }
            }

            // RL061: impl-extract-candidate → ExtractTrait
            // Dynamic check already verified this is an inherent impl with methods
            "RL061" => {
                let symbol_id = opportunity.primary_target();
                if let Some(impl_id) = symbol_id {
                    // Get self_ty (struct name) from detail store
                    let struct_name = ctx
                        .detail_store()
                        .impl_(impl_id)
                        .map(|d| d.self_ty.clone())
                        .unwrap_or_default();

                    if !struct_name.is_empty() {
                        // Generate a suggested trait name based on struct name
                        let trait_name = format!("{}Trait", struct_name);

                        Ok(vec![MutationSpec::ExtractTrait {
                            target: ryo_executor::MutationTargetSymbol::ById(impl_id),
                            trait_name,
                            methods: None, // Extract all methods
                        }])
                    } else {
                        Ok(Vec::new())
                    }
                } else {
                    Ok(Vec::new())
                }
            }

            // RL070: enum-to-trait-candidate → EnumToTrait
            "RL070" => {
                let symbol_id = opportunity.primary_target();
                if let Some(enum_id) = symbol_id {
                    self.generate_enum_to_trait_specs(ctx, enum_id)
                } else {
                    Ok(Vec::new())
                }
            }

            // Rules without automatic fixes
            _ => Ok(Vec::new()),
        }
    }

    /// Fill in runtime context for MutationSpec.
    ///
    /// Sets target (module path) and target_fn (function name) if they are None.
    fn fill_mutation_spec_context(
        spec: &mut MutationSpec,
        target_module: Option<SymbolPath>,
        target_fn: Option<String>,
    ) {
        // NOTE: This function is deprecated as MutationSpec now uses SymbolId instead of SymbolPath/String
        // The context filling logic needs to be refactored to work with SymbolIds
        // For now, we skip filling to avoid type mismatches
        let _ = (spec, target_module, target_fn);
    }

    /// Generate MutationSpecs for EnumToTrait refactoring (RL070).
    ///
    /// Converts enum variants to trait + struct implementations:
    /// - Creates a marker trait with the enum name
    /// - Creates a struct for each variant
    /// - Creates impl Trait for each struct
    /// - Updates all usage sites (Status::Running -> Running)
    /// - Removes the original enum
    fn generate_enum_to_trait_specs(
        &self,
        _ctx: &AnalysisContext,
        enum_id: SymbolId,
    ) -> SuggestResult<Vec<MutationSpec>> {
        // Return single EnumToTrait spec - all the work is done by EnumToTraitMutation
        Ok(vec![MutationSpec::EnumToTrait {
            target: ryo_executor::MutationTargetSymbol::ById(enum_id),
            trait_name: None, // Use same name as enum
            remove_enum: true,
            strategy: EnumToTraitStrategy::default(),
            match_handling: MatchHandling::default(),
        }])
    }
}

impl std::fmt::Debug for PatternBasedSuggest {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("PatternBasedSuggest")
            .field("rule_id", &self.rule.id)
            .field("rule_name", &self.rule.name)
            .finish()
    }
}

impl Suggest for PatternBasedSuggest {
    fn name(&self) -> &'static str {
        self.name
    }

    fn description(&self) -> &str {
        &self.description
    }

    fn category(&self) -> SuggestCategory {
        SuggestCategory::Lint
    }

    fn safety_level(&self) -> SafetyLevel {
        Self::to_safety_level(self.rule.severity)
    }

    fn priority_weight(&self) -> f32 {
        match self.rule.severity {
            PatternSeverity::Error => 1.0,
            PatternSeverity::Warning => 0.8,
            PatternSeverity::Info => 0.5,
            PatternSeverity::Hint => 0.3,
        }
    }

    fn rule_id(&self) -> Option<&str> {
        Some(&self.rule.id)
    }

    fn detect(&self, ctx: &AnalysisContext, symbols: &[SymbolId]) -> Vec<SuggestOpportunity> {
        let mut opportunities = Vec::new();
        let mut next_id = 0u32;

        // Determine which symbols to check
        let symbols_to_check: Box<dyn Iterator<Item = SymbolId>> = if symbols.is_empty() {
            // Check all symbols matching the query kind
            if let Some(kind) = &self.rule.query.kind {
                use ryo_pattern::SymbolKind as PatternKind;
                let symbol_kind = match kind {
                    PatternKind::Function => Some(SymbolKind::Function),
                    PatternKind::Struct => Some(SymbolKind::Struct),
                    PatternKind::Enum => Some(SymbolKind::Enum),
                    PatternKind::Trait => Some(SymbolKind::Trait),
                    PatternKind::Impl => Some(SymbolKind::Impl),
                    PatternKind::Mod => Some(SymbolKind::Mod),
                    _ => None,
                };

                if let Some(sk) = symbol_kind {
                    Box::new(ctx.registry.iter_by_kind(sk))
                } else {
                    Box::new(ctx.registry.iter().map(|(id, _)| id))
                }
            } else {
                Box::new(ctx.registry.iter().map(|(id, _)| id))
            }
        } else {
            Box::new(symbols.iter().copied())
        };

        // Pre-compute binary crate names if this rule has scope constraints
        let binary_crates = if self.rule.scope.is_empty() {
            None
        } else {
            Some(SymbolScope::binary_crate_names(ctx))
        };

        for symbol_id in symbols_to_check {
            // Check rule scope constraint (early exit before expensive checks)
            if let Some(ref bin_crates) = binary_crates {
                let symbol_scope = SymbolScope::resolve(ctx, symbol_id, bin_crates);
                let scope_str = symbol_scope.to_string();
                if !self.rule.scope.iter().any(|s| s == &scope_str) {
                    continue;
                }
            }

            // Check kind filter
            if !self.matches_kind(ctx, symbol_id) {
                continue;
            }

            // Check attribute filter
            if !self.matches_attrs(ctx, symbol_id) {
                continue;
            }

            // Dynamic checks for rules that need runtime analysis (RL060, RL061, etc.)
            let (passes_dynamic, custom_message) = self.matches_dynamic_checks(ctx, symbol_id);
            if !passes_dynamic {
                continue;
            }

            // Scan body for pattern matches
            let matches = self.scan_body(ctx, symbol_id);

            // If body pattern exists but no matches, skip
            if self.rule.query.body.is_some() && matches.is_empty() {
                continue;
            }

            // Create opportunity for each match
            let Some(location) = self.get_location(ctx, symbol_id) else {
                continue; // Skip if symbol location cannot be resolved
            };

            if matches.is_empty() && self.rule.query.body.is_none() {
                // No body pattern, just kind/attr match
                // Use custom message from dynamic checks if available
                let message = custom_message
                    .clone()
                    .unwrap_or_else(|| self.rule.message.clone());
                let opp = self.create_lint_opportunity(
                    OpportunityId::new(next_id),
                    vec![symbol_id],
                    location,
                    message,
                    LintDetails {
                        suggestion: self.rule.suggestion.clone(),
                        expected: None,
                        actual: None,
                    },
                );
                opportunities.push(opp);
                next_id += 1;
            } else if let Some(msg) = custom_message.clone() {
                // Dynamic check provided a summary message (e.g. RL040 collapsible-if
                // count). Emit a single suggestion per symbol instead of one per body
                // match, since the dynamic check already aggregated the count.
                let opp = self.create_lint_opportunity(
                    OpportunityId::new(next_id),
                    vec![symbol_id],
                    location,
                    msg,
                    LintDetails {
                        suggestion: self.rule.suggestion.clone(),
                        expected: None,
                        actual: None,
                    },
                );
                opportunities.push(opp);
                next_id += 1;
            } else {
                // Body pattern matches — one suggestion per match
                for match_result in matches {
                    let message = self.interpolate_message(&match_result);

                    let opp = self.create_lint_opportunity(
                        OpportunityId::new(next_id),
                        vec![symbol_id],
                        location.clone(),
                        message,
                        LintDetails {
                            suggestion: self.rule.suggestion.clone(),
                            expected: None,
                            actual: None,
                        },
                    );
                    opportunities.push(opp);
                    next_id += 1;
                }
            }
        }

        opportunities
    }

    fn to_mutation_specs(
        &self,
        ctx: &AnalysisContext,
        opportunity: &SuggestOpportunity,
    ) -> SuggestResult<Vec<MutationSpec>> {
        // Generate MutationSpecs based on rule ID using static registry
        self.generate_specs_for_rule(ctx, opportunity)
    }
}

impl LintSuggest for PatternBasedSuggest {
    fn code(&self) -> &'static str {
        // We need to return &'static str, but rule.id is dynamic
        // Using leaked string (acceptable for long-lived rules)
        // Alternative: use a static registry of rule codes
        Box::leak(self.rule.id.clone().into_boxed_str())
    }

    fn default_severity(&self) -> LintSeverity {
        Self::to_lint_severity(self.rule.severity)
    }
}

/// Count statements in a block (recursive)
fn count_statements(block: &PureBlock) -> usize {
    let mut count = 0;
    for stmt in &block.stmts {
        count += 1;
        // Count nested statements in control flow
        match stmt {
            PureStmt::Expr(expr) | PureStmt::Semi(expr) => {
                count += count_statements_in_expr(expr);
            }
            _ => {}
        }
    }
    count
}

/// Check if block ends with a return statement
fn is_trailing_return(block: &PureBlock) -> bool {
    match block.stmts.last() {
        Some(PureStmt::Semi(expr)) | Some(PureStmt::Expr(expr)) => {
            matches!(expr, ryo_source::pure::PureExpr::Return(Some(_)))
        }
        _ => false,
    }
}

/// Count nested statements in expressions (if/match/loop bodies)
fn count_statements_in_expr(expr: &ryo_source::pure::PureExpr) -> usize {
    use ryo_source::pure::PureExpr;
    match expr {
        PureExpr::Block { block, .. } => count_statements(block),
        PureExpr::If {
            then_branch,
            else_branch,
            ..
        } => {
            let mut count = count_statements(then_branch);
            if let Some(else_expr) = else_branch {
                count += count_statements_in_expr(else_expr);
            }
            count
        }
        PureExpr::Match { arms, .. } => {
            arms.iter().map(|a| count_statements_in_expr(&a.body)).sum()
        }
        PureExpr::Loop { body: block, .. }
        | PureExpr::While { body: block, .. }
        | PureExpr::For { body: block, .. } => count_statements(block),
        _ => 0,
    }
}

/// Count match expressions with empty wildcard arms (`_ => {}`) in a block.
fn count_empty_wildcard_arms(block: &PureBlock) -> usize {
    let mut count = 0;
    for stmt in &block.stmts {
        let expr = match stmt {
            PureStmt::Expr(e) | PureStmt::Semi(e) => e,
            _ => continue,
        };
        count += count_empty_wildcard_arms_in_expr(expr);
    }
    count
}

fn count_empty_wildcard_arms_in_expr(expr: &PureExpr) -> usize {
    let mut count = 0;
    match expr {
        PureExpr::Match { arms, .. } => {
            // Check if any arm is `_ => {}` (wildcard with empty body)
            for arm in arms {
                if is_empty_wildcard_arm(arm) {
                    count += 1;
                }
                // Recurse into arm bodies
                count += count_empty_wildcard_arms_in_expr(&arm.body);
            }
        }
        PureExpr::If {
            then_branch,
            else_branch,
            ..
        } => {
            count += count_empty_wildcard_arms(then_branch);
            if let Some(else_expr) = else_branch {
                count += count_empty_wildcard_arms_in_expr(else_expr);
            }
        }
        PureExpr::Block { block, .. } => {
            count += count_empty_wildcard_arms(block);
        }
        PureExpr::Loop { body: block, .. }
        | PureExpr::While { body: block, .. }
        | PureExpr::For { body: block, .. } => {
            count += count_empty_wildcard_arms(block);
        }
        PureExpr::Closure { body, .. } => {
            count += count_empty_wildcard_arms_in_expr(body);
        }
        _ => {}
    }
    count
}

/// Check if a match arm is a wildcard with empty body (`_ => {}`).
fn is_empty_wildcard_arm(arm: &PureMatchArm) -> bool {
    // Pattern must be wildcard `_`
    if !matches!(arm.pattern, PurePattern::Wild) {
        return false;
    }
    // Guard must be absent
    if arm.guard.is_some() {
        return false;
    }
    // Body must be empty: unit `()` or empty block `{}`
    match &arm.body {
        PureExpr::Tuple(items) if items.is_empty() => true,
        PureExpr::Block { block, .. } if block.stmts.is_empty() => true,
        PureExpr::Lit(s) if s == "()" => true,
        _ => false,
    }
}

/// Count collapsible if patterns in a block.
///
/// A collapsible if is: `if A { if B { body } }` where:
/// - Outer if has NO else branch
/// - then_branch contains exactly 1 statement
/// - That statement is an if with NO else branch
fn count_collapsible_ifs(block: &PureBlock) -> usize {
    let mut count = 0;
    for stmt in &block.stmts {
        let expr = match stmt {
            PureStmt::Expr(e) | PureStmt::Semi(e) => e,
            _ => continue,
        };
        count += count_collapsible_ifs_in_expr(expr);
    }
    count
}

fn count_collapsible_ifs_in_expr(expr: &ryo_source::pure::PureExpr) -> usize {
    use ryo_source::pure::PureExpr;
    let mut count = 0;

    match expr {
        PureExpr::If {
            cond: outer_cond,
            then_branch,
            else_branch: None,
        } => {
            // Check if then_branch is a single if-no-else (= collapsible)
            // Exclude if-let patterns (cond is PureExpr::Let) — Clippy doesn't flag these
            if then_branch.stmts.len() == 1 && !matches!(outer_cond.as_ref(), PureExpr::Let { .. })
            {
                let inner = match &then_branch.stmts[0] {
                    PureStmt::Expr(e) | PureStmt::Semi(e) => Some(e),
                    _ => None,
                };
                if let Some(PureExpr::If {
                    cond: inner_cond,
                    else_branch: None,
                    ..
                }) = inner
                {
                    // Inner must also not be if-let
                    if !matches!(inner_cond.as_ref(), PureExpr::Let { .. }) {
                        count += 1;
                    }
                }
            }
            // Recurse into then_branch
            count += count_collapsible_ifs(then_branch);
        }
        PureExpr::If {
            then_branch,
            else_branch: Some(else_expr),
            ..
        } => {
            count += count_collapsible_ifs(then_branch);
            count += count_collapsible_ifs_in_expr(else_expr);
        }
        PureExpr::Block { block, .. } => {
            count += count_collapsible_ifs(block);
        }
        PureExpr::Match { arms, .. } => {
            for arm in arms {
                count += count_collapsible_ifs_in_expr(&arm.body);
            }
        }
        PureExpr::Loop { body: block, .. }
        | PureExpr::While { body: block, .. }
        | PureExpr::For { body: block, .. } => {
            count += count_collapsible_ifs(block);
        }
        PureExpr::Closure { body, .. } => {
            count += count_collapsible_ifs_in_expr(body);
        }
        _ => {}
    }
    count
}

/// Count `.unwrap_or(Default::default())` calls in a block.
fn count_unwrap_or_default_calls(block: &PureBlock) -> usize {
    let mut count = 0;
    for stmt in &block.stmts {
        let expr = match stmt {
            PureStmt::Expr(e) | PureStmt::Semi(e) => e,
            PureStmt::Local { init: Some(e), .. } => e,
            _ => continue,
        };
        count += count_unwrap_or_default_calls_in_expr(expr);
    }
    count
}

fn count_unwrap_or_default_calls_in_expr(expr: &PureExpr) -> usize {
    let mut count = 0;
    match expr {
        PureExpr::MethodCall {
            receiver,
            method,
            args,
            ..
        } => {
            if method == "unwrap_or" && args.len() == 1 && is_default_default_call(&args[0]) {
                count += 1;
            }
            count += count_unwrap_or_default_calls_in_expr(receiver);
            for arg in args {
                count += count_unwrap_or_default_calls_in_expr(arg);
            }
        }
        PureExpr::Call { func, args } => {
            count += count_unwrap_or_default_calls_in_expr(func);
            for arg in args {
                count += count_unwrap_or_default_calls_in_expr(arg);
            }
        }
        PureExpr::Binary { left, right, .. } => {
            count += count_unwrap_or_default_calls_in_expr(left);
            count += count_unwrap_or_default_calls_in_expr(right);
        }
        PureExpr::Unary { expr, .. } | PureExpr::Try(expr) | PureExpr::Await(expr) => {
            count += count_unwrap_or_default_calls_in_expr(expr);
        }
        PureExpr::Field { expr, .. } => {
            count += count_unwrap_or_default_calls_in_expr(expr);
        }
        PureExpr::Index { expr, index } => {
            count += count_unwrap_or_default_calls_in_expr(expr);
            count += count_unwrap_or_default_calls_in_expr(index);
        }
        PureExpr::If {
            cond,
            then_branch,
            else_branch,
        } => {
            count += count_unwrap_or_default_calls_in_expr(cond);
            count += count_unwrap_or_default_calls(then_branch);
            if let Some(else_expr) = else_branch {
                count += count_unwrap_or_default_calls_in_expr(else_expr);
            }
        }
        PureExpr::Match { expr, arms } => {
            count += count_unwrap_or_default_calls_in_expr(expr);
            for arm in arms {
                count += count_unwrap_or_default_calls_in_expr(&arm.body);
            }
        }
        PureExpr::Block { block, .. } => {
            count += count_unwrap_or_default_calls(block);
        }
        PureExpr::Loop { body: block, .. }
        | PureExpr::While { body: block, .. }
        | PureExpr::For { body: block, .. } => {
            count += count_unwrap_or_default_calls(block);
        }
        PureExpr::Closure { body, .. } => {
            count += count_unwrap_or_default_calls_in_expr(body);
        }
        _ => {}
    }
    count
}

/// Check if an expression is `Default::default()` (a call to `Default::default` with no args).
fn is_default_default_call(expr: &PureExpr) -> bool {
    match expr {
        PureExpr::Call { func, args } if args.is_empty() => {
            matches!(func.as_ref(), PureExpr::Path(p) if p == "Default::default" || p == "Default :: default")
        }
        _ => false,
    }
}

/// Extract the root name from a PureExpr (e.g., `x`, `self.field` → `self`).
fn expr_root_name(expr: &PureExpr) -> Option<&str> {
    match expr {
        PureExpr::Path(name) => Some(name.as_str()),
        PureExpr::Field { expr, .. } => expr_root_name(expr),
        PureExpr::MethodCall { receiver, .. } => expr_root_name(receiver),
        _ => None,
    }
}

/// Walk a block and collect one MatchResult per Index expression on an unguarded collection.
fn scan_unguarded_indexes_block(
    block: &PureBlock,
    unguarded: &[String],
    results: &mut Vec<MatchResult>,
) {
    for stmt in &block.stmts {
        match stmt {
            PureStmt::Local {
                init: Some(expr), ..
            } => {
                scan_unguarded_indexes_expr(expr, unguarded, results);
            }
            PureStmt::Semi(expr) | PureStmt::Expr(expr) => {
                scan_unguarded_indexes_expr(expr, unguarded, results);
            }
            _ => {}
        }
    }
}

fn scan_unguarded_indexes_expr(
    expr: &PureExpr,
    unguarded: &[String],
    results: &mut Vec<MatchResult>,
) {
    // Check if this is an unguarded Index expression
    if let PureExpr::Index { expr: coll, index } = expr {
        if let Some(name) = expr_root_name(coll) {
            if unguarded.iter().any(|u| u == name) {
                results.push(MatchResult::matched());
            }
        }
        scan_unguarded_indexes_expr(coll, unguarded, results);
        scan_unguarded_indexes_expr(index, unguarded, results);
        return;
    }

    match expr {
        PureExpr::Binary { left, right, .. } => {
            scan_unguarded_indexes_expr(left, unguarded, results);
            scan_unguarded_indexes_expr(right, unguarded, results);
        }
        PureExpr::Unary { expr, .. } | PureExpr::Try(expr) | PureExpr::Field { expr, .. } => {
            scan_unguarded_indexes_expr(expr, unguarded, results);
        }
        PureExpr::MethodCall { receiver, args, .. } => {
            scan_unguarded_indexes_expr(receiver, unguarded, results);
            for a in args {
                scan_unguarded_indexes_expr(a, unguarded, results);
            }
        }
        PureExpr::Call { func, args } => {
            scan_unguarded_indexes_expr(func, unguarded, results);
            for a in args {
                scan_unguarded_indexes_expr(a, unguarded, results);
            }
        }
        PureExpr::If {
            cond,
            then_branch,
            else_branch,
            ..
        } => {
            scan_unguarded_indexes_expr(cond, unguarded, results);
            scan_unguarded_indexes_block(then_branch, unguarded, results);
            if let Some(e) = else_branch {
                scan_unguarded_indexes_expr(e, unguarded, results);
            }
        }
        PureExpr::Block { block, .. } => {
            scan_unguarded_indexes_block(block, unguarded, results);
        }
        PureExpr::Match { expr: e, arms } => {
            scan_unguarded_indexes_expr(e, unguarded, results);
            for arm in arms {
                scan_unguarded_indexes_expr(&arm.body, unguarded, results);
            }
        }
        PureExpr::Loop { body, .. } => {
            scan_unguarded_indexes_block(body, unguarded, results);
        }
        PureExpr::While { cond, body, .. } => {
            scan_unguarded_indexes_expr(cond, unguarded, results);
            scan_unguarded_indexes_block(body, unguarded, results);
        }
        PureExpr::For {
            expr: iter, body, ..
        } => {
            scan_unguarded_indexes_expr(iter, unguarded, results);
            scan_unguarded_indexes_block(body, unguarded, results);
        }
        PureExpr::Closure { body, .. } => {
            scan_unguarded_indexes_expr(body, unguarded, results);
        }
        PureExpr::Tuple(items) | PureExpr::Array(items) => {
            for item in items {
                scan_unguarded_indexes_expr(item, unguarded, results);
            }
        }
        PureExpr::Let { expr, .. } | PureExpr::Return(Some(expr)) => {
            scan_unguarded_indexes_expr(expr, unguarded, results);
        }
        _ => {}
    }
}

/// Collect names of collections that are directly indexed in a block.
fn collect_indexed_collections(block: &PureBlock, out: &mut Vec<String>) {
    for stmt in &block.stmts {
        let expr = match stmt {
            PureStmt::Expr(e) | PureStmt::Semi(e) => e,
            PureStmt::Local { init, .. } => {
                if let Some(init) = init {
                    collect_indexed_collections_in_expr(init, out);
                }
                continue;
            }
            _ => continue,
        };
        collect_indexed_collections_in_expr(expr, out);
    }
}

fn collect_indexed_collections_in_expr(expr: &PureExpr, out: &mut Vec<String>) {
    match expr {
        PureExpr::Index { expr: coll, index } => {
            if let Some(name) = expr_root_name(coll) {
                if !out.contains(&name.to_string()) {
                    out.push(name.to_string());
                }
            }
            collect_indexed_collections_in_expr(coll, out);
            collect_indexed_collections_in_expr(index, out);
        }
        PureExpr::Binary { left, right, .. } => {
            collect_indexed_collections_in_expr(left, out);
            collect_indexed_collections_in_expr(right, out);
        }
        PureExpr::Unary { expr, .. } | PureExpr::Try(expr) | PureExpr::Return(Some(expr)) => {
            collect_indexed_collections_in_expr(expr, out);
        }
        PureExpr::Call { func, args } => {
            collect_indexed_collections_in_expr(func, out);
            for a in args {
                collect_indexed_collections_in_expr(a, out);
            }
        }
        PureExpr::MethodCall { receiver, args, .. } => {
            collect_indexed_collections_in_expr(receiver, out);
            for a in args {
                collect_indexed_collections_in_expr(a, out);
            }
        }
        PureExpr::Field { expr, .. } => {
            collect_indexed_collections_in_expr(expr, out);
        }
        PureExpr::If {
            cond,
            then_branch,
            else_branch,
            ..
        } => {
            collect_indexed_collections_in_expr(cond, out);
            collect_indexed_collections(then_branch, out);
            if let Some(e) = else_branch {
                collect_indexed_collections_in_expr(e, out);
            }
        }
        PureExpr::Block { block, .. } => {
            collect_indexed_collections(block, out);
        }
        PureExpr::Match { expr, arms } => {
            collect_indexed_collections_in_expr(expr, out);
            for arm in arms {
                collect_indexed_collections_in_expr(&arm.body, out);
            }
        }
        PureExpr::Loop { body, .. } | PureExpr::While { body, .. } | PureExpr::For { body, .. } => {
            collect_indexed_collections(body, out);
        }
        PureExpr::Closure { body, .. } => {
            collect_indexed_collections_in_expr(body, out);
        }
        PureExpr::Tuple(items) | PureExpr::Array(items) => {
            for item in items {
                collect_indexed_collections_in_expr(item, out);
            }
        }
        PureExpr::Let { expr, .. } => {
            collect_indexed_collections_in_expr(expr, out);
        }
        _ => {}
    }
}

/// Collect `let var = collection.len()` aliases from a block.
fn collect_len_aliases(block: &PureBlock, aliases: &mut std::collections::HashMap<String, String>) {
    for stmt in &block.stmts {
        if let PureStmt::Local { pattern, init, .. } = stmt {
            if let (
                PurePattern::Ident { name: var_name, .. },
                Some(PureExpr::MethodCall {
                    receiver, method, ..
                }),
            ) = (pattern, init)
            {
                if matches!(method.as_str(), "len" | "find" | "rfind" | "position") {
                    if let Some(coll_name) = expr_root_name(receiver) {
                        aliases.insert(var_name.clone(), coll_name.to_string());
                    }
                }
            }
        }
        // Recurse into nested blocks
        if let Some(expr) = stmt.get_expr() {
            collect_len_aliases_in_expr(expr, aliases);
        }
    }
}

fn collect_len_aliases_in_expr(
    expr: &PureExpr,
    aliases: &mut std::collections::HashMap<String, String>,
) {
    match expr {
        PureExpr::If {
            then_branch,
            else_branch,
            ..
        } => {
            collect_len_aliases(then_branch, aliases);
            if let Some(e) = else_branch {
                collect_len_aliases_in_expr(e, aliases);
            }
        }
        PureExpr::Block { block, .. } => collect_len_aliases(block, aliases),
        PureExpr::Loop { body, .. } | PureExpr::While { body, .. } | PureExpr::For { body, .. } => {
            collect_len_aliases(body, aliases);
        }
        PureExpr::Match { arms, .. } => {
            for arm in arms {
                collect_len_aliases_in_expr(&arm.body, aliases);
            }
        }
        PureExpr::Closure { body, .. } => collect_len_aliases_in_expr(body, aliases),
        _ => {}
    }
}

/// Collect names of collections that have bounds checks in if/while/for conditions.
fn collect_len_checked_collections(
    block: &PureBlock,
    out: &mut Vec<String>,
    aliases: &std::collections::HashMap<String, String>,
) {
    for stmt in &block.stmts {
        let expr = match stmt {
            PureStmt::Expr(e) | PureStmt::Semi(e) => e,
            PureStmt::Local { init, .. } => {
                if let Some(init) = init {
                    collect_len_checked_in_expr(init, out, aliases);
                }
                continue;
            }
            _ => continue,
        };
        collect_len_checked_in_expr(expr, out, aliases);
    }
}

fn collect_len_checked_in_expr(
    expr: &PureExpr,
    out: &mut Vec<String>,
    aliases: &std::collections::HashMap<String, String>,
) {
    match expr {
        PureExpr::If {
            cond,
            then_branch,
            else_branch,
            ..
        } => {
            collect_len_names_from_cond(cond, out, aliases);
            collect_len_checked_collections(then_branch, out, aliases);
            if let Some(e) = else_branch {
                collect_len_checked_in_expr(e, out, aliases);
            }
        }
        PureExpr::While { cond, body, .. } => {
            collect_len_names_from_cond(cond, out, aliases);
            collect_len_checked_collections(body, out, aliases);
        }
        PureExpr::For { expr, body, .. } => {
            // Extract bounds from range iterator: `for i in 0..collection.len()`
            if let PureExpr::Range { end: Some(end), .. } = expr.as_ref() {
                if let Some(name) = extract_bounds_receiver(end) {
                    if !out.contains(&name) {
                        out.push(name);
                    }
                }
                // Alias: `for i in 0..len` where `let len = x.len()`
                if let PureExpr::Path(var) = end.as_ref() {
                    if let Some(coll) = aliases.get(var.as_str()) {
                        if !out.contains(coll) {
                            out.push(coll.clone());
                        }
                    }
                }
            }
            collect_len_checked_collections(body, out, aliases);
        }
        PureExpr::Block { block, .. } => {
            collect_len_checked_collections(block, out, aliases);
        }
        PureExpr::Loop { body, .. } => {
            collect_len_checked_collections(body, out, aliases);
        }
        PureExpr::Match { expr, arms } => {
            // C: `match x.len() { N => x[i] }` — match on .len() implies bounds
            if let Some(name) = extract_bounds_receiver(expr) {
                if !out.contains(&name) {
                    out.push(name);
                }
            }
            // Also check len-alias: `match len { N => x[i] }` where `let len = x.len()`
            if let PureExpr::Path(var) = expr.as_ref() {
                if let Some(coll) = aliases.get(var.as_str()) {
                    if !out.contains(coll) {
                        out.push(coll.clone());
                    }
                }
            }
            // Check tuple elements: `match (x, y) { (Some(a), Some(b)) => ... }`
            // where x/y are find/position aliases
            if let PureExpr::Tuple(items) = expr.as_ref() {
                for item in items {
                    if let Some(name) = extract_bounds_receiver(item) {
                        if !out.contains(&name) {
                            out.push(name);
                        }
                    }
                    if let PureExpr::Path(var) = item {
                        if let Some(coll) = aliases.get(var.as_str()) {
                            if !out.contains(coll) {
                                out.push(coll.clone());
                            }
                        }
                    }
                }
            }
            for arm in arms {
                collect_len_checked_in_expr(&arm.body, out, aliases);
            }
        }
        PureExpr::Closure { body, .. } => {
            collect_len_checked_in_expr(body, out, aliases);
        }
        // Traverse into arbitrary expressions to find nested control flow
        PureExpr::MethodCall { receiver, args, .. } => {
            collect_len_checked_in_expr(receiver, out, aliases);
            for a in args {
                collect_len_checked_in_expr(a, out, aliases);
            }
        }
        PureExpr::Call { func, args } => {
            collect_len_checked_in_expr(func, out, aliases);
            for a in args {
                collect_len_checked_in_expr(a, out, aliases);
            }
        }
        PureExpr::Binary { left, right, .. } => {
            collect_len_checked_in_expr(left, out, aliases);
            collect_len_checked_in_expr(right, out, aliases);
        }
        PureExpr::Unary { expr, .. } | PureExpr::Try(expr) | PureExpr::Field { expr, .. } => {
            collect_len_checked_in_expr(expr, out, aliases);
        }
        PureExpr::Tuple(items) | PureExpr::Array(items) => {
            for item in items {
                collect_len_checked_in_expr(item, out, aliases);
            }
        }
        PureExpr::Index { expr, index } => {
            collect_len_checked_in_expr(expr, out, aliases);
            collect_len_checked_in_expr(index, out, aliases);
        }
        PureExpr::Let { expr, .. } | PureExpr::Return(Some(expr)) => {
            collect_len_checked_in_expr(expr, out, aliases);
        }
        _ => {}
    }
}

/// Extract collection names from condition expressions.
/// Recognizes: `.len()` calls, `.is_empty()`, `.starts_with()/.ends_with()`,
/// `if let Some = .find()`, and len-aliased variables (`let n = x.len(); i < n`).
fn collect_len_names_from_cond(
    cond: &PureExpr,
    out: &mut Vec<String>,
    aliases: &std::collections::HashMap<String, String>,
) {
    match cond {
        PureExpr::Binary { op, left, right } => match op.as_str() {
            "<" | "<=" | ">" | ">=" | "==" | "!=" => {
                // Direct .len() call: `x.len() < n`
                for side in [left.as_ref(), right.as_ref()] {
                    if let Some(name) = extract_bounds_receiver(side) {
                        if !out.contains(&name) {
                            out.push(name);
                        }
                    }
                    // Len alias: `i < len` where `let len = x.len()`
                    if let PureExpr::Path(var) = side {
                        if let Some(coll) = aliases.get(var.as_str()) {
                            if !out.contains(coll) {
                                out.push(coll.clone());
                            }
                        }
                    }
                }
            }
            "&&" | "||" => {
                collect_len_names_from_cond(left, out, aliases);
                collect_len_names_from_cond(right, out, aliases);
            }
            _ => {}
        },
        PureExpr::Unary { op, expr } if op == "!" => {
            collect_len_names_from_cond(expr, out, aliases);
        }
        // Standalone method call: `x.is_empty()`, `x.starts_with()`, etc.
        PureExpr::MethodCall {
            receiver, method, ..
        } if is_bounds_implying_method(method) => {
            if let Some(name) = expr_root_name(receiver) {
                if !out.contains(&name.to_string()) {
                    out.push(name.to_string());
                }
            }
        }
        // `if let Some(idx) = x.find(...)` — find() returns valid index
        // Also handles combinator chains: x.find().or_else(|| x.find())
        PureExpr::Let { expr, .. } => {
            if let Some(name) = extract_bounds_receiver(expr) {
                if !out.contains(&name) {
                    out.push(name);
                }
            }
            // Alias: `if let Some(..) = variable` where `let variable = x.find(..)`
            if let PureExpr::Path(var) = expr.as_ref() {
                if let Some(coll) = aliases.get(var.as_str()) {
                    if !out.contains(coll) {
                        out.push(coll.clone());
                    }
                }
            }
        }
        _ => {}
    }
}

/// Methods that imply bounds awareness on the receiver.
fn is_bounds_implying_method(method: &str) -> bool {
    matches!(
        method,
        "is_empty"
            | "len"
            | "starts_with"
            | "ends_with"
            | "contains"
            | "find"
            | "rfind"
            | "position"
            | "get"
            | "get_mut"
    )
}

/// If expr is `something.len()`, `.is_empty()`, `.starts_with()`, etc., return root name.
fn extract_bounds_receiver(expr: &PureExpr) -> Option<String> {
    if let PureExpr::MethodCall {
        receiver, method, ..
    } = expr
    {
        if is_bounds_implying_method(method) {
            return expr_root_name(receiver).map(|s| s.to_string());
        }
        // Chain: x.len().saturating_sub(N) → extract from inner .len()
        if matches!(
            method.as_str(),
            "saturating_sub" | "checked_sub" | "wrapping_sub"
        ) {
            return extract_bounds_receiver(receiver);
        }
        // Combinator chain: x.find().or_else(), x.get().map(), etc.
        if matches!(
            method.as_str(),
            "or_else" | "and_then" | "map" | "unwrap_or" | "unwrap_or_else"
        ) {
            return extract_bounds_receiver(receiver);
        }
    }
    None
}

/// Collect variables derived from `split().collect()` — guaranteed non-empty.
///
/// `str::split()` always returns at least 1 element, so `parts[0]` after
/// `let parts: Vec<&str> = s.split('/').collect()` is always safe.
fn collect_split_derived(block: &PureBlock, out: &mut Vec<String>) {
    for stmt in &block.stmts {
        if let PureStmt::Local { pattern, init, .. } = stmt {
            if let (PurePattern::Ident { name, .. }, Some(init_expr)) = (pattern, init) {
                if is_split_collect_chain(init_expr) && !out.contains(name) {
                    out.push(name.clone());
                }
            }
        }
    }
}

/// Check if an expression is a `.split(...).collect()` chain (possibly with intermediate methods).
fn is_split_collect_chain(expr: &PureExpr) -> bool {
    if let PureExpr::MethodCall {
        receiver, method, ..
    } = expr
    {
        if method == "collect" {
            return has_split_in_chain(receiver);
        }
    }
    false
}

fn has_split_in_chain(expr: &PureExpr) -> bool {
    if let PureExpr::MethodCall {
        receiver, method, ..
    } = expr
    {
        if matches!(
            method.as_str(),
            "split" | "splitn" | "split_whitespace" | "split_terminator" | "split_ascii_whitespace"
        ) {
            return true;
        }
        return has_split_in_chain(receiver);
    }
    false
}

#[cfg(test)]
mod tests {
    use super::*;
    use ryo_pattern::{PatternQuery, Severity, SymbolKind as PatternKind};

    fn create_test_rule() -> Rule {
        Rule::new(
            "TEST001",
            "test-rule",
            Severity::Warning,
            PatternQuery::new().kind(PatternKind::Function),
            "Test message",
        )
    }

    #[test]
    fn test_pattern_based_suggest_basic() {
        let rule = create_test_rule();
        let suggest = PatternBasedSuggest::new(rule);

        assert_eq!(suggest.name(), "TEST001"); // name() returns rule.id
        assert_eq!(suggest.description(), "test-rule");
        assert_eq!(suggest.category(), SuggestCategory::Lint);
        assert_eq!(suggest.safety_level(), SafetyLevel::Confirm);
    }

    #[test]
    fn test_severity_conversion() {
        assert_eq!(
            PatternBasedSuggest::to_lint_severity(Severity::Error),
            LintSeverity::Error
        );
        assert_eq!(
            PatternBasedSuggest::to_lint_severity(Severity::Warning),
            LintSeverity::Warning
        );
        assert_eq!(
            PatternBasedSuggest::to_lint_severity(Severity::Info),
            LintSeverity::Info
        );
        assert_eq!(
            PatternBasedSuggest::to_lint_severity(Severity::Hint),
            LintSeverity::Info
        );
    }

    #[test]
    fn test_safety_level_conversion() {
        assert_eq!(
            PatternBasedSuggest::to_safety_level(Severity::Error),
            SafetyLevel::Manual
        );
        assert_eq!(
            PatternBasedSuggest::to_safety_level(Severity::Warning),
            SafetyLevel::Confirm
        );
        assert_eq!(
            PatternBasedSuggest::to_safety_level(Severity::Info),
            SafetyLevel::Auto
        );
    }

    #[test]
    fn test_glob_matching() {
        let suggest = PatternBasedSuggest::new(create_test_rule());

        assert!(suggest.matches_glob("*", "anything"));
        assert!(suggest.matches_glob("test_*", "test_foo"));
        assert!(!suggest.matches_glob("test_*", "foo_test"));
        assert!(suggest.matches_glob("*_test", "foo_test"));
        assert!(!suggest.matches_glob("*_test", "test_foo"));
        assert!(suggest.matches_glob("exact", "exact"));
        assert!(!suggest.matches_glob("exact", "different"));
    }

    #[test]
    fn test_mutation_spec_generation_rl001() {
        // Test RL001 (no-unwrap-in-public) generates UnwrapToQuestion
        let rule = Rule::new(
            "RL001",
            "no-unwrap-in-public",
            Severity::Warning,
            PatternQuery::new().kind(PatternKind::Function),
            "Avoid unwrap() in public function",
        );
        let suggest = PatternBasedSuggest::new(rule);

        // Verify rule ID is correctly mapped
        assert_eq!(suggest.rule.id, "RL001");

        // The generate_specs_for_rule would return UnwrapToQuestion for RL001
        // We verify the rule configuration matches our expectation
        assert!(suggest.rule.id.starts_with("RL001"));
    }

    #[test]
    fn test_mutation_spec_generation_rl020() {
        // Test RL020 (no-dbg-macro) generates RemoveStatement
        let rule = Rule::new(
            "RL020",
            "no-dbg-macro",
            Severity::Warning,
            PatternQuery::new().kind(PatternKind::Function),
            "Found dbg!() macro",
        );
        let suggest = PatternBasedSuggest::new(rule);

        assert_eq!(suggest.rule.id, "RL020");
    }

    #[test]
    fn test_mutation_spec_generation_unknown_rule() {
        // Test unknown rule returns empty Vec
        let rule = Rule::new(
            "UNKNOWN",
            "unknown-rule",
            Severity::Info,
            PatternQuery::new(),
            "Unknown",
        );
        let suggest = PatternBasedSuggest::new(rule);

        // Unknown rules should not have automatic fixes
        assert_eq!(suggest.rule.id, "UNKNOWN");
        // generate_specs_for_rule would return Ok(Vec::new()) for unknown rules
    }

    #[test]
    fn test_priority_weight() {
        let error_rule = Rule::new(
            "E001",
            "error-rule",
            Severity::Error,
            PatternQuery::new(),
            "Error",
        );
        let warning_rule = Rule::new(
            "W001",
            "warning-rule",
            Severity::Warning,
            PatternQuery::new(),
            "Warning",
        );
        let info_rule = Rule::new(
            "I001",
            "info-rule",
            Severity::Info,
            PatternQuery::new(),
            "Info",
        );

        assert_eq!(PatternBasedSuggest::new(error_rule).priority_weight(), 1.0);
        assert_eq!(
            PatternBasedSuggest::new(warning_rule).priority_weight(),
            0.8
        );
        assert_eq!(PatternBasedSuggest::new(info_rule).priority_weight(), 0.5);
    }
}