buck2_error -> lsp diagnostics #1152

cormacrelf · 2025-11-27T15:45:50Z

This fixes #1126 (no spans on parse errors in LSP) in a pretty comprehensive way. It is #1132, but cleaner and without the eval-on-save parts.

The simple way was to remove buck2_error wrapping of parse errors, leaving only starlark errors which preserved their spans. But this lost the error context for buck2 output, since buck2_error was providing that. And it did nothing for actual buck2_errors, like those from eval-on-save. So instead, I wrote an LSP formatter for arbitrary buck2_errors.

Features:

Does not render spans for the current file in LSP, since that's redundant. But spans from elsewhere are rendered.
If the error has multiple bits of starlark context in the current file, split into multiple errors
Spans in StarlarkContext can be LSP-only so we don't pollute buck2_error output.
CallStacks are preserved instead of stringified, which means we can find spans for LSP inside call stacks as well

This is good enough for eval-on-save, where errors come from all over the place and have starlark attached midway through, or twice, etc. I have another branch where this is adapted to provide workspace diagnostics to LSP, i.e. take you right to a parse error in another file.

Why was this so complicated?

buck2_error is very stringy. Firstly, we were losing spans almost as soon as they entered buck2_error, only preserving them in some cases, and other times immediately rendering them to string. We were then losing any spans we managed to save, due to pre-rendering StarlarkContext as strings basically whenever we convert the buck2_error to/from anything else. So this PR preserves StarlarkContext everywhere, which was a lot of work to get right.

Note that you do not get a span attached directly when you starlark::Error::from(buck2_error). It is hidden deep inside there in some buck2 context, which will resurface when you convert back to buck2_error.

There is a lot of change to buck2_error, but very few observable changes outside LSP. It changes only one golden test, with better merging of CallStacks.

There is very little point using the std::file!() macro outside of a macro.

Otherwise every buck2 startup involves scanning the target folder looking for build files. This gets old pretty quickly.

... when converting starlark errors to buck2_error::Error. Previously it only got added for starlark ErrorKind::Native, but we need it for every kind of evaluation error. We need to give the user a span for every kind of thing that goes wrong in starlark. This also changes the effect of skip_stacktrace to operate on the StarlarkContext, which is necessary to make it still make sense. I checked this works for the one place we use skip_stacktrace=True i.e. bxl's fail_no_stacktrace()

This improves the call stack rendering very slightly. It also enables us to search through call stacks to find more spans to attach diagnostics to in LSP.

There was a failing test here, may as well

LSP needs the span. We need to always attach StarlarkContext so the span is not lost. But we need to do more than that -- we need to delete any code that throws away StarlarkContext. In inject_starlark-context, we were previously just formatting StarlarkContext & overwriting the root's description with that. This is unacceptable, as it loses the span by rendering it into a string. So now, such a StarlarkContext sits right above the root and says "i have the same error message as root, but with a span, so render me instead." An alternative would be to make ErrorRoot span-aware and then handle both StarlarkContext and ErrorRoot when doing the callstack collapsing etc. We rejig the inject_starlark_context tests to just make assertions about non-repetition, instead of the pop/replace with StarlarkContext behaviour.

Useful for tests, when you have a Vec of them.

…ttached

This will be for .buck_error_context() calls that we don't currently pollute the buck output with spans for, and want to silently change for LSP only.

This is used with buck_error_context().

Just to check that I haven't broken anything or duplicated error messages.

I could not get the regular From<starlark_syntax::Error> + buck2_error formatter to do what I wanted for starlark LSP use. So I made an exporter. Sometimes error context makes LSP a bit verbose, like the 'Error parsing: cell//path/...' line of context for syntax errors. The error message in LSP is now Parse error: unexpected new indentation block here, expected one of "\n", "(", "+", "-", "...", ... Caused by: 0: Error parsing: `gh_facebook_buck2//:BUCK` 1: Parse error: unexpected new indentation block here, expected one of "\n", "(", "+", "-", "...", ... Whis is worse than if we just eliminated this context line and tried to keep everything as starlark_syntax::Error: Parse error: unexpected new indentation block here, expected one of "\n", "(", "+", "-", "...", ... But acceptable.

This grabs the project relative path to show/hide spans as necessary.

So we can see this as an LSP diagnostic. There is no change to the buck output, this only shows up in LSP.

Before: BUILD FAILED Error loading `load` of `abc` from `BUCK:5:6-11` Caused by: Import path must have suffix `.bzl`: `gh_facebook_buck2//abc` After: BUILD FAILED error: Error loading `load` of `abc` --> BUCK:5:6 | 5 | load("abc", "def") | ^^^^^ | Caused by: Import path must have suffix `.bzl`: `gh_facebook_buck2//abc`

meta-codesync · 2025-11-27T15:49:22Z

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D87982059. (Because this pull request was imported automatically, there will not be any future comments.)

ahornby

added suggestions to get it past the internal format lints. They currently show as red in the internal CI

ahornby · 2025-12-01T19:42:11Z

app/buck2_error/src/error.rs

    WithContext(ContextValue, Error),
+    /// The output of [`e.mark_emitted()`][Error::mark_emitted]; recursively contains Error.
+    /// There is eventually a [Self::Root] at the bottom.
+    ///


Suggested change

///

ahornby · 2025-12-01T19:42:45Z

app/buck2_error/src/context_value.rs

+    /// 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.
+    ///


Suggested change

///

ahornby · 2025-12-01T19:43:09Z

app/buck2_error/src/lsp.rs

+    /// 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.
+    ///


Suggested change

///

ahornby · 2025-12-01T19:43:41Z

app/buck2_server/src/lsp.rs

+
+    /// We have `AstModule::parse(project_relative_path.as_str(), ...)`
+    /// So all our codemaps, file spans, etc, are project relative paths.
+    ///


Suggested change

///

JakobDegen

Thanks, this is cool.

I reviewed this like I would internally, so that mostly means I stopped after the commit I left the big comment on with the expectation that the rest of the stack was going to have to change a bit following resolution of that.

Reviewing this I did find some decrepit code that should be gone, I might try and get that deleted which in turn may create some light conflicts, but only things that'll be trivial to resolve

JakobDegen · 2025-12-10T06:24:48Z

app/buck2_error/src/context_value.rs

+                .frames
+                .iter()
+                .chain(ctx.call_stack.frames.iter())
+                .cloned()


(nit)

.frames .iter() .cloned() .chain(ctx.call_stack.frames)

And maybe self by value and you can avoid the remaining clones?

JakobDegen · 2025-12-10T06:39:45Z

app/buck2_error/src/root.rs

        self.id
    }

+    #[allow(unused)]


FIXME(cormacrelf,JakobDegen): Can't be right?

That's a bit disconcerting

I was a bit alarmed when I saw this too. I deleted the only actual usage, which was to construct a new ErrorRoot with the same metadata as a previous one.

JakobDegen · 2025-12-10T07:05:06Z

app/buck2_error/src/starlark_error.rs

+                        // 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.


Wow, this is a bit insane lol.

So I apologize a bit for making you fix bugs you didn't write here, but this code is just super hard to reason about and that makes me a bit nervous.

Instead of doing this, can we just push all of this bullshit into the formatting? So, I'd propose:

Unimplement Display on ContextValue and StarlarkContext

Remove the error_msg field from StarlarkContext

Completely replace inject_starlark_context with just blindly pushing a StarlarkContext on top

When creating a starlark error in the non-native case, just inject_starlark_context(Error::new(description)), so create a chain like [starlark_msg, starlark_ctx]

When formatting, merge adjacent stuff together at that time so that you can render the starlark error however you like

I think you should be able to do this mostly without regressions, but I'd also be happy to take small rendering regressions in exchange for the simplicity

Basically yes, I would prefer it that way.

JakobDegen

Review automatically exported from Phabricator review in Meta.

cormacrelf added 25 commits November 28, 2025 02:02

Follow #{track_caller] to something that likely meant to use it

1d26b23

There is very little point using the std::file!() macro outside of a macro.

Ignore target/ folder for OSS buck2 developers

b7c3b48

Otherwise every buck2 startup involves scanning the target folder looking for build files. This gets old pretty quickly.

Document why not to use EvalMessage::from_any_error

d6b44d0

Add note about unlikely wanted behaviour

8e1e05d

Store a real CallStack in StarlarkContext

2e497be

This improves the call stack rendering very slightly. It also enables us to search through call stacks to find more spans to attach diagnostics to in LSP.

Add a missing enum value to match starlark_syntax::ErrorKind

ee8b40c

Document recover_crate_error

3676039

Rename error_with_starlark_context to inject_starlark_context

ccd8ebc

Fix a misplaced backtick

d4747e6

There was a failing test here, may as well

Add buck2_error::Error::find_starlark_context for tests

824ebbd

Update out-of-date doc strings on buck2_error::ErrorKind

1b0c4c7

impl Debug for ContextValue

5d7853c

Useful for tests, when you have a Vec of them.

Find relevant spans for lsp file in call stacks as well as directly a…

c11eca4

…ttached

Add show_span_in_buck_output flag on StarlarkContext

fb14575

This will be for .buck_error_context() calls that we don't currently pollute the buck output with spans for, and want to silently change for LSP only.

Add a function to create_starlark_context

77e2c47

This is used with buck_error_context().

Test for formatting starlark errors via buck

4d88c85

Just to check that I haven't broken anything or duplicated error messages.

Integrate buck2_error::Error::into_lsp_diagnostic with LSP

12bb357

This grabs the project relative path to show/hide spans as necessary.

Add test_recover_starlark_span_through_context

30db68b

Add starlark span to 'From load at BUCK:4'

149ad42

So we can see this as an LSP diagnostic. There is no change to the buck output, this only shows up in LSP.

Add a test for create_starlark_context

f44bb75

Preserve error messages when we concatenate starlark call stacks

16d5bb0

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 27, 2025

cormacrelf mentioned this pull request Nov 29, 2025

Tracking issue for LSP typechecking #1158

Open

ahornby reviewed Dec 1, 2025

View reviewed changes

JakobDegen reviewed Dec 10, 2025

View reviewed changes

JakobDegen requested changes Dec 10, 2025

View reviewed changes

buck2_error -> lsp diagnostics #1152

Are you sure you want to change the base?

buck2_error -> lsp diagnostics #1152

Uh oh!

Conversation

cormacrelf commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why was this so complicated?

Uh oh!

meta-codesync bot commented Nov 27, 2025

Uh oh!

ahornby left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobDegen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobDegen left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cormacrelf commented Nov 27, 2025 •

edited

Loading