From c5b1e3f2ae5c397eb98d86228186a8e533243bfe Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Fri, 20 Jan 2023 14:29:12 +0100 Subject: [PATCH] Fix markdown removal in hover handling whitespace weirdly --- crates/ide/src/hover.rs | 25 +++-- crates/ide/src/hover/render.rs | 57 ++++------- crates/ide/src/hover/tests.rs | 23 +++-- crates/ide/src/markdown_remove.rs | 147 ++++++++++++++++++++++++++- crates/ide/src/static_index.rs | 6 +- crates/rust-analyzer/src/config.rs | 5 +- crates/rust-analyzer/src/handlers.rs | 3 +- 7 files changed, 202 insertions(+), 64 deletions(-) diff --git a/crates/ide/src/hover.rs b/crates/ide/src/hover.rs index f0c6505ee6e..c46c1c1cd1e 100644 --- a/crates/ide/src/hover.rs +++ b/crates/ide/src/hover.rs @@ -19,6 +19,7 @@ use crate::{ doc_links::token_as_doc_comment, + markdown_remove::remove_markdown, markup::Markup, runnables::{runnable_fn, runnable_mod}, FileId, FilePosition, NavigationTarget, RangeInfo, Runnable, TryToNav, @@ -26,14 +27,9 @@ #[derive(Clone, Debug, PartialEq, Eq)] pub struct HoverConfig { pub links_in_hover: bool, - pub documentation: Option, + pub documentation: bool, pub keywords: bool, -} - -impl HoverConfig { - fn markdown(&self) -> bool { - matches!(self.documentation, Some(HoverDocFormat::Markdown)) - } + pub format: HoverDocFormat, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -90,12 +86,23 @@ pub struct HoverResult { // image::https://user-images.githubusercontent.com/48062697/113020658-b5f98b80-917a-11eb-9f88-3dbc27320c95.gif[] pub(crate) fn hover( db: &RootDatabase, - FileRange { file_id, range }: FileRange, + file_range: FileRange, config: &HoverConfig, ) -> Option> { let sema = &hir::Semantics::new(db); - let file = sema.parse(file_id).syntax().clone(); + let mut res = hover_impl(sema, file_range, config)?; + if let HoverDocFormat::PlainText = config.format { + res.info.markup = remove_markdown(res.info.markup.as_str()).into(); + } + Some(res) +} +fn hover_impl( + sema: &Semantics<'_, RootDatabase>, + FileRange { file_id, range }: FileRange, + config: &HoverConfig, +) -> Option> { + let file = sema.parse(file_id).syntax().clone(); if !range.is_empty() { return hover_ranged(&file, range, sema, config); } diff --git a/crates/ide/src/hover/render.rs b/crates/ide/src/hover/render.rs index cb537d7ef79..d7b62649578 100644 --- a/crates/ide/src/hover/render.rs +++ b/crates/ide/src/hover/render.rs @@ -26,13 +26,12 @@ use crate::{ doc_links::{remove_links, rewrite_links}, hover::walk_and_push_ty, - markdown_remove::remove_markdown, HoverAction, HoverConfig, HoverResult, Markup, }; pub(super) fn type_info( sema: &Semantics<'_, RootDatabase>, - config: &HoverConfig, + _config: &HoverConfig, expr_or_pat: &Either, ) -> Option { let TypeInfo { original, adjusted } = match expr_or_pat { @@ -55,19 +54,15 @@ pub(super) fn type_info( let adjusted = adjusted_ty.display(sema.db).to_string(); let static_text_diff_len = "Coerced to: ".len() - "Type: ".len(); format!( - "{bt_start}Type: {:>apad$}\nCoerced to: {:>opad$}\n{bt_end}", + "```text\nType: {:>apad$}\nCoerced to: {:>opad$}\n```\n", original, adjusted, apad = static_text_diff_len + adjusted.len().max(original.len()), opad = original.len(), - bt_start = if config.markdown() { "```text\n" } else { "" }, - bt_end = if config.markdown() { "```\n" } else { "" } ) .into() - } else if config.markdown() { - Markup::fenced_block(&original.display(sema.db)) } else { - original.display(sema.db).to_string().into() + Markup::fenced_block(&original.display(sema.db)) }; res.actions.push(HoverAction::goto_type_from_targets(sema.db, targets)); Some(res) @@ -75,7 +70,7 @@ pub(super) fn type_info( pub(super) fn try_expr( sema: &Semantics<'_, RootDatabase>, - config: &HoverConfig, + _config: &HoverConfig, try_expr: &ast::TryExpr, ) -> Option { let inner_ty = sema.type_of_expr(&try_expr.expr()?)?.original; @@ -151,14 +146,12 @@ pub(super) fn try_expr( let ppad = static_text_len_diff.min(0).abs() as usize; res.markup = format!( - "{bt_start}{} Type: {:>pad0$}\nPropagated as: {:>pad1$}\n{bt_end}", + "```text\n{} Type: {:>pad0$}\nPropagated as: {:>pad1$}\n```\n", s, inner_ty, body_ty, pad0 = ty_len_max + tpad, pad1 = ty_len_max + ppad, - bt_start = if config.markdown() { "```text\n" } else { "" }, - bt_end = if config.markdown() { "```\n" } else { "" } ) .into(); Some(res) @@ -166,7 +159,7 @@ pub(super) fn try_expr( pub(super) fn deref_expr( sema: &Semantics<'_, RootDatabase>, - config: &HoverConfig, + _config: &HoverConfig, deref_expr: &ast::PrefixExpr, ) -> Option { let inner_ty = sema.type_of_expr(&deref_expr.expr()?)?.original; @@ -195,15 +188,13 @@ pub(super) fn deref_expr( .max(adjusted.len() + coerced_len) .max(inner.len() + deref_len); format!( - "{bt_start}Dereferenced from: {:>ipad$}\nTo type: {:>apad$}\nCoerced to: {:>opad$}\n{bt_end}", + "```text\nDereferenced from: {:>ipad$}\nTo type: {:>apad$}\nCoerced to: {:>opad$}\n```\n", inner, original, adjusted, ipad = max_len - deref_len, apad = max_len - type_len, opad = max_len - coerced_len, - bt_start = if config.markdown() { "```text\n" } else { "" }, - bt_end = if config.markdown() { "```\n" } else { "" } ) .into() } else { @@ -213,13 +204,11 @@ pub(super) fn deref_expr( let deref_len = "Dereferenced from: ".len(); let max_len = (original.len() + type_len).max(inner.len() + deref_len); format!( - "{bt_start}Dereferenced from: {:>ipad$}\nTo type: {:>apad$}\n{bt_end}", + "```text\nDereferenced from: {:>ipad$}\nTo type: {:>apad$}\n```\n", inner, original, ipad = max_len - deref_len, apad = max_len - type_len, - bt_start = if config.markdown() { "```text\n" } else { "" }, - bt_end = if config.markdown() { "```\n" } else { "" } ) .into() }; @@ -233,7 +222,7 @@ pub(super) fn keyword( config: &HoverConfig, token: &SyntaxToken, ) -> Option { - if !token.kind().is_keyword() || !config.documentation.is_some() || !config.keywords { + if !token.kind().is_keyword() || !config.documentation || !config.keywords { return None; } let parent = token.parent()?; @@ -257,7 +246,7 @@ pub(super) fn keyword( /// i.e. `let S {a, ..} = S {a: 1, b: 2}` pub(super) fn struct_rest_pat( sema: &Semantics<'_, RootDatabase>, - config: &HoverConfig, + _config: &HoverConfig, pattern: &RecordPat, ) -> HoverResult { let missing_fields = sema.record_pattern_missing_fields(pattern); @@ -286,11 +275,7 @@ pub(super) fn struct_rest_pat( // get rid of trailing comma s.truncate(s.len() - 2); - if config.markdown() { - Markup::fenced_block(&s) - } else { - s.into() - } + Markup::fenced_block(&s) }; res.actions.push(HoverAction::goto_type_from_targets(sema.db, targets)); res @@ -344,13 +329,8 @@ pub(super) fn process_markup( config: &HoverConfig, ) -> Markup { let markup = markup.as_str(); - let markup = if !config.markdown() { - remove_markdown(markup) - } else if config.links_in_hover { - rewrite_links(db, markup, def) - } else { - remove_links(markup) - }; + let markup = + if config.links_in_hover { rewrite_links(db, markup, def) } else { remove_links(markup) }; Markup::from(markup) } @@ -463,8 +443,9 @@ pub(super) fn definition( Definition::DeriveHelper(it) => (format!("derive_helper {}", it.name(db)), None), }; - let docs = match config.documentation { - Some(_) => docs.or_else(|| { + let docs = docs + .filter(|_| config.documentation) + .or_else(|| { // docs are missing, for assoc items of trait impls try to fall back to the docs of the // original item of the trait let assoc = def.as_assoc_item(db)?; @@ -472,10 +453,8 @@ pub(super) fn definition( let name = Some(assoc.name(db)?); let item = trait_.items(db).into_iter().find(|it| it.name(db) == name)?; item.docs(db) - }), - None => None, - }; - let docs = docs.filter(|_| config.documentation.is_some()).map(Into::into); + }) + .map(Into::into); markup(docs, label, mod_path) } diff --git a/crates/ide/src/hover/tests.rs b/crates/ide/src/hover/tests.rs index f90ca86f9b6..db2aaddc0be 100644 --- a/crates/ide/src/hover/tests.rs +++ b/crates/ide/src/hover/tests.rs @@ -2,7 +2,7 @@ use ide_db::base_db::{FileLoader, FileRange}; use syntax::TextRange; -use crate::{fixture, hover::HoverDocFormat, HoverConfig}; +use crate::{fixture, HoverConfig, HoverDocFormat}; fn check_hover_no_result(ra_fixture: &str) { let (analysis, position) = fixture::position(ra_fixture); @@ -10,8 +10,9 @@ fn check_hover_no_result(ra_fixture: &str) { .hover( &HoverConfig { links_in_hover: true, - documentation: Some(HoverDocFormat::Markdown), + documentation: true, keywords: true, + format: HoverDocFormat::Markdown, }, FileRange { file_id: position.file_id, range: TextRange::empty(position.offset) }, ) @@ -26,8 +27,9 @@ fn check(ra_fixture: &str, expect: Expect) { .hover( &HoverConfig { links_in_hover: true, - documentation: Some(HoverDocFormat::Markdown), + documentation: true, keywords: true, + format: HoverDocFormat::Markdown, }, FileRange { file_id: position.file_id, range: TextRange::empty(position.offset) }, ) @@ -47,8 +49,9 @@ fn check_hover_no_links(ra_fixture: &str, expect: Expect) { .hover( &HoverConfig { links_in_hover: false, - documentation: Some(HoverDocFormat::Markdown), + documentation: true, keywords: true, + format: HoverDocFormat::Markdown, }, FileRange { file_id: position.file_id, range: TextRange::empty(position.offset) }, ) @@ -68,8 +71,9 @@ fn check_hover_no_markdown(ra_fixture: &str, expect: Expect) { .hover( &HoverConfig { links_in_hover: true, - documentation: Some(HoverDocFormat::PlainText), + documentation: true, keywords: true, + format: HoverDocFormat::PlainText, }, FileRange { file_id: position.file_id, range: TextRange::empty(position.offset) }, ) @@ -89,8 +93,9 @@ fn check_actions(ra_fixture: &str, expect: Expect) { .hover( &HoverConfig { links_in_hover: true, - documentation: Some(HoverDocFormat::Markdown), + documentation: true, keywords: true, + format: HoverDocFormat::Markdown, }, FileRange { file_id, range: position.range_or_empty() }, ) @@ -105,8 +110,9 @@ fn check_hover_range(ra_fixture: &str, expect: Expect) { .hover( &HoverConfig { links_in_hover: false, - documentation: Some(HoverDocFormat::Markdown), + documentation: true, keywords: true, + format: HoverDocFormat::Markdown, }, range, ) @@ -121,8 +127,9 @@ fn check_hover_range_no_results(ra_fixture: &str) { .hover( &HoverConfig { links_in_hover: false, - documentation: Some(HoverDocFormat::Markdown), + documentation: true, keywords: true, + format: HoverDocFormat::Markdown, }, range, ) diff --git a/crates/ide/src/markdown_remove.rs b/crates/ide/src/markdown_remove.rs index 3ec5c629e4f..07a3fe3f02b 100644 --- a/crates/ide/src/markdown_remove.rs +++ b/crates/ide/src/markdown_remove.rs @@ -14,9 +14,154 @@ pub(crate) fn remove_markdown(markdown: &str) -> String { Event::SoftBreak | Event::HardBreak | Event::Rule | Event::End(Tag::CodeBlock(_)) => { out.push('\n') } - _ => {} + Event::End(Tag::Paragraph) => { + out.push('\n'); + out.push('\n'); + } + Event::Start(_) + | Event::End(_) + | Event::Html(_) + | Event::FootnoteReference(_) + | Event::TaskListMarker(_) => (), } } + if let Some(p) = out.rfind(|c| c != '\n') { + out.drain(p + 1..); + } + out } + +#[cfg(test)] +mod tests { + use expect_test::expect; + + use super::*; + + #[test] + fn smoke_test() { + let res = remove_markdown( + r##" +A function or function pointer. + +Functions are the primary way code is executed within Rust. Function blocks, usually just +called functions, can be defined in a variety of different places and be assigned many +different attributes and modifiers. + +Standalone functions that just sit within a module not attached to anything else are common, +but most functions will end up being inside [`impl`] blocks, either on another type itself, or +as a trait impl for that type. + +```rust +fn standalone_function() { + // code +} + +pub fn public_thing(argument: bool) -> String { + // code + # "".to_string() +} + +struct Thing { + foo: i32, +} + +impl Thing { + pub fn new() -> Self { + Self { + foo: 42, + } + } +} +``` + +In addition to presenting fixed types in the form of `fn name(arg: type, ..) -> return_type`, +functions can also declare a list of type parameters along with trait bounds that they fall +into. + +```rust +fn generic_function(x: T) -> (T, T, T) { + (x.clone(), x.clone(), x.clone()) +} + +fn generic_where(x: T) -> T + where T: std::ops::Add + Copy +{ + x + x + x +} +``` + +Declaring trait bounds in the angle brackets is functionally identical to using a `where` +clause. It's up to the programmer to decide which works better in each situation, but `where` +tends to be better when things get longer than one line. + +Along with being made public via `pub`, `fn` can also have an [`extern`] added for use in +FFI. + +For more information on the various types of functions and how they're used, consult the [Rust +book] or the [Reference]. + +[`impl`]: keyword.impl.html +[`extern`]: keyword.extern.html +[Rust book]: ../book/ch03-03-how-functions-work.html +[Reference]: ../reference/items/functions.html +"##, + ); + expect![[r#" + A function or function pointer. + + Functions are the primary way code is executed within Rust. Function blocks, usually just + called functions, can be defined in a variety of different places and be assigned many + different attributes and modifiers. + + Standalone functions that just sit within a module not attached to anything else are common, + but most functions will end up being inside impl blocks, either on another type itself, or + as a trait impl for that type. + + fn standalone_function() { + // code + } + + pub fn public_thing(argument: bool) -> String { + // code + # "".to_string() + } + + struct Thing { + foo: i32, + } + + impl Thing { + pub fn new() -> Self { + Self { + foo: 42, + } + } + } + + In addition to presenting fixed types in the form of fn name(arg: type, ..) -> return_type, + functions can also declare a list of type parameters along with trait bounds that they fall + into. + + fn generic_function(x: T) -> (T, T, T) { + (x.clone(), x.clone(), x.clone()) + } + + fn generic_where(x: T) -> T + where T: std::ops::Add + Copy + { + x + x + x + } + + Declaring trait bounds in the angle brackets is functionally identical to using a where + clause. It's up to the programmer to decide which works better in each situation, but where + tends to be better when things get longer than one line. + + Along with being made public via pub, fn can also have an extern added for use in + FFI. + + For more information on the various types of functions and how they're used, consult the Rust + book or the Reference."#]].assert_eq(&res); + } +} diff --git a/crates/ide/src/static_index.rs b/crates/ide/src/static_index.rs index 7ada4f1be07..3f7f6885f61 100644 --- a/crates/ide/src/static_index.rs +++ b/crates/ide/src/static_index.rs @@ -16,8 +16,7 @@ inlay_hints::AdjustmentHintsMode, moniker::{def_to_moniker, MonikerResult}, parent_module::crates_for, - Analysis, Fold, HoverConfig, HoverDocFormat, HoverResult, InlayHint, InlayHintsConfig, - TryToNav, + Analysis, Fold, HoverConfig, HoverResult, InlayHint, InlayHintsConfig, TryToNav, }; /// A static representation of fully analyzed source code. @@ -137,8 +136,9 @@ fn add_file(&mut self, file_id: FileId) { }); let hover_config = HoverConfig { links_in_hover: true, - documentation: Some(HoverDocFormat::Markdown), + documentation: true, keywords: true, + format: crate::HoverDocFormat::Markdown, }; let tokens = tokens.filter(|token| { matches!( diff --git a/crates/rust-analyzer/src/config.rs b/crates/rust-analyzer/src/config.rs index fd2f934f9fe..c4d9ad7dff5 100644 --- a/crates/rust-analyzer/src/config.rs +++ b/crates/rust-analyzer/src/config.rs @@ -1393,7 +1393,8 @@ pub fn highlighting_config(&self) -> HighlightConfig { pub fn hover(&self) -> HoverConfig { HoverConfig { links_in_hover: self.data.hover_links_enable, - documentation: self.data.hover_documentation_enable.then(|| { + documentation: self.data.hover_documentation_enable, + format: { let is_markdown = try_or_def!(self .caps .text_document @@ -1409,7 +1410,7 @@ pub fn hover(&self) -> HoverConfig { } else { HoverDocFormat::PlainText } - }), + }, keywords: self.data.hover_documentation_keywords_enable, } } diff --git a/crates/rust-analyzer/src/handlers.rs b/crates/rust-analyzer/src/handlers.rs index 33ca7810667..4e08bd0a724 100644 --- a/crates/rust-analyzer/src/handlers.rs +++ b/crates/rust-analyzer/src/handlers.rs @@ -936,8 +936,7 @@ pub(crate) fn handle_hover( let line_index = snap.file_line_index(file_range.file_id)?; let range = to_proto::range(&line_index, info.range); - let markup_kind = - snap.config.hover().documentation.map_or(ide::HoverDocFormat::Markdown, |kind| kind); + let markup_kind = snap.config.hover().format; let hover = lsp_ext::Hover { hover: lsp_types::Hover { contents: HoverContents::Markup(to_proto::markup_content(