From 426125f5c0fcd8db30fdf31eb997e6d01b09211b Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Mon, 8 Dec 2025 12:30:32 -0500 Subject: [PATCH] [ty] Stabilize auto-import While still under development, it's far enough along now that we think it's worth enabling it by default. This should also help give us feedback for how it behaves. This PR adds a "completion settings" grouping similar to inlay hints. We only have an auto-import setting there now, but I expect we'll add more options to configure completion behavior in the future. Closes astral-sh/ty#1765 --- crates/ty_ide/src/completion.rs | 22 ++++- .../src/server/api/requests/completion.rs | 8 +- crates/ty_server/src/session/options.rs | 43 +++++++--- crates/ty_server/src/session/settings.rs | 12 +-- crates/ty_server/tests/e2e/completions.rs | 82 +++++++++++++++++++ crates/ty_server/tests/e2e/main.rs | 33 +++++++- crates/ty_server/tests/e2e/notebook.rs | 33 ++------ .../e2e__commands__debug_command.snap | 4 +- 8 files changed, 183 insertions(+), 54 deletions(-) create mode 100644 crates/ty_server/tests/e2e/completions.rs diff --git a/crates/ty_ide/src/completion.rs b/crates/ty_ide/src/completion.rs index 3fc0f95235..a5e342b247 100644 --- a/crates/ty_ide/src/completion.rs +++ b/crates/ty_ide/src/completion.rs @@ -380,11 +380,22 @@ pub enum CompletionKind { TypeParameter, } -#[derive(Clone, Debug, Default)] +#[derive(Clone, Debug)] pub struct CompletionSettings { pub auto_import: bool, } +// N.B. It's important for the defaults here to match the defaults +// established by `CompletionOptions::into_settings`. This is +// because `WorkspaceSettings::default()` uses this definition. +// But `WorkspaceOptions::default().into_settings()` will use the +// `CompletionOptions::into_settings` definition. +impl Default for CompletionSettings { + fn default() -> CompletionSettings { + CompletionSettings { auto_import: true } + } +} + pub fn completion<'db>( db: &'db dyn Db, settings: &CompletionSettings, @@ -6610,7 +6621,14 @@ collabc fn completion_test_builder(&self) -> CompletionTestBuilder { CompletionTestBuilder { cursor_test: self.build(), - settings: CompletionSettings::default(), + settings: CompletionSettings { + // The tests were originally written with auto-import + // disabled, since it was disabled by default. But then + // we enabled it by default. However, we kept the tests + // as written with the assumption that auto-import was + // disabled unless opted into. ---AG + auto_import: false, + }, skip_builtins: false, skip_keywords: false, type_signatures: false, diff --git a/crates/ty_server/src/server/api/requests/completion.rs b/crates/ty_server/src/server/api/requests/completion.rs index 1055f85b56..540546cf49 100644 --- a/crates/ty_server/src/server/api/requests/completion.rs +++ b/crates/ty_server/src/server/api/requests/completion.rs @@ -8,7 +8,7 @@ use lsp_types::{ }; use ruff_source_file::OneIndexed; use ruff_text_size::Ranged; -use ty_ide::{CompletionKind, CompletionSettings, completion}; +use ty_ide::{CompletionKind, completion}; use ty_project::ProjectDatabase; use crate::document::{PositionExt, ToRangeExt}; @@ -56,10 +56,8 @@ impl BackgroundDocumentRequestHandler for CompletionRequestHandler { ) else { return Ok(None); }; - let settings = CompletionSettings { - auto_import: snapshot.global_settings().is_auto_import_enabled(), - }; - let completions = completion(db, &settings, file, offset); + let settings = snapshot.workspace_settings().completions(); + let completions = completion(db, settings, file, offset); if completions.is_empty() { return Ok(None); } diff --git a/crates/ty_server/src/session/options.rs b/crates/ty_server/src/session/options.rs index 982dccd484..b202270aae 100644 --- a/crates/ty_server/src/session/options.rs +++ b/crates/ty_server/src/session/options.rs @@ -8,7 +8,7 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use ty_combine::Combine; -use ty_ide::InlayHintSettings; +use ty_ide::{CompletionSettings, InlayHintSettings}; use ty_project::metadata::Options as TyOptions; use ty_project::metadata::options::ProjectOptionsOverrides; use ty_project::metadata::value::{RangedValue, RelativePathBuf}; @@ -123,8 +123,11 @@ impl ClientOptions { } #[must_use] - pub fn with_experimental_auto_import(mut self, enabled: bool) -> Self { - self.global.experimental.get_or_insert_default().auto_import = Some(enabled); + pub fn with_auto_import(mut self, enabled: bool) -> Self { + self.workspace + .completions + .get_or_insert_default() + .auto_import = Some(enabled); self } @@ -155,7 +158,6 @@ impl GlobalOptions { .experimental .map(|experimental| ExperimentalSettings { rename: experimental.rename.unwrap_or(false), - auto_import: experimental.auto_import.unwrap_or(false), }) .unwrap_or_default(); @@ -178,6 +180,9 @@ pub(crate) struct WorkspaceOptions { /// Options to configure inlay hints. inlay_hints: Option, + /// Options to configure completions. + completions: Option, + /// Information about the currently active Python environment in the VS Code Python extension. /// /// This is relevant only for VS Code and is populated by the ty VS Code extension. @@ -235,6 +240,10 @@ impl WorkspaceOptions { .inlay_hints .map(InlayHintOptions::into_settings) .unwrap_or_default(), + completions: self + .completions + .map(CompletionOptions::into_settings) + .unwrap_or_default(), overrides, } } @@ -256,6 +265,26 @@ impl InlayHintOptions { } } +#[derive(Clone, Combine, Debug, Serialize, Deserialize, Default)] +#[serde(rename_all = "camelCase")] +struct CompletionOptions { + auto_import: Option, +} + +impl CompletionOptions { + // N.B. It's important for the defaults here to + // match the defaults for `CompletionSettings`. + // This is because `WorkspaceSettings::default()` + // uses `CompletionSettings::default()`. But + // `WorkspaceOptions::default().into_settings()` will use this + // definition. + fn into_settings(self) -> CompletionSettings { + CompletionSettings { + auto_import: self.auto_import.unwrap_or(true), + } + } +} + /// Diagnostic mode for the language server. #[derive(Clone, Copy, Debug, Default, PartialEq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] @@ -300,12 +329,6 @@ impl Combine for DiagnosticMode { pub(crate) struct Experimental { /// Whether to enable the experimental symbol rename feature. pub(crate) rename: Option, - /// Whether to enable the experimental "auto-import" feature. - /// - /// At time of writing (2025-08-29), this feature is still - /// under active development. It may not work right or may be - /// incomplete. - pub(crate) auto_import: Option, } #[derive(Clone, Debug, Serialize, Deserialize, Default)] diff --git a/crates/ty_server/src/session/settings.rs b/crates/ty_server/src/session/settings.rs index 7b3e95f3ed..5e73df0102 100644 --- a/crates/ty_server/src/session/settings.rs +++ b/crates/ty_server/src/session/settings.rs @@ -1,6 +1,6 @@ use super::options::DiagnosticMode; -use ty_ide::InlayHintSettings; +use ty_ide::{CompletionSettings, InlayHintSettings}; use ty_project::metadata::options::ProjectOptionsOverrides; /// Resolved client settings that are shared across all workspaces. @@ -14,10 +14,6 @@ impl GlobalSettings { pub(crate) fn is_rename_enabled(&self) -> bool { self.experimental.rename } - - pub(crate) fn is_auto_import_enabled(&self) -> bool { - self.experimental.auto_import - } } impl GlobalSettings { @@ -29,7 +25,6 @@ impl GlobalSettings { #[derive(Clone, Default, Debug, PartialEq)] pub(crate) struct ExperimentalSettings { pub(super) rename: bool, - pub(super) auto_import: bool, } /// Resolved client settings for a specific workspace. @@ -40,6 +35,7 @@ pub(crate) struct ExperimentalSettings { pub(crate) struct WorkspaceSettings { pub(super) disable_language_services: bool, pub(super) inlay_hints: InlayHintSettings, + pub(super) completions: CompletionSettings, pub(super) overrides: Option, } @@ -55,4 +51,8 @@ impl WorkspaceSettings { pub(crate) fn inlay_hints(&self) -> &InlayHintSettings { &self.inlay_hints } + + pub(crate) fn completions(&self) -> &CompletionSettings { + &self.completions + } } diff --git a/crates/ty_server/tests/e2e/completions.rs b/crates/ty_server/tests/e2e/completions.rs new file mode 100644 index 0000000000..80576100b6 --- /dev/null +++ b/crates/ty_server/tests/e2e/completions.rs @@ -0,0 +1,82 @@ +use anyhow::Result; +use lsp_types::{Position, notification::PublishDiagnostics}; +use ruff_db::system::SystemPath; +use ty_server::ClientOptions; + +use crate::TestServerBuilder; + +/// Tests that auto-import is enabled by default. +#[test] +fn default_auto_import() -> Result<()> { + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); + let foo_content = "\ +walktr +"; + + let mut server = TestServerBuilder::new()? + .with_initialization_options(ClientOptions::default()) + .with_workspace(workspace_root, None)? + .with_file(foo, foo_content)? + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(foo, foo_content, 1); + let _ = server.await_notification::(); + + let hints = server.completion_request(&server.file_uri(foo), Position::new(0, 6)); + + insta::assert_json_snapshot!(hints, @r#" + [ + { + "label": "walktree (import inspect)", + "kind": 3, + "sortText": "0", + "insertText": "walktree", + "additionalTextEdits": [ + { + "range": { + "start": { + "line": 0, + "character": 0 + }, + "end": { + "line": 0, + "character": 0 + } + }, + "newText": "from inspect import walktree\n" + } + ] + } + ] + "#); + + Ok(()) +} + +/// Tests that disabling auto-import works. +#[test] +fn disable_auto_import() -> Result<()> { + let workspace_root = SystemPath::new("src"); + let foo = SystemPath::new("src/foo.py"); + let foo_content = "\ +walktr +"; + + let mut server = TestServerBuilder::new()? + .with_initialization_options(ClientOptions::default().with_auto_import(false)) + .with_workspace(workspace_root, None)? + .with_file(foo, foo_content)? + .build() + .wait_until_workspaces_are_initialized(); + + server.open_text_document(foo, foo_content, 1); + let _ = server.await_notification::(); + + let hints = server.completion_request(&server.file_uri(foo), Position::new(0, 6)); + + insta::assert_json_snapshot!(hints, @"[]"); + + Ok(()) +} diff --git a/crates/ty_server/tests/e2e/main.rs b/crates/ty_server/tests/e2e/main.rs index 94557172d0..5ef434cef6 100644 --- a/crates/ty_server/tests/e2e/main.rs +++ b/crates/ty_server/tests/e2e/main.rs @@ -29,6 +29,7 @@ mod code_actions; mod commands; +mod completions; mod initialize; mod inlay_hints; mod notebook; @@ -51,11 +52,12 @@ use lsp_types::notification::{ Initialized, Notification, }; use lsp_types::request::{ - DocumentDiagnosticRequest, HoverRequest, Initialize, InlayHintRequest, Request, Shutdown, - WorkspaceConfiguration, WorkspaceDiagnosticRequest, + Completion, DocumentDiagnosticRequest, HoverRequest, Initialize, InlayHintRequest, Request, + Shutdown, WorkspaceConfiguration, WorkspaceDiagnosticRequest, }; use lsp_types::{ - ClientCapabilities, ConfigurationParams, DiagnosticClientCapabilities, + ClientCapabilities, CompletionItem, CompletionParams, CompletionResponse, + CompletionTriggerKind, ConfigurationParams, DiagnosticClientCapabilities, DidChangeTextDocumentParams, DidChangeWatchedFilesClientCapabilities, DidChangeWatchedFilesParams, DidCloseTextDocumentParams, DidOpenTextDocumentParams, DocumentDiagnosticParams, DocumentDiagnosticReportResult, FileEvent, Hover, HoverParams, @@ -872,6 +874,31 @@ impl TestServer { let id = self.send_request::(params); self.await_response::(&id) } + + /// Sends a `textDocument/completion` request for the document at the given URL and position. + pub(crate) fn completion_request( + &mut self, + uri: &Url, + position: Position, + ) -> Vec { + let completions_id = self.send_request::(CompletionParams { + text_document_position: TextDocumentPositionParams { + text_document: TextDocumentIdentifier { uri: uri.clone() }, + position, + }, + work_done_progress_params: lsp_types::WorkDoneProgressParams::default(), + partial_result_params: lsp_types::PartialResultParams::default(), + context: Some(lsp_types::CompletionContext { + trigger_kind: CompletionTriggerKind::TRIGGER_FOR_INCOMPLETE_COMPLETIONS, + trigger_character: None, + }), + }); + match self.await_response::(&completions_id) { + Some(CompletionResponse::Array(array)) => array, + Some(CompletionResponse::List(lsp_types::CompletionList { items, .. })) => items, + None => vec![], + } + } } impl fmt::Debug for TestServer { diff --git a/crates/ty_server/tests/e2e/notebook.rs b/crates/ty_server/tests/e2e/notebook.rs index 7c7847dfcb..c8d5c738db 100644 --- a/crates/ty_server/tests/e2e/notebook.rs +++ b/crates/ty_server/tests/e2e/notebook.rs @@ -1,5 +1,5 @@ use insta::assert_json_snapshot; -use lsp_types::{CompletionResponse, CompletionTriggerKind, NotebookCellKind, Position, Range}; +use lsp_types::{NotebookCellKind, Position, Range}; use ruff_db::system::SystemPath; use ty_server::ClientOptions; @@ -285,7 +285,7 @@ fn auto_import() -> anyhow::Result<()> { let mut server = TestServerBuilder::new()? .with_workspace( SystemPath::new("src"), - Some(ClientOptions::default().with_experimental_auto_import(true)), + Some(ClientOptions::default().with_auto_import(true)), )? .build() .wait_until_workspaces_are_initialized(); @@ -325,7 +325,7 @@ fn auto_import_same_cell() -> anyhow::Result<()> { let mut server = TestServerBuilder::new()? .with_workspace( SystemPath::new("src"), - Some(ClientOptions::default().with_experimental_auto_import(true)), + Some(ClientOptions::default().with_auto_import(true)), )? .build() .wait_until_workspaces_are_initialized(); @@ -360,7 +360,7 @@ fn auto_import_from_future() -> anyhow::Result<()> { let mut server = TestServerBuilder::new()? .with_workspace( SystemPath::new("src"), - Some(ClientOptions::default().with_experimental_auto_import(true)), + Some(ClientOptions::default().with_auto_import(true)), )? .build() .wait_until_workspaces_are_initialized(); @@ -397,7 +397,7 @@ fn auto_import_docstring() -> anyhow::Result<()> { let mut server = TestServerBuilder::new()? .with_workspace( SystemPath::new("src"), - Some(ClientOptions::default().with_experimental_auto_import(true)), + Some(ClientOptions::default().with_auto_import(true)), )? .build() .wait_until_workspaces_are_initialized(); @@ -521,31 +521,10 @@ fn literal_completions( cell: &lsp_types::Url, position: Position, ) -> Vec { - let completions_id = - server.send_request::(lsp_types::CompletionParams { - text_document_position: lsp_types::TextDocumentPositionParams { - text_document: lsp_types::TextDocumentIdentifier { uri: cell.clone() }, - position, - }, - work_done_progress_params: lsp_types::WorkDoneProgressParams::default(), - partial_result_params: lsp_types::PartialResultParams::default(), - context: Some(lsp_types::CompletionContext { - trigger_kind: CompletionTriggerKind::TRIGGER_FOR_INCOMPLETE_COMPLETIONS, - trigger_character: None, - }), - }); - + let mut items = server.completion_request(cell, position); // There are a ton of imports we don't care about in here... // The import bit is that an edit is always restricted to the current cell. That means, // we can't add `Literal` to the `from typing import TYPE_CHECKING` import in cell 1 - let completions = server.await_response::(&completions_id); - let mut items = match completions { - Some(CompletionResponse::Array(array)) => array, - Some(CompletionResponse::List(lsp_types::CompletionList { items, .. })) => items, - None => return vec![], - }; - items.retain(|item| item.label.starts_with("Litera")); - items } diff --git a/crates/ty_server/tests/e2e/snapshots/e2e__commands__debug_command.snap b/crates/ty_server/tests/e2e/snapshots/e2e__commands__debug_command.snap index 0daa6c768a..762b91f71c 100644 --- a/crates/ty_server/tests/e2e/snapshots/e2e__commands__debug_command.snap +++ b/crates/ty_server/tests/e2e/snapshots/e2e__commands__debug_command.snap @@ -10,7 +10,6 @@ Global settings: GlobalSettings { diagnostic_mode: OpenFilesOnly, experimental: ExperimentalSettings { rename: false, - auto_import: false, }, } Open text documents: 0 @@ -22,6 +21,9 @@ Settings: WorkspaceSettings { variable_types: true, call_argument_names: true, }, + completions: CompletionSettings { + auto_import: true, + }, overrides: None, }