diff --git a/Cargo.lock b/Cargo.lock index e6dd5f7b20..d04025dccf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4996,6 +4996,7 @@ version = "0.0.0" dependencies = [ "anyhow", "bitflags 2.13.1", + "by_transforms", "crossbeam", "dunce", "gen-lsp-types", @@ -5009,6 +5010,7 @@ dependencies = [ "ruff_macros", "ruff_notebook", "ruff_python_ast", + "ruff_python_parser", "ruff_ranged_value", "ruff_source_file", "ruff_text_size", diff --git a/crates/basedpython/Cargo.lock b/crates/basedpython/Cargo.lock index 44bbd09001..5e6898e247 100644 --- a/crates/basedpython/Cargo.lock +++ b/crates/basedpython/Cargo.lock @@ -3558,6 +3558,7 @@ dependencies = [ "strum", "strum_macros", "thiserror 2.0.18", + "toml 1.1.2+spec-1.1.0", "tracing", ] @@ -3685,6 +3686,7 @@ version = "0.0.0" dependencies = [ "anyhow", "bitflags", + "by_transforms", "crossbeam", "gen-lsp-types", "jod-thread", @@ -3695,6 +3697,7 @@ dependencies = [ "ruff_macros", "ruff_notebook", "ruff_python_ast", + "ruff_python_parser", "ruff_ranged_value", "ruff_source_file", "ruff_text_size", diff --git a/crates/ruff/src/commands/rule.rs b/crates/ruff/src/commands/rule.rs index 366ee56703..97dd389bb1 100644 --- a/crates/ruff/src/commands/rule.rs +++ b/crates/ruff/src/commands/rule.rs @@ -1,4 +1,3 @@ -use std::fmt::Write as _; use std::io::{self, BufWriter, Write}; use anyhow::Result; @@ -9,6 +8,7 @@ use strum::IntoEnumIterator; use ruff_linter::FixAvailability; use ruff_linter::codes::RuleGroup; use ruff_linter::registry::{Linter, Rule, RuleNamespace}; +use ruff_linter::rule_documentation; use crate::args::HelpFormat; @@ -52,57 +52,12 @@ impl<'a> Explanation<'a> { } } -fn format_rule_text(rule: Rule) -> String { - let mut output = String::new(); - let _ = write!(&mut output, "# {} ({})", rule.name(), rule.noqa_code()); - output.push('\n'); - output.push('\n'); - - let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap(); - let _ = write!( - &mut output, - "Derived from the **{}** linter.", - linter.name() - ); - output.push('\n'); - output.push('\n'); - - let fix_availability = rule.fixable(); - if matches!( - fix_availability, - FixAvailability::Always | FixAvailability::Sometimes - ) { - output.push_str(&fix_availability.to_string()); - output.push('\n'); - output.push('\n'); - } - - if rule.is_preview() { - output.push_str( - r"This rule is in preview and is not stable. The `--preview` flag is required for use.", - ); - output.push('\n'); - output.push('\n'); - } - - if let Some(explanation) = rule.explanation() { - output.push_str(explanation.trim()); - } else { - output.push_str("Message formats:"); - for format in rule.message_formats() { - output.push('\n'); - let _ = write!(&mut output, "* {format}"); - } - } - output -} - /// Explain a `Rule` to the user. pub(crate) fn rule(rule: Rule, format: HelpFormat) -> Result<()> { let mut stdout = BufWriter::new(io::stdout().lock()); match format { HelpFormat::Text => { - writeln!(stdout, "{}", format_rule_text(rule))?; + writeln!(stdout, "{}", rule_documentation(rule))?; } HelpFormat::Json => { serde_json::to_writer_pretty(stdout, &Explanation::from_rule(&rule))?; @@ -117,7 +72,7 @@ pub(crate) fn rules(format: HelpFormat) -> Result<()> { match format { HelpFormat::Text => { for rule in Rule::iter() { - writeln!(stdout, "{}", format_rule_text(rule))?; + writeln!(stdout, "{}", rule_documentation(rule))?; writeln!(stdout)?; } } diff --git a/crates/ruff_linter/src/lib.rs b/crates/ruff_linter/src/lib.rs index f971c43b4d..2a6573f8c7 100644 --- a/crates/ruff_linter/src/lib.rs +++ b/crates/ruff_linter/src/lib.rs @@ -9,6 +9,7 @@ pub use locator::Locator; pub use noqa::{SuppressionKind, generate_suppression_edits}; #[cfg(feature = "clap")] pub use registry::clap_completion::RuleParser; +pub use rule_documentation::rule_documentation; #[cfg(feature = "clap")] pub use rule_selector::clap_completion::UnresolvedRuleSelectorParser; pub use rule_selector::{RuleSelector, UnresolvedRuleSelector}; @@ -40,6 +41,7 @@ pub mod packaging; pub mod preview; pub mod registry; mod renamer; +pub mod rule_documentation; mod rule_redirects; pub mod rule_selector; pub mod rules; diff --git a/crates/ruff_linter/src/rule_documentation.rs b/crates/ruff_linter/src/rule_documentation.rs new file mode 100644 index 0000000000..d30116bcc8 --- /dev/null +++ b/crates/ruff_linter/src/rule_documentation.rs @@ -0,0 +1,95 @@ +//! The prose a rule is explained with, rendered once for everyone who asks. +//! +//! `buff rule F401` and the editor's *Explain Rule* are the same question, so they are the same +//! answer: one renderer here, in the crate that owns the rules, rather than one in the CLI and a +//! second wherever else the text was wanted. + +use std::fmt::Write as _; + +use crate::FixAvailability; +use crate::registry::{Linter, Rule, RuleNamespace}; + +/// The markdown explaining `rule`: what it is, where it came from, and what it does about it. +/// +/// Markdown because a rule's own `explanation` is written in it — the rest is built to match, so a +/// rule with documentation and one without read as the same kind of document. +pub fn rule_documentation(rule: Rule) -> String { + let mut output = String::new(); + let _ = write!(&mut output, "# {} ({})", rule.name(), rule.noqa_code()); + output.push('\n'); + output.push('\n'); + + let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()) + .expect("a rule's own noqa code is one its linter parses"); + let _ = write!( + &mut output, + "Derived from the **{}** linter.", + linter.name() + ); + output.push('\n'); + output.push('\n'); + + let fix_availability = rule.fixable(); + if matches!( + fix_availability, + FixAvailability::Always | FixAvailability::Sometimes + ) { + output.push_str(&fix_availability.to_string()); + output.push('\n'); + output.push('\n'); + } + + if rule.is_preview() { + output.push_str( + r"This rule is in preview and is not stable. The `--preview` flag is required for use.", + ); + output.push('\n'); + output.push('\n'); + } + + if let Some(explanation) = rule.explanation() { + output.push_str(explanation.trim()); + } else { + // Not every rule carries prose. The formats it reports under say what it looks for, which + // is more use than an empty document. + output.push_str("Message formats:"); + for format in rule.message_formats() { + output.push('\n'); + let _ = write!(&mut output, "* {format}"); + } + } + output +} + +#[cfg(test)] +mod tests { + use super::*; + use strum::IntoEnumIterator; + + #[test] + fn every_rule_renders_a_document_naming_itself() { + for rule in Rule::iter() { + let doc = rule_documentation(rule); + assert!( + doc.starts_with(&format!("# {} ({})", rule.name(), rule.noqa_code())), + "{} did not lead with its own name and code", + rule.name() + ); + assert!( + doc.contains("linter."), + "{} did not say which linter it came from", + rule.name() + ); + } + } + + /// A rule with no prose still has to say what it looks for, rather than trailing off. + #[test] + fn a_rule_without_an_explanation_lists_its_message_formats() { + let undocumented = Rule::iter().find(|rule| rule.explanation().is_none()); + let Some(rule) = undocumented else { + return; // every rule is documented, which is the better problem to have + }; + assert!(rule_documentation(rule).contains("Message formats:")); + } +} diff --git a/crates/ruff_server/src/server/api.rs b/crates/ruff_server/src/server/api.rs index d811dc7839..b62e181f87 100644 --- a/crates/ruff_server/src/server/api.rs +++ b/crates/ruff_server/src/server/api.rs @@ -58,6 +58,9 @@ pub(super) fn request(req: server::Request) -> Task { background_request_task::(req, BackgroundSchedule::Worker) } request::ExecuteCommand::METHOD => sync_request_task::(req), + request::ExplainRule::METHOD => { + background_request_task::(req, BackgroundSchedule::Worker) + } request::Format::METHOD => { background_request_task::(req, BackgroundSchedule::Fmt) } diff --git a/crates/ruff_server/src/server/api/requests.rs b/crates/ruff_server/src/server/api/requests.rs index 9f90d400bf..c9a9439f10 100644 --- a/crates/ruff_server/src/server/api/requests.rs +++ b/crates/ruff_server/src/server/api/requests.rs @@ -2,6 +2,7 @@ mod code_action; mod code_action_resolve; mod diagnostic; mod execute_command; +mod explain_rule; mod format; mod format_range; mod hover; @@ -18,6 +19,7 @@ pub(super) use code_action::CodeActions; pub(super) use code_action_resolve::CodeActionResolve; pub(super) use diagnostic::DocumentDiagnostic; pub(super) use execute_command::ExecuteCommand; +pub(super) use explain_rule::ExplainRule; pub(super) use format::Format; pub(super) use format_range::FormatRange; pub(super) use hover::Hover; diff --git a/crates/ruff_server/src/server/api/requests/explain_rule.rs b/crates/ruff_server/src/server/api/requests/explain_rule.rs new file mode 100644 index 0000000000..ec98192d2f --- /dev/null +++ b/crates/ruff_server/src/server/api/requests/explain_rule.rs @@ -0,0 +1,143 @@ +//! `buff/explainRule` — the documentation for one diagnostic code. +//! +//! A custom request rather than a subprocess on the client's side. An editor showing what a rule +//! means is asking the same tool the same question its diagnostics came from, and the running +//! server is the copy of that tool which already resolved this project's configuration. Spawning +//! `buff rule F401` to answer it resolves that configuration a second time, by a different route, +//! and pays a process for a lookup that is a table read. +//! +//! Not a `codeDescription` on the diagnostic: that is a URL, which sends the reader to a browser +//! for prose the server is holding. Not a hover, which is anchored to a position — a reader can +//! ask about a code they typed into a prompt, with no position to anchor to. + +use lsp_types::{LspRequestMethod, MessageDirection, Request}; +use ruff_linter::registry::Rule; +use ruff_linter::rule_documentation; + +use crate::server::api::traits::{BackgroundRequestHandler, RequestHandler}; +use crate::session::{Client, Session}; + +pub(crate) enum ExplainRuleRequest {} + +impl Request for ExplainRuleRequest { + type Params = ExplainRuleParams; + type Result = Option; + const METHOD: LspRequestMethod<'static> = LspRequestMethod::Custom("buff/explainRule"); + const MESSAGE_DIRECTION: MessageDirection = MessageDirection::ClientToServer; +} + +/// The code to look up, as the user wrote it. +#[derive(Debug, serde::Deserialize, serde::Serialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub(crate) struct ExplainRuleParams { + /// A noqa code (`F401`) or a rule name (`unused-import`). Both are what a reader has in hand: + /// the code is what a diagnostic shows, the name is what the documentation calls it. + pub(crate) code: String, +} + +/// What the rule is, ready to show. +#[derive(Debug, serde::Deserialize, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct RuleExplanation { + /// The rule's own name, e.g. `unused-import`. + pub(crate) name: String, + /// The noqa code, e.g. `F401`. + pub(crate) code: String, + /// The full explanation, in markdown. + pub(crate) documentation: String, +} + +pub(crate) struct ExplainRule; + +impl RequestHandler for ExplainRule { + type RequestType = ExplainRuleRequest; +} + +impl BackgroundRequestHandler for ExplainRule { + // The rules are a compiled-in table; there is no session state to snapshot. + type Snapshot = (); + + fn snapshot(_session: &Session, _params: &ExplainRuleParams) {} + + fn run_with_snapshot( + (): (), + _client: &Client, + params: ExplainRuleParams, + ) -> crate::server::Result> { + Ok(explanation_of(¶ms.code)) + } +} + +/// The explanation for [`code`], if this linter owns the rule it names. +fn explanation_of(code: &str) -> Option { + resolve(code).map(|rule| RuleExplanation { + name: rule.name().as_str().to_string(), + code: rule.noqa_code().to_string(), + documentation: rule_documentation(rule), + }) +} + +/// The rule [`code`] names, by either of the two names a rule has. +/// +/// `None` rather than an error for a code this linter does not own: the type checker owns a +/// disjoint set of rules under the same kind of name, so "not mine" is an ordinary answer that +/// leaves the client free to ask the other server. An error would make a routine miss look like a +/// failure. +fn resolve(code: &str) -> Option { + let code = code.trim(); + Rule::from_code(code) + .ok() + .or_else(|| Rule::from_name(code).ok()) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The wire form, exactly as a client sends it — the whole contract with a plugin written in + /// another language, and nothing else in the crate exercises it. + #[test] + fn the_params_a_client_sends_parse() { + let parsed: ExplainRuleParams = + serde_json::from_str(r#"{"code":"F401"}"#).expect("a client sends just the code"); + assert_eq!(parsed.code, "F401"); + } + + #[test] + fn a_code_resolves_to_its_rule() { + let rule = resolve("F401").expect("F401 is this linter's"); + assert_eq!(rule.noqa_code().to_string(), "F401"); + } + + /// A reader who has the name rather than the code is asking the same question. + #[test] + fn a_rule_name_resolves_to_the_same_rule() { + assert_eq!(resolve("unused-import"), resolve("F401")); + } + + /// Whitespace around a code pasted out of a diagnostic is not a different code. + #[test] + fn surrounding_whitespace_is_not_part_of_the_code() { + assert_eq!(resolve(" F401 "), resolve("F401")); + } + + /// The type checker's rules are named the same way and are not this server's to explain. + #[test] + fn a_code_this_linter_does_not_own_is_a_miss_rather_than_an_error() { + assert!(resolve("redundant-return-annotation").is_none()); + assert!(resolve("not-a-rule-at-all").is_none()); + } + + #[test] + fn an_explanation_carries_the_prose_and_both_names() { + let explanation = explanation_of("F401").expect("F401 is this linter's"); + + assert_eq!(explanation.code, "F401"); + assert_eq!(explanation.name, "unused-import"); + assert!( + explanation + .documentation + .starts_with("# unused-import (F401)") + ); + } +} diff --git a/crates/ty/src/rule.rs b/crates/ty/src/rule.rs index 4acbbade8c..84f182a5c6 100644 --- a/crates/ty/src/rule.rs +++ b/crates/ty/src/rule.rs @@ -15,6 +15,8 @@ struct Explanation<'a> { documentation: String, default_level: Level, status: LintStatus, + /// The rendered form, so a client reading the json gets the same prose the terminal shows. + markdown: String, } impl<'a> Explanation<'a> { @@ -25,25 +27,16 @@ impl<'a> Explanation<'a> { documentation: lint.documentation(), default_level: lint.default_level(), status: *lint.status(), + markdown: lint.documentation_markdown(), } } } impl std::fmt::Display for Explanation<'_> { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - writeln!(f, "# {}\n", self.name)?; - - let status = match self.status { - LintStatus::Stable { since } => format!("Stable (since {since})"), - LintStatus::Deprecated { since, reason } => { - format!("Deprecated (since {since}): {reason}") - } - LintStatus::Removed { since, reason } => format!("Removed (since {since}): {reason}"), - }; - - writeln!(f, "Default level: {} | {status}\n", self.default_level)?; - - f.write_str(self.documentation.trim()) + // The lint renders its own documentation, so this and the editor's *Explain Rule* show the + // same text rather than two renderings that drift. + f.write_str(&self.markdown) } } diff --git a/crates/ty/tests/cli/rule.rs b/crates/ty/tests/cli/rule.rs index 1d437b4f3d..aa6b83cbb2 100644 --- a/crates/ty/tests/cli/rule.rs +++ b/crates/ty/tests/cli/rule.rs @@ -54,7 +54,8 @@ fn rule_json_output() { "status": { "type": "stable", "since": "0.0.1-alpha.1" - } + }, + "markdown": "# invalid-return-type\n\nDefault level: error | Stable (since 0.0.1-alpha.1)\n\n## What it does\n\nDetects returned values that can't be assigned to the function's annotated return type.\n\nNote that the special case of a function with a non-`None` return type and an empty body\nis handled by the separate `empty-body` error code.\n\n## Why is this bad?\n\nReturning an object of a type incompatible with the annotated return type\nis unsound, and will lead to ty inferring incorrect types elsewhere.\n\n## Examples\n\n```python\ndef func() -> int:\n return \"a\" # error: [invalid-return-type]\n```" } ----- stderr ----- "###); diff --git a/crates/ty_python_semantic/src/lint.rs b/crates/ty_python_semantic/src/lint.rs index cfc55ac81a..fc85646a8e 100644 --- a/crates/ty_python_semantic/src/lint.rs +++ b/crates/ty_python_semantic/src/lint.rs @@ -135,6 +135,28 @@ impl LintMetadata { &self.status } + /// The markdown explaining this lint: what it is called, how it is reported, and its prose. + /// + /// Lives here rather than in whichever tool wants to show it. `by explain rule` and the + /// editor's *Explain Rule* are the same question, so they render the same answer — one + /// implementation, in the crate that owns the lints. + pub fn documentation_markdown(&self) -> String { + let status = match self.status() { + LintStatus::Stable { since } => format!("Stable (since {since})"), + LintStatus::Deprecated { since, reason } => { + format!("Deprecated (since {since}): {reason}") + } + LintStatus::Removed { since, reason } => format!("Removed (since {since}): {reason}"), + }; + + format!( + "# {name}\n\nDefault level: {level} | {status}\n\n{documentation}", + name = self.name(), + level = self.default_level(), + documentation = self.documentation().trim(), + ) + } + pub fn file(&self) -> &str { self.file } diff --git a/crates/ty_server/Cargo.toml b/crates/ty_server/Cargo.toml index 7c423fb5bc..a77472ae1e 100644 --- a/crates/ty_server/Cargo.toml +++ b/crates/ty_server/Cargo.toml @@ -14,11 +14,13 @@ license = { workspace = true } doctest = false [dependencies] +by_transforms = { workspace = true } ruff_db = { workspace = true, features = ["os"] } ruff_diagnostics = { workspace = true } ruff_macros = { workspace = true } ruff_notebook = { workspace = true } ruff_python_ast = { workspace = true } +ruff_python_parser = { workspace = true } ruff_source_file = { workspace = true } ruff_ranged_value = { workspace = true } ruff_text_size = { workspace = true } diff --git a/crates/ty_server/src/server/api.rs b/crates/ty_server/src/server/api.rs index 7a036977ae..692a56b94c 100644 --- a/crates/ty_server/src/server/api.rs +++ b/crates/ty_server/src/server/api.rs @@ -86,6 +86,17 @@ pub(super) fn request(req: server::Request) -> Task { >( req, BackgroundSchedule::LatencySensitive ), + // A table lookup with no document behind it: a reader can ask about a code they typed + // into a prompt, with no file open at all. + requests::ExplainRuleHandler::METHOD => { + background_request_task::(req, BackgroundSchedule::Worker) + } + requests::TranspileRequestHandler::METHOD => background_document_request_task::< + requests::TranspileRequestHandler, + >(req, BackgroundSchedule::Worker), + requests::ExplainTranspilationHandler::METHOD => background_document_request_task::< + requests::ExplainTranspilationHandler, + >(req, BackgroundSchedule::Worker), requests::SemanticTokensRequestHandler::METHOD => background_document_request_task::< requests::SemanticTokensRequestHandler, >(req, BackgroundSchedule::Worker), diff --git a/crates/ty_server/src/server/api/requests.rs b/crates/ty_server/src/server/api/requests.rs index 19bed31a4a..b87ba79bd4 100644 --- a/crates/ty_server/src/server/api/requests.rs +++ b/crates/ty_server/src/server/api/requests.rs @@ -22,6 +22,8 @@ mod diagnostic; mod doc_highlights; mod document_symbols; mod execute_command; +mod explain_rule; +mod explain_transpilation; mod folding_range; mod goto_declaration; mod goto_definition; @@ -39,6 +41,7 @@ mod semantic_tokens; mod semantic_tokens_range; mod shutdown; mod signature_help; +mod transpile; mod type_hierarchy_subtypes; mod type_hierarchy_supertypes; mod workspace_diagnostic; @@ -54,6 +57,8 @@ pub(super) use diagnostic::DocumentDiagnosticRequestHandler; pub(super) use doc_highlights::DocumentHighlightRequestHandler; pub(super) use document_symbols::DocumentSymbolRequestHandler; pub(super) use execute_command::ExecuteCommand; +pub(super) use explain_rule::ExplainRuleHandler; +pub(super) use explain_transpilation::ExplainTranspilationHandler; pub(super) use folding_range::FoldingRangeRequestHandler; pub(super) use goto_declaration::GotoDeclarationRequestHandler; pub(super) use goto_definition::GotoDefinitionRequestHandler; @@ -71,6 +76,7 @@ pub(super) use semantic_tokens::SemanticTokensRequestHandler; pub(super) use semantic_tokens_range::SemanticTokensRangeRequestHandler; pub(super) use shutdown::ShutdownHandler; pub(super) use signature_help::SignatureHelpRequestHandler; +pub(super) use transpile::TranspileRequestHandler; pub(super) use type_hierarchy_subtypes::TypeHierarchySubtypesRequestHandler; pub(super) use type_hierarchy_supertypes::TypeHierarchySupertypesRequestHandler; pub(super) use workspace_diagnostic::WorkspaceDiagnosticRequestHandler; diff --git a/crates/ty_server/src/server/api/requests/explain_rule.rs b/crates/ty_server/src/server/api/requests/explain_rule.rs new file mode 100644 index 0000000000..fda48b3ffd --- /dev/null +++ b/crates/ty_server/src/server/api/requests/explain_rule.rs @@ -0,0 +1,111 @@ +//! `by/explainRule` — the documentation for one type-checker lint. +//! +//! A custom request rather than a subprocess on the client's side. An editor showing what a rule +//! means is asking the same tool the same question its diagnostics came from, and the running +//! server is the copy of that tool which already resolved this project's configuration. Spawning +//! `by explain rule ` to answer it pays a process for a table lookup. +//! +//! The linter owns a disjoint set of rules under its own `buff/explainRule`. Neither server knows +//! the other's, so a name this one does not have is an ordinary miss rather than an error, and the +//! client is free to ask the other. + +use lsp_types::{LspRequestMethod, MessageDirection, Request}; +use ty_python_semantic::default_lint_registry; + +use crate::server::api::traits::{ + BackgroundRequestHandler, RequestHandler, RetriableRequestHandler, +}; +use crate::session::SessionSnapshot; +use crate::session::client::Client; + +pub(crate) enum ExplainRuleRequest {} + +impl Request for ExplainRuleRequest { + type Params = ExplainRuleParams; + type Result = Option; + const METHOD: LspRequestMethod<'static> = LspRequestMethod::Custom("by/explainRule"); + const MESSAGE_DIRECTION: MessageDirection = MessageDirection::ClientToServer; +} + +/// The rule to look up, as the user wrote it. +#[derive(Debug, serde::Deserialize, serde::Serialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub(crate) struct ExplainRuleParams { + /// A lint name, e.g. `redundant-return-annotation` — what a diagnostic reports under. + pub(crate) name: String, +} + +/// What the rule is, ready to show. +#[derive(Debug, serde::Deserialize, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct RuleExplanation { + /// The lint's own name. + pub(crate) name: String, + /// A one-line summary. + pub(crate) summary: String, + /// The full explanation, in markdown. + pub(crate) documentation: String, +} + +pub(crate) struct ExplainRuleHandler; + +impl RequestHandler for ExplainRuleHandler { + type RequestType = ExplainRuleRequest; +} + +impl BackgroundRequestHandler for ExplainRuleHandler { + fn run( + _snapshot: &SessionSnapshot, + _client: &Client, + params: ExplainRuleParams, + ) -> crate::server::Result> { + Ok(explanation_of(¶ms.name)) + } +} + +/// The explanation for [`name`], if this server owns the lint it names. +fn explanation_of(name: &str) -> Option { + let lint = default_lint_registry().get(name.trim()).ok()?; + Some(RuleExplanation { + name: lint.name().as_str().to_string(), + summary: lint.summary().to_string(), + documentation: lint.documentation_markdown(), + }) +} + +impl RetriableRequestHandler for ExplainRuleHandler {} + +#[cfg(test)] +mod tests { + use super::*; + + /// The wire form, exactly as a client sends it — the whole contract with a plugin written in + /// another language, and nothing else here exercises it. + #[test] + fn the_params_a_client_sends_parse() { + let parsed: ExplainRuleParams = + serde_json::from_str(r#"{"name":"unresolved-import"}"#).expect("a client sends a name"); + assert_eq!(parsed.name, "unresolved-import"); + } + + #[test] + fn a_name_resolves_to_its_lint() { + let explanation = explanation_of("unresolved-import").expect("this server owns it"); + assert_eq!(explanation.name, "unresolved-import"); + assert!(explanation.documentation.starts_with("# unresolved-import")); + assert!(explanation.documentation.contains("Default level:")); + } + + /// Whitespace around a name pasted out of a diagnostic is not a different name. + #[test] + fn surrounding_whitespace_is_not_part_of_the_name() { + assert!(explanation_of(" unresolved-import ").is_some()); + } + + /// The linter's rules are not this server's to explain, and saying so is not a failure. + #[test] + fn a_name_this_server_does_not_own_is_a_miss_rather_than_an_error() { + assert!(explanation_of("F401").is_none()); + assert!(explanation_of("not-a-rule-at-all").is_none()); + } +} diff --git a/crates/ty_server/src/server/api/requests/explain_transpilation.rs b/crates/ty_server/src/server/api/requests/explain_transpilation.rs new file mode 100644 index 0000000000..5855f1bdcf --- /dev/null +++ b/crates/ty_server/src/server/api/requests/explain_transpilation.rs @@ -0,0 +1,281 @@ +//! `by/explainTranspilation` — which basedpython constructs a document uses, and what each lowers to. +//! +//! The recognition belongs here rather than in an editor plugin, and not only on principle. A +//! client that wanted this had to guess from the source text — a regex for `?.`, another for `??`, +//! another for `data class` — and a regex cannot tell an operator from the same characters inside a +//! string or a comment, cannot see that `?` in a type position means something else, and drifts +//! from the language the moment a construct is added. The parser here is the one the transpiler +//! itself runs, so the answer is the same one the lowering is about to act on. + +use std::borrow::Cow; + +use lsp_types::{LspRequestMethod, MessageDirection, Request, TextDocumentIdentifier, Uri}; +use ruff_python_ast::visitor::source_order::{SourceOrderVisitor, walk_expr, walk_stmt}; +use ruff_python_ast::{Expr, Operator, Stmt, UnaryOp}; +use ruff_source_file::LineIndex; +use ruff_text_size::{Ranged, TextRange}; +use ty_project::ProjectDatabase; + +use crate::server::api::traits::{ + BackgroundDocumentRequestHandler, RequestHandler, RetriableRequestHandler, +}; +use crate::session::DocumentSnapshot; +use crate::session::client::Client; + +pub(crate) enum ExplainTranspilationRequest {} + +impl Request for ExplainTranspilationRequest { + type Params = ExplainTranspilationParams; + type Result = Option>; + const METHOD: LspRequestMethod<'static> = LspRequestMethod::Custom("by/explainTranspilation"); + const MESSAGE_DIRECTION: MessageDirection = MessageDirection::ClientToServer; +} + +#[derive(Debug, serde::Deserialize, serde::Serialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub(crate) struct ExplainTranspilationParams { + pub(crate) text_document: TextDocumentIdentifier, +} + +/// One construct found, and what the transpiler does with it. +#[derive(Debug, PartialEq, Eq, serde::Deserialize, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct TranspilationNote { + /// A short, stable name for the construct, e.g. `null-safe access`. + pub(crate) construct: String, + /// The source it was written as. + pub(crate) snippet: String, + /// What it lowers to, in a sentence. + pub(crate) explanation: String, + /// The one-based line it is on. + pub(crate) line: u32, +} + +pub(crate) struct ExplainTranspilationHandler; + +impl RequestHandler for ExplainTranspilationHandler { + type RequestType = ExplainTranspilationRequest; +} + +impl BackgroundDocumentRequestHandler for ExplainTranspilationHandler { + fn document_uri(params: &ExplainTranspilationParams) -> Cow<'_, Uri> { + Cow::Borrowed(¶ms.text_document.uri) + } + + fn run_with_snapshot( + db: &ProjectDatabase, + snapshot: &DocumentSnapshot, + _client: &Client, + _params: ExplainTranspilationParams, + ) -> crate::server::Result>> { + let Some(file) = snapshot.to_notebook_or_file(db) else { + return Ok(None); + }; + let source = ruff_db::source::source_text(db, file); + Ok(Some(notes_in(source.as_str()))) + } +} + +impl RetriableRequestHandler for ExplainTranspilationHandler {} + +/// Every construct [`source`] uses, in source order. +/// +/// Parsed rather than scanned. A construct is only reported where the parser built the node for it, +/// so the same characters inside a string, a comment or a type position are not mistaken for one. +pub(crate) fn notes_in(source: &str) -> Vec { + let parsed = ruff_python_parser::parse_unchecked_source( + source, + ruff_python_ast::PySourceType::BasedPython, + ); + let index = LineIndex::from_source_text(source); + let mut collector = Collector { + source, + index: &index, + notes: Vec::new(), + }; + collector.visit_body(parsed.suite()); + collector.notes.sort_by_key(|note| note.line); + collector.notes +} + +struct Collector<'a> { + source: &'a str, + index: &'a LineIndex, + notes: Vec, +} + +impl Collector<'_> { + fn note(&mut self, range: TextRange, construct: &str, explanation: &str) { + let snippet = self + .source + .get(range.start().into()..range.end().into()) + .unwrap_or_default() + .trim(); + self.notes.push(TranspilationNote { + construct: construct.to_string(), + snippet: snippet.to_string(), + explanation: explanation.to_string(), + // A line number past `u32` needs a file no editor would open. + line: u32::try_from(self.index.line_index(range.start()).get()).unwrap_or(u32::MAX), + }); + } +} + +impl<'a> SourceOrderVisitor<'a> for Collector<'_> { + fn visit_stmt(&mut self, stmt: &'a Stmt) { + match stmt { + Stmt::Match(_) => self.note( + stmt.range(), + "pattern match", + "Lowered to a Python `match` statement, or to an `if`/`elif` chain when the \ + configured minimum version predates structural pattern matching.", + ), + Stmt::ClassDef(class) if class.decorator_list.iter().any(is_data_decorator) => self + .note( + class.range(), + "data-class modifier", + "Lowered to a `@dataclasses.dataclass` class, with `__init__`, `__repr__` and \ + `__eq__` generated from the annotated fields.", + ), + _ => {} + } + walk_stmt(self, stmt); + } + + fn visit_expr(&mut self, expr: &'a Expr) { + match expr { + Expr::BinOp(op) if op.op == Operator::Coalesce => self.note( + expr.range(), + "null-coalescing operator", + "Lowered to a conditional that evaluates the right operand only when the left is \ + `None`.", + ), + Expr::UnaryOp(op) => match op.op { + UnaryOp::Force => self.note( + expr.range(), + "force unwrap", + "Lowered to a check that raises when the value is absent, and yields the value \ + otherwise.", + ), + UnaryOp::Propagate => self.note( + expr.range(), + "propagate operator", + "Lowered to an early return of the absent case, so the rest of the function \ + sees only the present one.", + ), + UnaryOp::Optional => self.note( + expr.range(), + "optional type", + "A type-level marker: lowered to `Optional[T]`, i.e. `T | None`.", + ), + _ => {} + }, + // The parser records `?.` as a flag on the access rather than leaving it in the + // text, so this is what was written and not what the characters look like. + Expr::Attribute(access) if access.optional => self.note( + expr.range(), + "null-safe access", + "Lowered to a conditional that yields `None` when the receiver is `None`, and the \ + attribute otherwise. A chain evaluates the receiver once.", + ), + _ => {} + } + walk_expr(self, expr); + } +} + +fn is_data_decorator(decorator: &ruff_python_ast::Decorator) -> bool { + let name = match &decorator.expression { + Expr::Name(name) => name.id.as_str(), + Expr::Call(call) => match call.func.as_ref() { + Expr::Name(name) => name.id.as_str(), + _ => return false, + }, + _ => return false, + }; + matches!(name, "data" | "dataclass") +} + +#[cfg(test)] +mod tests { + use super::*; + + fn constructs(source: &str) -> Vec { + notes_in(source) + .into_iter() + .map(|note| note.construct) + .collect() + } + + #[test] + fn the_params_a_client_sends_parse() { + let parsed: ExplainTranspilationParams = + serde_json::from_str(r#"{"textDocument":{"uri":"file:///a.by"}}"#) + .expect("a client sends just the document"); + assert_eq!(parsed.text_document.uri.path().to_string(), "/a.by"); + } + + #[test] + fn the_postfix_operators_are_recognised() { + assert_eq!(constructs("x = a ?? b\n"), ["null-coalescing operator"]); + assert_eq!(constructs("x = a!\n"), ["force unwrap"]); + assert_eq!(constructs("x = a^\n"), ["propagate operator"]); + } + + #[test] + fn a_null_safe_access_is_recognised() { + assert_eq!(constructs("x = a?.b\n"), ["null-safe access"]); + } + + /// The whole reason for parsing rather than scanning: a plain access is not a null-safe one. + #[test] + fn a_plain_access_is_not_reported() { + assert!(constructs("x = a.b\n").is_empty()); + assert!(constructs("x = a[0]\n").is_empty()); + } + + /// The other half of that reason: the same characters inside a string are not an operator. + #[test] + fn a_marker_inside_a_string_is_not_an_operator() { + assert!(constructs("x = \"a?.b\"\n").is_empty()); + assert!(constructs("x = 1 # a ?? b\n").is_empty()); + } + + #[test] + fn a_match_statement_is_recognised() { + assert_eq!( + constructs("match x:\n case 1:\n pass\n"), + ["pattern match"] + ); + } + + #[test] + fn a_data_class_is_recognised() { + assert_eq!( + constructs("@data\nclass Point:\n x: int\n"), + ["data-class modifier"] + ); + } + + #[test] + fn notes_come_back_in_source_order() { + let lines: Vec = notes_in("x = a!\ny = b ?? c\nz = d?.e\n") + .into_iter() + .map(|note| note.line) + .collect(); + assert_eq!(lines, [1, 2, 3]); + } + + #[test] + fn a_note_carries_the_source_it_was_written_as() { + let notes = notes_in("x = a ?? b\n"); + assert_eq!(notes[0].snippet, "a ?? b"); + assert_eq!(notes[0].line, 1); + } + + /// Source that does not parse is an ordinary state mid-edit, not something to fail on. + #[test] + fn unparsable_source_yields_no_notes_rather_than_an_error() { + let _ = notes_in("def (\n"); + } +} diff --git a/crates/ty_server/src/server/api/requests/transpile.rs b/crates/ty_server/src/server/api/requests/transpile.rs new file mode 100644 index 0000000000..7dab0f3584 --- /dev/null +++ b/crates/ty_server/src/server/api/requests/transpile.rs @@ -0,0 +1,205 @@ +//! `by/transpile` — the python a `.by` document lowers to, and the reverse. +//! +//! A custom request rather than a subprocess the client spawns over a path. The two differ in what +//! they are looking at: a subprocess reads the file, and an editor's copy of a file is the buffer, +//! not the bytes on disk. Asking `by transpile` about a document with unsaved edits shows the last +//! saved version of it, which is the wrong answer and a quiet one. +//! +//! It is also the wrong *tool*. The server has this project's configuration already resolved and a +//! db with its modules indexed, so it transpiles with cross-module types available; a subprocess +//! rediscovers all of that per call, by a different route, and can disagree with the diagnostics in +//! the same window about what the file means. + +use std::borrow::Cow; + +use by_transforms::Config; +use lsp_types::{LspRequestMethod, MessageDirection, Request, TextDocumentIdentifier, Uri}; +use ty_project::{Db as _, ProjectDatabase}; + +use crate::server::api::traits::{ + BackgroundDocumentRequestHandler, RequestHandler, RetriableRequestHandler, +}; +use crate::session::DocumentSnapshot; +use crate::session::client::Client; + +pub(crate) enum TranspileRequest {} + +impl Request for TranspileRequest { + type Params = TranspileParams; + type Result = Option; + const METHOD: LspRequestMethod<'static> = LspRequestMethod::Custom("by/transpile"); + const MESSAGE_DIRECTION: MessageDirection = MessageDirection::ClientToServer; +} + +/// Which document, and which way. +#[derive(Debug, serde::Deserialize, serde::Serialize)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub(crate) struct TranspileParams { + /// The document to transpile. Its *buffer* — what the editor holds, which is the point. + pub(crate) text_document: TextDocumentIdentifier, + + /// When true, go the other way: python in, basedpython out. + #[serde(default)] + pub(crate) reverse: bool, + + /// Text to transpile instead of the document's own. + /// + /// For a fragment that is not a file: a selection the user asked about has no document of its + /// own, and the alternative — the client writing it to a temp file and running the CLI over + /// that — is the very thing this request exists to remove. `text_document` still says which + /// document the fragment came from, because that is what routes the request to a server; the + /// fragment is checked on its own, which is all a fragment can be. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub(crate) source: Option, +} + +/// What came out, or why nothing did. +/// +/// A failed transpile is an answer rather than a protocol error: source that does not lower yet is +/// an ordinary state for a file being edited, and an error response would make the client render it +/// as a fault in the server. +#[derive(Debug, serde::Deserialize, serde::Serialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct TranspileResponse { + /// The generated source, absent when the transpile failed. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) source: Option, + + /// Why it failed, absent when it did not. + #[serde(skip_serializing_if = "Option::is_none")] + pub(crate) error: Option, +} + +impl TranspileResponse { + fn generated(source: String) -> Self { + Self { + source: Some(source), + error: None, + } + } + + fn failed(error: String) -> Self { + Self { + source: None, + error: Some(error), + } + } +} + +pub(crate) struct TranspileRequestHandler; + +impl RequestHandler for TranspileRequestHandler { + type RequestType = TranspileRequest; +} + +impl BackgroundDocumentRequestHandler for TranspileRequestHandler { + fn document_uri(params: &TranspileParams) -> Cow<'_, Uri> { + Cow::Borrowed(¶ms.text_document.uri) + } + + fn run_with_snapshot( + db: &ProjectDatabase, + snapshot: &DocumentSnapshot, + _client: &Client, + params: TranspileParams, + ) -> crate::server::Result> { + let Some(file) = snapshot.to_notebook_or_file(db) else { + return Ok(None); + }; + + let config = config_for(db, snapshot.uri()); + let document = ruff_db::source::source_text(db, file); + let source = params.source.as_deref().unwrap_or(document.as_str()); + + // Reverse always goes through the text form: the rewrite is syntactic, and there is no + // python project db to infer against. Forward uses the project db when it is the whole + // document that was asked about — that is the reason for answering here, since cross-module + // types resolve — and the single-file path for a fragment, which has no module to resolve + // against however it is transpiled. + let response = if params.reverse { + match by_transforms::reverse_transpile(source, &config) { + Ok(out) => TranspileResponse::generated(out), + Err(error) => TranspileResponse::failed(error), + } + } else if params.source.is_some() { + match by_transforms::transpile(source, &config) { + Ok(out) => TranspileResponse::generated(out), + Err(error) => TranspileResponse::failed(error), + } + } else { + match by_transforms::transpile_typed(db, file, &config, None) { + Ok(out) => TranspileResponse::generated(out), + Err(error) => TranspileResponse::failed(error.to_string()), + } + }; + + Ok(Some(response)) + } +} + +impl RetriableRequestHandler for TranspileRequestHandler {} + +/// The transpile config for the document at [`uri`]. +/// +/// The minimum version is the project's own, read off the db the server is already holding — +/// rather than rediscovered from the filesystem, which is what a subprocess would have to do and +/// what lets the two disagree. +fn config_for(db: &ProjectDatabase, uri: &Uri) -> Config { + let path = uri.path().to_string(); + let extension = path.rsplit('.').next().unwrap_or_default(); + Config { + is_python: matches!(extension, "py" | "pyi"), + is_stub: matches!(extension, "pyi" | "byi"), + min_version: db + .project() + .program(db) + .python_version(db) + .to_string() + .parse() + .unwrap_or_else(|_| Config::default().min_version), + ..Config::default() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The wire form, exactly as a client sends it. Nothing else here exercises it, and a serde + /// attribute that makes the typed value unreachable from json would not show up anywhere else. + #[test] + fn the_params_a_client_sends_parse() { + let forward: TranspileParams = + serde_json::from_str(r#"{"textDocument":{"uri":"file:///a.by"}}"#) + .expect("reverse is optional"); + assert!(!forward.reverse); + + let reverse: TranspileParams = + serde_json::from_str(r#"{"textDocument":{"uri":"file:///a.py"},"reverse":true}"#) + .expect("both fields are accepted"); + assert!(reverse.reverse); + assert!(reverse.source.is_none()); + + let fragment: TranspileParams = + serde_json::from_str(r#"{"textDocument":{"uri":"file:///a.by"},"source":"x = 1\n"}"#) + .expect("a fragment carries its own text"); + assert_eq!(fragment.source.as_deref(), Some("x = 1\n")); + } + + /// A failure travels as a result, so the client can show the reason rather than a dead request. + #[test] + fn a_failure_serializes_as_a_reason_and_no_source() { + let json = serde_json::to_value(TranspileResponse::failed("nope".to_string())) + .expect("the response is plain data"); + assert_eq!(json["error"], "nope"); + assert!(json.get("source").is_none()); + } + + #[test] + fn output_serializes_as_source_and_no_reason() { + let json = serde_json::to_value(TranspileResponse::generated("x = 1\n".to_string())) + .expect("the response is plain data"); + assert_eq!(json["source"], "x = 1\n"); + assert!(json.get("error").is_none()); + } +}