diff --git a/.buckconfig b/.buckconfig index de91633a32013..f4531232a5f2e 100644 --- a/.buckconfig +++ b/.buckconfig @@ -15,7 +15,8 @@ ignore = \ app_dep_graph_rules, \ examples, \ integrations/rust-project/tests, \ - tests + tests, \ + target [rust] default_edition = 2024 diff --git a/app/buck2_error/src/any.rs b/app/buck2_error/src/any.rs index 1e16ec03fe471..1005a9e115dcb 100644 --- a/app/buck2_error/src/any.rs +++ b/app/buck2_error/src/any.rs @@ -36,6 +36,7 @@ fn maybe_add_context_from_metadata(mut e: crate::Error, context: &dyn StdError) } } +/// Walk a StdError (e.g. anyhow::Error) down to its source, and rebuild it as a [struct@crate::Error]. pub fn recover_crate_error( value: &'_ (dyn StdError + 'static), source_location: SourceLocation, @@ -81,6 +82,7 @@ pub fn recover_crate_error( // context that is not included in the `base` error yet. let mut e = base; for context_value in context_stack.into_iter().rev() { + // XXX: this seems sus. If the downcast succeeds, we just write N copies of {starlark_err}. if let Some(starlark_err) = cur.downcast_ref::() { e = e.context(format!("{starlark_err}")); } else { diff --git a/app/buck2_error/src/context_value.rs b/app/buck2_error/src/context_value.rs index 06279d1c5e8cc..33c10cbbf51dd 100644 --- a/app/buck2_error/src/context_value.rs +++ b/app/buck2_error/src/context_value.rs @@ -12,6 +12,7 @@ use std::fmt; use std::sync::Arc; use smallvec::SmallVec; +use starlark_syntax::call_stack::CallStack; use starlark_syntax::codemap::FileSpan; use starlark_syntax::span_display::span_display; @@ -76,6 +77,12 @@ impl std::fmt::Display for ContextValue { } } +impl std::fmt::Debug for ContextValue { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + std::fmt::Display::fmt(&self, f) + } +} + impl From for ContextValue { fn from(value: String) -> Self { ContextValue::Dyn(value.into()) @@ -106,29 +113,67 @@ impl From for ContextValue { #[derive(Clone, allocative::Allocative, Debug, PartialEq, Eq, Hash)] pub struct StarlarkContext { - // TODO(minglunli): We could take in the CallStack type and do some magic to make it look nicer - // but I think just concatenating the string form and it's accurate and look good enough - pub call_stack: String, + pub call_stack: CallStack, + /// May be empty when we try to inject some StarlarkContext and there is already a StarlarkContext + /// near the top of the error. pub error_msg: String, pub span: Option, + /// If true, we render the span / call stack in buck output. + /// Otherwise, data is only for LSP to fish out. + pub show_span_in_buck_output: bool, + /// + /// We don't render the root error if `replaces_root_error` is set. + /// We render this in preference because it has a starlark + /// span and call stack. + /// + /// This is only to be set if error_msg contains a rendering of the root error. + /// + /// This field enables us to keep an actual StarlarkContext tha can be rendered + /// as an LSP span instead of literally replacing the root context with a + /// stringified span. + /// + pub replaces_root_error: bool, } impl StarlarkContext { - pub fn concat(&self, other: Option) -> Self { - if let Some(ctx) = other { - let trimmed = ctx + /// Concatenate call stacks. Keeps self.error_msg, drops inner.error_msg. + /// + /// Pass the inner context as the argument, i.e. the one closer to the root cause. + pub fn concat(&self, inner: Option) -> Self { + if let Some(inner) = inner { + let frames = self .call_stack - .trim_start_matches(starlark_syntax::call_stack::CALL_STACK_TRACEBACK_PREFIX); - let call_stack = format!("{}{}", self.call_stack, trimmed); + .frames + .iter() + .chain(inner.call_stack.frames.iter()) + .cloned() + .collect(); + Self { - call_stack, - error_msg: ctx.error_msg.clone(), - span: ctx.span.clone(), + call_stack: CallStack { frames }, + error_msg: self.error_msg.clone(), + span: inner.span.clone(), + replaces_root_error: inner.replaces_root_error, + show_span_in_buck_output: true, } } else { self.clone() } } + + pub fn find_span_in_file(&self, filename: &str) -> Option { + self.span + .as_ref() + .filter(|x| x.filename() == filename) + .or_else(|| { + self.call_stack + .frames + .iter() + .filter_map(|f| f.location.as_ref()) + .find(|span| span.filename() == filename) + }) + .cloned() + } } impl std::fmt::Display for StarlarkContext { diff --git a/app/buck2_error/src/error.rs b/app/buck2_error/src/error.rs index 0c786f68f2801..56ff7b2a3e396 100644 --- a/app/buck2_error/src/error.rs +++ b/app/buck2_error/src/error.rs @@ -52,9 +52,14 @@ pub struct Error(pub(crate) Arc); /// Right now, this type can represent an error root, together with a stack of context information. #[derive(allocative::Allocative)] pub(crate) enum ErrorKind { + /// The originating error, before context is added Root(Box), - /// For now we use untyped context to maximize compatibility with anyhow. + /// The output of [`e.context(...)`][Error::context]; recursively contains Error. + /// There is eventually a [Self::Root] at the bottom. WithContext(ContextValue, Error), + /// The output of [`e.mark_emitted()`][Error::mark_emitted]; recursively contains Error. + /// There is eventually a [Self::Root] at the bottom. + /// /// Indicates that the error has been emitted, ie shown to the user. // This `Arc` should ideally be a `Box`. However, that doesn't work right now because of the // implementation of `into_anyhow_for_format`. @@ -262,6 +267,16 @@ impl Error { }) } + /// Useful to check if StarlarkContext got injected properly + #[cfg(test)] + #[allow(unused)] + pub(crate) fn find_starlark_context(&self) -> Option<&StarlarkContext> { + self.iter_context().find_map(|kind| match kind { + ContextValue::StarlarkError(sc) => Some(sc), + _ => None, + }) + } + pub fn string_tags(&self) -> Vec { let mut tags: Vec = self .iter_context() diff --git a/app/buck2_error/src/format.rs b/app/buck2_error/src/format.rs index 984cdff2ad45f..bf336dd6a2dc7 100644 --- a/app/buck2_error/src/format.rs +++ b/app/buck2_error/src/format.rs @@ -59,11 +59,34 @@ pub(crate) fn into_anyhow_for_format( let mut out: anyhow::Error = base.into(); for context in context_stack.into_iter().rev() { if let ContextValue::StarlarkError(ctx) = context { - // Because context_stack is reversed, the right ordering would be first error last to preserve stack ordering - starlark_error = Some(ctx.concat(starlark_error)); + if ctx.replaces_root_error { + // ignore the root error, treat this as the root + out = AnyhowWrapperForFormat::Root(ctx.to_string()).into(); + } else if !ctx.show_span_in_buck_output { + out = out.context(ctx.error_msg.clone()) + } else { + // Because context_stack is reversed, the right ordering is first error + // last to preserve stack ordering. + // Inner's error message are dropped in concat. But if there is one, we add it as + // context first. + let mut outer = ctx.clone(); + if let Some(inner) = &mut starlark_error { + // Inner is getting concatenated to outer. Concat will drop its message, so + // preserve it in one way or another. + // + // Error messages may be empty when inject_starlark_context finds another StarlarkContext. + let inner_message = std::mem::take(&mut inner.error_msg); + if outer.error_msg.is_empty() { + outer.error_msg = inner_message; + } else if !inner_message.is_empty() { + out = out.context(inner_message); + } + } + starlark_error = Some(outer.concat(starlark_error)); + } continue; } - if let Some(ctx) = starlark_error { + if let Some(ref ctx) = starlark_error { out = out.context(format!("{ctx}")); starlark_error = None; } diff --git a/app/buck2_error/src/lib.rs b/app/buck2_error/src/lib.rs index 441cddce8d8bc..f522ec04ec00e 100644 --- a/app/buck2_error/src/lib.rs +++ b/app/buck2_error/src/lib.rs @@ -21,6 +21,7 @@ mod derive_tests; mod error; mod exit_code; mod format; +pub mod lsp; pub mod macros; mod root; pub mod source_location; diff --git a/app/buck2_error/src/lsp.rs b/app/buck2_error/src/lsp.rs new file mode 100644 index 0000000000000..a0f10a120cc68 --- /dev/null +++ b/app/buck2_error/src/lsp.rs @@ -0,0 +1,325 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is dual-licensed under either the MIT license found in the + * LICENSE-MIT file in the root directory of this source tree or the Apache + * License, Version 2.0 found in the LICENSE-APACHE file in the root directory + * of this source tree. You may select, at your option, one of the + * above-listed licenses. + */ + +use std::fmt; + +use starlark_syntax::codemap::FileSpan; + +use crate::context_value::ContextValue; +use crate::context_value::StarlarkContext; +use crate::error::ErrorKind; + +/// A diagnostic rendered for LSP. This is specific to which +/// file was being evaluated at the time. For example, we don't +/// pollute the printout with a rendering of the span that the +/// diagnostic is attached to and placed next to in the editor. +pub struct LspDiagnostic { + /// Generally Some; however may be None if we could not find + /// any spans for this file in the entire error trace. + /// We look everywhere, through all the call stacks, etc. + pub span: Option, + /// Rendered from anyhow::Error, it's a stack of error context. + pub rendered: String, +} + +impl fmt::Debug for LspDiagnostic { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if let Some(span) = self.span.as_ref() { + write!(f, "{}: ", span)?; + } + write!(f, "{}", self.rendered.trim()) + } +} + +impl crate::Error { + /// Transform into a list of LSP diagnostics for the file being + /// evaluated-on-save. + /// + /// For the same error, we give a different rendering depending + /// on the file that is being evaluated. For example, if there is + /// a fail() in a .bzl file, you'll see the error attached to a + /// span of the call when you evaluate that .bzl file. + /// But evaluate a BUCK file that imports the same .bzl file, + /// and you see "eval error during load". Technically these would + /// be different errors but you'll also see the context frames + /// rendered with full source listings from the BUCK file, e.g. + /// + /// error: fail: message here + /// --> blah.bzl:10:2-15 + /// | + /// 10 | fail("message here") + /// | + /// + /// Whereas if you render this in blah.bzl, you do not need to see + /// a printout of the code you are literally already looking at. + /// So we skip rendering this. + /// + pub fn into_lsp_diagnostics(self, eval_file_project_relative_path: &str) -> Vec { + { + let mut buck2_error = self; + let mut full_context_stack = vec![]; + + loop { + match buck2_error.0.as_ref() { + ErrorKind::Root(_root) => { + full_context_stack + .push(ContextValue::Dyn(format!("{buck2_error:?}").into())); + break; + } + ErrorKind::WithContext(context_value, inner) => { + // When we see a context + match context_value { + ContextValue::Tags(..) | ContextValue::StringTag(..) => (), // ignore + _ => { + full_context_stack.push(context_value.clone()); + } + } + + buck2_error = inner.clone(); + } + _ => return vec![], + } + } + + let mut context_span = None; + let mut context_stack: Vec = Vec::new(); + let mut stacks = vec![]; + + // rev => first iteration is root cause + // + // If root cause is in this file, we want to gather C3 and + // C2 with it, giving [C3, C2, Root]. This should highlight + // the span for Root. + // + // Then, additionally, we want [Context1, Context0], highlighted on + // the span for Context1. + // + // C0 + // + // Caused by: + // C1 this.bzl:29:4-7 + // C2 + // C3 + // Root this.bzl:80:9-16 + // + // + // If the root is not in this file: + // + // C0 + // + // Caused by: + // C1 this.bzl:29:4-7 + // C2 + // C3 this.bzl:80:9-16 (e.g. attr label parsing) + // C4 (some native buck context) + // Root (some native buck error) + // + // + // Then we should gather [C2, C3, C4, Root] in one origin diagnostic. + // We call C3 the proximal cause for this file. + // + // + // For a BUCK file loading some other file with an eval error, and + // LSP wants diagnostics for the BUCK file: + // + // C0 From load at thing/BUCK:4 + // + // Caused by: + // + // C1 other.bzl:123:4-5 + // C2 + // C3 another.bzl:456:7-8 + // Root + // + // Then you apply the same algorithm as for gathering [C2, C3, C4, Root] + // above and you get [C0, C1, C2, C3, Root] highlighted at C0's span. + // We call C0 the proximal cause for thing/BUCK. + // + let mut seen_proximal = false; + for ctx in full_context_stack.into_iter().rev() { + if let ContextValue::StarlarkError(sc) = &ctx { + if sc.replaces_root_error { + context_stack.pop(); + } + if let Some(span) = sc.find_span_in_file(eval_file_project_relative_path) { + if seen_proximal && (context_span.is_some() || !context_stack.is_empty()) { + stacks.push(( + context_span.replace(span.clone()), + std::mem::take(&mut context_stack), + )); + } + context_span.replace(span.clone()); + seen_proximal = true; + } + } + context_stack.push(ctx); + } + if !context_stack.is_empty() { + stacks.push((context_span.clone(), context_stack)); + } + + let is_eval_file = |sc: &StarlarkContext| { + sc.find_span_in_file(eval_file_project_relative_path) + .is_some() + }; + + let format_sc = |sc: &StarlarkContext| -> String { + if is_eval_file(sc) { + sc.error_msg.clone() + } else { + format!("{sc}") + } + }; + + let format_ctx = |ctx: &ContextValue| -> String { + if let ContextValue::StarlarkError(sc) = ctx { + format_sc(sc) + } else { + format!("{ctx}") + } + }; + + let format_stack = |stack: &mut Vec| { + let mut iter = stack.drain(..); + let root = iter.next().unwrap(); + + let mut err = anyhow::format_err!("{}", format_ctx(&root)); + let mut file_relevant = None; + let mut write_fr = false; + if let ContextValue::StarlarkError(sc) = &root + && is_eval_file(sc) + { + file_relevant = Some(sc.error_msg.clone()); + } + for ctx in iter { + if let ContextValue::StarlarkError(sc) = &ctx + && is_eval_file(sc) + { + err = err.context(format_sc(sc)); + file_relevant = Some(sc.error_msg.clone()); + write_fr = false; + continue; + } + err = err.context(format!("{ctx}")); + write_fr = true; + } + + // Lift the file-relevant one from which we derived the span + // to the top so it becomes the error message in the editor + if let Some(fr) = file_relevant.filter(|_| write_fr) { + err = err.context(fr); + } + format!("{err:?}") + }; + + let mut out = vec![]; + let mut leftover_stack = Vec::new(); + + for (span, mut stack) in stacks { + leftover_stack.append(&mut stack); + let Some(span) = span else { + continue; + }; + let rendered = format_stack(&mut leftover_stack); + out.push(LspDiagnostic { + span: Some(span), + rendered, + }); + } + + if !leftover_stack.is_empty() { + let rendered = format_stack(&mut leftover_stack); + out.push(LspDiagnostic { + span: None, + rendered, + }); + } + + out + } + } +} + +#[cfg(test)] +mod tests { + use crate::starlark_error::create_starlark_context; + + #[test] + fn into_lsp_diagnostic() { + use starlark_syntax::codemap::CodeMap; + use starlark_syntax::codemap::Pos; + use starlark_syntax::codemap::Span; + // these errors are all a bit nonsense, they're made up. + let bzl_file = CodeMap::new( + "path/to/test.bzl".to_owned(), + "# invalid\ndef and(): pass\ncall()".to_owned(), + ); + let buck_file = CodeMap::new( + "somecell/BUCK".to_owned(), + r#"load("@path//to:test.bzl")"#.to_owned(), + ); + let def_span = Span::new(Pos::new(14), Pos::new(17)); + let call_span = Span::new(Pos::new(26), Pos::new(30)); + let origin = starlark_syntax::Error::new_spanned( + starlark_syntax::ErrorKind::Parser(anyhow::format_err!("root cause",)), + def_span, + &bzl_file, + ); + let buck = crate::Error::from(origin) + .context("inner context during call") + .context(create_starlark_context( + format!( + "called at {}", + bzl_file.file_span(call_span).resolve_span().begin, + ), + Some(bzl_file.file_span(call_span)), + true, + )) + .context("outer context") + .context(create_starlark_context( + format!("From load at {}:1", buck_file.filename(),), + Some(buck_file.file_span(buck_file.full_span())), + true, + )); + let diag_bzl = buck.clone().into_lsp_diagnostics(bzl_file.filename()); + eprintln!("diag from bzl: {:#?}", diag_bzl); + + eprintln!(); + eprintln!(); + eprintln!("------------------------------------------------"); + eprintln!("------------------------------------------------"); + eprintln!("------------------------------------------------"); + eprintln!(); + let diag_buck = buck.clone().into_lsp_diagnostics(buck_file.filename()); + eprintln!("diag from BUCK: {:#?}", diag_buck); + } + + #[test] + fn attach_context() { + use starlark_syntax::codemap::CodeMap; + // these errors are all a bit nonsense, they're made up. + let bzl_file = CodeMap::new("path/to/test.bzl".to_owned(), "load(\"abc\"".to_owned()); + let load_span = bzl_file.file_span(bzl_file.full_span()); + let root_msg = "root cause"; + let buck = crate::Error::from(root_msg.to_owned()) + // .. + .context(create_starlark_context( + "Error loading load at blah".to_owned(), + Some(load_span.clone()), + true, + )); + let diag_bzl = buck.clone().into_lsp_diagnostics(bzl_file.filename()); + eprintln!("diag from bzl: {:#?}", diag_bzl); + + assert_eq!(diag_bzl.len(), 1); + let diag = diag_bzl.into_iter().next().unwrap(); + assert_eq!(diag.span.as_ref(), Some(&load_span)); + } +} diff --git a/app/buck2_error/src/root.rs b/app/buck2_error/src/root.rs index 8dfc36a53fb37..c11f376ddbffc 100644 --- a/app/buck2_error/src/root.rs +++ b/app/buck2_error/src/root.rs @@ -74,6 +74,7 @@ impl ErrorRoot { self.id } + #[allow(unused)] pub(crate) fn error_tag(&self) -> ErrorTag { self.error_tag } diff --git a/app/buck2_error/src/starlark_error.rs b/app/buck2_error/src/starlark_error.rs index b3d8a58feffd3..559536ab64038 100644 --- a/app/buck2_error/src/starlark_error.rs +++ b/app/buck2_error/src/starlark_error.rs @@ -13,6 +13,7 @@ use std::fmt; use ref_cast::RefCast; +use starlark_syntax::codemap::FileSpan; use crate::__for_macro::ContextValue; use crate::any::recover_crate_error; @@ -28,6 +29,28 @@ impl From for starlark_syntax::Error { } } +/// If true, we render the span / call stack in buck output. +/// Otherwise, the span data is only for LSP to fish out. +/// +/// The purpose of passing "false" is to add a span for the LSP +/// diagnostic formatter. +/// +// (Span is optional because some callsites only have one 99% of the time, and +// it's very inconvenient to span.map(create_starlark_context).unwrap_or(...).) +pub fn create_starlark_context( + message: String, + span: Option, + show_span_in_buck_output: bool, +) -> ContextValue { + ContextValue::StarlarkError(StarlarkContext { + error_msg: message, + span, + call_stack: Default::default(), + show_span_in_buck_output, + replaces_root_error: false, + }) +} + /// Whether or not to mark a starlark error as an input/user error. /// TODO(minglunli): This is used as an intermediate step so Native errors are categorized. /// This is not 100% accurate and should be remove once Native errors are categorized properly @@ -56,27 +79,29 @@ pub fn from_starlark_with_options( // If the top context is a string context, then it should be popped off as otherwise // the error will be duplicated since the top level error is used to build the stacktrace. -fn error_with_starlark_context( +fn inject_starlark_context( e: &crate::Error, - starlark_context: StarlarkContext, + mut starlark_context: StarlarkContext, ) -> crate::Error { let mut context_stack: Vec = Vec::new(); let mut buck2_error = e.clone(); + if starlark_context.span.is_none() && starlark_context.call_stack.is_empty() { + return e.clone(); + } + + // Not unless we reach the root in this loop. + starlark_context.replaces_root_error = false; + loop { match buck2_error.0.as_ref() { - ErrorKind::Root(root) => { - let source_location = root.source_location().clone(); - let action_error = root.action_error().cloned(); - - // We want to keep the metadata but want to change the error message - let mut err = crate::Error::new( - format!("{starlark_context}"), - root.error_tag(), - source_location, - action_error, - ); + ErrorKind::Root(_root) => { + // We are right next to the root. + starlark_context.replaces_root_error = true; + starlark_context.error_msg = format!("{buck2_error}"); + let mut err = buck2_error.context_for_starlark_backtrace(starlark_context); + // None of these are Dyn/Typed. for context in context_stack.into_iter().rev() { err = err.context(context); } @@ -86,17 +111,42 @@ fn error_with_starlark_context( ErrorKind::WithContext(context_value, inner) => { match context_value { ContextValue::Dyn(_) | ContextValue::Typed(_) => { + // walk back up from where we got to let mut inner_err = inner.clone(); for context in context_stack.into_iter().rev() { inner_err = inner_err.context(context); } + // A string or other context_value is popped & replaced by a StarlarkContext + // that describes it. + // + // XXX: we are losing the Typed nature of the context. + // We could modify WithContext(ctx, err) to contain a StarlarkContext + // directly as WithContext(ctx, starlark, err), and replace + // ContextValue::StarlarkError(...) with a unit variant. + // Thus Typed could have StarlarkContext attached. + // + starlark_context.error_msg = format!("{context_value}"); + return inner_err .clone() .context_for_starlark_backtrace(starlark_context); } ContextValue::StarlarkError(_) => { - return buck2_error.context_for_starlark_backtrace(starlark_context); + // walk back up from where we got to + let mut err = inner.clone(); + + err = err.context(context_value.clone()); + + for context in context_stack.into_iter().rev() { + err = err.context(context); + } + + // This code is paired with format.rs -- we could concatenate StarlarkContext + // early here, but prefer during formatting because it retains more spans. + // We have to handle empty error_msg there. + starlark_context.error_msg = String::new(); + return err.context_for_starlark_backtrace(starlark_context); } ContextValue::Tags(_) | ContextValue::StringTag(_) => { context_stack.push(context_value.clone()) @@ -110,27 +160,34 @@ fn error_with_starlark_context( } } +#[track_caller] fn from_starlark_impl( e: starlark_syntax::Error, error_handling: NativeErrorHandling, skip_stacktrace: bool, ) -> crate::Error { - if let starlark_syntax::ErrorKind::Native(err) = e.kind() { - if let Some(wrapper) = err.downcast_ref::() { - if !e.has_diagnostic() { - return wrapper.0.clone(); - } - - let starlark_context = StarlarkContext { - call_stack: format!("{}", e.call_stack()), - error_msg: format!("{}", e.without_diagnostic()), - span: e.span().cloned(), - }; - - return error_with_starlark_context(&wrapper.0, starlark_context); - } + let starlark_context = if e.has_diagnostic() && !skip_stacktrace { + Some(StarlarkContext { + call_stack: e.call_stack().clone(), + span: e.span().cloned(), + show_span_in_buck_output: true, + // + // These two are overwritten by inject_starlark_context + error_msg: String::new(), + replaces_root_error: false, + }) + } else { + None }; + if let starlark_syntax::ErrorKind::Native(err) = e.kind() + && let Some(wrapper) = err.downcast_ref::() + { + return starlark_context + .map(|sc| inject_starlark_context(&wrapper.0, sc)) + .unwrap_or_else(|| wrapper.0.clone()); + } + let tag = match e.kind() { starlark_syntax::ErrorKind::Fail(_) => crate::ErrorTag::StarlarkFail, starlark_syntax::ErrorKind::StackOverflow(_) => crate::ErrorTag::StarlarkStackOverflow, @@ -158,14 +215,13 @@ fn from_starlark_impl( starlark_syntax::ErrorKind::Native(_) => "StarlarkError::Native", _ => "StarlarkError", }; - let source_location = SourceLocation::new(std::file!()).with_type_name(variant_name); - let description = if skip_stacktrace { - format!("{}", e.without_diagnostic()) - } else { - format!("{e}") - }; + let caller_location = std::panic::Location::caller(); + let source_location = SourceLocation::new(caller_location.file()) + .with_source_line(caller_location.line()) + .with_type_name(variant_name); + let description = format!("{}", e.without_diagnostic()); - match e.into_kind() { + let crate_error = match e.into_kind() { starlark_syntax::ErrorKind::Fail(e) | starlark_syntax::ErrorKind::StackOverflow(e) | starlark_syntax::ErrorKind::Internal(e) @@ -174,6 +230,7 @@ fn from_starlark_impl( | starlark_syntax::ErrorKind::Scope(e) | starlark_syntax::ErrorKind::Parser(e) | starlark_syntax::ErrorKind::Other(e) + | starlark_syntax::ErrorKind::Freeze(e) | starlark_syntax::ErrorKind::Native(e) => { let error: anyhow::Error = Into::into(BuckStarlarkError(e, description)); let std_err: &'_ (dyn std::error::Error + 'static) = error.as_ref(); @@ -181,7 +238,11 @@ fn from_starlark_impl( recover_crate_error(std_err, source_location, tag) } _ => crate::Error::new(description, tag, source_location, None), - } + }; + + starlark_context + .map(|sc| inject_starlark_context(&crate_error, sc)) + .unwrap_or(crate_error) } pub(crate) struct BuckStarlarkError(pub(crate) anyhow::Error, String); @@ -226,12 +287,15 @@ mod tests { use std::sync::Arc; use allocative::Allocative; + use starlark_syntax::call_stack::CallStack; + use starlark_syntax::frame::Frame; use crate::any::ProvidableMetadata; use crate::buck2_error; use crate::context_value::StarlarkContext; use crate::source_location::SourceLocation; - use crate::starlark_error::error_with_starlark_context; + use crate::starlark_error::create_starlark_context; + use crate::starlark_error::inject_starlark_context; #[derive(Debug, Allocative, derive_more::Display)] struct FullMetadataError; @@ -266,6 +330,21 @@ mod tests { } } + fn example_call_stack() -> CallStack { + CallStack { + frames: vec![ + Frame { + name: "frame".to_owned(), + location: None, + }, + Frame { + name: "function_name".to_owned(), + location: None, + }, + ], + } + } + #[test] fn test_roundtrip_starlark() { // Tests buck2_error->starlark->buck2_error @@ -302,13 +381,16 @@ mod tests { #[test] fn test_pop_dyn_error_from_context() { + let sc_message = "This should be ignored"; let context_error = "Some error message that's getting popped"; let context_key = "Some context key"; let error_tag = crate::ErrorTag::IoWindowsSharingViolation; let starlark_context = StarlarkContext { - call_stack: "Some call stack".to_owned(), - error_msg: "Some error message".to_owned(), + call_stack: example_call_stack(), + error_msg: sc_message.to_owned(), span: None, + replaces_root_error: false, + show_span_in_buck_output: true, }; let e = crate::Error::from(FullMetadataError); @@ -316,31 +398,42 @@ mod tests { let e = e.tag([error_tag]); let e = e.string_tag(context_key); - let context_popped = error_with_starlark_context(&e, starlark_context); + let context_popped = inject_starlark_context(&e, starlark_context); - assert!(!context_popped.to_string().contains(context_error)); + let debug_out = format!("{context_popped:?}"); + assert_eq!(debug_out.matches(context_error).count(), 1); + assert!(!debug_out.contains(sc_message)); assert!(context_popped.tags().contains(&error_tag)); assert!(context_popped.category_key().ends_with(context_key)); + assert!( + context_popped + .find_starlark_context() + .is_some_and(|x| x.error_msg == context_error) + ); } #[test] fn test_pop_base_error_from_context() { let base_error = "Some base error"; - let starlark_error = "Starlark error message"; - let starlark_call_stack = "Some call stack"; + let starlark_error = "This should be ignored"; + let starlark_call_stack = example_call_stack(); let starlark_context = StarlarkContext { - call_stack: starlark_call_stack.to_owned(), + call_stack: starlark_call_stack.clone(), error_msg: starlark_error.to_owned(), span: None, + replaces_root_error: false, + show_span_in_buck_output: true, }; let e = buck2_error!(crate::ErrorTag::StarlarkError, "{}", base_error); - let base_replaced = error_with_starlark_context(&e, starlark_context); + let injected = inject_starlark_context(&e, starlark_context); - assert!(!base_replaced.to_string().contains(base_error)); - assert!(base_replaced.to_string().contains(starlark_call_stack)); - assert!(base_replaced.to_string().contains(starlark_error)); + let debug_out = format!("{injected:?}"); + assert_eq!(debug_out.matches(base_error).count(), 1); + assert!(debug_out.contains(&starlark_call_stack.to_string())); + assert!(!injected.to_string().contains(starlark_error)); + assert!(injected.find_starlark_context().is_some()); } #[test] @@ -352,20 +445,157 @@ mod tests { || FullMetadataError, ); - let starlark_call_stack = "Some call stack"; - let starlark_error_msg = "Starlark error message"; + let starlark_call_stack = example_call_stack(); + let starlark_error_msg = "This should be ignored"; let starlark_context = StarlarkContext { - call_stack: starlark_call_stack.to_owned(), + call_stack: starlark_call_stack.clone(), error_msg: starlark_error_msg.to_owned(), span: None, + replaces_root_error: false, + show_span_in_buck_output: true, }; - let starlark_err = error_with_starlark_context(&e, starlark_context); - let starlark_err_string = format!("{starlark_err:#}"); + let injected = inject_starlark_context(&e, starlark_context); + let debug_out = format!("{injected:?}"); - assert!(starlark_err_string.contains(starlark_call_stack)); - assert!(starlark_err_string.contains(starlark_error_msg)); + assert!(debug_out.contains(&starlark_call_stack.to_string())); + assert!(!debug_out.contains(starlark_error_msg)); + assert!(injected.find_starlark_context().is_some()); // Root error shouldn't be lost - assert!(starlark_err_string.contains(base_error)); + assert_eq!(debug_out.matches(base_error).count(), 1); + } + + #[test] + fn test_multiple_starlark_context() { + // We are concatenating stack frames. + // We have to be careful not to duplicate things when rendering the errors. + // + let base_error = "Some base error"; + let sc1_message = "sc1_message"; + let sc1_frame = "sc1_frame"; + let sc2_message = "sc2_message"; + let sc2_frame = "sc2_frame"; + + let e = buck2_error!(crate::ErrorTag::StarlarkError, "{}", base_error) + .context_for_starlark_backtrace(StarlarkContext { + call_stack: CallStack { + frames: vec![ + Frame { + name: sc1_frame.to_owned(), + location: None, + }, + Frame { + name: "function_name".to_owned(), + location: None, + }, + ], + }, + error_msg: sc1_message.to_owned(), + span: None, + replaces_root_error: false, + show_span_in_buck_output: true, + }) + .context_for_starlark_backtrace(StarlarkContext { + call_stack: CallStack { + frames: vec![ + Frame { + name: sc2_frame.to_owned(), + location: None, + }, + Frame { + name: "function_name".to_owned(), + location: None, + }, + ], + }, + error_msg: sc2_message.to_owned(), + span: None, + replaces_root_error: false, + show_span_in_buck_output: true, + }); + + let debug_out = format!("{e:?}"); + eprintln!("{debug_out}"); + + assert_eq!(debug_out.matches(sc1_message).count(), 1); + assert_eq!(debug_out.matches(sc1_frame).count(), 1); + assert_eq!(debug_out.matches(sc2_message).count(), 1); + assert_eq!(debug_out.matches(sc2_frame).count(), 1); + assert_eq!(debug_out.matches(base_error).count(), 1); + } + + #[test] + fn test_format_starlark_error_via_buck() { + use starlark_syntax::codemap::CodeMap; + use starlark_syntax::codemap::Pos; + use starlark_syntax::codemap::Span; + let code_map = CodeMap::new( + "test.bzl".to_owned(), + "# invalid\ndef and(): pass".to_owned(), + ); + let span = Span::new(Pos::new(14), Pos::new(17)); + let starlark = starlark_syntax::Error::new_spanned( + starlark_syntax::ErrorKind::Native(anyhow::format_err!("test_recover_starlark_span")), + span, + &code_map, + ); + eprintln!("starlark: {:#?}", starlark); + let expect_debug = starlark.to_string().trim().to_owned(); + let expect_display = starlark.to_string().trim().to_owned(); + let buck = crate::Error::from(starlark); + eprintln!("buck: {:?}", buck); + eprintln!("buck: {:#?}", buck); + + assert_eq!( + format!("{:?}", buck).trim(), + expect_debug, + "(Debug): Buck error should format the same as the original" + ); + + // Check we haven't duplicated the source printout + assert_eq!( + buck.to_string().trim(), + expect_display, + "(Display): Buck error should format the same as the original" + ); + } + + #[test] + fn test_recover_starlark_span_through_context() { + use starlark_syntax::codemap::CodeMap; + use starlark_syntax::codemap::Pos; + use starlark_syntax::codemap::Span; + let code_map = CodeMap::new( + "test.bzl".to_owned(), + "# invalid\ndef and(): pass".to_owned(), + ); + let span = Span::new(Pos::new(14), Pos::new(17)); + let starlark = starlark_syntax::Error::new_spanned( + starlark_syntax::ErrorKind::Native(anyhow::format_err!("test_recover_starlark_span")), + span, + &code_map, + ); + eprintln!("starlark: {:#?}", starlark); + let buck = crate::Error::from(starlark).context("wrapper"); + eprintln!("buck: {:?}", buck); + eprintln!("buck: {:#?}", buck); + let recovered = starlark_syntax::Error::from(buck.clone()); + eprintln!("recovered: {:#?}", recovered); + assert_eq!(recovered.span(), None); + + let recovered2 = starlark_syntax::Error::from(buck.clone()); + assert_eq!( + format!("{:?}", crate::Error::from(recovered2)), + "wrapper + +Caused by: +\x20\x20\x20\x20 + error: test_recover_starlark_span + --> test.bzl:2:5 + | + 2 | def and(): pass + | ^^^ + |" + ); } } diff --git a/app/buck2_error_tests/src/golden/test_starlark_callstack_backtrace.golden b/app/buck2_error_tests/src/golden/test_starlark_callstack_backtrace.golden index 035311fb9fd66..12c3982fbdc6f 100644 --- a/app/buck2_error_tests/src/golden/test_starlark_callstack_backtrace.golden +++ b/app/buck2_error_tests/src/golden/test_starlark_callstack_backtrace.golden @@ -6,8 +6,7 @@ Traceback (most recent call last): outer_fail() * outer_import.bzl:4, in outer_fail outer_rust_failure() - - * assert.bzl:3, in + * assert.bzl:3, in outer_rust_failure should_fail() * imported.bzl:9, in should_fail rust_failure() diff --git a/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs b/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs index eb787a78802da..bcd4ba005e7ae 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/dice_calculation_delegate.rs @@ -216,13 +216,19 @@ impl<'c, 'd: 'c> DiceCalculationDelegate<'c, 'd> { ctx.get_loaded_module(import.borrow()) .await .with_buck_error_context(|| { - format!( - "From load at {}", - span.as_ref() - .map_or("implicit location".to_owned(), |file_span| file_span - .resolve() - .begin_file_line() - .to_string()) + buck2_error::starlark_error::create_starlark_context( + format!( + "From load at {}", + span.as_ref().map_or( + "implicit location".to_owned(), + |file_span| file_span + .resolve() + .begin_file_line() + .to_string() + ) + ), + span.clone(), + false, ) }) } diff --git a/app/buck2_interpreter_for_build/src/interpreter/interpreter_for_dir.rs b/app/buck2_interpreter_for_build/src/interpreter/interpreter_for_dir.rs index 9b6ca18a21cb5..d2282759c1177 100644 --- a/app/buck2_interpreter_for_build/src/interpreter/interpreter_for_dir.rs +++ b/app/buck2_interpreter_for_build/src/interpreter/interpreter_for_dir.rs @@ -113,9 +113,11 @@ impl ParseData { let path = resolver .resolve_load(x.module_id, Some(&x.span)) .with_buck_error_context(|| { - format!( - "Error loading `load` of `{}` from `{}`", - x.module_id, x.span + // This is for e.g. imports not ending with .bzl + buck2_error::starlark_error::create_starlark_context( + format!("Error loading `load` of `{}`", x.module_id,), + Some(x.span.clone()), + true, ) })?; loads.push((Some(x.span), path)); diff --git a/app/buck2_server/src/lsp.rs b/app/buck2_server/src/lsp.rs index e8cc2f8bf3c08..94682740713ec 100644 --- a/app/buck2_server/src/lsp.rs +++ b/app/buck2_server/src/lsp.rs @@ -28,6 +28,7 @@ use buck2_core::fs::paths::abs_path::AbsPath; use buck2_core::fs::paths::forward_rel_path::ForwardRelativePath; use buck2_core::fs::project::ProjectRoot; use buck2_core::fs::project_rel_path::ProjectRelativePath; +use buck2_core::fs::project_rel_path::ProjectRelativePathBuf; use buck2_core::package::package_relative_path::PackageRelativePath; use buck2_core::package::source_path::SourcePath; use buck2_core::pattern::pattern::ParsedPattern; @@ -61,14 +62,16 @@ use futures::StreamExt; use futures::channel::mpsc::UnboundedSender; use lsp_server::Connection; use lsp_server::Message; +use lsp_types::Diagnostic; +use lsp_types::DiagnosticSeverity; +use lsp_types::NumberOrString; +use lsp_types::Range; use lsp_types::Url; use starlark::analysis::find_call_name::AstModuleFindCallName; use starlark::codemap::Span; use starlark::docs::DocModule; use starlark::docs::markdown::render_doc_item_no_link; -use starlark::errors::EvalMessage; use starlark::syntax::AstModule; -use starlark_lsp::error::eval_message_to_lsp_diagnostic; use starlark_lsp::server::LspContext; use starlark_lsp::server::LspEvalResult; use starlark_lsp::server::LspUrl; @@ -405,25 +408,28 @@ impl<'a> BuckLspContext<'a> { } } - async fn parse_file_with_contents(&self, uri: &LspUrl, content: String) -> LspEvalResult { + async fn parse_file_with_contents( + &self, + uri: &LspUrl, + project_relative: &ProjectRelativePath, + content: String, + ) -> LspEvalResult { match self - .parse_file_from_contents_and_handle_diagnostic(uri, content) + .parse_file_from_contents_and_handle_diagnostic(uri, project_relative, content) .await { Ok(res) => res, - Err(e) => { - let message = EvalMessage::from_any_error(uri.path(), &e); - LspEvalResult { - diagnostics: vec![eval_message_to_lsp_diagnostic(message)], - ast: None, - } - } + Err(e) => LspEvalResult { + diagnostics: self.to_diagnostics(project_relative, e), + ast: None, + }, } } async fn parse_file_from_contents_and_handle_diagnostic( &self, uri: &LspUrl, + project_relative: &ProjectRelativePath, content: String, ) -> buck2_error::Result { let import_path: OwnedStarlarkModulePath = match uri { @@ -450,9 +456,10 @@ impl<'a> BuckLspContext<'a> { ast: Some(ast), }), Err(e) => { - let message = EvalMessage::from_error(uri.path(), &e.into()); + // Do not use starlark_syntax::Error From / EvalMessage. + // It does not get a span. Ok(LspEvalResult { - diagnostics: vec![eval_message_to_lsp_diagnostic(message)], + diagnostics: self.to_diagnostics(project_relative, e), ast: None, }) } @@ -556,6 +563,57 @@ impl<'a> BuckLspContext<'a> { fn find_target(ast: &AstModule, target: TargetName) -> Option { ast.find_function_call_with_name(target.as_str()) } + + /// We have `AstModule::parse(project_relative_path.as_str(), ...)` + /// So all our codemaps, file spans, etc, are project relative paths. + /// + fn project_relative_path_from_url( + &self, + uri: &LspUrl, + ) -> buck2_error::Result { + match uri { + LspUrl::File(path) => { + let abs_path = AbsPath::new(path)?; + self.fs.relativize_any(abs_path) + } + LspUrl::Starlark(path) => { + let path_str = path.to_str().buck_error_context("Path is not UTF-8")?; + ProjectRelativePath::new(path_str.strip_prefix('/').unwrap_or(path_str)) + .map(|p| p.to_owned()) + } + LspUrl::Other(_) => Err(BuckLspContextError::WrongScheme( + "file:// or starlark:".to_owned(), + uri.clone(), + ) + .into()), + } + } + + fn to_diagnostics( + &self, + current_file_path: &ProjectRelativePath, + e: buck2_error::Error, + ) -> Vec { + e.into_lsp_diagnostics(current_file_path.as_str()) + .into_iter() + .map(|diag| { + let buck2_error::lsp::LspDiagnostic { span, rendered } = diag; + let origin_range = match span { + Some(s) => s.resolve_span().into(), + _ => Range::default(), + }; + lsp_types::Diagnostic::new( + origin_range, + Some(DiagnosticSeverity::ERROR), + Some(NumberOrString::String("error".to_owned())), + Some("eval".to_owned()), + rendered, + None, + None, + ) + }) + .collect() + } } impl LspContext for BuckLspContext<'_> { @@ -565,7 +623,14 @@ impl LspContext for BuckLspContext<'_> { .block_on(with_dispatcher_async(dispatcher, async { match uri { LspUrl::File(_) | LspUrl::Starlark(_) => { - self.parse_file_with_contents(uri, content).await + let Ok(project_relative) = self.project_relative_path_from_url(uri) else { + return LspEvalResult { + diagnostics: vec![], + ast: None, + }; + }; + self.parse_file_with_contents(uri, &project_relative, content) + .await } _ => LspEvalResult::default(), } diff --git a/starlark-rust/starlark/src/analysis/types.rs b/starlark-rust/starlark/src/analysis/types.rs index 6d7ce48338155..3b19ecf241e73 100644 --- a/starlark-rust/starlark/src/analysis/types.rs +++ b/starlark-rust/starlark/src/analysis/types.rs @@ -155,7 +155,8 @@ impl EvalMessage { /// Create an `EvalMessage` from any kind of error /// - /// Prefer to use `from_error` if at all possible. + /// Prefer to use `from_error` if at all possible. This method will return an `EvalMessage` + /// without a span. pub fn from_any_error(file: &Path, x: &impl std::fmt::Display) -> Self { Self { path: file.display().to_string(), diff --git a/starlark-rust/starlark/src/values/unpack.rs b/starlark-rust/starlark/src/values/unpack.rs index b955baf3f7a16..38cc22a2e68e4 100644 --- a/starlark-rust/starlark/src/values/unpack.rs +++ b/starlark-rust/starlark/src/values/unpack.rs @@ -240,7 +240,7 @@ pub trait UnpackValue<'v>: Sized + StarlarkTypeRepr { .into_anyhow_result() .with_context(|| { format!( - "Error unpacking value for parameter `{}` of type `{}", + "Error unpacking value for parameter `{}` of type `{}`", param_name, Self::starlark_type_repr() ) diff --git a/starlark-rust/starlark_syntax/src/call_stack.rs b/starlark-rust/starlark_syntax/src/call_stack.rs index 1dbca1874f43c..c6ea99a5b6ed5 100644 --- a/starlark-rust/starlark_syntax/src/call_stack.rs +++ b/starlark-rust/starlark_syntax/src/call_stack.rs @@ -28,12 +28,14 @@ use std::fmt; use std::fmt::Debug; use std::fmt::Display; +use allocative::Allocative; + use crate::frame::Frame; pub const CALL_STACK_TRACEBACK_PREFIX: &str = "Traceback (most recent call last):"; /// Owned call stack. -#[derive(Debug, Default, Clone, PartialEq, Eq, Hash)] +#[derive(Debug, Default, Clone, PartialEq, Eq, Hash, Allocative)] pub struct CallStack { /// The frames. pub frames: Vec, diff --git a/starlark-rust/starlark_syntax/src/frame.rs b/starlark-rust/starlark_syntax/src/frame.rs index c2e2d6f1a2eeb..503c2cfe44ead 100644 --- a/starlark-rust/starlark_syntax/src/frame.rs +++ b/starlark-rust/starlark_syntax/src/frame.rs @@ -19,12 +19,14 @@ use std::fmt; use std::fmt::Display; use std::fmt::Formatter; +use allocative::Allocative; + use crate::codemap::FileSpan; use crate::fast_string; use crate::fast_string::CharIndex; /// A frame of the call-stack. -#[derive(Debug, PartialEq, Eq, Hash, Clone)] +#[derive(Debug, PartialEq, Eq, Hash, Clone, Allocative)] pub struct Frame { /// The name of the entry on the call-stack. pub name: String,