Use anyhow more idiomatically (#31052)

https://github.com/zed-industries/zed/issues/30972 brought up another
case where our context is not enough to track the actual source of the
issue: we get a general top-level error without inner error.

The reason for this was `.ok_or_else(|| anyhow!("failed to read HEAD
SHA"))?; ` on the top level.

The PR finally reworks the way we use anyhow to reduce such issues (or
at least make it simpler to bubble them up later in a fix).
On top of that, uses a few more anyhow methods for better readability.

* `.ok_or_else(|| anyhow!("..."))`, `map_err` and other similar error
conversion/option reporting cases are replaced with `context` and
`with_context` calls
* in addition to that, various `anyhow!("failed to do ...")` are
stripped with `.context("Doing ...")` messages instead to remove the
parasitic `failed to` text
* `anyhow::ensure!` is used instead of `if ... { return Err(...); }`
calls
* `anyhow::bail!` is used instead of `return Err(anyhow!(...));`

Release Notes:

- N/A
This commit is contained in:
Kirill Bulatov 2025-05-21 02:06:07 +03:00 committed by GitHub
parent 1e51a7ac44
commit 16366cf9f2
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
294 changed files with 2037 additions and 2610 deletions

View file

@ -1,5 +1,5 @@
use crate::schema::json_schema_for;
use anyhow::{Result, anyhow};
use anyhow::{Context as _, Result, anyhow};
use assistant_tool::{ActionLog, Tool, ToolResult};
use gpui::AnyWindowHandle;
use gpui::{App, AppContext, Entity, Task};
@ -107,17 +107,13 @@ impl Tool for CopyPathTool {
});
cx.background_spawn(async move {
match copy_task.await {
Ok(_) => Ok(
format!("Copied {} to {}", input.source_path, input.destination_path).into(),
),
Err(err) => Err(anyhow!(
"Failed to copy {} to {}: {}",
input.source_path,
input.destination_path,
err
)),
}
let _ = copy_task.await.with_context(|| {
format!(
"Copying {} to {}",
input.source_path, input.destination_path
)
})?;
Ok(format!("Copied {} to {}", input.source_path, input.destination_path).into())
})
.into()
}

View file

@ -1,5 +1,5 @@
use crate::schema::json_schema_for;
use anyhow::{Result, anyhow};
use anyhow::{Context as _, Result, anyhow};
use assistant_tool::{ActionLog, Tool, ToolResult};
use gpui::AnyWindowHandle;
use gpui::{App, Entity, Task};
@ -86,7 +86,7 @@ impl Tool for CreateDirectoryTool {
project.create_entry(project_path.clone(), true, cx)
})?
.await
.map_err(|err| anyhow!("Unable to create directory {destination_path}: {err}"))?;
.with_context(|| format!("Creating directory {destination_path}"))?;
Ok(format!("Created directory {destination_path}").into())
})

View file

@ -1,5 +1,5 @@
use crate::schema::json_schema_for;
use anyhow::{Result, anyhow};
use anyhow::{Context as _, Result, anyhow};
use assistant_tool::{ActionLog, Tool, ToolResult};
use futures::{SinkExt, StreamExt, channel::mpsc};
use gpui::{AnyWindowHandle, App, AppContext, Entity, Task};
@ -122,19 +122,17 @@ impl Tool for DeletePathTool {
}
}
let delete = project.update(cx, |project, cx| {
project.delete_file(project_path, false, cx)
})?;
match delete {
Some(deletion_task) => match deletion_task.await {
Ok(()) => Ok(format!("Deleted {path_str}").into()),
Err(err) => Err(anyhow!("Failed to delete {path_str}: {err}")),
},
None => Err(anyhow!(
"Couldn't delete {path_str} because that path isn't in this project."
)),
}
let deletion_task = project
.update(cx, |project, cx| {
project.delete_file(project_path, false, cx)
})?
.with_context(|| {
format!("Couldn't delete {path_str} because that path isn't in this project.")
})?;
deletion_task
.await
.with_context(|| format!("Deleting {path_str}"))?;
Ok(format!("Deleted {path_str}").into())
})
.into()
}

View file

@ -6,7 +6,6 @@ use crate::{
list_directory_tool::ListDirectoryToolInput,
};
use Role::*;
use anyhow::anyhow;
use assistant_tool::ToolRegistry;
use client::{Client, UserStore};
use collections::HashMap;
@ -1207,10 +1206,7 @@ impl EvalAssertion {
}
}
Err(anyhow!(
"No score found in response. Raw output: {}",
output
))
anyhow::bail!("No score found in response. Raw output: {output}");
})
}

View file

@ -98,21 +98,21 @@ impl BlameEntry {
let sha = parts
.next()
.and_then(|line| line.parse::<Oid>().ok())
.ok_or_else(|| anyhow!("failed to parse sha"))?;
.with_context(|| format!("parsing sha from {line}"))?;
let original_line_number = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse original line number"))?;
.with_context(|| format!("parsing original line number from {line}"))?;
let final_line_number = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse final line number"))?;
.with_context(|| format!("parsing final line number from {line}"))?;
let line_count = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse final line number"))?;
.with_context(|| format!("parsing line count from {line}"))?;
let start_line = final_line_number.saturating_sub(1);
let end_line = start_line + line_count;

View file

@ -80,7 +80,7 @@ async fn run_git_blame(
.stdout(Stdio::piped())
.stderr(Stdio::piped())
.spawn()
.map_err(|e| anyhow!("Failed to start git blame process: {}", e))?;
.context("starting git blame process")?;
let stdin = child
.stdin
@ -92,10 +92,7 @@ async fn run_git_blame(
}
stdin.flush().await?;
let output = child
.output()
.await
.map_err(|e| anyhow!("Failed to read git blame output: {}", e))?;
let output = child.output().await.context("reading git blame output")?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
@ -103,7 +100,7 @@ async fn run_git_blame(
if trimmed == GIT_BLAME_NO_COMMIT_ERROR || trimmed.contains(GIT_BLAME_NO_PATH) {
return Ok(String::new());
}
return Err(anyhow!("git blame process failed: {}", stderr));
anyhow::bail!("git blame process failed: {stderr}");
}
Ok(String::from_utf8(output.stdout)?)
@ -144,21 +141,21 @@ impl BlameEntry {
let sha = parts
.next()
.and_then(|line| line.parse::<Oid>().ok())
.ok_or_else(|| anyhow!("failed to parse sha"))?;
.with_context(|| format!("parsing sha from {line}"))?;
let original_line_number = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse original line number"))?;
.with_context(|| format!("parsing original line number from {line}"))?;
let final_line_number = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse final line number"))?;
.with_context(|| format!("parsing final line number from {line}"))?;
let line_count = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse final line number"))?;
.with_context(|| format!("parsing line count from {line}"))?;
let start_line = final_line_number.saturating_sub(1);
let end_line = start_line + line_count;

View file

@ -5272,7 +5272,7 @@ impl Editor {
task.await?;
}
Ok::<_, anyhow::Error>(())
anyhow::Ok(())
})
.detach_and_log_err(cx);
}
@ -10369,8 +10369,8 @@ impl Editor {
.map(|line| {
line.strip_prefix(&line_prefix)
.or_else(|| line.trim_start().strip_prefix(&line_prefix.trim_start()))
.ok_or_else(|| {
anyhow!("line did not start with prefix {line_prefix:?}: {line:?}")
.with_context(|| {
format!("line did not start with prefix {line_prefix:?}: {line:?}")
})
})
.collect::<Result<Vec<_>, _>>()
@ -16944,7 +16944,7 @@ impl Editor {
Err(err) => {
let message = format!("Failed to copy permalink: {err}");
Err::<(), anyhow::Error>(err).log_err();
anyhow::Result::<()>::Err(err).log_err();
if let Some(workspace) = workspace {
workspace
@ -16999,7 +16999,7 @@ impl Editor {
Err(err) => {
let message = format!("Failed to open permalink: {err}");
Err::<(), anyhow::Error>(err).log_err();
anyhow::Result::<()>::Err(err).log_err();
if let Some(workspace) = workspace {
workspace

View file

@ -80,7 +80,7 @@ async fn run_git_blame(
.stdout(Stdio::piped())
.stderr(Stdio::piped())
.spawn()
.map_err(|e| anyhow!("Failed to start git blame process: {}", e))?;
.context("starting git blame process")?;
let stdin = child
.stdin
@ -92,10 +92,7 @@ async fn run_git_blame(
}
stdin.flush().await?;
let output = child
.output()
.await
.map_err(|e| anyhow!("Failed to read git blame output: {}", e))?;
let output = child.output().await.context("reading git blame output")?;
handle_command_output(output)
}
@ -107,7 +104,7 @@ fn handle_command_output(output: std::process::Output) -> Result<String> {
if trimmed == GIT_BLAME_NO_COMMIT_ERROR || trimmed.contains(GIT_BLAME_NO_PATH) {
return Ok(String::new());
}
return Err(anyhow!("git blame process failed: {}", stderr));
anyhow::bail!("git blame process failed: {stderr}");
}
Ok(String::from_utf8(output.stdout)?)
@ -148,21 +145,21 @@ impl BlameEntry {
let sha = parts
.next()
.and_then(|line| line.parse::<Oid>().ok())
.ok_or_else(|| anyhow!("failed to parse sha"))?;
.with_context(|| format!("parsing sha from {line}"))?;
let original_line_number = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse original line number"))?;
.with_context(|| format!("parsing original line number from {line}"))?;
let final_line_number = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse final line number"))?;
.with_context(|| format!("parsing final line number from {line}"))?;
let line_count = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse final line number"))?;
.with_context(|| format!("parsing line count from {line}"))?;
let start_line = final_line_number.saturating_sub(1);
let end_line = start_line + line_count;

View file

@ -80,7 +80,7 @@ async fn run_git_blame(
.stdout(Stdio::piped())
.stderr(Stdio::piped())
.spawn()
.map_err(|e| anyhow!("Failed to start git blame process: {}", e))?;
.context("starting git blame process")?;
let stdin = child
.stdin
@ -92,10 +92,7 @@ async fn run_git_blame(
}
stdin.flush().await?;
let output = child
.output()
.await
.map_err(|e| anyhow!("Failed to read git blame output: {}", e))?;
let output = child.output().await.context("reading git blame output")?;
if !output.status.success() {
let stderr = String::from_utf8_lossy(&output.stderr);
@ -103,7 +100,7 @@ async fn run_git_blame(
if trimmed == GIT_BLAME_NO_COMMIT_ERROR || trimmed.contains(GIT_BLAME_NO_PATH) {
return Ok(String::new());
}
return Err(anyhow!("git blame process failed: {}", stderr));
anyhow::bail!("git blame process failed: {stderr}");
}
Ok(String::from_utf8(output.stdout)?)
@ -144,21 +141,21 @@ impl BlameEntry {
let sha = parts
.next()
.and_then(|line| line.parse::<Oid>().ok())
.ok_or_else(|| anyhow!("failed to parse sha"))?;
.with_context(|| format!("parsing sha from {line}"))?;
let original_line_number = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse original line number"))?;
.with_context(|| format!("parsing original line number from {line}"))?;
let final_line_number = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse final line number"))?;
.with_context(|| format!("parsing final line number from {line}"))?;
let line_count = parts
.next()
.and_then(|line| line.parse::<u32>().ok())
.ok_or_else(|| anyhow!("Failed to parse final line number"))?;
.with_context(|| format!("parsing line count from {line}"))?;
let start_line = final_line_number.saturating_sub(1);
let end_line = start_line + line_count;

View file

@ -20,7 +20,7 @@ use std::{
#[cfg(any(feature = "tree-sitter-highlight", feature = "tree-sitter-tags"))]
use anyhow::Error;
use anyhow::{Context, Result, anyhow};
use anyhow::{Context as _, Result, anyhow};
use etcetera::BaseStrategy as _;
use fs4::fs_std::FileExt;
use indoc::indoc;
@ -875,16 +875,13 @@ impl Loader {
FileExt::unlock(lock_file)?;
fs::remove_file(lock_path)?;
if output.status.success() {
Ok(())
} else {
Err(anyhow!(
"Parser compilation failed.\nStdout: {}\nStderr: {}",
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr)
))
}
anyhow::ensure!(
output.status.success(),
"Parser compilation failed.\nStdout: {}\nStderr: {}",
String::from_utf8_lossy(&output.stdout),
String::from_utf8_lossy(&output.stderr)
);
Ok(())
}
#[cfg(unix)]
@ -941,17 +938,13 @@ impl Loader {
.map(|f| format!(" `{f}`"))
.collect::<Vec<_>>()
.join("\n");
anyhow::bail!(format!(indoc! {"
Missing required functions in the external scanner, parsing won't work without these!
return Err(anyhow!(format!(
indoc! {"
Missing required functions in the external scanner, parsing won't work without these!
{missing}
{}
You can read more about this at https://tree-sitter.github.io/tree-sitter/creating-parsers/4-external-scanners
"},
missing,
)));
You can read more about this at https://tree-sitter.github.io/tree-sitter/creating-parsers/4-external-scanners
"}));
}
}
}
@ -1008,9 +1001,9 @@ impl Loader {
{
EmccSource::Podman
} else {
return Err(anyhow!(
anyhow::bail!(
"You must have either emcc, docker, or podman on your PATH to run this command"
));
);
};
let mut command = match source {
@ -1103,12 +1096,11 @@ impl Loader {
.spawn()
.with_context(|| "Failed to run emcc command")?
.wait()?;
if !status.success() {
return Err(anyhow!("emcc command failed"));
}
fs::rename(src_path.join(output_name), output_path)
.context("failed to rename wasm output file")?;
anyhow::ensure!(status.success(), "emcc command failed");
let source_path = src_path.join(output_name);
fs::rename(&source_path, &output_path).with_context(|| {
format!("failed to rename wasm output file from {source_path:?} to {output_path:?}")
})?;
Ok(())
}
@ -1185,11 +1177,8 @@ impl Loader {
.map(|path| {
let path = parser_path.join(path);
// prevent p being above/outside of parser_path
if path.starts_with(parser_path) {
Ok(path)
} else {
Err(anyhow!("External file path {path:?} is outside of parser directory {parser_path:?}"))
}
anyhow::ensure!(path.starts_with(parser_path), "External file path {path:?} is outside of parser directory {parser_path:?}");
Ok(path)
})
.collect::<Result<Vec<_>>>()
}).transpose()?,
@ -1324,11 +1313,8 @@ impl Loader {
let name = GRAMMAR_NAME_REGEX
.captures(&first_three_lines)
.and_then(|c| c.get(1))
.ok_or_else(|| {
anyhow!(
"Failed to parse the language name from grammar.json at {}",
grammar_path.display()
)
.with_context(|| {
format!("Failed to parse the language name from grammar.json at {grammar_path:?}")
})?;
Ok(name.as_str().to_string())
@ -1347,7 +1333,7 @@ impl Loader {
{
Ok(config.0)
} else {
Err(anyhow!("Unknown scope '{scope}'"))
anyhow::bail!("Unknown scope '{scope}'")
}
} else if let Some((lang, _)) = self
.language_configuration_for_file_name(path)
@ -1371,7 +1357,7 @@ impl Loader {
} else if let Some(lang) = self.language_configuration_for_first_line_regex(path)? {
Ok(lang.0)
} else {
Err(anyhow!("No language found"))
anyhow::bail!("No language found");
}
}

View file

@ -3,7 +3,7 @@ use crate::{
edit_agent::{EditAgent, EditAgentOutput, EditAgentOutputEvent},
schema::json_schema_for,
};
use anyhow::{Result, anyhow};
use anyhow::{Context as _, Result, anyhow};
use assistant_tool::{
ActionLog, AnyToolCard, Tool, ToolCard, ToolResult, ToolResultContent, ToolResultOutput,
ToolUseStatus,
@ -279,15 +279,15 @@ impl Tool for EditFileTool {
let input_path = input.path.display();
if diff.is_empty() {
if hallucinated_old_text {
Err(anyhow!(formatdoc! {"
Some edits were produced but none of them could be applied.
Read the relevant sections of {input_path} again so that
I can perform the requested edits.
"}))
} else {
Ok("No edits were made.".to_string().into())
}
anyhow::ensure!(
!hallucinated_old_text,
formatdoc! {"
Some edits were produced but none of them could be applied.
Read the relevant sections of {input_path} again so that
I can perform the requested edits.
"}
);
Ok("No edits were made.".to_string().into())
} else {
Ok(ToolResultOutput {
content: ToolResultContent::Text(format!(
@ -347,53 +347,52 @@ fn resolve_path(
EditFileMode::Edit | EditFileMode::Overwrite => {
let path = project
.find_project_path(&input.path, cx)
.ok_or_else(|| anyhow!("Can't edit file: path not found"))?;
.context("Can't edit file: path not found")?;
let entry = project
.entry_for_path(&path, cx)
.ok_or_else(|| anyhow!("Can't edit file: path not found"))?;
if !entry.is_file() {
return Err(anyhow!("Can't edit file: path is a directory"));
}
.context("Can't edit file: path not found")?;
anyhow::ensure!(entry.is_file(), "Can't edit file: path is a directory");
Ok(path)
}
EditFileMode::Create => {
if let Some(path) = project.find_project_path(&input.path, cx) {
if project.entry_for_path(&path, cx).is_some() {
return Err(anyhow!("Can't create file: file already exists"));
}
anyhow::ensure!(
project.entry_for_path(&path, cx).is_none(),
"Can't create file: file already exists"
);
}
let parent_path = input
.path
.parent()
.ok_or_else(|| anyhow!("Can't create file: incorrect path"))?;
.context("Can't create file: incorrect path")?;
let parent_project_path = project.find_project_path(&parent_path, cx);
let parent_entry = parent_project_path
.as_ref()
.and_then(|path| project.entry_for_path(&path, cx))
.ok_or_else(|| anyhow!("Can't create file: parent directory doesn't exist"))?;
.context("Can't create file: parent directory doesn't exist")?;
if !parent_entry.is_dir() {
return Err(anyhow!("Can't create file: parent is not a directory"));
}
anyhow::ensure!(
parent_entry.is_dir(),
"Can't create file: parent is not a directory"
);
let file_name = input
.path
.file_name()
.ok_or_else(|| anyhow!("Can't create file: invalid filename"))?;
.context("Can't create file: invalid filename")?;
let new_file_path = parent_project_path.map(|parent| ProjectPath {
path: Arc::from(parent.path.join(file_name)),
..parent
});
new_file_path.ok_or_else(|| anyhow!("Can't create file"))
new_file_path.context("Can't create file")
}
}
}
@ -917,8 +916,6 @@ async fn build_buffer_diff(
#[cfg(test)]
mod tests {
use std::result::Result;
use super::*;
use client::TelemetrySettings;
use fs::FakeFs;
@ -1019,7 +1016,7 @@ mod tests {
mode: &EditFileMode,
path: &str,
cx: &mut TestAppContext,
) -> Result<ProjectPath, anyhow::Error> {
) -> anyhow::Result<ProjectPath> {
init_test(cx);
let fs = FakeFs::new(cx.executor());
@ -1046,7 +1043,7 @@ mod tests {
result
}
fn assert_resolved_path_eq(path: Result<ProjectPath, anyhow::Error>, expected: &str) {
fn assert_resolved_path_eq(path: anyhow::Result<ProjectPath>, expected: &str) {
let actual = path
.expect("Should return valid path")
.path

View file

@ -109,7 +109,7 @@ impl Tool for GrepTool {
let input = match serde_json::from_value::<GrepToolInput>(input) {
Ok(input) => input,
Err(error) => {
return Task::ready(Err(anyhow!("Failed to parse input: {}", error))).into();
return Task::ready(Err(anyhow!("Failed to parse input: {error}"))).into();
}
};
@ -122,7 +122,7 @@ impl Tool for GrepTool {
) {
Ok(matcher) => matcher,
Err(error) => {
return Task::ready(Err(anyhow!("invalid include glob pattern: {}", error))).into();
return Task::ready(Err(anyhow!("invalid include glob pattern: {error}"))).into();
}
};

View file

@ -1,5 +1,5 @@
use crate::schema::json_schema_for;
use anyhow::{Result, anyhow};
use anyhow::{Context as _, Result, anyhow};
use assistant_tool::{ActionLog, Tool, ToolResult};
use gpui::{AnyWindowHandle, App, AppContext, Entity, Task};
use language_model::{LanguageModel, LanguageModelRequest, LanguageModelToolSchemaFormat};
@ -117,17 +117,10 @@ impl Tool for MovePathTool {
});
cx.background_spawn(async move {
match rename_task.await {
Ok(_) => {
Ok(format!("Moved {} to {}", input.source_path, input.destination_path).into())
}
Err(err) => Err(anyhow!(
"Failed to move {} to {}: {}",
input.source_path,
input.destination_path,
err
)),
}
let _ = rename_task.await.with_context(|| {
format!("Moving {} to {}", input.source_path, input.destination_path)
})?;
Ok(format!("Moved {} to {}", input.source_path, input.destination_path).into())
})
.into()
}

View file

@ -1,5 +1,5 @@
use crate::schema::json_schema_for;
use anyhow::{Result, anyhow};
use anyhow::{Context as _, Result, anyhow};
use assistant_tool::{ActionLog, Tool, ToolResult};
use assistant_tool::{ToolResultContent, outline};
use gpui::{AnyWindowHandle, App, Entity, Task};
@ -129,7 +129,7 @@ impl Tool for ReadFileTool {
let language_model_image = cx
.update(|cx| LanguageModelImage::from_image(image, cx))?
.await
.ok_or_else(|| anyhow!("Failed to process image"))?;
.context("processing image")?;
Ok(ToolResultOutput {
content: ToolResultContent::Image(language_model_image),
@ -152,7 +152,7 @@ impl Tool for ReadFileTool {
.as_ref()
.map_or(true, |file| !file.disk_state().exists())
})? {
return Err(anyhow!("{} not found", file_path));
anyhow::bail!("{file_path} not found");
}
project.update(cx, |project, cx| {

View file

@ -382,13 +382,11 @@ fn working_dir(
match worktrees.next() {
Some(worktree) => {
if worktrees.next().is_none() {
Ok(Some(worktree.read(cx).abs_path().to_path_buf()))
} else {
Err(anyhow!(
"'.' is ambiguous in multi-root workspaces. Please specify a root directory explicitly.",
))
}
anyhow::ensure!(
worktrees.next().is_none(),
"'.' is ambiguous in multi-root workspaces. Please specify a root directory explicitly.",
);
Ok(Some(worktree.read(cx).abs_path().to_path_buf()))
}
None => Ok(None),
}
@ -409,9 +407,7 @@ fn working_dir(
}
}
Err(anyhow!(
"`cd` directory {cd:?} was not in any of the project's worktrees."
))
anyhow::bail!("`cd` directory {cd:?} was not in any of the project's worktrees.");
}
}