Unnamed repository; edit this file 'description' to name the repository.
Merge #10221
10221: internal: prevent possible bugs when adding magical comments r=matklad a=matklad
bors r+
🤖
Co-authored-by: Aleksey Kladov <[email protected]>
| -rw-r--r-- | crates/ide_assists/src/handlers/convert_comment_block.rs | 30 | ||||
| -rw-r--r-- | crates/ide_assists/src/lib.rs | 62 | ||||
| -rw-r--r-- | crates/ide_assists/src/tests/generated.rs | 17 | ||||
| -rw-r--r-- | crates/ide_completion/src/completions/flyimport.rs | 184 | ||||
| -rw-r--r-- | crates/ide_completion/src/completions/fn_param.rs | 2 | ||||
| -rw-r--r-- | crates/parser/src/grammar.rs | 10 | ||||
| -rw-r--r-- | crates/parser/src/grammar/expressions.rs | 1 | ||||
| -rw-r--r-- | crates/parser/src/grammar/patterns.rs | 7 | ||||
| -rw-r--r-- | crates/sourcegen/src/lib.rs | 93 | ||||
| -rw-r--r-- | crates/syntax/test_data/parser/inline/ok/0110_use_path.rast | 74 | ||||
| -rw-r--r-- | crates/syntax/test_data/parser/inline/ok/0110_use_path.rs | 5 | ||||
| -rw-r--r-- | crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rast | 47 | ||||
| -rw-r--r-- | crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rs | 4 |
13 files changed, 368 insertions, 168 deletions
diff --git a/crates/ide_assists/src/handlers/convert_comment_block.rs b/crates/ide_assists/src/handlers/convert_comment_block.rs index 749e8685bf..472aef2648 100644 --- a/crates/ide_assists/src/handlers/convert_comment_block.rs +++ b/crates/ide_assists/src/handlers/convert_comment_block.rs @@ -6,21 +6,21 @@ use syntax::{ use crate::{AssistContext, AssistId, AssistKind, Assists}; -/// Assist: line_to_block -/// -/// Converts comments between block and single-line form -/// -/// ``` -/// // Multi-line -/// // comment -/// ``` -/// -> -/// ``` -/// /** -/// Multi-line -/// comment -/// */ -/// ``` +// Assist: line_to_block +// +// Converts comments between block and single-line form. +// +// ``` +// // Multi-line$0 +// // comment +// ``` +// -> +// ``` +// /* +// Multi-line +// comment +// */ +// ``` pub(crate) fn convert_comment_block(acc: &mut Assists, ctx: &AssistContext) -> Option<()> { let comment = ctx.find_token_at_offset::<ast::Comment>()?; // Only allow comments which are alone on their line diff --git a/crates/ide_assists/src/lib.rs b/crates/ide_assists/src/lib.rs index 7c074a4f63..b4fb5c190f 100644 --- a/crates/ide_assists/src/lib.rs +++ b/crates/ide_assists/src/lib.rs @@ -1,10 +1,63 @@ -//! `assists` crate provides a bunch of code assists, also known as code -//! actions (in LSP) or intentions (in IntelliJ). +//! `assists` crate provides a bunch of code assists, also known as code actions +//! (in LSP) or intentions (in IntelliJ). //! //! An assist is a micro-refactoring, which is automatically activated in //! certain context. For example, if the cursor is over `,`, a "swap `,`" assist //! becomes available. - +//! +//! ## Assists Guidelines +//! +//! Assists are the main mechanism to deliver advanced IDE features to the user, +//! so we should pay extra attention to the UX. +//! +//! The power of assists comes from their context-awareness. The main problem +//! with IDE features is that there are a lot of them, and it's hard to teach +//! the user what's available. Assists solve this problem nicely: 💡 signifies +//! that *something* is possible, and clicking on it reveals a *short* list of +//! actions. Contrast it with Emacs `M-x`, which just spits an infinite list of +//! all the features. +//! +//! Here are some considerations when creating a new assist: +//! +//! * It's good to preserve semantics, and it's good to keep the code compiling, +//! but it isn't necessary. Example: "flip binary operation" might change +//! semantics. +//! * Assist shouldn't necessary make the code "better". A lot of assist come in +//! pairs: "if let <-> match". +//! * Assists should have as narrow scope as possible. Each new assists greatly +//! improves UX for cases where the user actually invokes it, but it makes UX +//! worse for every case where the user clicks 💡 to invoke some *other* +//! assist. So, a rarely useful assist which is always applicable can be a net +//! negative. +//! * Rarely useful actions are tricky. Sometimes there are features which are +//! clearly useful to some users, but are just noise most of the time. We +//! don't have a good solution here, our current approach is to make this +//! functionality available only if assist is applicable to the whole +//! selection. Example: `sort_items` sorts items alphabetically. Naively, it +//! should be available more or less everywhere, which isn't useful. So +//! instead we only show it if the user *selects* the items they want to sort. +//! * Consider grouping related assists together (see [`Assists::add_group`]). +//! * Make assists robust. If the assist depends on results of type-inference to +//! much, it might only fire in fully-correct code. This makes assist less +//! useful and (worse) less predictable. The user should have a clear +//! intuition when each particular assist is available. +//! * Make small assists, which compose. Example: rather than auto-importing +//! enums in `fill_match_arms`, we use fully-qualified names. There's a +//! separate assist to shorten a fully-qualified name. +//! * Distinguish between assists and fixits for diagnostics. Internally, fixits +//! and assists are equivalent. They have the same "show a list + invoke a +//! single element" workflow, and both use [`Assist`] data structure. The main +//! difference is in the UX: while 💡 looks only at the cursor position, +//! diagnostics squigglies and fixits are calculated for the whole file and +//! are presented to the user eagerly. So, diagnostics should be fixable +//! errors, while assists can be just suggestions for an alternative way to do +//! something. If something *could* be a diagnostic, it should be a +//! diagnostic. Conversely, it might be valuable to turn a diagnostic with a +//! lot of false errors into an assist. +//! * +//! +//! See also this post: +//! <https://rust-analyzer.github.io/blog/2020/09/28/how-to-make-a-light-bulb.html> #[allow(unused)] macro_rules! eprintln { ($($tt:tt)*) => { stdx::eprintln!($($tt)*) }; @@ -28,6 +81,9 @@ pub use ide_db::assists::{ }; /// Return all the assists applicable at the given position. +/// +// NOTE: We don't have a `Feature: ` section for assists, they are special-cased +// in the manual. pub fn assists( db: &RootDatabase, config: &AssistConfig, diff --git a/crates/ide_assists/src/tests/generated.rs b/crates/ide_assists/src/tests/generated.rs index 39ab8c7b74..46dd409409 100644 --- a/crates/ide_assists/src/tests/generated.rs +++ b/crates/ide_assists/src/tests/generated.rs @@ -1065,6 +1065,23 @@ fn main() { } #[test] +fn doctest_line_to_block() { + check_doc_test( + "line_to_block", + r#####" + // Multi-line$0 + // comment +"#####, + r#####" + /* + Multi-line + comment + */ +"#####, + ) +} + +#[test] fn doctest_make_raw_string() { check_doc_test( "make_raw_string", diff --git a/crates/ide_completion/src/completions/flyimport.rs b/crates/ide_completion/src/completions/flyimport.rs index 1eb45036a9..9e6d26640e 100644 --- a/crates/ide_completion/src/completions/flyimport.rs +++ b/crates/ide_completion/src/completions/flyimport.rs @@ -1,95 +1,4 @@ -//! Feature: completion with imports-on-the-fly -//! -//! When completing names in the current scope, proposes additional imports from other modules or crates, -//! if they can be qualified in the scope, and their name contains all symbols from the completion input. -//! -//! To be considered applicable, the name must contain all input symbols in the given order, not necessarily adjacent. -//! If any input symbol is not lowercased, the name must contain all symbols in exact case; otherwise the containing is checked case-insensitively. -//! -//! ``` -//! fn main() { -//! pda$0 -//! } -//! # pub mod std { pub mod marker { pub struct PhantomData { } } } -//! ``` -//! -> -//! ``` -//! use std::marker::PhantomData; -//! -//! fn main() { -//! PhantomData -//! } -//! # pub mod std { pub mod marker { pub struct PhantomData { } } } -//! ``` -//! -//! Also completes associated items, that require trait imports. -//! If any unresolved and/or partially-qualified path precedes the input, it will be taken into account. -//! Currently, only the imports with their import path ending with the whole qualifier will be proposed -//! (no fuzzy matching for qualifier). -//! -//! ``` -//! mod foo { -//! pub mod bar { -//! pub struct Item; -//! -//! impl Item { -//! pub const TEST_ASSOC: usize = 3; -//! } -//! } -//! } -//! -//! fn main() { -//! bar::Item::TEST_A$0 -//! } -//! ``` -//! -> -//! ``` -//! use foo::bar; -//! -//! mod foo { -//! pub mod bar { -//! pub struct Item; -//! -//! impl Item { -//! pub const TEST_ASSOC: usize = 3; -//! } -//! } -//! } -//! -//! fn main() { -//! bar::Item::TEST_ASSOC -//! } -//! ``` -//! -//! NOTE: currently, if an assoc item comes from a trait that's not currently imported, and it also has an unresolved and/or partially-qualified path, -//! no imports will be proposed. -//! -//! .Fuzzy search details -//! -//! To avoid an excessive amount of the results returned, completion input is checked for inclusion in the names only -//! (i.e. in `HashMap` in the `std::collections::HashMap` path). -//! For the same reasons, avoids searching for any path imports for inputs with their length less than 2 symbols -//! (but shows all associated items for any input length). -//! -//! .Import configuration -//! -//! It is possible to configure how use-trees are merged with the `importMergeBehavior` setting. -//! Mimics the corresponding behavior of the `Auto Import` feature. -//! -//! .LSP and performance implications -//! -//! The feature is enabled only if the LSP client supports LSP protocol version 3.16+ and reports the `additionalTextEdits` -//! (case-sensitive) resolve client capability in its client capabilities. -//! This way the server is able to defer the costly computations, doing them for a selected completion item only. -//! For clients with no such support, all edits have to be calculated on the completion request, including the fuzzy search completion ones, -//! which might be slow ergo the feature is automatically disabled. -//! -//! .Feature toggle -//! -//! The feature can be forcefully turned off in the settings with the `rust-analyzer.completion.autoimport.enable` flag. -//! Note that having this flag set to `true` does not guarantee that the feature is enabled: your client needs to have the corresponding -//! capability enabled. - +//! See [`import_on_the_fly`]. use ide_db::helpers::{ import_assets::{ImportAssets, ImportCandidate}, insert_use::ImportScope, @@ -105,6 +14,97 @@ use crate::{ use super::Completions; +// Feature: Completion With Autoimport +// +// When completing names in the current scope, proposes additional imports from other modules or crates, +// if they can be qualified in the scope, and their name contains all symbols from the completion input. +// +// To be considered applicable, the name must contain all input symbols in the given order, not necessarily adjacent. +// If any input symbol is not lowercased, the name must contain all symbols in exact case; otherwise the containing is checked case-insensitively. +// +// ``` +// fn main() { +// pda$0 +// } +// # pub mod std { pub mod marker { pub struct PhantomData { } } } +// ``` +// -> +// ``` +// use std::marker::PhantomData; +// +// fn main() { +// PhantomData +// } +// # pub mod std { pub mod marker { pub struct PhantomData { } } } +// ``` +// +// Also completes associated items, that require trait imports. +// If any unresolved and/or partially-qualified path precedes the input, it will be taken into account. +// Currently, only the imports with their import path ending with the whole qualifier will be proposed +// (no fuzzy matching for qualifier). +// +// ``` +// mod foo { +// pub mod bar { +// pub struct Item; +// +// impl Item { +// pub const TEST_ASSOC: usize = 3; +// } +// } +// } +// +// fn main() { +// bar::Item::TEST_A$0 +// } +// ``` +// -> +// ``` +// use foo::bar; +// +// mod foo { +// pub mod bar { +// pub struct Item; +// +// impl Item { +// pub const TEST_ASSOC: usize = 3; +// } +// } +// } +// +// fn main() { +// bar::Item::TEST_ASSOC +// } +// ``` +// +// NOTE: currently, if an assoc item comes from a trait that's not currently imported, and it also has an unresolved and/or partially-qualified path, +// no imports will be proposed. +// +// .Fuzzy search details +// +// To avoid an excessive amount of the results returned, completion input is checked for inclusion in the names only +// (i.e. in `HashMap` in the `std::collections::HashMap` path). +// For the same reasons, avoids searching for any path imports for inputs with their length less than 2 symbols +// (but shows all associated items for any input length). +// +// .Import configuration +// +// It is possible to configure how use-trees are merged with the `importMergeBehavior` setting. +// Mimics the corresponding behavior of the `Auto Import` feature. +// +// .LSP and performance implications +// +// The feature is enabled only if the LSP client supports LSP protocol version 3.16+ and reports the `additionalTextEdits` +// (case-sensitive) resolve client capability in its client capabilities. +// This way the server is able to defer the costly computations, doing them for a selected completion item only. +// For clients with no such support, all edits have to be calculated on the completion request, including the fuzzy search completion ones, +// which might be slow ergo the feature is automatically disabled. +// +// .Feature toggle +// +// The feature can be forcefully turned off in the settings with the `rust-analyzer.completion.autoimport.enable` flag. +// Note that having this flag set to `true` does not guarantee that the feature is enabled: your client needs to have the corresponding +// capability enabled. pub(crate) fn import_on_the_fly(acc: &mut Completions, ctx: &CompletionContext) -> Option<()> { if !ctx.config.enable_imports_on_the_fly { return None; diff --git a/crates/ide_completion/src/completions/fn_param.rs b/crates/ide_completion/src/completions/fn_param.rs index d8d3b8e85b..28da6d69c8 100644 --- a/crates/ide_completion/src/completions/fn_param.rs +++ b/crates/ide_completion/src/completions/fn_param.rs @@ -1,4 +1,4 @@ -//! See `complete_fn_param`. +//! See [`complete_fn_param`]. use rustc_hash::FxHashMap; use syntax::{ diff --git a/crates/parser/src/grammar.rs b/crates/parser/src/grammar.rs index 382cc4fcc5..7243b89583 100644 --- a/crates/parser/src/grammar.rs +++ b/crates/parser/src/grammar.rs @@ -194,10 +194,12 @@ fn opt_visibility(p: &mut Parser) -> bool { // crate fn main() { } // struct S { crate field: u32 } // struct T(crate u32); - // - // test crate_keyword_path - // fn foo() { crate::foo(); } - T![crate] if !p.nth_at(1, T![::]) => { + T![crate] => { + if p.nth_at(1, T![::]) { + // test crate_keyword_path + // fn foo() { crate::foo(); } + return false; + } let m = p.start(); p.bump(T![crate]); m.complete(p, VISIBILITY); diff --git a/crates/parser/src/grammar/expressions.rs b/crates/parser/src/grammar/expressions.rs index 686a643456..001be099e6 100644 --- a/crates/parser/src/grammar/expressions.rs +++ b/crates/parser/src/grammar/expressions.rs @@ -374,7 +374,6 @@ fn lhs(p: &mut Parser, r: Restrictions) -> Option<(CompletedMarker, BlockLike)> // let mut p = F{x: 5}; // {p}.x = 10; // } - // let (lhs, blocklike) = atom::atom_expr(p, r)?; return Some(postfix_expr(p, lhs, blocklike, !(r.prefer_stmt && blocklike.is_block()))); } diff --git a/crates/parser/src/grammar/patterns.rs b/crates/parser/src/grammar/patterns.rs index fed5cca512..81e2051abb 100644 --- a/crates/parser/src/grammar/patterns.rs +++ b/crates/parser/src/grammar/patterns.rs @@ -17,7 +17,7 @@ pub(crate) fn pattern(p: &mut Parser) { pattern_r(p, PAT_RECOVERY_SET); } -/// Parses a pattern list separated by pipes `|` +/// Parses a pattern list separated by pipes `|`. pub(super) fn pattern_top(p: &mut Parser) { pattern_top_r(p, PAT_RECOVERY_SET) } @@ -27,14 +27,15 @@ pub(crate) fn pattern_single(p: &mut Parser) { } /// Parses a pattern list separated by pipes `|` -/// using the given `recovery_set` +/// using the given `recovery_set`. pub(super) fn pattern_top_r(p: &mut Parser, recovery_set: TokenSet) { p.eat(T![|]); pattern_r(p, recovery_set); } /// Parses a pattern list separated by pipes `|`, with no leading `|`,using the -/// given `recovery_set` +/// given `recovery_set`. + // test or_pattern // fn main() { // match () { diff --git a/crates/sourcegen/src/lib.rs b/crates/sourcegen/src/lib.rs index 2d28066342..6a332bce85 100644 --- a/crates/sourcegen/src/lib.rs +++ b/crates/sourcegen/src/lib.rs @@ -43,10 +43,12 @@ pub fn list_files(dir: &Path) -> Vec<PathBuf> { res } +#[derive(Clone)] pub struct CommentBlock { pub id: String, pub line: usize, pub contents: Vec<String>, + is_doc: bool, } impl CommentBlock { @@ -54,59 +56,60 @@ impl CommentBlock { assert!(tag.starts_with(char::is_uppercase)); let tag = format!("{}:", tag); - let mut res = Vec::new(); - for (line, mut block) in do_extract_comment_blocks(text, true) { - let first = block.remove(0); - if let Some(id) = first.strip_prefix(&tag) { - let id = id.trim().to_string(); - let block = CommentBlock { id, line, contents: block }; - res.push(block); - } - } - res + // Would be nice if we had `.retain_mut` here! + CommentBlock::extract_untagged(text) + .into_iter() + .filter_map(|mut block| { + let first = block.contents.remove(0); + first.strip_prefix(&tag).map(|id| { + if block.is_doc { + panic!( + "Use plain (non-doc) comments with tags like {}:\n {}", + tag, first + ) + } + + block.id = id.trim().to_string(); + block + }) + }) + .collect() } pub fn extract_untagged(text: &str) -> Vec<CommentBlock> { let mut res = Vec::new(); - for (line, block) in do_extract_comment_blocks(text, false) { - let id = String::new(); - let block = CommentBlock { id, line, contents: block }; - res.push(block); - } - res - } -} - -fn do_extract_comment_blocks( - text: &str, - allow_blocks_with_empty_lines: bool, -) -> Vec<(usize, Vec<String>)> { - let mut res = Vec::new(); - let prefix = "// "; - let lines = text.lines().map(str::trim_start); - - let mut block = (0, vec![]); - for (line_num, line) in lines.enumerate() { - if line == "//" && allow_blocks_with_empty_lines { - block.1.push(String::new()); - continue; - } - - let is_comment = line.starts_with(prefix); - if is_comment { - block.1.push(line[prefix.len()..].to_string()); - } else { - if !block.1.is_empty() { - res.push(mem::take(&mut block)); + let lines = text.lines().map(str::trim_start); + + let dummy_block = + CommentBlock { id: String::new(), line: 0, contents: Vec::new(), is_doc: false }; + let mut block = dummy_block.clone(); + for (line_num, line) in lines.enumerate() { + match line.strip_prefix("//") { + Some(mut contents) => { + if let Some('/' | '!') = contents.chars().next() { + contents = &contents[1..]; + block.is_doc = true; + } + if let Some(' ') = contents.chars().next() { + contents = &contents[1..]; + } + block.contents.push(contents.to_string()); + } + None => { + if !block.contents.is_empty() { + let block = mem::replace(&mut block, dummy_block.clone()); + res.push(block); + } + block.line = line_num + 2; + } } - block.0 = line_num + 2; } + if !block.contents.is_empty() { + res.push(block) + } + res } - if !block.1.is_empty() { - res.push(block) - } - res } #[derive(Debug)] diff --git a/crates/syntax/test_data/parser/inline/ok/0110_use_path.rast b/crates/syntax/test_data/parser/inline/ok/0110_use_path.rast index c9fad5f8c5..ac51eb91d2 100644 --- a/crates/syntax/test_data/parser/inline/ok/0110_use_path.rast +++ b/crates/syntax/test_data/parser/inline/ok/0110_use_path.rast @@ -1,4 +1,4 @@ [email protected] "use" @@ -35,4 +35,74 @@ [email protected] [email protected] "// Rust 2018 - Unifor ..." - [email protected] "\n" + [email protected] "\n\n" + [email protected] "use" + [email protected] " " + [email protected] "self" + [email protected] "::" + [email protected] "module" + [email protected] "::" + [email protected] "Item" + [email protected] ";" + [email protected] "\n" + [email protected] "use" + [email protected] " " + [email protected] "crate" + [email protected] "::" + [email protected] "Item" + [email protected] ";" + [email protected] "\n" + [email protected] "use" + [email protected] " " + [email protected] "self" + [email protected] "::" + [email protected] "some" + [email protected] "::" + [email protected] "Struct" + [email protected] ";" + [email protected] "\n" + [email protected] "use" + [email protected] " " + [email protected] "crate_name" + [email protected] "::" + [email protected] "some_item" + [email protected] ";" + [email protected] "\n" diff --git a/crates/syntax/test_data/parser/inline/ok/0110_use_path.rs b/crates/syntax/test_data/parser/inline/ok/0110_use_path.rs index 328e947360..1e436a6bc2 100644 --- a/crates/syntax/test_data/parser/inline/ok/0110_use_path.rs +++ b/crates/syntax/test_data/parser/inline/ok/0110_use_path.rs @@ -1,3 +1,8 @@ use ::crate_name; // Rust 2018 - All flavours use crate_name; // Rust 2018 - Anchored paths use item_in_scope_or_crate_name; // Rust 2018 - Uniform Paths + +use self::module::Item; +use crate::Item; +use self::some::Struct; +use crate_name::some_item; diff --git a/crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rast b/crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rast index db2b645b06..533f738e15 100644 --- a/crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rast +++ b/crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rast @@ -1,4 +1,4 @@ [email protected] "struct" @@ -39,4 +39,47 @@ [email protected] [email protected] "\n" - [email protected] "\n" + [email protected] "\n\n" + [email protected] "enum" + [email protected] " " + [email protected] "S" + [email protected] " " + [email protected] "{" + [email protected] "\n " + [email protected] "Uri" + [email protected] "(" + [email protected] "#" + [email protected] "[" + [email protected] "serde" + [email protected] "(" + [email protected] "with" + [email protected] " " + [email protected] "=" + [email protected] " " + [email protected] "\"url_serde\"" + [email protected] ")" + [email protected] "]" + [email protected] " " + [email protected] "Uri" + [email protected] ")" + [email protected] "," + [email protected] "\n" + [email protected] "}" + [email protected] "\n" diff --git a/crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rs b/crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rs index 635b9ac21a..4da379d0ed 100644 --- a/crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rs +++ b/crates/syntax/test_data/parser/inline/ok/0115_tuple_field_attrs.rs @@ -2,3 +2,7 @@ struct S ( #[serde(with = "url_serde")] pub Uri, ); + +enum S { + Uri(#[serde(with = "url_serde")] Uri), +} |