Files that changed from the base of the PR and between c4ec1ef and 11a326c.

• crates/cli/src/run.rs (7 hunks)
• crates/cli/src/scan.rs (4 hunks)
• crates/cli/src/utils/mod.rs (1 hunks)
• crates/cli/src/utils/tracing.rs (1 hunks)
• crates/cli/src/utils/worker.rs (3 hunks)

crates/cli/src/utils/tracing.rs (5)

26-33: LGTM! Tracing enum is well-defined.

The Tracing enum is correctly defined with the Summary variant. The TODO comment for implementing other levels aligns with the file header description.

---

35-56: LGTM! FileStats struct is well-implemented for thread-safe operations.

The FileStats struct uses AtomicUsize for thread-safe counters. The methods for adding and retrieving stats are correctly implemented using appropriate atomic operations, ensuring thread safety and efficiency.

---

58-62: LGTM! SummaryStats struct is well-designed with generic support.

The SummaryStats struct is well-designed to accommodate different types of statistics using generics. This approach provides flexibility for various use cases.

---

64-96: LGTM! PatternStats and RuleStats are well-implemented.

The PatternStats and RuleStats structs, along with the RuleStats implementation, are correctly designed and implemented. They align well with the tracing requirements:

1. The comment about not needing atomic operations is accurate.
2. The count_match method in RuleStats handles all severity cases exhaustively.
3. The methods for updating rule counts are straightforward and effective.

---

98-109: LGTM! Type aliases and Default implementation are well-defined.

The type aliases for RunStats and ScanStats provide clear and concise names for specific SummaryStats instances. The Default implementation for SummaryStats<T> is correct and initializes all fields appropriately.

crates/cli/src/utils/worker.rs (2)

1-1: LGTM: New import for FileStats

The new import for FileStats is correctly added and is necessary for the new functionality being introduced.

---

117-120: Approve changes, but consider thread safety

The usage of get_stats to update file processing statistics is implemented correctly. However, there's a potential thread safety concern.

Multiple threads might be updating the stats concurrently, which could lead to race conditions. Consider using atomic operations or a mutex to ensure thread-safe updates to the stats.

To verify the thread safety of FileStats, let's check its implementation:

✅ Verification successful

FileStats is thread-safe

The implementation of FileStats uses atomic operations (AtomicUsize) with fetch_add and appropriate memory ordering (Ordering::AcqRel), ensuring thread-safe updates to the statistics.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check the implementation of FileStats for thread safety

# Test: Search for the FileStats struct definition
rg --type rust -A 10 'struct FileStats'

Length of output: 670

crates/cli/src/utils/mod.rs (2)

4-4: LGTM: New tracing module added.

The addition of the tracing module is a good enhancement to the utility functions. It follows Rust naming conventions and is placed consistently with other module declarations.

---

10-10: LGTM: New tracing types exported.

The public export of FileStats, RunStats, and ScanStats from the tracing module is appropriate and consistent with the module structure. These types will likely be useful for tracking various statistics throughout the application.

To ensure these new types are being utilized effectively, you may want to run the following command to check their usage across the codebase:

✅ Verification successful

Verified: New tracing types are actively used across the codebase.
The addition of FileStats, RunStats, and ScanStats has been confirmed through their usage in multiple modules. This ensures that the public exports are necessary and correctly integrated.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check usage of new tracing types

# Test: Search for usage of FileStats, RunStats, and ScanStats
rg --type rust -e "FileStats" -e "RunStats" -e "ScanStats"

Length of output: 1273

crates/cli/src/scan.rs (3)

20-20: LGTM: Import for ScanStats added.

The import for ScanStats is correctly placed with other imports from the crate::utils module. This addition suggests the introduction of scanning statistics functionality, which aligns with the PR objective of adding a tracing level summary.

---

105-105: LGTM: ScanStats field added to ScanWithConfig struct.

The addition of the stats field of type ScanStats to the ScanWithConfig struct is consistent with the PR objective of adding a tracing level summary. This field will allow for tracking and reporting of scanning statistics.

---

123-123: LGTM: ScanStats initialized in ScanWithConfig::try_new.

The stats field is correctly initialized with ScanStats::default() in the try_new method. This ensures that every new ScanWithConfig instance starts with a clean slate for statistics, which is crucial for accurate tracing level summaries.

Files that changed from the base of the PR and between 11a326c and b247a7b.

• crates/cli/src/config.rs (4 hunks)
• crates/cli/src/lsp.rs (1 hunks)
• crates/cli/src/scan.rs (5 hunks)
• crates/cli/src/utils/mod.rs (1 hunks)
• crates/cli/src/utils/tracing.rs (1 hunks)
• crates/cli/src/verify.rs (1 hunks)
• crates/config/src/rule_collection.rs (1 hunks)

• crates/cli/src/utils/mod.rs

crates/cli/src/lsp.rs (1)

28-36: 🛠️ Refactor suggestion

Consider simplifying error handling and clarifying the tuple extraction

The changes to config_result_std processing add complexity to the code. While the error handling is more comprehensive, it might impact readability. Additionally, the extraction of the first element of the result tuple (r.0) is not immediately clear without context.

Suggestions for improvement:

1. Consider moving the error chain processing to a separate function for better readability.
2. Add a comment explaining why only the first element of the result tuple is needed.
3. If possible, consider using a custom error type instead of converting to String to maintain more structure in error handling.

Here's a potential refactoring to improve readability:

fn process_error_chain(e: anyhow::Error) -> String {
    e.chain()
        .map(|e| e.to_string())
        .collect::<Vec<_>>()
        .join(". ")
}

// In run_language_server_impl
let config_result_std: std::result::Result<_, String> = config_result
    .map_err(process_error_chain)
    .map(|r| r.0); // Extract first element of the tuple (add comment explaining why)

To ensure this change doesn't affect other parts of the codebase, let's verify the usage of config_result_std:

✅ Verification successful

Verification Successful: config_result_std is only used within crates/cli/src/lsp.rs

The config_result_std variable is exclusively declared and utilized in crates/cli/src/lsp.rs. The proposed refactoring and simplifications are localized and do not impact other parts of the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other occurrences of config_result_std
rg 'config_result_std' --type rust

Length of output: 245

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 28-29: crates/cli/src/lsp.rs#L28-L29
Added lines #L28 - L29 were not covered by tests

---

[warning] 31-32: crates/cli/src/lsp.rs#L31-L32
Added lines #L31 - L32 were not covered by tests

---

[warning] 36-36: crates/cli/src/lsp.rs#L36
Added line #L36 was not covered by tests

crates/cli/src/utils/tracing.rs (2)

72-79: Approve Default implementation for SummaryStats

The Default implementation for SummaryStats<T> is correct and well-structured:

• It properly initializes both file_stats and inner fields.
• The use of T: Default constraint ensures that the inner type can be defaulted.

This implementation provides a convenient way to create new SummaryStats instances with default values, which is particularly useful for initializing collections or temporary variables.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 76-76: crates/cli/src/utils/tracing.rs#L76
Added line #L76 was not covered by tests

---

49-50: Improve test coverage

The static analysis tools have identified several lines that are not covered by tests:

• Lines 49-50: scanned() method in FileStats
• Lines 52-53: skipped() method in FileStats
• Line 76: Initialization of inner field in SummaryStats::default()

To ensure the reliability and correctness of the code, it's important to have comprehensive test coverage, especially for public methods.

Consider adding unit tests for these methods and the Default implementation. Here's a script to verify the current test coverage:

Would you like assistance in writing unit tests for these uncovered methods?

Also applies to: 52-53, 76-76

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 49-50: crates/cli/src/utils/tracing.rs#L49-L50
Added lines #L49 - L50 were not covered by tests

crates/config/src/rule_collection.rs (1)

141-145: Positive addition to the RuleCollection API

The total_rule_count method is a valuable addition to the RuleCollection struct. It provides a convenient way to retrieve the total number of rules across all buckets, which can be useful for various purposes such as logging, debugging, or reporting.

This method enhances the API without introducing any side effects or modifying existing behavior, making it a safe and beneficial addition to the codebase.

crates/cli/src/verify.rs (2)

Line range hint 91-93: Improved readability with let-else syntax

Great job on using the let-else syntax for handling the Option type. This change:

1. Enhances readability by simplifying the control flow.
2. Makes the code more concise and idiomatic.
3. Maintains the same logic while improving the structure.

---

58-58: Verify the impact of using only the first element of the tuple.

The change to use only the first element of the tuple returned by find_rules looks intentional. However, please ensure that:

1. The second element of the tuple is not needed elsewhere in the function or in any calling functions.
2. This change doesn't break any existing functionality that might have relied on the full tuple.

To confirm that the second element of the tuple is not used elsewhere, you can run the following command:

This will search for any usage of the second element of the tuple returned by find_rules in the crates directory. If there are no results, it's likely safe to proceed with this change.

crates/cli/src/scan.rs (5)

20-20: LGTM: New import for statistics types.

The new import statement for FileStats, RuleStats, and ScanStats from the utils module is consistent with the changes made in the rest of the file. These types are used to track statistics during the scanning process.

---

105-105: LGTM: Added stats field to ScanWithConfig struct.

The addition of the stats field of type ScanStats to the ScanWithConfig struct is consistent with the goal of adding a tracing level summary. This change allows for tracking scanning statistics throughout the scanning process.

---

Line range hint 109-120: LGTM: Updated try_new method to initialize stats.

The changes to the try_new method properly initialize the stats field of ScanWithConfig:

1. A rule_stats variable is initialized and updated based on the result of find_rules.
2. The stats field is correctly initialized with FileStats::default() and the rule_stats.

These changes are consistent with the goal of adding a tracing level summary and tracking scanning statistics.

Also applies to: 126-129

---

176-178: LGTM: Added get_stats method to PathWorker implementation.

The new get_stats method is a good addition to the PathWorker trait implementation for ScanWithConfig. It provides external access to the FileStats collected during the scanning process, which is consistent with the goal of adding a tracing level summary.

---

Line range hint 1-378: Overall assessment: Changes look good with a minor suggestion.

The changes made to crates/cli/src/scan.rs successfully implement the tracing level summary feature. The additions and modifications are consistent and well-integrated into the existing code structure. Here's a summary of the review:

1. New imports for statistics types were added.
2. The ScanWithConfig struct was updated to include a stats field.
3. The try_new method was modified to initialize the stats field properly.
4. A debug print statement was added to the consume_items method (with a suggestion to use a proper logging mechanism).
5. A new get_stats method was added to provide access to file statistics.

These changes effectively implement the tracing level summary feature. The only suggestion is to replace the eprintln! statement with a proper logging mechanism for better control over log output.

crates/cli/src/config.rs (5)

2-2: Importing RuleStats is appropriate for tracking rule statistics.

The addition of RuleStats to the imports is necessary to handle the new functionality related to rule statistics.

---

60-60: Updating find_rules to return RuleStats alongside RuleCollection is appropriate.

The modified function signature reflects the enhanced functionality of providing detailed statistics about the rules processed.

---

135-135: Updating read_directory_yaml to return RuleStats enhances reporting capabilities.

The change in the function's return type to include RuleStats allows for comprehensive feedback on rule processing outcomes.

---

157-157: Calculating total_rule_count before filtering is correct.

Initializing total_rule_count with the length of configs accurately captures the number of rules processed before any filtering is applied.

---

164-170: Properly computing and returning RuleStats enhances functionality.

The calculation of effective_rule_count and skipped_rule_count is correctly implemented, ensuring accurate tracking of rule processing statistics.

Files that changed from the base of the PR and between b247a7b and c00b54f.

• crates/cli/src/config.rs (4 hunks)
• crates/cli/src/lsp.rs (1 hunks)
• crates/cli/src/run.rs (8 hunks)
• crates/cli/src/scan.rs (5 hunks)
• crates/cli/src/utils/mod.rs (1 hunks)
• crates/cli/src/utils/tracing.rs (1 hunks)
• crates/cli/src/utils/worker.rs (3 hunks)
• crates/cli/src/verify.rs (1 hunks)
• crates/config/src/rule_collection.rs (1 hunks)

• crates/cli/src/lsp.rs
• crates/cli/src/scan.rs
• crates/cli/src/utils/mod.rs
• crates/cli/src/utils/worker.rs
• crates/cli/src/verify.rs
• crates/config/src/rule_collection.rs

crates/cli/src/utils/tracing.rs (4)

90-91: LGTM! Clear and useful type aliases.

The type aliases for RunStats and ScanStats are well-defined and provide clear, concise names for specific instantiations of SummaryStats. This improves code readability and maintainability.

---

93-100: LGTM! Well-implemented Default trait for SummaryStats.

The Default implementation for SummaryStats<T> is correct and flexible. It properly initializes both file_stats and inner fields, using the Default trait for the generic type T. This allows for easy instantiation of SummaryStats with various inner types that implement Default.

---

1-100: Overall, excellent implementation of the tracing functionality.

This new file introduces a well-structured and flexible tracing system for the ast-grep CLI tool. Key strengths include:

1. Comprehensive documentation outlining the tracing levels and their statistics.
2. Appropriate use of atomic operations for thread-safety in FileStats.
3. Flexible generic structures allowing for different types of statistics.
4. Clear type aliases improving code readability.

Some minor suggestions for improvement have been made, including:

• Adding a brief usage example to the documentation.
• Creating GitHub issues for TODO items.
• Adding a total files method to FileStats.
• Considering a generic print method for SummaryStats.
• Verifying thread-safety requirements for RuleStats.

These changes lay a solid foundation for the tracing feature and align well with the objectives outlined in issue #905.

---

76-88: RuleStats implementation looks good. Consider thread-safety for concurrent use.

The RuleStats struct and its implementation are straightforward and serve their purpose well.

If RuleStats is intended to be used in concurrent contexts, consider using AtomicUsize for the counters, similar to FileStats. This would ensure thread-safety. Here's a suggested modification:

use std::sync::atomic::{AtomicUsize, Ordering};

#[derive(Default, Debug, Serialize, Deserialize)]
pub struct RuleStats {
    pub effective_rule_count: AtomicUsize,
    pub skipped_rule_count: AtomicUsize,
}

impl RuleStats {
    pub fn print(&self) -> String {
        format!(
            "Effective rules: {}, Skipped rules: {}",
            self.effective_rule_count.load(Ordering::Acquire),
            self.skipped_rule_count.load(Ordering::Acquire)
        )
    }
}

To verify if RuleStats is used in concurrent contexts, we can search for its usage:

crates/cli/src/config.rs (4)

2-2: LGTM: Import of RuleStats is appropriate.

The addition of the RuleStats import from the utils module is consistent with the changes described in the summary and is necessary for the subsequent modifications in the file.

---

60-60: LGTM: Updated return type for find_rules function.

The modification of the return type to include RuleStats is consistent with the summary and enhances the function's capability to provide detailed information about rule processing.

---

135-135: LGTM: Updated return type for read_directory_yaml function.

The modification of the return type to include RuleStats is consistent with the changes made to find_rules and allows for the propagation of rule statistics.

---

Line range hint 1-248: Overall assessment: Changes look good and align with PR objectives.

The modifications to introduce rule statistics tracking are well-implemented and consistent throughout the file. The changes enhance the rule processing capabilities as intended.

Key points:

1. Appropriate imports have been added.
2. Function signatures for find_rules and read_directory_yaml have been updated correctly.
3. The implementation of rule statistics tracking in read_directory_yaml is accurate.

Please address the minor optimization suggestion and verify the impact on function callers as mentioned in the previous comments.

crates/cli/src/run.rs (9)

15-15: Imported DebugFormat, FileStats, and RunStats Successfully

The import statement correctly adds DebugFormat, FileStats, and RunStats from crate::utils.

---

201-206: Initialized RunWithInferredLang with New stats Field

The RunWithInferredLang struct is now initialized with the new stats field using RunStats::default(), ensuring statistics are tracked during execution.

---

213-213: Common Fields and Methods Could Be Abstracted

Both RunWithInferredLang and RunWithSpecificLang structs have a stats field. Consider abstracting common fields into a shared trait or base struct to enhance code reusability.

---

236-240: Implemented Statistics Printing

The consume_items method now prints run statistics using eprintln!("{}", self.stats.print());, addressing the previous TODO and providing valuable execution metrics.

---

249-251: Added get_stats Method to RunWithInferredLang

The get_stats method returns a reference to FileStats, allowing access to file statistics.

---

274-274: Added stats Field to RunWithSpecificLang<Printer>

The stats field has been added to RunWithSpecificLang<Printer> to enable statistics tracking.

---

294-294: Initialized stats Field in RunWithSpecificLang::new Method

The stats field is properly initialized with RunStats::default(), ensuring statistics are tracked from the default state.

---

311-311: Printed Run Statistics in consume_items

The consume_items method now outputs run statistics with eprintln!("{}", self.stats.print());, providing execution metrics after processing.

---

325-327: Implemented get_stats Method in RunWithSpecificLang

The get_stats method correctly returns a reference to FileStats, consistent with its implementation in RunWithInferredLang.