Skip to content

chore: workspace lint table and no panics on external data in pgls_lsp - #943

Open
psteinroe wants to merge 1 commit into
fix/lsp-arguments-uris-positionsfrom
chore/workspace-lints
Open

psteinroe wants to merge 1 commit into
fix/lsp-arguments-uris-positionsfrom
chore/workspace-lints

Conversation

@psteinroe

@psteinroe psteinroe commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Stacked on #928.

Adds a [workspace.lints] table and opts every member crate into it ([lints] workspace = true, also in the just new-crate template), and turns on the panic lints for pgls_lsp, the crate that handles client input first.

Workspace lints

clippy::todo, clippy::unimplemented and clippy::dbg_macro are now denied everywhere (dbg! stays allowed in tests, as before). The existing occurrences are fixed, not allowed:

  • MemoryFileSystem::resolve_configuration was a todo!(), so a configuration that extends a package name (not a path) panicked on the memory file system, which the WASM build uses. It returns ResolveError::NotFound now, which is reported as "can't resolve" like an unresolvable package on disk.
  • The TypeScript bindings generator had unimplemented!() for array and object literals and for boolean schemas. It now prints [a, b], { "k": v }, unknown and never. The generated bindings don't change.
  • generate_tool_configuration returns an error for assists and the transformation category instead of panicking. unreachable!() got a message.

pgls_lsp: no unwrap, expect or indexing

pgls_lsp/src/lib.rs warns on clippy::unwrap_used, clippy::expect_used and clippy::indexing_slicing, and CI's -D warnings turns them into errors. Tests are exempt (allow-*-in-tests in clippy.toml). On top of #928, which already removed the params.arguments[0] indexing, there were 21 findings (19 unwrap, 2 expect):

  • 12 were std::sync lock unwraps. pgls_lsp uses parking_lot locks now, which can't be poisoned. Before, a request handler that panicked while holding the documents lock (panic_to_lsp_error catches the panic) poisoned the lock, and every later request on that session panicked too.
  • get_cursor_position and completions expected/unwrapped the client capabilities. They use Session::position_encoding(), which falls back to UTF-16 like the other handlers.
  • A completion item whose range can't be converted is left out with a warning, instead of panicking the whole completion request.
  • diagnostic.advices(..) errors are returned through ?. Serialising the executeStatement arguments drops the action on failure. The unused print_markup (two unwraps) is removed.
  • Two can't fail and are #[expect(..., reason = "...")]: the u32 offset in LineIndex::new (documents over 4 GiB aren't supported anywhere) and the ConfigurationStatus atomic, which only ever stores a ConfigurationStatus as u8.

Counts for the next crate

pgls_workspace (non-test code) has 76 findings: 48 unwrap, 5 expect, 16 indexing and 7 slicing. About 34 of the unwraps are lock unwraps, which parking_lot would remove the same way. matcher/pattern.rs (20, the glob matcher's index arithmetic) and server.rs (16) have most of the rest. For reference, the same measurement over the other crates found 178 in pgls_configuration (mostly generated code), 89 in pgls_pretty_print, 70 in pgls_catalog and 39 in pgls_treesitter.

Risks

Open PRs that add std::sync locks with .unwrap() to pgls_lsp (#895, #908) will fail clippy once this is merged. The fix is to use parking_lot::{Mutex, RwLock} and drop the .unwrap(). client_capabilities() stays because #908, #911 and #919 use it.

Tests: cargo clippy --workspace --all-targets --all-features is clean, pgls_lsp and pgls_fs tests pass, and just gen-lint leaves the generated files unchanged.

Follow-ups: #895, #908 and #936 now use parking_lot locks. Only #881's own projects and workspace_folders locks remain to be switched when the two meet.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant