feat: Add error messaging for rule file parsing failures #968
Conversation
WalkthroughThe update introduces Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
Cargo.lockis excluded by:!**/*.lock
Files selected for processing (3)
- crates/cli/src/lsp.rs (1 hunks)
- crates/lsp/Cargo.toml (1 hunks)
- crates/lsp/src/lib.rs (5 hunks)
Additional comments: 4
crates/lsp/Cargo.toml (1)
- 16-16: The addition of
anyhowas a workspace dependency is appropriate and aligns with the PR's objectives to improve error handling within the LSP component. This change follows best practices for dependency management in Rust projects.crates/cli/src/lsp.rs (1)
- 11-11: Storing the result of
find_rulesinconfig_resultand using it when creating theBackendis a good practice. This change enhances error handling flexibility within theBackend, aligning with the PR's objectives to improve error messaging and handling.crates/lsp/src/lib.rs (2)
- 26-26: The modification of the
rulesfield in theBackendstruct to hold ananyhow::ResultwrappingRuleCollectionis a significant improvement. This change enables more graceful error handling related to rule parsing, aligning with the PR's objectives to enhance error messaging and handling.- 253-269: The error handling in the
publish_diagnosticsmethod, including the use ofshow_messageto display error messages to the user, is well-implemented. This approach effectively communicates parsing errors, aligning with the PR's objectives to enhance error messaging and handling.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #968 +/- ##
==========================================
- Coverage 71.88% 71.58% -0.31%
==========================================
Files 72 72
Lines 4706 4726 +20
==========================================
Hits 3383 3383
- Misses 1323 1343 +20 ☔ View full report in Codecov by Sentry. |
|
I like publishing a diagnostic to a user. If the user is trying to use ast-grep as a linter, they will already be looking for diagnostics so the error is right where they will see it. One challenge is that the diagnostic is visible on every file and we may want to restrict it to only the files that the user is targeting, but in the case of malformed rule files we can't have confidence that we know exactly which languages are targeted. |
crates/lsp/src/lib.rs
Outdated
| @@ -239,7 +239,7 @@ fn url_to_code_description(url: &Option<String>) -> Option<CodeDescription> { | |||
| } | |||
|
|
|||
| impl<L: LSPLang> Backend<L> { | |||
| pub fn new(client: Client, rules: RuleCollection<L>) -> Self { | |||
| pub fn new(client: Client, rules: anyhow::Result<RuleCollection<L>>) -> Self { | |||
There was a problem hiding this comment.
This change will assume lsp must be used with anyhow. However, IMHO LSP should be neutral to error handling.
What about Using std::Result<RuleCollection, String> here?
There was a problem hiding this comment.
I have modified the LSP to use std::result::Result and avoid anyhow::Result. The find_rules function still creates an anyhow::Result but before we pass it in to the Backend::new constructor, we convert it into a std::result::Result.
Can you help me understand the cost associated with using the different Results from different libraries? I chose anyhow because it has some convenience functions and it is being used in other parts of the ast-grep rust project (though not in the LSP).
|
Thanks for the update!
I think it will be too annoying for users to see diagnostics every file. Showing it once on start up will be a better choice. Also, I notice that currently one rule file error will abort the whole rule collection finding. It is better to have valid rules usable still. I will change the behavior later and keep this PR simple. |
I agree and I am looking at ways to achieve this. I was able to quickly change We can structure the code such that the pub struct RuleCollection<L: Language + Eq> {
tenured: Vec<RuleBucket<L>>,
contingent: Vec<ContingentRule<L>>,
}Does that sound right to you? |
|
Thanks for researching the code! But let's make the PR simple and small. So just reporting the error to client is good. Regarding to the error, I will like to have an additional struct to store RuleCollection + errors. So correct code path will not pay extra overhead for rare wrong cases. |
…errors upon initialization
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- crates/cli/src/lsp.rs (1 hunks)
- crates/lsp/src/lib.rs (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- crates/cli/src/lsp.rs
- crates/lsp/src/lib.rs
As stated in #722 the LSP doesn't report errors clearly if there is a problem parsing rule files.
Code structure
In this PR I needed to get the potential Error from
find_rulesaccessible to theLanguageClientin order to report it to the user. I chose to do this by taking theResult<RuleCollection<SgLang>>fromfind_rulesand passing into theBackendstruct without unwrapping it. Then any time we need to access the rules insideBackendwe just have to pattern match the Result. Handling the error insideBackendis trivial as we just publish no diagnostics and no code actions.There are other approaches to consider like just calling
find_rulesfrom insideBackendinstead of passing it into the constructor. Because the logging is async, it is preferable to await the log in one ofBackend's async handlers likepublish_diagnosticsrather than in theBackend::new()constructor. It might belong better inBackend::on_open, but this could depend on which method we choose to alert the user.How to alert the user
I went with option 1 below, but we can do any combination so let me know what you think.
show_messagelog_messageSummary by CodeRabbit
anyhowfor clearer, more manageable code.anyhowas a dependency to improve error handling capabilities.