Skip to content

[ty] Configure check mode for all projects #23121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
BurntSushi merged 1 commit into from
Feb 9, 2026
Merged

[ty] Configure check mode for all projects #23121

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/ty_server/src/server/api/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ use crate::system::{AnySystemPath, file_to_url};
use crate::{DIAGNOSTIC_NAME, Db, DiagnosticMode};
use crate::{PositionEncoding, Session};

#[derive(Debug)]
pub(super) struct Diagnostics {
items: Vec<ruff_db::diagnostic::Diagnostic>,
encoding: PositionEncoding,
Expand Down
9 changes: 6 additions & 3 deletions crates/ty_server/src/server/api/notifications/did_open.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use lsp_types::notification::DidOpenTextDocument;
use lsp_types::{DidOpenTextDocumentParams, TextDocumentItem};

use crate::TextDocument;
use crate::document::LanguageId;
use crate::server::Result;
use crate::server::api::diagnostics::publish_diagnostics_if_needed;
use crate::server::api::traits::{NotificationHandler, SyncNotificationHandler};
Expand Down Expand Up @@ -30,10 +31,12 @@ impl SyncNotificationHandler for DidOpenTextDocumentHandler {
},
} = params;

let document = session.open_text_document(
TextDocument::new(uri, text, version).with_language_id(&language_id),
);
let text_doc = TextDocument::new(uri, text, version).with_language_id(&language_id);
if matches!(text_doc.language_id(), Some(LanguageId::Other)) {
return Ok(());
}
Comment on lines +35 to +37
Copy link
Member

@dhruvmanila dhruvmanila Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check does seem to be causing some confusion because if this is true then the document won't be opened and the following notifications for this document would result in "Document ... is not open in the session" logs (astral-sh/ty#2824 (comment), astral-sh/ty#2937).

Copy link
Member

@MichaReiser MichaReiser Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should probably move it below line 39 (I'm also not entirely sure why it's needed)

Copy link
Member Author

@BurntSushi BurntSushi Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea was to mirror what we do in ty_project/src/walk.rs: #23121 (comment)

I think I was thinking about this in particular: https://github.com/astral-sh/ruff/blob/f8d1d29ec7f1ff0da7e0272c8388036f35e815b7/crates/ty_project/src/walk.rs#L240-L228

But it's possible I misinterpreted what that code is doing. @MichaReiser Can you say why this should be moved? Or would it make sense to emit a log or something in this case?

Specifically, this was my attempt at fixing the changing_language_of_file_without_extension test, which started failing after I unwound the check mode configuration stuff.

Copy link
Member

@MichaReiser MichaReiser Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with not opening the document in the session is that any LSP action on any document with LanguageId::Other will fail to look up the document (e.g., didChange, didClose, etc.), and they'll log a warning on every request.

That's why I think that we always need to add the document to Index (let handle = self.index_mut().open_text_document(document);), but we may want to skip calling project.open_file(db, file); if the document is a non Python file in open_document_in_db.

Copy link
Member Author

@BurntSushi BurntSushi Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I put up a PR for that change here: #23704


let document = session.open_text_document(text_doc);
Comment on lines -33 to +39
Copy link
Member

@dhruvmanila dhruvmanila Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this sounds correct but it would be useful to have a E2E test case for this scenario if it doesn't already exists. What do you think?

Copy link
Member Author

@BurntSushi BurntSushi Feb 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what the changing_language_of_file_without_extension test covers. That the test was previously passing was a mirage. :-) Once I untangled the AllFiles/OpenFilesOnly stuff, it started failing. Adding this was required to make it pass because otherwise we treat the open file as Python code.

publish_diagnostics_if_needed(&document, session, client);

Ok(())
Expand Down
12 changes: 6 additions & 6 deletions crates/ty_server/src/session.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use ruff_python_ast::PySourceType;
use ty_combine::Combine;
use ty_project::metadata::Options;
use ty_project::watch::{ChangeEvent, CreatedKind};
use ty_project::{ChangeResult, CheckMode, Db as _, ProjectDatabase, ProjectMetadata};
use ty_project::{ChangeResult, Db as _, ProjectDatabase, ProjectMetadata};

use index::DocumentError;
use ty_python_semantic::MisconfigurationMode;
Expand Down Expand Up @@ -530,13 +530,13 @@ impl Session {

if let Some(global_options) = global_options {
let global_settings = global_options.into_settings();
if global_settings.diagnostic_mode().is_workspace() {
for project in self.projects.values_mut() {
project.db.set_check_mode(CheckMode::AllFiles);
}
}
self.global_settings = Arc::new(global_settings);
}
if let Some(check_mode) = self.global_settings.diagnostic_mode().to_check_mode() {
for project in self.projects.values_mut() {
project.db.set_check_mode(check_mode);
}
}

self.register_capabilities(client);
}
Expand Down
12 changes: 12 additions & 0 deletions crates/ty_server/src/session/options.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use serde::{Deserialize, Serialize};
use serde_json::{Map, Value};
use ty_combine::Combine;
use ty_ide::{CompletionSettings, InlayHintSettings};
use ty_project::CheckMode;
use ty_project::metadata::Options as TyOptions;
use ty_project::metadata::options::ProjectOptionsOverrides;
use ty_project::metadata::value::{RangedValue, RelativePathBuf, ValueSource};
Expand Down Expand Up @@ -369,6 +370,17 @@ impl DiagnosticMode {
matches!(self, DiagnosticMode::OpenFilesOnly)
}

/// Returns this diagnostic mode as a check mode.
///
/// This returns `None` when diagnostics are disabled.
pub(crate) const fn to_check_mode(self) -> Option<CheckMode> {
match self {
DiagnosticMode::Off => None,
DiagnosticMode::OpenFilesOnly => Some(CheckMode::OpenFiles),
DiagnosticMode::Workspace => Some(CheckMode::AllFiles),
}
}

pub(crate) const fn is_off(self) -> bool {
matches!(self, DiagnosticMode::Off)
}
Expand Down
2 changes: 0 additions & 2 deletions crates/ty_server/tests/e2e/publish_diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -326,8 +326,6 @@ def foo() -> str:
},
};
server.send_notification::<DidOpenTextDocument>(params);
let _close_diagnostics = server.await_notification::<PublishDiagnostics>();

Copy link
Member Author

@BurntSushi BurntSushi Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it was intentional to have the diagnostics send back twice, but this is no longer the case. It was happening previously because:

  1. Closing the document (above) would result in clearing diagnostics only when DocumentHandle::close returns true. And that would return true because session.global_settings().diagnostic_mode().is_open_files_only() returned true. Because the session global settings used OpenFilesOnly by default.
  2. Re-opening the document (with the language ID changed) caused publish_diagnostics_if_needed inside of the DidOpenTextDocumentHandler handler to run. This would eventually do Project::check_file and that would call should_check_file. And that would return false because the projects have been defaulting to AllFiles as a check mode by default, and the file in question is only in the "open file" set. (Which is perhaps a bug on its own, but I digress.)

The end result is that the test got no diagnostics which was what was expected, but it got for a reason other than respecting the change in language ID. Once I fixed the AllFiles/OpenFilesOnly problem, this test started failing because we would still send back diagnostics for the plain text file.

I ended up fixing this by checking the language ID of the file in the DidOpenTextDocumentHandler handler. If it's Other, then we ignore it, hopefully mirroring what we do in ty_project/src/walk.rs.

let diagnostics = server.await_notification::<PublishDiagnostics>();

insta::assert_debug_snapshot!(diagnostics);
Expand Down
4 changes: 1 addition & 3 deletions ...2e/snapshots/e2e__publish_diagnostics__changing_language_of_file_without_extension-2.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,5 @@ PublishDiagnosticsParams {
fragment: None,
},
diagnostics: [],
version: Some(
1,
),
version: None,
}