From 0e1bf0d2f6bf100ec93f164efcd98514fb12b348 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 20 Oct 2026 09:51:56 -0700 Subject: [PATCH] Avoid retaining rule documentation for diagnostic URLs (#29517) --- crates/ruff/src/commands/rule.rs | 12 +- crates/ruff_dev/src/generate_docs.rs | 221 +++++++++--------- crates/ruff_dev/src/generate_rules_table.rs | 8 +- crates/ruff_linter/src/message/mod.rs | 2 +- crates/ruff_linter/src/message/sarif.rs | 12 +- crates/ruff_linter/src/registry.rs | 20 +- crates/ruff_linter/src/violation.rs | 2 +- crates/ruff_macros/src/map_codes.rs | 2 +- crates/ruff_macros/src/violation_metadata.rs | 4 +- .../src/server/api/requests/hover.rs | 7 +- 10 files changed, 133 insertions(+), 157 deletions(-) diff --git a/crates/ruff/src/commands/rule.rs b/crates/ruff/src/commands/rule.rs index 835dc3e2b8..bce1a0a83e 100644 --- a/crates/ruff/src/commands/rule.rs +++ b/crates/ruff/src/commands/rule.rs @@ -22,7 +22,7 @@ struct Explanation<'a> { fix: String, fix_availability: FixAvailability, #[expect(clippy::struct_field_names)] - explanation: Option<&'a str>, + explanation: &'a str, preview: bool, status: RuleStatus, category: Category, @@ -94,15 +94,7 @@ fn format_rule_text(rule: Rule) -> String { 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.push_str(rule.explanation().trim()); output } diff --git a/crates/ruff_dev/src/generate_docs.rs b/crates/ruff_dev/src/generate_docs.rs index 05efc52fbe..279e5fabe8 100644 --- a/crates/ruff_dev/src/generate_docs.rs +++ b/crates/ruff_dev/src/generate_docs.rs @@ -27,137 +27,134 @@ pub(crate) struct Args { pub(crate) fn main(args: &Args) -> Result<()> { for rule in Rule::iter() { - if let Some(explanation) = rule.explanation() { - let mut output = String::new(); + let explanation = rule.explanation(); + let mut output = String::new(); - let _ = writeln!(&mut output, "# {}", rule.name_and_code()); + let _ = writeln!(&mut output, "# {}", rule.name_and_code()); - let status_text = match rule.status() { - RuleStatus::Stable { since } => { - format!( - r#"Added in {since}"# - ) - } - RuleStatus::Preview { since } => { - format!( - r#"Preview (since {since})"# - ) - } - RuleStatus::Deprecated { since } => { - format!( - r#"Deprecated (since {since})"# - ) - } - RuleStatus::Removed { since } => { - format!( - r#"Removed (since {since})"# - ) - } - }; + let status_text = match rule.status() { + RuleStatus::Stable { since } => { + format!( + r#"Added in {since}"# + ) + } + RuleStatus::Preview { since } => { + format!( + r#"Preview (since {since})"# + ) + } + RuleStatus::Deprecated { since } => { + format!( + r#"Deprecated (since {since})"# + ) + } + RuleStatus::Removed { since } => { + format!( + r#"Removed (since {since})"# + ) + } + }; - let issue_search = format!( - "(%27{encoded_name}%27{code})", - encoded_name = - url::form_urlencoded::byte_serialize(rule.name().as_str().as_bytes()) - .collect::(), - code = rule - .noqa_code() - .map(|code| format!("%20OR%20{code}")) - .unwrap_or_default(), - ); + let issue_search = format!( + "(%27{encoded_name}%27{code})", + encoded_name = url::form_urlencoded::byte_serialize(rule.name().as_str().as_bytes()) + .collect::(), + code = rule + .noqa_code() + .map(|code| format!("%20OR%20{code}")) + .unwrap_or_default(), + ); - let _ = writeln!( - &mut output, - r#" + let _ = writeln!( + &mut output, + r#" {status_text} · Related issues · View source "#, - file = - url::form_urlencoded::byte_serialize(rule.file().replace('\\', "/").as_bytes()) - .collect::(), - line = rule.line(), + file = url::form_urlencoded::byte_serialize(rule.file().replace('\\', "/").as_bytes()) + .collect::(), + line = rule.line(), + ); + if let Some(linter) = rule + .noqa_code() + .and_then(|code| Linter::parse_code(&code.to_string()).map(|(linter, _)| linter)) + .filter(|linter| linter.url().is_some()) + { + let common_prefix: String = match linter.common_prefix() { + "" => linter + .upstream_categories() + .unwrap() + .iter() + .map(|c| c.prefix) + .join("-"), + prefix => prefix.to_string(), + }; + let anchor = format!( + "{}-{}", + linter.name().to_lowercase(), + common_prefix.to_lowercase() ); - if let Some(linter) = rule - .noqa_code() - .and_then(|code| Linter::parse_code(&code.to_string()).map(|(linter, _)| linter)) - .filter(|linter| linter.url().is_some()) - { - let common_prefix: String = match linter.common_prefix() { - "" => linter - .upstream_categories() - .unwrap() - .iter() - .map(|c| c.prefix) - .join("-"), - prefix => prefix.to_string(), - }; - let anchor = format!( - "{}-{}", - linter.name().to_lowercase(), - common_prefix.to_lowercase() - ); - let _ = write!( - output, - "Derived from the **[{}](../rules.md#{})** linter.", - linter.name(), - anchor, - ); - output.push('\n'); - output.push('\n'); - } + let _ = write!( + output, + "Derived from the **[{}](../rules.md#{})** linter.", + linter.name(), + anchor, + ); + output.push('\n'); + output.push('\n'); + } - if rule.is_deprecated() { - output.push_str( - r"**Warning: This rule is deprecated and will be removed in a future release.**", - ); - output.push('\n'); - output.push('\n'); - } + if rule.is_deprecated() { + output.push_str( + r"**Warning: This rule is deprecated and will be removed in a future release.**", + ); + output.push('\n'); + output.push('\n'); + } - if rule.is_removed() { - output.push_str( - r"**Warning: This rule has been removed and its documentation is only available for historical reasons.**", - ); - output.push('\n'); - output.push('\n'); - } + if rule.is_removed() { + output.push_str( + r"**Warning: This rule has been removed and its documentation is only available for historical reasons.**", + ); + 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'); - } + 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 unstable and in [preview](../preview.md). The `--preview` flag is required for use.", - ); - output.push('\n'); - output.push('\n'); - } + if rule.is_preview() { + output.push_str( + r"This rule is unstable and in [preview](../preview.md). The `--preview` flag is required for use.", + ); + output.push('\n'); + output.push('\n'); + } - process_documentation(explanation.trim(), &mut output, rule.name().as_str()); + process_documentation(explanation.trim(), &mut output, rule.name().as_str()); - let filename = PathBuf::from(ROOT_DIR) - .join("docs") - .join("rules") - .join(&*rule.name()) - .with_extension("md"); + let filename = PathBuf::from(ROOT_DIR) + .join("docs") + .join("rules") + .join(&*rule.name()) + .with_extension("md"); - if args.dry_run { - println!("{output}"); - } else { - fs::create_dir_all("docs/rules")?; - fs::write(filename, output)?; - } + if args.dry_run { + println!("{output}"); + } else { + fs::create_dir_all("docs/rules")?; + fs::write(filename, output)?; } } Ok(()) diff --git a/crates/ruff_dev/src/generate_rules_table.rs b/crates/ruff_dev/src/generate_rules_table.rs index 713488a902..6d7c258a92 100644 --- a/crates/ruff_dev/src/generate_rules_table.rs +++ b/crates/ruff_dev/src/generate_rules_table.rs @@ -170,19 +170,13 @@ fn generate_table( // Message placeholders can be mistaken for an attribute list on the table cell. // Keep the identity class on an HTML wrapper so messages retain their Markdown. - #[expect(clippy::or_fun_call)] let _ = write!( table_out, - "| {ss}{explanation}
{message}{se} \ + "| {ss}[`{rule_name}`](rules/{rule_name}.md)
{message}{se} \ | {ss}{category}{se} {{ .rule-category }} \ | {ss}{linter_name}{se} {{ {linter_anchor} .rule-linter data-linter='{linter_slug}' }} \ |
{status_token}{fix_token}{default_token}
{{ .rule-status data-status='{status}' }} |", category = rule.category(), - explanation = rule - .explanation() - .is_some() - .then_some(format_args!("[`{rule_name}`](rules/{rule_name}.md)")) - .unwrap_or(format_args!("`{rule_name}`")), ); table_out.push('\n'); } diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index 55cb3e6ad4..47c5cc5b50 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -128,7 +128,7 @@ where if let Some(code) = rule.noqa_code() { diagnostic.set_secondary_code(SecondaryCode::new(code.to_string())); } - diagnostic.set_documentation_url(rule.url()); + diagnostic.set_documentation_url(Some(rule.url())); diagnostic } diff --git a/crates/ruff_linter/src/message/sarif.rs b/crates/ruff_linter/src/message/sarif.rs index 0fe21bd996..9ea54b8cc9 100644 --- a/crates/ruff_linter/src/message/sarif.rs +++ b/crates/ruff_linter/src/message/sarif.rs @@ -117,11 +117,9 @@ struct SarifDriver<'a> { #[derive(Debug, Clone, Serialize)] #[serde(rename_all = "camelCase")] struct SarifRule<'a> { - #[serde(skip_serializing_if = "Option::is_none")] - full_description: Option>, + full_description: SarifMessage<'a>, help: SarifMessage<'a>, - #[serde(skip_serializing_if = "Option::is_none")] - help_uri: Option, + help_uri: String, id: &'a str, properties: SarifProperties<'a>, short_description: SarifMessage<'a>, @@ -150,9 +148,9 @@ impl<'a> From<(&'a str, SarifLevel)> for SarifRule<'a> { short_description: SarifMessage { text: rule.message_formats()[0].into(), }, - full_description: rule - .explanation() - .map(|text| SarifMessage { text: text.into() }), + full_description: SarifMessage { + text: rule.explanation().into(), + }, help: SarifMessage { text: rule.message_formats()[0].into(), }, diff --git a/crates/ruff_linter/src/registry.rs b/crates/ruff_linter/src/registry.rs index cf529e4e0a..128906741c 100644 --- a/crates/ruff_linter/src/registry.rs +++ b/crates/ruff_linter/src/registry.rs @@ -351,15 +351,15 @@ impl Rule { } } - /// Return the URL for the rule documentation, if it exists. - pub fn url(&self) -> Option { - self.explanation().is_some().then(|| { - format!( - "{}/rules/{name}", - env!("CARGO_PKG_HOMEPAGE"), - name = self.name() - ) - }) + /// Return the URL for the rule documentation. + pub fn url(&self) -> String { + // Every rule has documentation. Calling `explanation()` here would retain all + // documentation strings in binaries that only need the URLs. + format!( + "{}/rules/{name}", + env!("CARGO_PKG_HOMEPAGE"), + name = self.name() + ) } pub fn name(&self) -> LintName { @@ -485,7 +485,7 @@ mod tests { fn documentation() { for rule in Rule::iter() { assert!( - rule.explanation().is_some(), + !rule.explanation().trim().is_empty(), "Rule {} is missing documentation", rule.name() ); diff --git a/crates/ruff_linter/src/violation.rs b/crates/ruff_linter/src/violation.rs index 77bed26b99..b0b83abe13 100644 --- a/crates/ruff_linter/src/violation.rs +++ b/crates/ruff_linter/src/violation.rs @@ -34,7 +34,7 @@ pub trait ViolationMetadata { /// Returns an explanation of what this violation catches, /// why it's bad, and what users should do instead. - fn explain() -> Option<&'static str>; + fn explain() -> &'static str; /// Returns the rule status for this violation. fn status() -> RuleStatus; diff --git a/crates/ruff_macros/src/map_codes.rs b/crates/ruff_macros/src/map_codes.rs index 1fc23f5e27..940034c8d3 100644 --- a/crates/ruff_macros/src/map_codes.rs +++ b/crates/ruff_macros/src/map_codes.rs @@ -484,7 +484,7 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { } /// Returns the documentation for this rule. - pub fn explanation(&self) -> Option<&'static str> { + pub fn explanation(&self) -> &'static str { use crate::ViolationMetadata; match self { #rule_explanation_match_arms } } diff --git a/crates/ruff_macros/src/violation_metadata.rs b/crates/ruff_macros/src/violation_metadata.rs index e52c140e10..d14094f34a 100644 --- a/crates/ruff_macros/src/violation_metadata.rs +++ b/crates/ruff_macros/src/violation_metadata.rs @@ -36,8 +36,8 @@ pub(crate) fn violation_metadata(input: DeriveInput) -> syn::Result crate::registry::Rule::#name } - fn explain() -> Option<&'static str> { - Some(#docs) + fn explain() -> &'static str { + #docs } fn status() -> crate::codes::RuleStatus { diff --git a/crates/ruff_server/src/server/api/requests/hover.rs b/crates/ruff_server/src/server/api/requests/hover.rs index cefc9b78d0..d72f68dd9b 100644 --- a/crates/ruff_server/src/server/api/requests/hover.rs +++ b/crates/ruff_server/src/server/api/requests/hover.rs @@ -145,11 +145,6 @@ fn format_rule_text(rule: Rule) -> String { output.push('\n'); } - if let Some(explanation) = rule.explanation() { - output.push_str(explanation.trim()); - } else { - tracing::warn!("Rule {} does not have an explanation", rule.name_and_code()); - output.push_str("An issue occurred: an explanation for this rule was not found."); - } + output.push_str(rule.explanation().trim()); output }